980478 - Sync fails to get token - invalid timestamp issues (TokenServerInvalidCredentialsException)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Android Sync, defect, P1)

Description

[Aaron Train [:aaronmt]]

Attachment: logcat.log

Splitting this off from bug 975625.

See my attached log where I reported this in that bug.

{"reason":"assertion issued later than verification date","status":"failure"}

https://bugzilla.mozilla.org/show_bug.cgi?id=975625#c19

> > {"reason":"assertion issued later than verification date","status":"failure"}
> 
> This is new and unusual, and not treated by any server-side mitigation.  It
> suggests that your network-time-synced device has its clock a tiny bit ahead
> of the verifier, and you're hitting this longstanding bug in jwcrypto:
> https://github.com/mozilla/jwcrypto/issues/58
> 
> Nick, does android apply the "issue things 10 seconds in the past" hack that
> we had to apply on the server to work around this issue?

and

> Aaron's issue is timestamp-related and could be partially mitigated by the
> server-side fixes, and maybe with the issue-in-the-past hack if it's not
> done already.

Comment 1

[Ryan Kelly [:rfkelly]]

/cc :ckarlof because this is pretty clearly an instance of the "Other Timestamp Issue" that we were hoping would not actually be an issue.  Chris: this is a mismatch between the client-provided timestamps in the assertion, and the timestamp on the remote verifier.  Specifically it seems to be hitting the "future timestamp cliff" from https://github.com/mozilla/jwcrypto/issues/58 but with the assertion iat time, not the cert iat time.

Nick, does android employ the "set iat to 10 seconds in the past" hack that other services apply to work around this issue?

Comment 2

[Richard Newman [:rnewman]]

No. It corrects for skew but nothing else.

Comment 3

[Ryan Kelly [:rfkelly]]

OK.  We should really fix the verifier so that this is not an issue.  But it may be worth adding the client-side mitigation here too.

Comment 4

[Chris Karlof [:ckarlof]]

> Chris: this is a mismatch between the client-provided timestamps in the assertion, 
> and the timestamp on the remote verifier.  Specifically it seems to be hitting the
> "future timestamp cliff" from https://github.com/mozilla/jwcrypto/issues/58 but with
> the assertion iat time, not the cert iat time.

Solution is easy then: remove the iat field from the assertion generated on the client. iat is not present in "legacy" assertions and are accepted by the verifier: http://lloyd.io/evolving-browserid-data-formats/

E.g, we don't include iat in assertions generated on Desktop: http://mxr.mozilla.org/mozilla-central/source/toolkit/identity/jwcrypto.jsm#159

There is still possibly an issue that one could generate assertions that are already expired because exp is not time skew adjusted to the server (i.e., in the past from the server perspective). Solution: exp: 99999999999999999999999

Comment 5

[Ryan Kelly [:rfkelly]]

I concur with Chris's assessment above, the safest thing for the client to do is to omit the iat field entirely.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Ryan Kelly [:rfkelly] from comment #5)
> I concur with Chris's assessment above, the safest thing for the client to
> do is to omit the iat field entirely.

OK, I'll implement the following:

iat: undefined (not present in the assertion);
exp: 99999999999999999999999 (actually, I'll figure out what is the smallest integer power of 10 greater than, say, 2050).

Comment 7

[Nick Alexander :nalexander [he/him]]

Attachment: github PR

This ended up being a lot of refactoring and a tiny patch.

This works in live tests and local sync testing; you'll get assertion payloads like

I FxAccounts(23009)           fennec_nalexander :: Married :: $$FxA PII$$: aPayload  : {"iss":"127.0.0.1","aud":"https:\/\/token.services.mozilla.com","exp":9999999999999}

(Observe no iat and exp way in the future.)  I can confirm that this avoids the "first sync after process start" failure.

I didn't tear out the token server skew tracking (even though we no longer use the tracked skew) since we may want it in future.  In fact, we might start to track skew on *every* request.

Comment 8

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/923bd540edde

Comment 9

[Edwin Wong [:edwong]]

verified fixed on android 2.3 with 3/11 nightly

Comment 10

[Aaron Train [:aaronmt]]

This isn't on Nightly (m-c) yet.

Comment 11

[Edwin Wong [:edwong]]

not sure why my android 2.3 is working now... possible fix from another bug.

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to Edwin Wong [:edwong] from comment #11)
> not sure why my android 2.3 is working now... possible fix from another bug.

Presumably the server-side fixes for invalid-timestamp errors are fixing you.  This is slightly different -- it avoids the rather rare case of invalid-certificate based on local timestamp errors.  Since we were already skew tracking the token server, these were not common.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/923bd540edde

Comment 14

[Aaron Train [:aaronmt]]

With Nightly (03/12), I am not running into the originally reported issue anymore \(◕ ◡ ◕\)

Comment 15

[Nick Alexander :nalexander [he/him]]

Attachment: 923bd540edde

[Approval Request Comment]
Bug caused by (feature/regressing bug #): initial FxA landing.

User impact if declined: terrible Sync experience if you have bad luck.

Testing completed (on m-c, etc.): couple days of Nightly; tested by AaronMT, fixes his issues.

Risk to taking this patch (and alternatives if risky): low, but it could "break everything" since it changes certain formats presented to the server.  In an extreme case, the server could recognize the new format and handle it specially; but the server folks gave the instructions, and it's working so far...

String or IDL/UUID changes made by this patch: none.

Comment 16

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e45532fe13ad