Skip to content
This repository has been archived by the owner. It is now read-only.

add gryphon pubkeys api #29

Closed
wants to merge 1 commit into from
Closed

add gryphon pubkeys api #29

wants to merge 1 commit into from

Conversation

@seanmonstar
Copy link
Member

@seanmonstar seanmonstar commented Apr 16, 2014

using gryphon request signing instead of bearer tokens

fixes #28

I'll be working on profile-server and 123done patches that use this endpoint, so that everything can continue to work together.

@seanmonstar
Copy link
Member Author

@seanmonstar seanmonstar commented Apr 21, 2014

@warner r?

unique per user, and you authorize it with use by passing a [code][] and
the corresponding public-key. This ends up being more secure in multiple
ways.

This comment has been minimized.

@warner

warner Apr 21, 2014

One way pubkeys get misunderstood is when folks think there should be exactly one key per human being. The long time it takes to generate a traditional RSA key, and the layers of PKI rigmarole around "signing keys" and "certificates", encourages this confusion. I'm wondering if the "This secret-key should be unique per user" phrase here might encourage that sort of confusion.

How about something like: ".. that only you know. Each secret key grants access to specific scopes for a single user account. There may be multiple active keys for each user, and each one is independently revocable at any time. You can authorize (empower) a secret-key by submitting the corresponding public-key and a code to the fxa-oauth-server." ?

This comment has been minimized.

@seanmonstar

seanmonstar Apr 21, 2014
Author Member

Ah good point. This could make people think 1-key-ever per user. I wanted to explain that a client should NOT generate just 1 keypair, and use that for all users/scopes. If we see an existing pubkey, we'll error.

@@ -6,7 +6,7 @@
const AppError = require('./error');
const config = require('./config');
const logger = require('./logging').getLogger('fxa.assertion');
const Promise = require('./promise');
const P = require('./promise');

This comment has been minimized.

@warner

warner Apr 21, 2014

nit: next time, might be nice to have these cleanup changes in a separate commit (same PR, but two commits), just to make review easier

This comment has been minimized.

@seanmonstar

seanmonstar Apr 21, 2014
Author Member

indeed. i fixed this because that commit was failing the tests on travis, but a second commit would have done that as well

@warner
Copy link

@warner warner commented Apr 21, 2014

Hm, I just talked with @ckarlof, and his opinion was that we should retain the token-based API so that we don't freak out marketplace and the other RPs that will be using this feature: get them comfortable using FxA at all, then switch them over to pubkeys (because deadlines). So based on that, I guess I'd ask you to rearrange this PR to add the new API points instead of replacing them.

@seanmonstar
Copy link
Member Author

@seanmonstar seanmonstar commented Apr 21, 2014

Hmm, that seems like more work to maintain, and means RPs will have to go through an upgrade process, as well as our attached services supporting both. It seems like the best time to make this switch is before anyone is using it.

@seanmonstar
Copy link
Member Author

@seanmonstar seanmonstar commented Apr 22, 2014

the work to support this in profile server and 123done has been done, and the PRs linked to this one.

@ckarlof
Copy link
Contributor

@ckarlof ckarlof commented Apr 22, 2014

we should retain the token-based API so that we don't freak out marketplace and the other RPs that will be using this feature

I feel that going with Gryphon alone has too many unknowns at this point in time. Although our Oauth flows in general are unproven at scale and in production, the fact that they are similar enough to Github's and Google's flows, I am confident that we are unlikely to encounter any insurmountable problems with the bearer token flows.

I would support having Gryphon as an experimental API extension, but I think the following deficiencies need to be addressed before it is a viable contender to the bearer token flow (meaning we should consider pushing Gryphon as the preferred approach and deprecating the bearer token approach):

  • A detailed "@warner" specification, including crypto details and diagrams
  • An analysis and comparison of Gryphon compared to bearer tokens in Oauth2.
  • The socialization and vetting of this approach. In particular, I'd love hear Eran Hammer's comments.
  • Gryphon client libraries for some server environments (Node, Python, PHP)
  • Gryphon JS client library suitable for a browser environment, including support for legacy browsers supported by our reliers (e.g., IE8)
  • Performance measurements for the browser JS library, particularly for legacy browsers
  • Experience with using Gryphon in production with at least one relier (e.g., an internal Mozilla Cloud Services service)

IMO, for us to not get the stink eye from potential reliers, this needs to be as (close to) easy and fast as using bearer tokens. The primary beneficiary of this approach is us, the API service provider and Oauth provider. I don't see any direct advantages to reliers (particularly third party reliers) in using this approach over bearer tokens. To best align the costs with the benefits (and thus maximizing the chances of its success), the costs to reliers should be negligible compared to bearer tokens.

@seanmonstar
Copy link
Member Author

@seanmonstar seanmonstar commented Apr 23, 2014

Ok, I concede.

One exception: I believe the primary beneficiary is the user, not us. It's more work for us to create something new. This adds additional security to the whole chain, protecting users. So, besides the costs being minimal (in my opinion), reliers will also be improving their users' security.

As for adjusting this PR: I'll keep both endpoints in. I can either keep the db tables separate, and or keep them all in one tokens table, since there is already a token_type column. The tokens column is keyed of the sha256 of a unique 32-byte string, the pubkeys are also unique 32-byte strings. Keeping the same table would mean hashing the pubkeys, which isn't really needed...

@warner
Copy link

@warner warner commented Apr 23, 2014

Yeah, I'd go with separate tables, since they'll be accessed by separate APIs.

I know token_type is in the OAuth spec, so it's probably best to leave it in, but I wonder if it's all that useful given that different token types probably mean differently-shaped tokens (and thus maybe different storage types for the token column). Depends upon how comfortable you are with polymorphic data, I guess. But yeah, we shouldn't hash things unnecessarily, just to make the code or schema factor out better.

I'll challenge one of @ckarlof's points: I'm not convinced that web-browsers-as-clients is such a universal use-case that we should block the gryphon approach on being able to run the client code comfortably in IE8. I don't yet have a clear idea of what people are going to do with this whole delegated-access thing, and while I can imagine web-content wanting to play client, I suspect there will be a lot of applications that only need server-to-server uses, in which case python or node.js gryphon client libraries would be sufficient. Getting any serious crypto to work within a web-page is a drag, and it'd be nice if that didn't prevent us from improving the security of server-to-server exercise of delegated authority. (I'd probably be ok with IE8 support blocking the removal of bearer-token-based APIs, but I'd like to encourage server-to-server -only applications to switch to gryphon even if we didn't get IE8 to work).

@ckarlof
Copy link
Contributor

@ckarlof ckarlof commented Apr 23, 2014

One exception: I believe the primary beneficiary is the user, not us.

Yes, I agree. These incentives are a tangled web. The user is the end beneficiary, but I'd it's ultimately our responsibility to protect our users, and gryphon would help make that easier.

@ckarlof
Copy link
Contributor

@ckarlof ckarlof commented Apr 23, 2014

I'm not convinced that web-browsers-as-clients is such a universal use-case that we should block the gryphon approach on being able to run the client code comfortably in IE8.

Point taken. We could totally punt on browser environments and initially target the server <-> server use case exclusively. I'd be fine with that. As I mentioned in my above comment, the list of requirements were intended to be prereqs for the removal of the bearer token API.

using gryphon request signing

fixes #28
@seanmonstar
Copy link
Member Author

@seanmonstar seanmonstar commented May 7, 2014

@warner adjust this PR to add a /pubkey endpoint, but keeping the /token endpoint around.

@seanmonstar
Copy link
Member Author

@seanmonstar seanmonstar commented May 7, 2014

Actually, I need some more work in gryphon and intel, as this branch is pointing at my github repos instead of npm.

@seanmonstar seanmonstar changed the title changes token usages to pubkeys add gryphon pubkeys api Jun 30, 2014
@ckarlof ckarlof added the label Jun 30, 2014
@rfk
Copy link
Member

@rfk rfk commented Mar 9, 2015

@seanmonstar are you interested in revisiting this in future, or should we just close it out?

@seanmonstar
Copy link
Member Author

@seanmonstar seanmonstar commented Mar 9, 2015

:(

@rfk
Copy link
Member

@rfk rfk commented Mar 9, 2015

I second your :-(

@vladikoff vladikoff deleted the pubkey branch Sep 21, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants