Closed Bug 1241496 Opened 8 years ago Closed 8 years ago

implement login v3

Categories

(Taskcluster :: Services, defect)

Product:
Taskcluster
Component:
Services
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: dustin, Assigned: dustin)

References

Details

The latter two are ready to land when I have the time.  Jabba, I'll need an assist modifying some URLs in the Okta config.  Can I ping you on Monday for that?
Flags: needinfo?(jdow)
WIP on making temp creds' clientId more free-form in
  https://github.com/taskcluster/taskcluster-lib-api/pull/8
needs tests, a req.clientId attribute, and docs for all of the req.<foo> things in one place.
(In reply to Dustin J. Mitchell [:dustin] from comment #9)
> WIP on making temp creds' clientId more free-form in
>   https://github.com/taskcluster/taskcluster-lib-api/pull/8
> needs tests, a req.clientId attribute, and docs for all of the req.<foo>
> things in one place.

The go client supports creating temporary credentials from permanent credentials, based on http://docs.taskcluster.net/auth/temporary-credentials/ - is this spec something that will change, and will I need to port to the go client?
Flags: needinfo?(dustin)
It will be compatible, but yes, the go client should be upgraded to support it.

Note to self: when issuer != clientId, clientId needs to be covered by the cert, which means changing the set of things hashed into the cert.  Jonas, the cert signature currently only covers things in the cert:
  https://github.com/taskcluster/taskcluster-lib-api/blob/master/src/api.js#L881
would it be better to duplicate the credential clientId in the cert (meaning a longer ext) or just reference it in the hash?  (In either case, I don't think there's a reason to bump the version)
Flags: needinfo?(dustin) → needinfo?(jopsen)
From Jonas in https://github.com/taskcluster/taskcluster-docs/pull/70#issuecomment-178116599

---

Note: it might be worth considering to add an impersonate option next to authorizedScopes.
Basically, when mozilla-taskcluster makes a request to create a task from a try-push it should use impersonate: <clientId-for-try-pusher>. So APIs will see the impersonated clientId, rather than the real clientId.

Same relation would imply, you need auth:create-client:<clientId> to impersonate a clientId.
It occurred to me this weekend that I've been making an invalid assumption: that clientIds were being logged based on the clientId supplied with the request.  It turns out, the clientId is not currently exposed by the signatureValidator.  The clientId *is* one of the three text fields we currently require users to copy/paste quite a bit, but ideally there won't be as much copy/pasting anymore once login v3 is in place.

The assumption was the driver behind changing the supplied clientId (thus requiring reading the ext to find the issuer before fetching its accessKey).

Jonas is solving a slightly different question (selecting a clientId on a per-request basis) that I don't want to solve right now, but I think the idea is sound: let's add a "tempClientId" property to the certificate which, if present, is treated as the clientId for purposes of identifying the credentials and logging its use.
> It occurred to me this weekend that I've been making an invalid assumption:
> that clientIds were being logged based on the clientId supplied with the request.
But they should be... We are not logging them because they didn't imply identity. Now they do,
so now we should start to log them. It egg and chicken thing all over again :)

@dustin: impersonate was to get the same functionality as "issuer", when calling with permanent credentials on behalf of someone else. I don't see a need for "tempClientId", we have "issuer".

----
On-topic if the signature:

It's not 'issuer' that needs to be covered by the signature. It's the clientId that is impersonated.

### When clientId == issuer.clientId (cert.issuer === undefined)

tempClientId:  issuer.clientId
cert:          {version, seed, start, expiry, scopes, signature}
signature:
  HMAC(
    version:1\n
    seed:<seed>\n
    start:<start>\n
    expiry:<expiry>\n
    scopes:\n
    <scope1>\n
    ...\n
    <scopeN>
  , issuer.accessToken)
accessToken:   HMAC(<seed>, issuer.accessToken)


### When clientId != issuer.clientId (cert.issuer !== undefined)

tempClientId:  ... // where issuer has 'auth:create-client:<tempClientId>'
cert:          {version, issuer: issuer.clientId, seed, start, expiry, scopes, signature}
signature:
  HMAC(
    version:1\n
    impersonate:<TempclientId>\n
    seed:<seed>\n
    start:<start>\n
    expiry:<expiry>\n
    scopes:\n
    <scope1>\n
    ...\n
    <scopeN>
  , issuer.accessToken)
accessToken:   HMAC(<seed>, issuer.accessToken)


------------------------
IMO, it's okay to say that "issuer" (and by implication "impersonate" in the signature) are optional.
Hence, not introduction a version 2 where cert.issuer is required.
It doesn't really make sense to carry cert.issuer, if tempClientId == cert.issuer.
The key "identity:" could also be used instead of "impersonate".


----
The impersonate option I was talking about before was unrelated to temporary credentials.
I'll propose it separately.
Flags: needinfo?(jopsen)
> But they should be... We are not logging them because they didn't imply identity. Now they do,
> so now we should start to log them. It egg and chicken thing all over again :)

My point was only that we don't need to do backflips to squeeze that value into the clientId provided in the hawk authentication -- we can put it anywhere (for example, in the cert).

In particular, I viscerally dislike the idea of having the signature cover data that is not included in ext.cert.  Modifying your proposal slightly:

### When clientId == issuer.clientId (cert.issuer === undefined)

clientId:  issuer.clientId
cert:          {version, seed, start, expiry, scopes, signature}
signature:
  HMAC(
    version:1\n
    seed:<seed>\n
    start:<start>\n
    expiry:<expiry>\n
    scopes:\n
    <scope1>\n
    ...\n
    <scopeN>
  , issuer.accessToken)
accessToken:   HMAC(<seed>, issuer.accessToken)


### When clientId != issuer.clientId (cert.issuer !== undefined)

clientId:  tempClientId
ext:
  issuer:        issuer.clientId
  cert:          {version, tempClientId, seed, start, expiry, scopes, signature}
signature:
  HMAC(
    version:1\n
    tempClientId:<tempclientId>\n
    seed:<seed>\n
    start:<start>\n
    expiry:<expiry>\n
    scopes:\n
    <scope1>\n
    ...\n
    <scopeN>
  , issuer.accessToken)
accessToken:   HMAC(<seed>, issuer.accessToken)

------

This uses the tempClientId as the clientId (so it's easily seen by humans), but also includes it in ext.cert and in the signature (maintaining the invariant that the signature covers the cert, no more and no less).  Finally, it includes, but does not sign, the issuer's clientId.

r?
I just landed the latest login changes, with jabba's help to update URLs in the Okta configuration.  While I was there Jabba also showed me how to set up https://sso.mozilla.com/taskcluster.  Right now that won't help you too much, since you'll dead-end on the login page.  But when login automatically redirects to tools, hitting that URL will get you to a full login with fresh temp credentials on tools.

I also put three PR's up for review to refactor auth/tc-lib-api/tc-lib-testing:

    https://github.com/taskcluster/taskcluster-auth/pull/39
    https://github.com/taskcluster/taskcluster-lib-api/pull/9
    https://github.com/taskcluster/taskcluster-lib-testing/pull/5

the end result is the ability to do a lot better testing of auth's handling of hawk credentials (which is now getting very complicated), at the cost of all other components needing to upgrade from mockAuthServer to fakeauth when they want to update their tc-lib-testing dependency.
Flags: needinfo?(jdow)
This is largely complete.  I'm migrating all of the old clientIds to the new form.  As always, the details are in the Gist, https://gist.github.com/djmitche/f59a481e0c2580d6cdfb
The old clientIds all expired about 20 hours ago.  I'll delete them soon.
Gone!  Done!
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Component: Login → Services
You need to log in before you can comment on or make changes to this bug.