959441 - Add tokenserver support for changing uid when the sync key changes

Status: VERIFIED FIXED

(Cloud Services Graveyard :: Server: Token, defect)

Description

[Ryan Kelly [:rfkelly]]

Attachment: tokenserver-appconfig.diff

In an attempt to prevent ugly edge-cases and race-conditions in the FxA+sync system, we plan for the tokenserver to assign a new userid whenever a user's sync encryption key changes.  This ensures that devices using the new key will sync in a separate, newly-allocated bucket, where devices still using the old key cannot clobber their data.

Here's my attempt to cast that into the tokenserver API in a mildly-generic way.

Each token request takes an optional "appConfig" field, which can be an application-specific opaque string identifing a particular device configuration.  The tokenserver will allocate a new uid (and hence new endpoint_url) for each unique (identity, appConfig) pair.

Thoughts:

  * Bikeshedding welcome on the name, I had trouble coming up with something generic and descriptive and I really didn't want to call it "syncKeyHash".

  * I morphed the token request from a GET into a POST, since we're now submitting multiple pieces of data.  Alternately we could submit an "X-App-Config" header on the GET request for slightly less churn, but that just feels ick to me.

    * Hmmm...actually we should normalize that with the handling of X-Conditions-Accepted, either both in a POST body or both as headers...bleh.

  * Should we commit to keeping allocations for old values of appConfig?  Should we explicitly declare that only the most recently-submitted on is kept?

@ckarlof does this match your expectations?

@telliott r? - although frankly  I think we need to tighten it up a bit before you sign off on it.

Comment 1

[Richard Newman [:rnewman]]

(In reply to Ryan Kelly [:rfkelly] from comment #0)

>   * Bikeshedding welcome on the name, I had trouble coming up with something
> generic and descriptive and I really didn't want to call it "syncKeyHash".

The two things dodgy about appConfig are (a) it's not really a config, and (b) it's not scoped to an app :P

Fingerprint? App-print?
Invariant? (If it varies, you get a new UID.)
Client state?
User state?
State identifier?


>   * I morphed the token request from a GET into a POST, since we're now
> submitting multiple pieces of data.  Alternately we could submit an
> "X-App-Config" header on the GET request for slightly less churn, but that
> just feels ick to me.

POST sounds sane to me.


>   * Should we commit to keeping allocations for old values of appConfig? 
> Should we explicitly declare that only the most recently-submitted on is
> kept?

I could see three perspectives here.

One: just because another client is ready to switch, doesn't mean I am. I still want access to the old UID/server for some period of time. Let me switch over when (a) some other mechanism tells me that the appConfig is going to change (password reset), and (b) I'm ready to do so.

An example of something this supports: I can look at the old server to figure out how many client records I should be expecting to arrive on the new server, or to peek at meta/global to figure out which engines should be enabled.

Two: keeping old allocations around is pointless. At least for Sync, all of our clients will do a fresh sync immediately, forgetting about the old server.

Three: if you're really thinking of the token server as being generalizable, I could definitely imagine clients that want to (ab)use appConfig to get multiple concurrent assignments.

So if it's cheap, I'd say keep the old ones around for some period of time. If it's not, then don't worry about it. And if you explicitly don't want to support multiple assignments, then don't make this possible.

Comment 2

[Ryan Kelly [:rfkelly]]

> The two things dodgy about appConfig are (a) it's not really a config,
> and (b) it's not scoped to an app :P

Heh.  I also considered "extraUserData" as a nice explicit name.  "clientState" or "clientStateID" or something aren't bad either.

> One: just because another client is ready to switch, doesn't mean I am. I still want access to the
> old UID/server for some period of time. Let me switch over when (a) some other mechanism tells me
> that the appConfig is going to change (password reset), and (b) I'm ready to do so.

This is definitely the sanest semantic from a client perspective.  I guess it's safe to have an app-specific ttl/eviction policy for unused records.  After all, we'realready not guaranteeing that you'll get the same endpoint_url back each time, due to node-reassignment etc.

Comment 3

[Toby Elliott [:telliott]]

I vote for "fingerprint". It's descriptive and doesn't use any of those words ("app"!) that have lost all meaning.

Comment 4

[Toby Elliott [:telliott]]

(In reply to Ryan Kelly [:rfkelly] from comment #0)
>   * I morphed the token request from a GET into a POST, since we're now
> submitting multiple pieces of data.  Alternately we could submit an
> "X-App-Config" header on the GET request for slightly less churn, but that
> just feels ick to me.

I don't care if we use GET or POST (the main question is whether this should be considered cachable!). However, I actually like an (optional) X-Fingerprint header. That lets us ignore it if the service isn't making use of one.

>   * Should we commit to keeping allocations for old values of appConfig? 
> Should we explicitly declare that only the most recently-submitted on is
> kept?

From an external standpoint, only the most recently-submitted (valid) one is kept. Internally, we may need to store the old ones for post-processing once all tokens associated with it have expired.



> 
> @ckarlof does this match your expectations?
> 
> @telliott r? - although frankly  I think we need to tighten it up a bit
> before you sign off on it.

Comment 5

[Chris Karlof [:ckarlof]]

(In reply to Toby Elliott [:telliott] from comment #3)
> I vote for "fingerprint". It's descriptive and doesn't use any of those
> words ("app"!) that have lost all meaning.

I'm fine with "fingerprint".

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Chris Karlof [:ckarlof] from comment #5)
> (In reply to Toby Elliott [:telliott] from comment #3)
> > I vote for "fingerprint". It's descriptive and doesn't use any of those
> > words ("app"!) that have lost all meaning.
> 
> I'm fine with "fingerprint".

Fingerprint means "unique identifier" to the populace and has attachments to crime, criminality, and the overseers.  We're going to have a hard time explaining that this fingerprint does not in fact do what fingerprints do in real life.  I'd prefer "Cthulu name tag".

Comment 7

[Chris Karlof [:ckarlof]]

(In reply to Toby Elliott [:telliott] from comment #4)
> (In reply to Ryan Kelly [:rfkelly] from comment #0)
> >   * Should we commit to keeping allocations for old values of appConfig? 
> > Should we explicitly declare that only the most recently-submitted on is
> > kept?
> 
> From an external standpoint, only the most recently-submitted (valid) one is
> kept. Internally, we may need to store the old ones for post-processing once
> all tokens associated with it have expired.

If a client shows up with an older fingerprint, this client should get an error that signals it should re-login to FxA. I recall it came up yesterday that we need a way to figure out what's an "older" fingerprint without the token server needing to remember all previous fingerprints. Our proposal addressed this with generations in the FxA assertions. I'll arrange to have the generation information returned by the remote verifier. 

-chris

Comment 8

[Ryan Kelly [:rfkelly]]

>> I vote for "fingerprint".
> I'd prefer "Cthulu name tag".

Or we just call it "keyHash" and stop pretending it's some generic functionality...

Comment 9

[Ryan Kelly [:rfkelly]]

Attachment: tokenserver-client-state.diff

Updated based on IRL feedback from Toby.  It's now "X-Client-State-Hash" and it's submitted as a header on the original GET request.  Please resist further bikeshedding unless this name is completely unacceptable to you :-)

There's a new error state for when you submit an old version of this hash.  

Internally, I think we should track the old versions using timestamps rather than generation numbers.  If we succeed in ressurecting JPAKE as an advanced option, we may also see client-state-hash changes without a corresponding generation change.  I suspect it will also make GC a bit easier, since we can deal with batches of "all obsolete records between time X and Y" rather than caring about per-user generation numbers.

Comment 10

[Toby Elliott [:telliott]]

Comment on attachment 8360134 [details] [diff] [review]
tokenserver-client-state.diff

Review of attachment 8360134 [details] [diff] [review]:
-----------------------------------------------------------------

Maybe just X-Client-State and define it as a hash? I don't really care, but that's a long header.

Comment 11

[Ryan Kelly [:rfkelly]]

Docs committed as X-Client-State, lock it in: https://github.com/mozilla-services/docs/commit/7cd27c82eab168734c6b2f25b12d7901ce4263f7

Comment 12

[Richard Newman [:rnewman]]

Do we already have client bugs?

Comment 13

[Ryan Kelly [:rfkelly]]

> Do we already have client bugs?

I don't think so; feel free to create them, otherwise I'll get around to it later today.

Comment 14

[Ryan Kelly [:rfkelly]]

This is done, and should go out to dev servers this week (hopefully prod as well!).  Clients are free to start sending the header before the changes roll out, it will simply be ignored.

Comment 15

[James Bonacci [:jbonacci]]

:rfkelly - how about Stage?
Or will this only get pushed to Dev and Production?

Comment 16

[Ryan Kelly [:rfkelly]]

it'll hit stage as well, at some point - just not sure what the status of TS stage/prod envs are right now, but sure they're not far away

Comment 17

[James Bonacci [:jbonacci]]

OK. For bugs like this, I wonder if we need to keep it Resolved/Fixed until all envs are updated, or do we create deployment tickets in [puppet-config] for each env, or ... ?

Comment 18

[Ryan Kelly [:rfkelly]]

I think we can do similar to what sync has done in the past, close out the bugs as they land on master and then create "get this deployed" tickets as we need to push them out.

Comment 19

[James Bonacci [:jbonacci]]

OK. Got it.

Comment 20

[James Bonacci [:jbonacci]]

(from nalexander):
@rfk Way after the fact, but it looks like you deleted a line you didn't intend to. Also, these docs are not easy to find on docs.services, and in fact don't appear to be what's actually presented at https://docs.services.mozilla.com/token/apis.html.

https://github.com/mozilla-services/docs/commit/7cd27c82eab168734c6b2f25b12d7901ce4263f7#commitcomment-5188813