983256 - Change the client generated expiration time in FxA assertions to be "forever"

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Chris Karlof [:ckarlof]]

Since expiration time is limited by the exp time in the signed certificate, having an expiration time in the client signed portion has limited value. If the client has high time skew and we don't correct for it, then we could have spurious authentication failures. Android, I believe set it to 1999999999999, which is a considerable time in the future. :) See Bug 980478.

Comment 1

[Chris Karlof [:ckarlof]]

Attachment: 0001-Bug-983256-Change-the-client-generated-expiration-ti.patch

Comment 2

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 8390709 [details] [diff] [review]
0001-Bug-983256-Change-the-client-generated-expiration-ti.patch

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

It's a hard problem.  This seems like a reasonable compromise to make things work reliably for all users.

::: services/fxaccounts/FxAccountsCommon.js
@@ +39,5 @@
> +this.ASSERTION_LIFETIME = 1000 * 3600 * 24 * 365 * 25; // 25 years
> +// This is a time period we want to guarantee that the assertion will be
> +// valid after we generate it (e.g., the signed cert won't expire in this
> +// period).
> +this.ASSERTION_USE_PERIOD = 1000 * 60 * 5; // 5 minutes

Have we hit the limit of our ability to correct for time skew?  Does this need to be the permanent solution, or can we try to improve on skew correction?  For the circumstances, though, this seems to me to be a reasonable trade-off.  But I'm curious, why stop at 25 years?  Why not maxfloat or something?

Comment 3

[Chris Karlof [:ckarlof]]

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #2) 
> Have we hit the limit of our ability to correct for time skew? 

No, but there is significant complexity in doing it right, and we decided to remove this risk for Fx29.

> Does this
> need to be the permanent solution, or can we try to improve on skew
> correction?

No, we're planning on measure skew errors on the server and revisit.

> For the circumstances, though, this seems to me to be a
> reasonable trade-off.  But I'm curious, why stop at 25 years?  Why not
> maxfloat or something?

I don't want to get too fancy. Too big and I risk hitting codepaths god-knows-where for dates that are rarely used.

Comment 4

[Chris Karlof [:ckarlof]]

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

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ecbac0e39fdd

Comment 6

[Chris Karlof [:ckarlof]]

Comment on attachment 8390709 [details] [diff] [review]
0001-Bug-983256-Change-the-client-generated-expiration-ti.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Time skew issues between clients and FxA servers
User impact if declined: some users with large time skew issues may not be able to log in to FxA Sync
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch:

Ideally this can get uplifted before 29 beta branch.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

The merge has been done already.

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8390709 [details] [diff] [review]
0001-Bug-983256-Change-the-client-generated-expiration-ti.patch

[Triage Comment]
I don't see any string or IDL changes and we're in early Beta so this looks safe to uplift now.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/86bdb244c648