1182740 - Firefox may generate invalid assertions.

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, defect, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

I've managed to reproduce desktop generating invalid assertions due to a happy confluence of unrelated bugs :)  This happens when multiple calls to getAssertion are in-flight at the same time - we may end up using a mismatched keypair and certificate.

I've instrumented my build to prove this and can see it happening, and I've outlined a theoretical failure scenario below - I'm not sure if that scenario is *exactly* what I reproduced, but it is close.

The issue stems from the fact that we persist the keypair and the certificate at different times, so this can get racey. The solution is probably fairly simple - only persist the certificate and keyPair together (ie, consider them atomic). This would still leave the potential for races, but at least whoeever wins the race would write consistent data.

Another thing we could do in addition is to try and avoid getAssertion calls racing by reusing the same promise when one is already in flight. The assertion itself also has a validity lifetime, so we could consider caching the last assertion we generated and reusing it while it is valid - but these are all optimizations rather than the correctness issue outlined above re storage.

Theoretical scenario:

    How getAssertion is implemented in desktop:
     
    getKeypair() {
        yield getStorage()
        if (storage.keyPair) {
            return storage.keyPair;
        }
        let keypair = yield generateKeyPair();
        yield setStorage(keypair=keypair, cert = null);
        return keypair;
    }
     
    getCert(keypair) {
        yield getStorage()
        if (storage.cert) {
            return storage.cert;
        }
        let cert = yield getCertificateSigned(sessionToken, keypair);
        yield setStorage(cert=cert);
        return cert;
    }
     
    getAssertion() {
        yield getStorage();
        let kp = yield getKeypair()
        let cert = yield getCert(kp);
        return getAssertionFromCert(keypair, cert);
    }
     
    Possible failure scenario:
     
    promise1:
    * enter getAssertion, enter getKeyPair, not found in storage so calls generateKeyPair().
    promise2:
    * (same) - enter getAssertion, enter getKeyPair, not found in storage so calls generateKeyPair().
    promise1:
    * saves keypair1
    promise2:
    * saves keypair2
    * saves cert2 (built from kp2) (ie, somehow this promise "skipped ahead" of promise 1.)
    promise1:
    * saves cert1 (built from kp1)
     
    Now in a state where we have cert1 and keypair2 in the storage, getAssertion generates an invalid one.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0005-Bug-1182740-write-keypair-and-certificate-to-the-sto.patch

I think we need to do something like this. It seems to work, but tests will need updating.

Comment 2

[Chris Karlof [:ckarlof]]

Towards the goal of add other potential reviewers of FxA Desktop patches, I'm delegating stomlinson to "get off his bottom" and review this one. :)

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8632406 [details] [diff] [review]
0005-Bug-1182740-write-keypair-and-certificate-to-the-sto.patch

So this patch is still wrong :( The problem is that a keypair and a cert must be treated as a pair (ie, if a new keypair is generated we *must* generate a new cert.) This still isn't guaranteed in this patch.

Specifically, in my test profile I have the pref services.sync.debug.ignoreCachedAuthCredentials set to true - so a new keyPair is *always* generated. Then we enter getCertificate and it is found in the store - even though it was generated with a different keypair.

I'm starting to wonder if it is worth storing the keypair and cert at all, and just cache the assertion?

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0003-Bug-1182740-treat-keypair-and-certificate-as-an-atom.patch

This patch makes the keypair and certificate handling live in a single function so it's easier to guarantee that they are treated as a pair. The fact there are multiple lifetimes for each of these complicates things a little, but not so bad I felt it worth dropping that caching.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0003-Bug-1182740-treat-keypair-and-certificate-as-an-atom.patch

I tidied up the tests.

Comment 6

[Chris Karlof [:ckarlof]]

> I'm starting to wonder if it is worth storing the keypair and cert at all, and just cache the assertion?

The actual assertions are encoded with the domain of the target, so caching those doesn't make too much sense in a world of multiple services.

Comment 7

[Shane Tomlinson [:stomlinson]]

Mark, which Firefox branch/repo does the latest patch target? I cannot cleanly merge the patch against any of central, aurora or beta.

Comment 8

[Shane Tomlinson [:stomlinson]]

Comment on attachment 8633406 [details] [diff] [review]
0003-Bug-1182740-treat-keypair-and-certificate-as-an-atom.patch

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

Mark, which Firefox branch/repo does this target? I cannot cleanly merge the patch against any of central, aurora, beta, or fx-team.

I cannot see any race conditions w.r.t. persisting the certificate/keyPair that could exist since both are written at once. From this perspective, this seems like a huge technical improvement. I noted the double "dereference" of "keyPair" and "cert", though that convention existed previous to this patch. The one area I think could use improved is how "currentState" is accessed in two functions, "_generateAssertion" and "getKeypairAndCertificate". I think future maintainers would find the code slightly easier to understand if "currentState" accesses were constrained to a single function and "getKeypairAndCertificate" was side-effect free.

::: services/fxaccounts/FxAccounts.jsm
@@ +523,5 @@
> +        // should be impossible - make log noise about it.
> +        log.error("getAssertion called without a session token!");
> +        return null;
> +      }
> +      return this.getKeypairAndCertificate(currentState, data).then(

Is there any reason why |currentState| and |data| need to be passed in separately when |data| is fetched from |currentState|? Is this for testing? Essentially, I'm wondering if likes 512-526 could be moved inside |getKeypariAndCertificate|, or as an alternative, only pass in |data|, get the |keyPair| and |certificate| when the promise resolves and move line 870 to within this |then|. It seems like working with |currentState| would at least be contained to a single function.

@@ +819,5 @@
>      // TODO: get the lifetime from the cert's .exp field
> +    if (keyPairValid && certValid) {
> +      log.debug("getKeypairAndCertificate: already have keyPair and certificate");
> +      return {
> +        keyPair: accountData.keyPair.keyPair,

I see the logic has not changed from the old code here, but why are both keyPair and cert "derefrenced" twice, e.g., |accountData.keyPair.keyPair|

@@ +820,5 @@
> +    if (keyPairValid && certValid) {
> +      log.debug("getKeypairAndCertificate: already have keyPair and certificate");
> +      return {
> +        keyPair: accountData.keyPair.keyPair,
> +        certificate: accountData.cert.cert

Perhaps it's beyond the scope of this patch, but it seems like the code would be simpler/easier to read if the |validUntil| field were part of the wrapped |cert| or |keyPair|, and the double dereference eliminated.

@@ +863,1 @@
>        let toUpdate = {

IIUC, this branch is the goal of the patch, save both the keyPair and cert to the user's account data atomically to avoid strange race conditions between two concurrent assertion requests.

::: services/fxaccounts/tests/xpcshell/test_accounts.js
@@ +170,5 @@
> + * Some tests want a "real" fxa instance - however, we still mock the storage
> + * to keep the tests fast on b2g.
> + */
> +function MakeFxAccounts(internal = {}) {
> +  if (!internal.newAccountState) {

Good cleanup.

@@ +210,5 @@
>    run_next_test();
>  });
>  
>  add_task(function test_get_signed_in_user_initially_unset() {
> +  let account = MakeFxAccounts();

Doe this task need a |_(...)| statement like the other tasks?

@@ +257,2 @@
>      sessionToken: "dead",
> +    verified: true,

Do you want the trailing ,?

@@ +295,5 @@
> +
> +  let {keyPair, certificate} = yield fxa.internal.getKeypairAndCertificate(fxa.currentAccountState, credentials);
> +  // should have the same keypair and cert.
> +  do_check_eq(keyPair, credentials.keyPair.keyPair);
> +  do_check_eq(certificate, credentials.cert.cert);

Does this task need a |yield fxa.signOut| like the two following tasks? My guess is no since the other tasks call |fxa.setSignedInUser|.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Tomlinson [:stomlinson] from comment #8)
> Comment on attachment 8633406 [details] [diff] [review]
> 0003-Bug-1182740-treat-keypair-and-certificate-as-an-atom.patch
> 
> Review of attachment 8633406 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mark, which Firefox branch/repo does this target? I cannot cleanly merge the
> patch against any of central, aurora, beta, or fx-team.

Sorry, I should have mentioned - it's on top of bug 1157529.

> The one area I think could use improved is how "currentState" is accessed in
> two functions, "_generateAssertion" and "getKeypairAndCertificate". I think
> future maintainers would find the code slightly easier to understand if
> "currentState" accesses were constrained to a single function and
> "getKeypairAndCertificate" was side-effect free.

I'm not sure I can do that both correctly and efficiently :(

> ::: services/fxaccounts/FxAccounts.jsm
> @@ +523,5 @@
> > +        // should be impossible - make log noise about it.
> > +        log.error("getAssertion called without a session token!");
> > +        return null;
> > +      }
> > +      return this.getKeypairAndCertificate(currentState, data).then(
> 
> Is there any reason why |currentState| and |data| need to be passed in
> separately when |data| is fetched from |currentState|?

I could avoid passing data with the cost of another redundant getUserAccountData() - that's relatively cheap but not free, so I judged the fact an extra param was necessary makes it worthwhile. I can change this if you feel strongly about it.

> Is this for testing?
> Essentially, I'm wondering if likes 512-526 could be moved inside
> |getKeypariAndCertificate|,

It could, but we still need the accountData in the caller to pass to getAssertionFromCert(), so if we did this there's an extra getUserAccountData() the parent would need to make that we could otherwise avoid.

> or as an alternative, only pass in |data|, get
> the |keyPair| and |certificate| when the promise resolves and move line 870
> to within this |then|. It seems like working with |currentState| would at
> least be contained to a single function.

We don't want to re-save the account data if we re-used the keypair and cert (ie, if the account data hasn't changed) - that has a real cost. So to do this we'd need to change the result to flag whether a cached version was used or not and only update when not changed. That seems even more complex than an extra arg - what do you think?

> @@ +819,5 @@
> >      // TODO: get the lifetime from the cert's .exp field
> > +    if (keyPairValid && certValid) {
> > +      log.debug("getKeypairAndCertificate: already have keyPair and certificate");
> > +      return {
> > +        keyPair: accountData.keyPair.keyPair,
> 
> I see the logic has not changed from the old code here, but why are both
> keyPair and cert "derefrenced" twice, e.g., |accountData.keyPair.keyPair|

keyPair is a record of 'keyPair' - the raw result from jwcrypto.generateKeyPair(), and 'validUntil' - the validity we calculated and asked for. I'm reluctant to treat the result of jwcrypto.generateKeyPair() as anything other than opaque blob.  Ditto for "cert", which is the raw result of getCertificateSigned() and the local validity we calculated.

So the "full" record, including the validity, is written to storage, but only the raw values are returned to the callers. IOW, this is limited to only this function. I think fixing that would break the abstractions we have (ie, we would start to need to make assumptions about the format of the data returned by generateKeyPair() and getCertificateSigned(), which would be bad IMO.)

> @@ +820,5 @@
> > +    if (keyPairValid && certValid) {
> > +      log.debug("getKeypairAndCertificate: already have keyPair and certificate");
> > +      return {
> > +        keyPair: accountData.keyPair.keyPair,
> > +        certificate: accountData.cert.cert
> 
> Perhaps it's beyond the scope of this patch, but it seems like the code
> would be simpler/easier to read if the |validUntil| field were part of the
> wrapped |cert| or |keyPair|, and the double dereference eliminated.

As above, I think that would be worse than the status-quo, where we don't need to make any assumptions about what is in these fields, especially given this strangeness is limited to this single function.

> @@ +863,1 @@
> >        let toUpdate = {
> 
> IIUC, this branch is the goal of the patch, save both the keyPair and cert
> to the user's account data atomically to avoid strange race conditions
> between two concurrent assertion requests.

Yes, but also a subtlety that exists *without* a race - if we ever generate a new keyPair we *must* generate a new certificate, even if an existing certificate remains valid. An earlier version of this patch which is attached to the bug handled the race, but not this subtlety.  It was quite a bit simpler, just not actually correct ;)

> >  add_task(function test_get_signed_in_user_initially_unset() {
> > +  let account = MakeFxAccounts();
> 
> Doe this task need a |_(...)| statement like the other tasks?

_ is just mapped to do_print, so it doesn't actually need one, but I'm happy to add one.

> @@ +257,2 @@
> >      sessionToken: "dead",
> > +    verified: true,
> 
> Do you want the trailing ,?

I tend to use trailing commas on literal objects so the addition of a new field at the end doesn't mean touching the line above.

> @@ +295,5 @@
> > +
> > +  let {keyPair, certificate} = yield fxa.internal.getKeypairAndCertificate(fxa.currentAccountState, credentials);
> > +  // should have the same keypair and cert.
> > +  do_check_eq(keyPair, credentials.keyPair.keyPair);
> > +  do_check_eq(certificate, credentials.cert.cert);
> 
> Does this task need a |yield fxa.signOut| like the two following tasks? My
> guess is no since the other tasks call |fxa.setSignedInUser|.

None of the tests actually need it at all :) But yeah, I should add it for consistency.

Adding needinfo for any further thoughts you have particularly around whether the additional getUserAccountData() is worth it to avoid the param, and to check you are happy with me not making assumptions about the keyPair and certificate data formats.

Comment 10

[Shane Tomlinson [:stomlinson]]

(In reply to Mark Hammond [:markh] from comment #9)
> (In reply to Shane Tomlinson [:stomlinson] from comment #8)
> > Comment on attachment 8633406 [details] [diff] [review]
> > 0003-Bug-1182740-treat-keypair-and-certificate-as-an-atom.patch
> > 
> > Review of attachment 8633406 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Mark, which Firefox branch/repo does this target? I cannot cleanly merge the
> > patch against any of central, aurora, beta, or fx-team.
> 
> Sorry, I should have mentioned - it's on top of bug 1157529.

Thanks, when merging against m-c, I am able to apply the patch from 1157529, but still not this patch. I have saved a log of the |hg qpush| output to https://pastebin.mozilla.org/8839706



> > ::: services/fxaccounts/FxAccounts.jsm
> > @@ +523,5 @@
> > > +        // should be impossible - make log noise about it.
> > > +        log.error("getAssertion called without a session token!");
> > > +        return null;
> > > +      }
> > > +      return this.getKeypairAndCertificate(currentState, data).then(
> > 
> > Is there any reason why |currentState| and |data| need to be passed in
> > separately when |data| is fetched from |currentState|?
> 
> I could avoid passing data with the cost of another redundant
> getUserAccountData() - that's relatively cheap but not free, so I judged the
> fact an extra param was necessary makes it worthwhile. I can change this if
> you feel strongly about it.







> > or as an alternative, only pass in |data|, get
> > the |keyPair| and |certificate| when the promise resolves and move line 870
> > to within this |then|. It seems like working with |currentState| would at
> > least be contained to a single function.
> 
> We don't want to re-save the account data if we re-used the keypair and cert
> (ie, if the account data hasn't changed) - that has a real cost. 

I see, I did not realize this. What are the side effects of saving account data,
and in particular, if the data has not actually been updated?

> So to do
> this we'd need to change the result to flag whether a cached version was
> used or not and only update when not changed. That seems even more complex
> than an extra arg - what do you think?


Indeed it does. My suggestion was based on the assumption that saving account data was free.


> 
> > @@ +819,5 @@
> > >      // TODO: get the lifetime from the cert's .exp field
> > > +    if (keyPairValid && certValid) {
> > > +      log.debug("getKeypairAndCertificate: already have keyPair and certificate");
> > > +      return {
> > > +        keyPair: accountData.keyPair.keyPair,
> > 
> > I see the logic has not changed from the old code here, but why are both
> > keyPair and cert "derefrenced" twice, e.g., |accountData.keyPair.keyPair|
> 
> keyPair is a record of 'keyPair' - the raw result from
> jwcrypto.generateKeyPair(), and 'validUntil' - the validity we calculated
> and asked for. I'm reluctant to treat the result of
> jwcrypto.generateKeyPair() as anything other than opaque blob.  Ditto for
> "cert", which is the raw result of getCertificateSigned() and the local
> validity we calculated.
> 
> So the "full" record, including the validity, is written to storage, but
> only the raw values are returned to the callers. IOW, this is limited to
> only this function. I think fixing that would break the abstractions we have
> (ie, we would start to need to make assumptions about the format of the data
> returned by generateKeyPair() and getCertificateSigned(), which would be bad
> IMO.)
> 


As soon as re-read my feedback, I thought the same thing you wrote here. Is there
value in calling the inner |keyPair| or |cert| something like |rawKeyPair|/|rawCert|,
or the wrapper variants |wrappedKeyPair|/|wrappedCert|?


> > @@ +820,5 @@
> > > +    if (keyPairValid && certValid) {
> > > +      log.debug("getKeypairAndCertificate: already have keyPair and certificate");
> > > +      return {
> > > +        keyPair: accountData.keyPair.keyPair,
> > > +        certificate: accountData.cert.cert
> > 
> > Perhaps it's beyond the scope of this patch, but it seems like the code
> > would be simpler/easier to read if the |validUntil| field were part of the
> > wrapped |cert| or |keyPair|, and the double dereference eliminated.
> 
> As above, I think that would be worse than the status-quo, where we don't
> need to make any assumptions about what is in these fields, especially given
> this strangeness is limited to this single function.

Yes, I definitely agree.


> 
> > @@ +863,1 @@
> > >        let toUpdate = {
> > 
> > IIUC, this branch is the goal of the patch, save both the keyPair and cert
> > to the user's account data atomically to avoid strange race conditions
> > between two concurrent assertion requests.
> 
> Yes, but also a subtlety that exists *without* a race - if we ever generate
> a new keyPair we *must* generate a new certificate, even if an existing
> certificate remains valid. An earlier version of this patch which is
> attached to the bug handled the race, but not this subtlety.  It was quite a
> bit simpler, just not actually correct ;)

Ahha, that obviously takes place in the code, but the rationale was obvious.
Can you add a little note about why a cert must be generated if the keys are
regenerated so in 10 years time when you and I revisit this code, we know not
to break that requirement?


> > >  add_task(function test_get_signed_in_user_initially_unset() {
> > > +  let account = MakeFxAccounts();
> > 
> > Doe this task need a |_(...)| statement like the other tasks?
> 
> _ is just mapped to do_print, so it doesn't actually need one, but I'm happy
> to add one.
> 
> > @@ +257,2 @@
> > >      sessionToken: "dead",
> > > +    verified: true,
> > 
> > Do you want the trailing ,?
> 
> I tend to use trailing commas on literal objects so the addition of a new
> field at the end doesn't mean touching the line above.

OK.

> 
> > @@ +295,5 @@
> > > +
> > > +  let {keyPair, certificate} = yield fxa.internal.getKeypairAndCertificate(fxa.currentAccountState, credentials);
> > > +  // should have the same keypair and cert.
> > > +  do_check_eq(keyPair, credentials.keyPair.keyPair);
> > > +  do_check_eq(certificate, credentials.cert.cert);
> > 
> > Does this task need a |yield fxa.signOut| like the two following tasks? My
> > guess is no since the other tasks call |fxa.setSignedInUser|.
> 
> None of the tests actually need it at all :) But yeah, I should add it for
> consistency.
> 
> Adding needinfo for any further thoughts you have particularly around
> whether the additional getUserAccountData() is worth it to avoid the param,
> and to check you are happy with me not making assumptions about the keyPair
> and certificate data formats.


I'm trying to understand the costs of fetching/saving user account data. Obviously
I don't want to cause performance regressions, but a marginal hit in performance
may be warranted for code clarity.

And I am 100% in agreement about modifying the original keyPair and certificate data
formats, what a terrible suggestion. Maybe a slightly clearer naming convention
could be used, but this isn't a blocker.

Really, my only blocker is for me to be able to cleanly merge the code and run the tests. From
the description of the problems you have described, I am comfortable with the changes
that have been made.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1182740-treat-keypair-and-certificate-as-an-atom.patch

Here's a new version. Due to a change we made in bug 1156752 (which also prevented this merging, but just landed on fx-team) I needed to change the data fetch code anyway - these "memory" fields are only returned when explicitly asked for.  I think I addressed the other suggestions you made too.

Comment 12

[Shane Tomlinson [:stomlinson]]

Comment on attachment 8639155 [details] [diff] [review]
0001-Bug-1182740-treat-keypair-and-certificate-as-an-atom.patch

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

This looks great Mark! My lone worry is whether the |cert|->|rawCert| & |keyPair|->|rawKeyPair| name change could cause either of those values to be undefined if |getKeypairAndCertificate| is called by a user that has the old format stored.

If that is not possible, lemme know and I'll switch to r+ immediately.

::: services/fxaccounts/FxAccounts.jsm
@@ +824,5 @@
>      // TODO: get the lifetime from the cert's .exp field
> +    if (keyPairValid && certValid) {
> +      log.debug("getKeypairAndCertificate: already have keyPair and certificate");
> +      return {
> +        keyPair: accountData.keyPair.rawKeyPair,

YES! Thank you for the name change (keyPair.keyPair -> keyPair.rawKeyPair)!

I'm going to throw a possible wrench into the rename. Could |getKeypairAndCertificate| ever be called with the old format which used |accountData.keyPair.keyPair| and |accountData.cert.cert|? If so, and if |validUntil| indicates either the keypair/cert are still valid, then the returned |keyPair| and |certificate| could be undefined.

If this scenario is possible, then an xpcshell test would be useful.

@@ +841,2 @@
>      if (Services.io.offline) {
>        throw new Error(ERROR_OFFLINE);

I know this branch is completely unrelated to this PR, but what happens if the user is offline? Is there a user facing error message?

@@ +861,5 @@
> +      });
> +    }
> +
> +    // and generate the cert.
> +    let certWillBeValidUntil = this.now() + CERT_LIFETIME;

Again, unrelated to this PR - since timestamps are generated locally, do we ever have to worry about clock skew between the local machine and the servers?

Comment 13

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Tomlinson [:stomlinson] from comment #12)
> Comment on attachment 8639155 [details] [diff] [review]
> 0001-Bug-1182740-treat-keypair-and-certificate-as-an-atom.patch
> 
> Review of attachment 8639155 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks great Mark! My lone worry is whether the |cert|->|rawCert| &
> |keyPair|->|rawKeyPair| name change could cause either of those values to be
> undefined if |getKeypairAndCertificate| is called by a user that has the old
> format stored.

These are only persisted in memory, so there's no concern there.

> I know this branch is completely unrelated to this PR, but what happens if
> the user is offline? Is there a user facing error message?

Nope - just log spew. Our story here isn't great.

> Again, unrelated to this PR - since timestamps are generated locally, do we
> ever have to worry about clock skew between the local machine and the
> servers?

That's a good question. On one hand, we are making an assumption that the cert is available for some duration - which should not be impacted by wall time (ie, we pass the duration, not the time of expiry, so even if the server and us disagree on what the clock says, we should both agree on how long n-hours is.)  OTOH though, there is an old comment:

> get the lifetime from the cert's .exp field

which implies to me that the result we get from the server self-describes the expiry, which we should probably trust more than the (arbitrary) time we requested. However, if that .exp() field is expressed as a time rather than a duration then *that* is where I'd expect to hit clock-skew issues.

Shane, do you think it is worth pursuing this? If so, I think a first step would be to determine exactly what that .exp field is.

Comment 14

[Ryan Kelly [:rfkelly]]

> if that .exp() field is expressed as a time rather than a duration

It is indeed a timestamp, not a duration.

Comment 15

[Mark Hammond [:markh] [:mhammond]]

(In reply to Ryan Kelly [:rfkelly] from comment #14)
> > if that .exp() field is expressed as a time rather than a duration
> 
> It is indeed a timestamp, not a duration.

If the certificate could also be served with a "now" field the client could convert it to a duration ;)

Comment 16

[Ryan Kelly [:rfkelly]]

We send a "timestamp" header that you could use for this purpose.

Comment 17

[Shane Tomlinson [:stomlinson]]

(In reply to Mark Hammond [:markh] from comment #13)

> 
> > get the lifetime from the cert's .exp field
> 
> which implies to me that the result we get from the server self-describes
> the expiry, which we should probably trust more than the (arbitrary) time we
> requested. However, if that .exp() field is expressed as a time rather than
> a duration then *that* is where I'd expect to hit clock-skew issues.
> 
> Shane, do you think it is worth pursuing this? If so, I think a first step
> would be to determine exactly what that .exp field is.


Clock skew issue is orthogonal to this PR and can be handled separately. I'm going to r+ this PR because it's an improvement over the current implementation. Do you want to file a follow on bug for the clock skew issue, or should I?

Comment 18

[Shane Tomlinson [:stomlinson]]

rfk - Do we have the same clock skew problem on the content server?

Comment 19

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Tomlinson [:stomlinson] from comment #17)
> Clock skew issue is orthogonal to this PR and can be handled separately. I'm
> going to r+ this PR because it's an improvement over the current
> implementation. Do you want to file a follow on bug for the clock skew
> issue, or should I?

I don't think we actually have a clock-skew issue yet. We will once we start honoring the expiry specified by the server, but it looks like we can mitigate that using the "timestamp" header. There's no bug on file to actually implement that.

Comment 20

[Chris Karlof [:ckarlof]]

Mark, does this need to uplift to 41? I'm assuming this bug got tickled because of requests related to profile images.

Comment 21

[Ryan Kelly [:rfkelly]]

> Do we have the same clock skew problem on the content server?

We would, except IIRC we don't cache certificates in content-server yet so it shouldn't bite us.

Comment 22

[Mark Hammond [:markh] [:mhammond]]

(In reply to Chris Karlof [:ckarlof] from comment #20)
> Mark, does this need to uplift to 41? I'm assuming this bug got tickled
> because of requests related to profile images.

It was tickled by the fact we sometimes had multiple fetches for profile info in-flight concurrently, which has since been uplifted to 41 (bug 1181952). While it's theoretically possible it happens in the wild, I've not seen any evidence it does in practice. This bug also depends on bug 1157529 and bug 1156752, and I've also no real evidence of these being hit in the wild, I'm leaning towards none of them being uplifted.

On the other hand though, a risk does exist - eg, let's say we start Syncing at the same time as we fetch a profile image - both fetch an assertion so may hit this. So it's certainly possible I'm being blasé and forgetting the "one in a million chances happen multiple times per day" maxim. The obvious tradeoff is that these patches themselves may have their own bugs.

Chris, what do you think?

Comment 23

[Pulsebot]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/836ab5febfee