Service Client Tokens #336

seanmonstar · 2015-09-11T23:35:52Z

This currently works, posting an assertion to /v1/token with the right properties in the token will give you an access token.

One potential blocker that has shown up implementing this:

Since the token is requested without an fxa-assertion, we cannot store the user's email address with the token, and therefore cannot fulfill requests to the profile server for /v1/email.

Closes #328
Closes #329

rfk · 2015-09-14T00:23:22Z

Since the token is requested without an fxa-assertion, we cannot store the user's email address
with the token, and therefore cannot fulfill requests to the profile server for /v1/email

That's going to get a bit messy, but TBH I'm glad this has bitten us so squarely because it needs to be refactored away sooner or later anyway. The right thing is for us to directly ask the auth-server for the email associated with the account, passing the OAuth bearer token along to authorize the request.

rfk · 2015-09-15T05:19:51Z

Since these clients are only in config, the foreign key restriction on access_tokens.clientId will prevent
inserting these tokens into the MySQL database.

Based on discussion today, ISTM that we either:

Create stub entries for the service clients in the clients table, but keep their special JWK stuff separate so that there's no risk of accidentally allowing other clients to act as service clients.
Just drop the foreign key constraint and be done with it.

I feel like the constraint has some value so I'd lean slghtly towards (1), but don't feel strongly. @seanmonstar thoughts?

seanmonstar · 2015-09-28T19:59:53Z

Woo! tests passed! As in... the missing email field won't blow up the mysql db. It still won't be able to respond with an email until mozilla/fxa-auth-server#915 is merged.

rfk · 2015-09-29T03:04:03Z

lib/db/mysql/schema.sql

@@ -41,7 +41,7 @@ CREATE TABLE IF NOT EXISTS tokens (
  FOREIGN KEY (clientId) REFERENCES clients(id) ON DELETE CASCADE,
  userId BINARY(16) NOT NULL,
  INDEX tokens_user_id(userId),
-  email VARCHAR(256) NOT NULL,
+  email VARCHAR(256),


I don't know about this intermediate state where some tokens can't fetch the email address...ISTM maybe we should go through with something like mozilla/fxa-auth-server#1053 and then we can just drop this column entirely.

In fact, I wonder if we can deprecate having the email returned implicitly by /verify, as it's a bit of an abstraction violation anyway...

Yea, that'd be ideal. However, I don't know my way around the auth server much, and to completely drop this column, the auth server would need that PR merged, or else ALL requests for email would no longer be serviceable.

I didn't want to block landing this based on that, but if it's possible to get that landed asap, then we could just drop all the email columns in the oauth db.

rfk · 2015-09-29T03:05:33Z

lib/routes/token.js

 const MAX_TTL_S = config.get('expiration.accessToken') / 1000;
 const GRANT_AUTHORIZATION_CODE = 'authorization_code';
 const GRANT_REFRESH_TOKEN = 'refresh_token';
+const GRANT_JWT = 'urn:ietf:params:oauth:grant-type:jwt-bearer';
+
+const JWT_AUD = config.get('publicUrl') + '/v1/token';


Is it usual to have audiences with a path component? I don't recall seeing this pattern used elsewhere. I guess it can't hurt though...

I don't recall seeing this pattern used elsewhere

It's used in both [1] and [2] so I guess I just haven't been paying attention. 👍

[1] https://developers.google.com/identity/protocols/OAuth2ServiceAccount
[2] http://self-issued.info/docs/draft-ietf-oauth-jwt-bearer.html

rfk · 2015-09-29T03:07:59Z

lib/routes/token.js

+        allowed: client.scope,
+        requested: payload.scope
+      });
+      throw AppError.invalidScopes();


Pass the scopes to the constructor, for client-visible error message.

rfk · 2015-09-29T03:08:33Z

lib/routes/token.js

+      logger.debug('jwt.invalid.iat', { now: now, iat: payload.iat });
+      throw AppError.invalidAssertion();
+    }
+    if ((payload.exp || -Infinity) < now) {


IMHO we should flatly refuse to accept JWTs without an explicit expiry timestamp.

This does that, without introducing another branch (which jscs likes to complain about complexity). If there's no payload.exp, then -Infinity is used, and -Infinity < now will always be true, such that this would be considered expired.

Ah yes, right you are

rfk · 2015-09-29T03:14:36Z

lib/routes/token.js

+    if ((payload.iat || Infinity) > now) {
+      logger.debug('jwt.invalid.iat', { now: now, iat: payload.iat });
+      throw AppError.invalidAssertion();
+    }


Clock skew has caused similar checks to fail surprisingly often. Since this is for server-to-server communication, we can assume reasonably well-synchronised clocks, but I think we should still add some small window for clock skew between the two systems.

Also, it's a bit strange that we don't get this expiration-time checking for free from fxa-jwtool. I wonder if there's an API we could add to that library to simplify some of the work here, e.g. a validate() method that we can pass in a timestamp and aud and have it enforce these checks for us. @dannycoates thoughts?

I've never understood the clock skew problem that well. I know what it is, it just seems that if there is an issue, just push expiration out another couple minutes or something?

It can also be a problem with iat. If the service-client's clock is slightly ahead of the oauth-server's clock, it can generate an assertion that appears to the oauth-server to be from the future, and hence will be rejected. In other situations we've worked around this by checking like if (iat || Infinity > now - 5) just to add a bit of leeway.

if (iat || Infinity > now - 5)

Re-reading this, it should be + 5 for "was issued more than five seconds in the future". I still think it's worth adding some small allowance for skew here, we have seen this cause problems in other systems in the past.

rfk · 2015-09-29T03:31:21Z

lib/routes/token.js

+  }).then(function(payload) {
+    logger.verbose('jwt.payload', payload);
+
+    var client = SERVICE_CLIENTS[payload.iss];


IIUC, the iss field here is the client's opaque hex id. I think it would be quite neat if the client didn't need to know this value, and could just put their domain-name in directly like iss: basket.mozilla.org, in a similar manner to what we do for identity assertions. One less piece of coupling between the two systems.

That might complicate the data model though.

That's probably doable.

Ok, here's how I'm doing this: I don't even check for a iss claim anymore.

The verification won't work if the assertion doesn't include a jku in the header that we don't already trust. The current design is that each client has only 1 jku, so I can look up the client based on the jku in the assertion. The only caveat: this requires that every client have a unique jku. No 2 clients can share one. This seems reasonable to me...

rfk · 2015-09-29T03:32:12Z

@seanmonstar by the way I found [1] while googling for the urn: string used here, is the API here based on that document, or just from the google API?

[1] http://self-issued.info/docs/draft-ietf-oauth-jwt-bearer.html

rfk · 2015-09-29T03:39:25Z

lib/routes/token.js

+    })
+    .then(function(vals) {
+      vals.ttl = params.ttl;
+      return vals;


This file's getting pretty long and growing a lot of concerns; I think it would benefit from a few high-level comments to assist new readers in navigating the file.

btw I never want to open this file again

rfk · 2015-09-29T03:42:41Z

lib/routes/token.js

+    if (!uid || !(uid.length === 32 && HEX_STRING.test(uid))) {
+      logger.debug('jwt.invalid.sub', uid);
+      throw AppError.invalidAssertion();
+    }


In #328 you proposed "id@accounts.firefox.com" for the sub field, but this just uses the raw uid. Any comments on which is superior and why?

(FWIW I'm generally in favour of using namespaced identifeirs, but there's no real ambiguity here so I think the plain id is fine as well. The only thing we'd do if you gave us an unexpected domain in the identifier would be return an error)

I don't think 1 is really superior to the other. I think I proposed that because I was looking at the existing browserid module used with fxa's browserid assertions, and then proceeded to forget about when writing the code.

rfk · 2015-09-29T03:44:37Z

@seanmonstar this is looking good. I think we should also have #329 ready before merging though, either as part of this PR or separately, since it will make it easier to think through the shape of the feature as a whole.

rfk · 2015-10-01T04:56:39Z

docs/service-clients.md

+var header = {
+  alg: 'RS256',
+  typ: 'JWT',
+  jku: 'https://basket.accounts.firefox.com/.well-known/jku',


nit: may as well use the real basket domain name of basket.mozilla.org

rfk · 2015-10-01T05:04:44Z

lib/db/index.js

+        return exports.registerClient({
+          id: client.id,
+          name: client.name,
+          hashedSecret: encrypt.hash(unique.secret()),


In other places we've used "0000000000000000000000000000000000000000000000000000000000000000" as the hashedSecret for clients that aren't supposed to use a secret. Is that a pattern we should repeat for these service clients for consistency?

No, that pattern was actually a poor idea, and I clarified in bugs that
those should NOT be the hashes for those clients (cause something can
indeed hash to 0), but that the clients should have random hashes that were
thrown away afterwards.

However, this inserts into the db at startup, which @jrgm may object too.

On Wed, Sep 30, 2015, 10:04 PM Ryan Kelly notifications@github.com wrote:

In lib/db/index.js
#336 (comment)
:

+function serviceClients() {

var clients = config.get('serviceClients');

if (clients && clients.length) {

logger.debug('serviceClients.loading', clients);

return P.all(clients.map(function(client) {

return exports.getClient(client.id).then(function(existing) {

if (existing) {

logger.verbose('seviceClients.existing', client);

return;

}

return exports.registerClient({

id: client.id,

name: client.name,

hashedSecret: encrypt.hash(unique.secret()),

In other places we've used
"0000000000000000000000000000000000000000000000000000000000000000" as the
hashedSecret for clients that aren't supposed to use a secret. Is that a
pattern we should repeat for these service clients for consistency?

—
Reply to this email directly or view it on GitHub
https://github.com/mozilla/fxa-oauth-server/pull/336/files#r40879361.

rfk · 2015-10-01T05:10:29Z

docs/service-clients.md

+  aud: 'https://oauth.accounts.firefox.com/v1/token',
+  iat: now,
+  exp: now + (60 * 5),
+  sub: userId + '@accounts.firefox.com'


nit: I think this will have to be "@api.accounts.firefox.com" in production, which is pretty ugly, but it's what we have...

rfk · 2015-10-01T05:16:55Z

lib/routes/token.js

+//   - An options object is passed to `generateTokens()`.
+//     - An access_token is generated.
+//     - If req.payload.access_type = 'offline', a refresh_token is also
+//       generated.


It sounds like this will allow all grant types to generate refresh tokens. I wonder if we should restrict it to only authorization_code clients. Service clients have no legitimate use for a refresh token, since they can generate access tokens on demand, and having them able to generate refresh tokens could make it harder to e.g. thoroughly revoke service client access.

Hm, woops. This line is inaccurate. You cannot pass access_type to this route. I'll adjust this line to point out that a refresh token is generated if the authorization_code was created with offline access.

rfk · 2015-10-01T05:22:25Z

lib/routes/token.js

+  }
+
+  return uid;
+}


After sleeping on it, I lean towards allowing raw FxA uids here rather than requiring them to be namespaced. It's the form we've been encouraging clients and reliers to use in other places.

It has been? Where's an example? I settled on using namespaces, cause they don't hurt, and it's backwards compatible to eventually drop the namespace, but potentially confusing if we need to add one down the road.

It's what we tell reliers to store in their db as the account primary key, for example. Basket and pocket both store it as "" rather than "@api.accounts.firefox.com".

Ok, I'll rip the namespaces back out :)

The other alternative it so accept both forms, but we should probably just keep it simple :-)

rfk · 2015-10-01T05:29:04Z

However, this inserts into the db at startup, which @jrgm may object too.

I'm going to kick this over to @jrgm for comment on that one. It seems to me that a reasonable compromise would be to auto-create any clients that do not exist in the database, but not attempt to modify any existing client records in the db. (instead logging a warning for manual review).

seanmonstar · 2015-10-01T17:03:07Z

but not attempt to modify any existing client records in the db. (instead logging a warning for manual review).

Current code looks up via the client id in the config, and if found, continues the loop (not inserting). It will never edit an exist record. This will happen at every process startup, so logging a warning would happen every time after the first, and weaken the strength of warnings. Also, there shouldn't be anything to manually edit, since the only reason for it to be in the database is for the foreign key restriction. Any changes we might need to make would be in the config (changing the scope or jku).

rfk · 2015-10-09T00:14:53Z

This will happen at every process startup, so logging a warning would happen
every time after the first, and weaken the strength of warnings

I meant log a warning iff the config and the db differ, but yeah, if there's nothing really in the db to edit anyway then it's a bit of a waste of time.

Closes #328

seanmonstar · 2015-10-13T22:39:59Z

@rfk updated to use plain userids, instead of a namespace.

rfk · 2015-10-14T04:37:04Z

We chatted with @jrgm briefly today and got tentative support for the "insert but don't modify" approach to handling these clients on startup; assigning back to myself for final review

rfk · 2015-10-16T10:17:55Z

docs/api.md

    - `code`: A string that was received from the [authorization][] endpoint.
  - If `refresh_token`:
+     - `client_id`: The id returned from client registration.


nit: extra indentation here

rfk · 2015-10-16T10:47:52Z

r+ with a couple of final nits; we'll need a follow-up bug for using mozilla/fxa-auth-server#1070 once it's landed, but assuming the MODIFY COLUMN is not a painful migration, we can land this chunk of work in the meantime.

Closes #329

seanmonstar · 2015-10-16T19:27:29Z

The failure is because a test in db/mysql is calling process.exit(1), and that seems to be intercepted correctly on node 0.10, but not 0.12?

Service Client Tokens

seanmonstar added waffle:in review and removed waffle:in review labels Sep 11, 2015

rfk mentioned this pull request Sep 14, 2015

WIP feat(auth): allow backend server access to our API via oauth tokens mozilla/fxa-auth-server#915

Closed

rfk added this to the FxA-31: service tokens milestone Sep 14, 2015

rfk assigned seanmonstar Sep 14, 2015

rfk mentioned this pull request Sep 15, 2015

Provide oauth-token-authenticated access to account profile data mozilla/fxa-auth-server#1053

Closed

seanmonstar force-pushed the 328-service-client-tokens branch 3 times, most recently from e61f73e to 0529a36 Sep 28, 2015

seanmonstar changed the title ~~[wip] Service Client Tokens~~ Service Client Tokens Sep 28, 2015

seanmonstar assigned rfk and unassigned seanmonstar Sep 28, 2015

seanmonstar added waffle:review and removed waffle:progress labels Sep 28, 2015

rfk reviewed Sep 29, 2015
View changes

lib/routes/token.js

allowed: client.scope,

requested: payload.scope

});

throw AppError.invalidScopes();

This comment has been minimized.

Sign in to view

rfk Sep 29, 2015
Member

Pass the scopes to the constructor, for client-visible error message.

rfk reviewed Sep 29, 2015
View changes

rfk assigned seanmonstar and unassigned rfk Sep 29, 2015

seanmonstar assigned rfk and unassigned seanmonstar Oct 1, 2015

rfk reviewed Oct 1, 2015
View changes

docs/service-clients.md

var header = {

alg: 'RS256',

typ: 'JWT',

jku: 'https://basket.accounts.firefox.com/.well-known/jku',

This comment has been minimized.

Sign in to view

rfk Oct 1, 2015
Member

nit: may as well use the real basket domain name of basket.mozilla.org

rfk reviewed Oct 1, 2015
View changes

rfk assigned jrgm and unassigned rfk Oct 1, 2015

seanmonstar force-pushed the 328-service-client-tokens branch from 1815128 to 763e261 Oct 1, 2015

feat(tokens): allow using JWT grants from Service Clients

0a0e303

Closes #328

seanmonstar force-pushed the 328-service-client-tokens branch from 763e261 to 84a2661 Oct 13, 2015

rfk assigned rfk and unassigned jrgm Oct 14, 2015

rfk reviewed Oct 16, 2015
View changes

docs/api.md

- `code`: A string that was received from the [authorization][] endpoint.

- If `refresh_token`:

- `client_id`: The id returned from client registration.

This comment has been minimized.

Sign in to view

rfk Oct 16, 2015
Member

nit: extra indentation here

rfk mentioned this pull request Oct 16, 2015

chore(clients): remove deprecated 'whitelisted' column from clients table #344

Merged

docs(service-clients): Document Service Clients, JKUs, and JWTs

799f0e2

Closes #329

seanmonstar force-pushed the 328-service-client-tokens branch from 84a2661 to 799f0e2 Oct 16, 2015

seanmonstar added a commit that referenced this pull request Oct 16, 2015

Merge pull request #336 from mozilla/328-service-client-tokens

f16fac4

Service Client Tokens

seanmonstar merged commit f16fac4 into master Oct 16, 2015
0 of 2 checks passed

0 of 2 checks passed

continuous-integration/travis-ci/push The Travis CI build failed
Details

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

seanmonstar removed the waffle:review label Oct 16, 2015

seanmonstar deleted the 328-service-client-tokens branch Oct 16, 2015

rfk mentioned this pull request Oct 28, 2015

Backout the "service tokens" feature #351

Merged

mozilla / fxa-oauth-server Archived

Service Client Tokens #336

Service Client Tokens #336

Conversation

seanmonstar commented Sep 11, 2015

rfk commented Sep 14, 2015

rfk commented Sep 15, 2015

seanmonstar commented Sep 28, 2015

This comment has been minimized.

rfk Sep 29, 2015 Member

This comment has been minimized.

seanmonstar Sep 29, 2015 Author Member

This comment has been minimized.

rfk Sep 29, 2015 Member

This comment has been minimized.

rfk Sep 29, 2015 Member

This comment has been minimized.

rfk Sep 29, 2015 Member

This comment has been minimized.

rfk Sep 29, 2015 Member

This comment has been minimized.

seanmonstar Sep 29, 2015 Author Member

This comment has been minimized.

rfk Sep 30, 2015 Member

This comment has been minimized.

rfk Sep 29, 2015 Member

This comment has been minimized.

rfk Sep 29, 2015 Member

This comment has been minimized.

seanmonstar Sep 29, 2015 Author Member

This comment has been minimized.

rfk Oct 1, 2015 Member

This comment has been minimized.

rfk Oct 16, 2015 Member

This comment has been minimized.

rfk Sep 29, 2015 Member

This comment has been minimized.

seanmonstar Sep 29, 2015 Author Member

This comment has been minimized.

seanmonstar Sep 30, 2015 Author Member

rfk commented Sep 29, 2015

This comment has been minimized.

rfk Sep 29, 2015 Member

This comment has been minimized.

seanmonstar Sep 29, 2015 Author Member

This comment has been minimized.

rfk Sep 29, 2015 Member

This comment has been minimized.

seanmonstar Sep 29, 2015 Author Member

rfk commented Sep 29, 2015

This comment has been minimized.

rfk Oct 1, 2015 Member

This comment has been minimized.

rfk Oct 1, 2015 Member

This comment has been minimized.

seanmonstar Oct 1, 2015 Author Member

This comment has been minimized.

rfk Oct 1, 2015 Member

This comment has been minimized.

rfk Oct 1, 2015 Member

This comment has been minimized.

seanmonstar Oct 1, 2015 Author Member

This comment has been minimized.

rfk Oct 1, 2015 Member

This comment has been minimized.

seanmonstar Oct 1, 2015 Author Member

This comment has been minimized.

rfk Oct 9, 2015 Member

This comment has been minimized.

seanmonstar Oct 10, 2015 Author Member

This comment has been minimized.

rfk Oct 11, 2015 Member

rfk commented Oct 1, 2015

seanmonstar commented Oct 1, 2015

rfk commented Oct 9, 2015

seanmonstar commented Oct 13, 2015

rfk commented Oct 14, 2015

This comment has been minimized.

rfk Oct 16, 2015 Member

rfk commented Oct 16, 2015

rfk Sep 29, 2015
Member

seanmonstar Sep 29, 2015
Author Member

rfk Sep 29, 2015
Member

rfk Sep 29, 2015
Member

rfk Sep 29, 2015
Member

rfk Sep 29, 2015
Member

seanmonstar Sep 29, 2015
Author Member

rfk Sep 30, 2015
Member

rfk Sep 29, 2015
Member

rfk Sep 29, 2015
Member

seanmonstar Sep 29, 2015
Author Member

rfk Oct 1, 2015
Member

rfk Oct 16, 2015
Member

rfk Sep 29, 2015
Member

seanmonstar Sep 29, 2015
Author Member

seanmonstar Sep 30, 2015
Author Member

rfk Sep 29, 2015
Member

seanmonstar Sep 29, 2015
Author Member

rfk Sep 29, 2015
Member

seanmonstar Sep 29, 2015
Author Member

rfk Oct 1, 2015
Member

rfk Oct 1, 2015
Member

seanmonstar Oct 1, 2015
Author Member

rfk Oct 1, 2015
Member

rfk Oct 1, 2015
Member

seanmonstar Oct 1, 2015
Author Member

rfk Oct 1, 2015
Member

seanmonstar Oct 1, 2015
Author Member

rfk Oct 9, 2015
Member

seanmonstar Oct 10, 2015
Author Member

rfk Oct 11, 2015
Member

rfk Oct 16, 2015
Member