Hi Nick, These are great comments. We've started to make updates to the API to integrate your suggestions. I don't consider the changes to major, but there are some minor refactorings and naming changes. Brian will sent out updated documentation within a day.
Given that we need some time to implement these changes and you (and others) might have some further comments, you might want to hold off on implementation until things settle down. thanks, -chris On Jul 26, 2013, at 5:34 PM, Nick Alexander <[email protected]> wrote: > Hi folks, > > I was asked to comment on the first cut of the key server auth API at > > https://github.com/mozilla/picl-idp/blob/8a37f40ce8a0b8a0447ec28ec378258865e27c76/docs/api.md > > I apologize for the long and unfocused email, but I'd prefer to get > something out there and break out tickets over time rather than write > a better email. Plus, it's mfbt! > > These comments are also posted (with formatting) at > > https://gist.github.com/ncalexan/6093119 > > In general, I am pleased with how this is developing and expect I am > tabulating already recognized concerns. > > Best, > Nick > > # General comments about the API > > * Throughout we need to be explicit about which HTTP response codes > will be generated for which API calls. Returning a JSON error body > on non-200 is good; I'm a fan of 201 or 204 rather than 200 with an > empty JSON body {}. > > * Different endpoints are authenticated with different tokens. If > possible, I'd like the token to correspond to the top level of the > namespace. For example, `/account/recovery_methods` is > authenticated with `sessionToken` but `/account/keys` is > authenticated with `keyFetchToken` (and is, so far, the only > endpoint authenticated with `keyFetchToken`). Could we make that > `/keys/fetch`? > > * We need to bake a client backoff protocol into the first revision. > A 50x (with or without a header) and a 20x with a header has served > Sync well. > > * Many requests are identified only by the token. This shields > identifying information, but also might make it hard to rate-limit a > badly behaving client. Is it worth including the account identifier > more liberally? > > * Looking ahead to a pairing interface, can we fix a special value for > kA and wrap(kB) that signals that user-managed key (kC) is in play? > > # Specific comments > > ## POST /account/create > > `params.srp.N_bits` makes no sense. The client can't actually > change the SRP group by changing this parameter, so it's acting as a > name for a specific group. In general, I think fixing one group is > the right way to go. Changing groups suggests something is seriously > broken in our implementation or that the cryptographic landscape has > changed significantly; either warrants a protocol bump. > > ## GET /session/status > > This is confusing. What is the intended use? Reading between the > lines, this will return HTTP status code 20x or 401. But then the > client could just attempt whatever action it cares about and handle > 20x or 401. > > ## POST /session/auth/start > > It would be nice if the `srp` object included the same `params` > member as the `/account/create` request. Again, the return value > for `N_bits` is a clumsy identifier. > > I would like the returned JSON to include some indication of how long > the SRP token is good for. Since this isn't authoritative (but > instead provides a client a hint) a duration sounds best. > > ## GET /account/recovery\_methods > > This isn't yet an API. You can interrogate which recovery methods are > known, but it's not specified how to use that information in `POST > /account/recovery_methods/send_code`. If we really want to support > an arbitrary list of recovery methods, let's be explicit about how you > identify the endpoint/token/whatever in the `POST send_code` body. > > The example response is indicative but needs to be specified. What > happens if two methods are tagged as `primary`? We already have > `recoveryMethods` as a list; let's use the order when presenting to > the user. > > ## POST /account/recovery\_methods/verify\_code > > This is under-specified. What happens if the `code` is fake? This > isn't a 40x, since the POST should succeed. What if the code has expired? > > What happens if the `code` is real, but the user has verified their > account using a different method (Web portal?) before the code is > redeemed? > > Can I get confirmation that we expect users to do an email challenge > (in the style of the Persona fallback), at least at first, but that > this is intended for things like SMS account verification? This is > perhaps even more worrisome, because now anybody with a phone can > register and verify via SMS an email address they don't actually own. > I don't understand what the use cases are here. > > Another issue is that you can't *remove* verified recovery methods. > People leave email addresses behind, or change phone numbers, or... > But maybe this isn't an issue: I believe that most services get > compromised all at once; so first your device is stolen, and in turn > your email is compromised, leading to your device being used to reset. > > ## POST /account/reset > > We need to clarify the role of `kB`. The documentation for the > `bundle` parameter says: > > > bundle - a base16 string of encrypted kB|salt|verifier > > This defeats the purpose -- `kB` can never be returned to the server > in the clear. By comparison, the protocol document glossary says: > > > wrap(kB): an encrypted copy of kB. The keyserver stores wrap(kB) > > and never sees kB itself. > > ## POST /certificate/sign > > This is logically distinct from the keyserver API, but we need to > flesh this out immediately. As a first comment, a link to `jwcrypto` > and some vague mutterings about key parameters isn't helpful. Is > there any reason to not sign an arbitrary JSON blob? It's not like > the keyserver is going to be vetting key parameters. > > As a second comment, I'd like these signed certificates to -- in some > way -- include their intended domain. This could be as simple as the > client embedding a URL in the JSON blob, or the key server accepting a > Host-like header that is then embedded in the blob. The point being > that a valid certificate might be rejected by a particular storage > server for being too general. > > ## POST /password/forgot/send\_code > > The API needs to specify (and hopefully, reveal via returned JSON) > details that are covered in the protocol document, such as: > > forgotPasswordToken can be used three times before it is > > exhausted. If the user guesses incorrectly this often, the client must > > call send_code again to get a new token and code. Each account has at > > most one token+code active at a time. > > > > The recovery code is initially a random 8-digit decimal number. If an > > attacker tries to sign in as someone else, hits the "forgot my > > password" button, then submits a guess to > > /password/forgot/verify_code, they will have a 1-in-100-million chance > > of success. If the server detects too many wrong guesses, it should > > increase the length of new codes. > > Changing the code length *arbitrarily* makes it harder for the client > to provide great UI. 8, 12, 16 is probably more than enough choice. > Or maybe 8 (4x2), 15 (5x3). In any case, changing the code length > (without saying as much) makes it hard for the client to provide good > UI for entering the new code. (I'm thinking of an online reset via > SMS here.) > > Also, knowing how long a returned `forgotPasswordToken` is valid > for would help. And how many tokens can be outstanding a once? > > ## GET /account/devices > > This device API as a whole is not very clear: > > * I expect we want to manage this via a web interface; it's not > obvious that there's win providing a REST API for this. For > example, what can an authed device do for/to other devices? > > * At the moment, "Attach a new device" doesn't identify a device in > any way. > > * In the example listed, presumably "id" is in some way tied to the > device (so that wiping a device and starting again doesn't duplicate > device records in your account, a common complaint with Sync), and > therefore is very strongly user identifying. This needs to be > specified, and might need discussion with privacy and legal. > > # Nits > > * `SRP` is mis-abbreviated `SPR` throughout. > > * Be clear which numbers are encoded as strings. Sync suffered from > supporting timestamps as numbers and as strings. > > * Base16 encoding numbers makes sense but is not what I expected for > binary data (I expected Base64). > > * Some examples include an explicit "Host" header and some do not. > Let's be clear on what is needed. > > _______________________________________________ > Sync-dev mailing list > [email protected] > https://mail.mozilla.org/listinfo/sync-dev _______________________________________________ Sync-dev mailing list [email protected] https://mail.mozilla.org/listinfo/sync-dev
