Closed Bug 983256 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 + fixed
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: ckarlof, Assigned: ckarlof)

References

Details

Attachments

(1 file)

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.
Blocks: 969593
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?
Attachment #8390709 - Flags: review?(jparsons) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/ecbac0e39fdd
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
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.
Attachment #8390709 - Flags: approval-mozilla-aurora?
The merge has been done already.
Attachment #8390709 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
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.
Attachment #8390709 - Flags: approval-mozilla-beta+
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.