Closed Bug 980478 Opened 6 years ago Closed 6 years ago

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

Categories

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

Firefox 30
All
Android
defect

Tracking

()

VERIFIED FIXED
Tracking Status
firefox29 --- fixed
firefox30 --- verified
fennec 29+ ---

People

(Reporter: aaronmt, Assigned: nalexander)

Details

(Keywords: reproducible, Whiteboard: [qa+])

Attachments

(3 files)

Attached file 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.
Whiteboard: [qa+]
/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?
Flags: needinfo?(nalexander)
No. It corrects for skew but nothing else.
Flags: needinfo?(nalexander)
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.
> 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
I concur with Chris's assessment above, the safest thing for the client to do is to omit the iat field entirely.
(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).
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Priority: -- → P1
Hardware: ARM → All
Attached file 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.
Attachment #8388864 - Flags: review?(rnewman)
Attachment #8388864 - Flags: review?(rnewman) → review+
verified fixed on android 2.3 with 3/11 nightly
This isn't on Nightly (m-c) yet.
not sure why my android 2.3 is working now... possible fix from another bug.
(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.
https://hg.mozilla.org/mozilla-central/rev/923bd540edde
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
With Nightly (03/12), I am not running into the originally reported issue anymore \(◕ ◡ ◕\)
Status: RESOLVED → VERIFIED
tracking-fennec: ? → 29+
Attached patch 923bd540eddeSplinter Review
[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.
Attachment #8391275 - Flags: approval-mozilla-aurora?
Attachment #8391275 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Android Background Services → Firefox for Android
You need to log in before you can comment on or make changes to this bug.