Fix race condition within A4A verify by keithwrightbos · Pull Request #6837 · ampproject/amphtml

keithwrightbos · 2016-12-29T16:30:49Z

See issue #6780 where some was returning on first provider failure to validate due to returning null instead of Promise.reject. Added optimization which guarantees any other awaiting promises "cancel" as soon as any provider successfully verifiers (reduces resource allocation).

Moves crypto into its owner service allowing for easier testing and ensuring that common code currently used by amp-analytics and a4a are utilized.

tdrl · 2017-01-03T21:15:05Z

extensions/amp-a4a/0.1/amp-a4a.js

        // service, keyInfoPromise, can verify the signature, then the
        // creative is valid AMP.
-        return some(keyInfoSet.map(keyInfoPromise => {
+        return !alreadyVerified() && some(keyInfoSet.map(keyInfoPromise => {


Won't this drop the Promise.reject() on the floor when verificationFound? !Promise.reject('foo') == false, so you'll go on to the some clause, but the Promise will be uncaptured and produce an uncaught promise exception, won't it?

Modified so that checkAlreadyVerified function returns Promise.reject or Promise.resolve after which I call then... let me know what you think.

I'd drop this alreadyVerified function entirely for an if:

if (verificationFound) { return Promise.reject('noop'); } return some(...);

tdrl · 2017-01-03T21:21:40Z

extensions/amp-a4a/0.1/amp-a4a.js

+    };
    return some(keyInfoSetPromises.map(keyInfoSetPromise => {
      // Resolve Promise into Array of Promises of signing keys.
      return keyInfoSetPromise.then(keyInfoSet => {


Would it be worth having an alreadyVerified check here as well? I still am not sure I'm tracking the logic of this method fully, but I think you can be in the case where a previous keyInfoSetPromise has already verified the sig. Is that right? If so, you could skip out entirely here.

Array.prototype.map is not an async operation but instead executes the given function on each item in the array and returns an array whose value is the result of the given function in order of the items in the array. Given its not async, I see no need to recheck already verified.

tdrl · 2017-01-03T21:22:56Z

extensions/amp-a4a/0.1/amp-a4a.js

+                  // Reject to ensure the some operation waits for other
+                  // possible providers to properly verify and resolve.
+                  return Promise.reject(
+                      `${keyInfo.serviceName} failed to verify`);


I know that writing unit tests for async code / testing race conditions is a nightmare, but I'd be a lot more comfortable if we could have one or more tests that fails when the race condition is triggered, but passes with your change to the code here.

jridgewell · 2017-01-05T18:52:53Z

extensions/amp-a4a/0.1/amp-a4a.js

+    let verificationFound = false;
+    const alreadyVerified = () => {
+      return verificationFound ?
+          Promise.reject('verification already found') : null;


Why are you returning a promise instance if we just ! the value?

jridgewell · 2017-01-05T18:56:55Z

extensions/amp-a4a/0.1/amp-a4a.js

        // service, keyInfoPromise, can verify the signature, then the
        // creative is valid AMP.
-        return some(keyInfoSet.map(keyInfoPromise => {
+        return !alreadyVerified() && some(keyInfoSet.map(keyInfoPromise => {


I'd drop this alreadyVerified function entirely for an if:

if (verificationFound) { return Promise.reject('noop'); } return some(...);

jridgewell · 2017-01-05T18:57:09Z

extensions/amp-a4a/0.1/amp-a4a.js

+    // Track if verification found as it will ensure that promises yet to
+    // resolve will "cancel" as soon as possible saving unnecessary resource
+    // allocation.
+    let verificationFound = false;


verified?

…y_race_fix

keithwrightbos · 2017-01-11T16:46:55Z

@jridgewell & @tdrl PTAL. Note that in order to easy in creating test coverage, I moved crypto into a proper service which resulted in combining crypto usage across amp-analytics and a4a. Ideally I could get this into today's canary cut. Thanks!

lannka · 2017-01-11T17:47:16Z

@keithwrightbos promoting the CID code to AMP core has been in our backlog for a while: #4898 .
One thing blocked the work is we wanted to lazy load the sha384 polyfill, otherwise it can increase runtime code size by about 10k (by my original rough estimation). Actually, can you check the compiled v0.js, see what the real value is, so we can decide if it's worth for a lazy load.

keithwrightbos · 2017-01-11T18:09:19Z

@lannka v0.js at current head is 188k vs 200k in PR (+6%) Let me know how you want to proceed

lannka · 2017-01-11T18:30:35Z

6% is non-neglectable. At this point, are you able to split that part out of this PR, and get the bug fix in first?

tdrl

Still reading, but wanted to send comments so far.

tdrl · 2017-01-11T22:07:57Z

extensions/amp-a4a/0.1/amp-a4a.js

-  verifyHashVersion,
-  PublicKeyInfoDef,
-} from './crypto-verifier';
+import {PublicKeyInfoDef, cryptoFor} from '../../../src/crypto';


Do you actually need to import PublicKeyInfoDef if you're just using it for typing? I thought you could get away without it. IIRC, you then declare your params like:

/** @type {!../../../src/crypto.PublicKeyInfoDef} */ let foo = ...

tdrl · 2017-01-11T22:10:49Z

extensions/amp-a4a/0.1/amp-a4a.js

    // signature, then the creative is valid AMP.
    /** @type {!AllServicesCryptoKeysDef} */
    const keyInfoSetPromises = this.win.ampA4aValidationKeys;
+    // Track if verification found as it will ensure that promises yet to


Pedantic uber-nit: "... found as ..." => "... found, as ..."

tdrl · 2017-01-11T22:16:22Z

extensions/amp-a4a/0.1/amp-a4a.js

+        .then(returnedArray => returnedArray[0], () => {
+          // Rejection occurs if all keys for this provider fail to validate.
+          return Promise.reject(
+              `All keys ${keyInfoSet.serviceName} failed to verify`);


Nit: Feels like the message needs another word. "All keys for...", "All keys from..."...?

tdrl · 2017-01-11T22:22:11Z

extensions/amp-a4a/0.1/amp-a4a.js

+    }, () => {
+      // rejection occurs if all providers fail to verify.
+      this.protectedEmitLifecycleEvent_('adResponseValidateEnd');
+      return null;


I think this will cause the whole chain to be fulfilled, not rejected. If I read MDN properly, returning anything other than a reject from a .then( , onRejected) block, then the promise is considered resolved. Is that the semantic you want here? I'd think you'd want something like return Promise.reject('No validation service could verify this key')?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then

tdrl · 2017-01-11T22:25:04Z

extensions/amp-a4a/0.1/amp-a4a.js

        }))
        // some() returns an array of which we only need a single value.
-        .then(returnedArray => returnedArray[0]);
+        .then(returnedArray => returnedArray[0], () => {


Style debate: Should we prefer .then(onFulfilled, onRejected) style or .then(onFullfilled).catch(onRejected) style? I feel the latter is slightly more readable, but I'm flexible. I haven't surveyed the AMP code base to see if there's a clear preference.

Will leave as is unless AMP owners object

tdrl

LGTM, though possibly @avimehta should double-check the parts that touch amp-analytics crypto?

tdrl · 2017-01-13T21:51:20Z

extensions/amp-a4a/0.1/test/test-amp-a4a.js

+      });
+
+      it('properly handles multiple providers that all fail', () => {
+        // Single provider with first key fails but second key passes validation


Not sure I understand this comment. Test name claims that all providers fail, but if one key passes wouldn't some provider pass?

tdrl · 2017-01-13T22:10:38Z

extensions/amp-a4a/0.1/test/test-amp-a4a.js

+        // From testing have found that need to yield prior to calling last
+        // key info resolver to ensure previous keys have had a chance to
+        // execute.
+        setTimeout(() => {keyInfoResolver({}); }, 0);


Spacing: I think you either need a space before keyInfo... or to delete one before the following }.

General: If I understand this right, this is delaying the third promise until after the first two have resolved and showing that the verifier will only call the first two, because one of them returns true, right?

So let's think... We want to test that the race condition is eliminated, which means that we need to make sure that it behaves correctly regardless of order of operations. So I think (?) that means that we need the following tests:

true verifier resolves before false verifier.

false resolves before true.

One verify rejects before the other returns true.

One verify rejects before the other returns false.

true resolves before a reject.

false resolves before a reject.

I think that's all the cases?

@taymonbeal is working on modifying key signature to ensure we know explicitly which provider and public key to use so this will be going away. As a result, I would prefer not to invest too much into additional test coverage given priorities to work on other items.

tdrl · 2017-01-13T22:26:57Z

src/crypto.js

-      !../extensions/amp-analytics/0.1/crypto-impl.Crypto>} */ (
-      getElementService(window, 'crypto', 'amp-analytics')));
+  return (/** @type {!./service/crypto-impl.Crypto} */ (
+      getExistingServiceForWindow(window, 'crypto')));


Right -- so I've been unclear why the old API returned a promise and the new one doesn't. I assume that you're now doing an eager initialization, while it was formerly doing a lazy one? Is that a problem? Like, are you incurring some significant initialization cost on pages that don't need crypto services or something?

This has changed after @lannka PR to make lazily inflate of polyfill

…y_race_fix

keithwrightbos · 2017-02-01T22:06:02Z

@lannka PTAL - updated to take your lazy crypto polyfill change into account where amp-analytics crypto has been moved into new crypto-impl service along with A4A specific items. This will allow for code re-use across A4A implementations (AdSense, Doubleclick, Triplelift)

…y_race_fix

lannka · 2017-02-03T18:20:53Z

@keithwrightbos could you resolve the conflicts?

Don't remember the review. 😬

…y_race_fix

keithwrightbos · 2017-02-03T21:20:27Z

I just synced to upstream master and didn't get any conflicts that it could not resolve automatically. In addition, build seems to be showing the following error from gulp dep-check yet when I run locally I don't see any issues. Any ideas? Thanks

src/service/crypto-impl.js must not depend on third_party/closure-library/sha384-generated.js. Rule: {"filesMatching":"/*.js","mustNotDependOn":"third_party//.js","whitelist":["extensions/amp-analytics/**/.js->third_party/closure-library/sha384-generated.js","extensions/amp-mustache/0.1/amp-mustache.js->third_party/mustache/mustache.js","3p/polyfills.js->third_party/babel/custom-babel-helpers.js","src/sanitizer.js->third_party/caja/html-sanitizer.js","extensions/amp-viz-vega/->third_party/vega/vega.js","extensions/amp-viz-vega/->third_party/d3/d3.js","src/dom.js->third_party/css-escape/css-escape.js","src/shadow-embed.js->third_party/webcomponentsjs/ShadowCSS.js"]}.

lannka · 2017-02-03T21:27:45Z

did you forget to push? We do see a list of conflicting files here.

src/service/crypto-impl.js must not depend on third_party/closure-library/sha384-generated.js.

After you merging with the latest code, src/service/crypto-impl.js should not be depending on the lib directly.

keithwrightbos · 2017-02-03T21:33:50Z

I accidentally pushed to upstream instead of origin. Should be good now. Apologies.

keithwrightbos · 2017-02-03T21:56:06Z

Looks like gulp type-checks is failing and I'm not sure why. lannka@ any ideas?

[16:53:46] Starting closure compiler for [ './3p/integration.js' ]
Error compiling ./3p/integration.js
WARNING - Failed to load module "./crypto-impl.js"

lannka

Some minor comments

lannka · 2017-02-03T22:03:46Z

extensions/amp-a4a/0.1/amp-a4a.js

                new Uint8Array(creative),
                signature,
                keyInfo)
                .then(isValid => {


the indentation is a bit off, is there enough space to fit all params in one line?

Cannot easily fit all on one line and linter is happy with as is

lannka · 2017-02-03T22:04:19Z

extensions/amp-a4a/0.1/amp-a4a.js

        // service, keyInfoPromise, can verify the signature, then the
        // creative is valid AMP.
+        if (verified) {
+          return Promise.reject('noop');


nit: just keep reason empty?

lannka · 2017-02-03T22:08:47Z

extensions/amp-a4a/0.1/amp-a4a.js

-                  if (keyInfo && verifyHashVersion(signature, keyInfo)) {
+                  if (keyInfo &&
+                      this.crypto_.verifyHashVersion(signature, keyInfo)) {
                    user().error(TAG, this.element.getAttribute('type'),


should be a dev error instead? and how likely this is to occur?

It should be very infrequent and will result in loss of preferential treatment. Does dev error send error so we can track frequency?

lannka · 2017-02-03T22:10:52Z

extensions/amp-a4a/0.1/amp-a4a.js

+                      `${keyInfo.serviceName} key failed to verify`);
                },
                err => {
                  user().error(


…y_race_fix

keithwrightbos · 2017-02-04T14:12:04Z

@lannka PTAL. I'm still getting the following error on gulp check-types although gulp build does not error. Any ideas?

[09:04:52] Starting closure compiler for [ './3p/integration.js' ]
Error compiling ./3p/integration.js
WARNING - Failed to load module "./crypto-impl.js"

…y_race_fix

keithwrightbos · 2017-02-06T22:07:45Z

@lannka PTAL. Note that I had to revert to Promise.reject() otherwise presubmit fails indicating reject must have reason. Please merge when green. Thanks!

* Address issue 6780 * Move crypto into its own service; add test coverage * fix merge conflicts * PR review * Move amp analytics into crypto service * PR review * PR reviews * test/presubmit fixes * fix linter error

Address issue 6780

958afe3

keithwrightbos mentioned this pull request Dec 29, 2016

A4A has apparent race condition in crypto validation #6780

Closed

keithwrightbos requested a review from lannka December 29, 2016 16:32

keithwrightbos assigned lannka Dec 29, 2016

keithwrightbos added the WG: monetization label Dec 29, 2016

keithwrightbos requested a review from tdrl January 3, 2017 19:59

tdrl reviewed Jan 3, 2017

View reviewed changes

jridgewell previously requested changes Jan 5, 2017

View reviewed changes

keithwrightbos added 5 commits January 10, 2017 09:28

Merge branch 'master' of github.com:ampproject/amphtml into a4a_verif…

ac87ee7

…y_race_fix

Move crypto into its own service; add test coverage

0a4eb62

Merge branch 'master' of github.com:ampproject/amphtml into a4a_verif…

f939549

…y_race_fix

fix merge conflicts

d10992e

PR review

b065376

tdrl reviewed Jan 11, 2017

View reviewed changes

lannka mentioned this pull request Jan 12, 2017

Moves closure sha384 into a new extension amp-crypto-polyfill for lazy load #7006

Merged

tdrl approved these changes Jan 13, 2017

View reviewed changes

keithwrightbos added 5 commits January 26, 2017 10:27

Merge branch 'master' of github.com:ampproject/amphtml into a4a_verif…

bf14d09

…y_race_fix

Merge branch 'master' of github.com:ampproject/amphtml into a4a_verif…

a220be4

…y_race_fix

Move amp analytics into crypto service

d6255f3

Merge branch 'master' of github.com:ampproject/amphtml into a4a_verif…

b9b3d14

…y_race_fix

PR review

71df6fb

Merge branch 'master' of github.com:ampproject/amphtml into a4a_verif…

c3c16a6

…y_race_fix

Merge branch 'master' of github.com:ampproject/amphtml into a4a_verif…

1d020d4

…y_race_fix

lannka approved these changes Feb 3, 2017

View reviewed changes

keithwrightbos added 2 commits February 4, 2017 09:04

Merge branch 'master' of github.com:ampproject/amphtml into a4a_verif…

bc2c631

…y_race_fix

PR reviews

f06ec7d

keithwrightbos added 2 commits February 6, 2017 16:48

Merge branch 'master' of github.com:ampproject/amphtml into a4a_verif…

8ea6571

…y_race_fix

test/presubmit fixes

2ff175a

fix linter error

acdb085

lannka merged commit 211119b into ampproject:master Feb 7, 2017

taymonbeal deleted the a4a_verify_race_fix branch February 7, 2017 16:23

Conversation

keithwrightbos commented Dec 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithwrightbos commented Jan 11, 2017

Uh oh!

lannka commented Jan 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keithwrightbos commented Jan 11, 2017

Uh oh!

lannka commented Jan 11, 2017

Uh oh!

tdrl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdrl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithwrightbos commented Feb 1, 2017

Uh oh!

lannka commented Feb 3, 2017

Uh oh!

keithwrightbos commented Feb 3, 2017

Uh oh!

lannka commented Feb 3, 2017

Uh oh!

keithwrightbos commented Feb 3, 2017

keithwrightbos commented Dec 29, 2016 •

edited

Loading

lannka commented Jan 11, 2017 •

edited

Loading