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
