New Fast Fetch signature scheme by taymonbeal · Pull Request #9040 · ampproject/amphtml

taymonbeal · 2017-04-28T20:33:17Z

Fixes #7618; see there for details of what this does and why it's needed.

Not ready to merge yet; still needs tests and documentation (and breaks a lot of existing tests due to API changes).

glevitzky

Minor changes requested, but mostly LGTM. I'll wait until all build/test issues are resolved before adding more comments, if necessary.

glevitzky · 2017-04-28T23:12:02Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

-      this.handleResize_(adResponse.size.width, adResponse.size.height);
-      return Promise.resolve(adResponse);
-    });
+    this.ampAnalyticsConfig = extractAmpAnalyticsConfig(responseHeaders);


Did you make the appropriate changes to this utility function? It's possible I might have missed something, but we need those extra arguments to construct the appropriate CSI pings.

glevitzky · 2017-04-28T23:14:48Z

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

    this.iframe = null;
+
+    for (const signingServiceName of this.getSigningServiceNames()) {
+      signatureVerifierFor(this.win).loadKeyset(signingServiceName);


We call signatureVerifierFor(this.win) multiple times; maybe store it as a local variable instead?

bradfrizzell

Looks good to me, just have some comments about style/documentation. I'm not that familiar with the existing crypto stuff so take my lgtm with a grain of salt

bradfrizzell · 2017-05-01T15:51:57Z

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

+  return win[propertyName];
+}
+
+class SignatureVerifier {


Please add some comments to the class and its methods describing what it does.

Working on adding a lot more documentation and tests to everything; it'll be up soon.

bradfrizzell · 2017-05-01T15:53:54Z

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

+            (response) => {
+              if (response.status == 200) {
+                return response.json().then(
+                    (jwkSet) => {


Can you maybe add a comment explaining what jwkSet is here?

bradfrizzell · 2017-05-01T15:57:50Z

src/service/crypto-impl.js

  /**
   * Convert a JSON Web Key object to a browser-native cryptographic key and
   * compute a hash for it.  The caller must verify that Web Cryptography is
   * available using isCryptoAvailable before calling this function.


Is this still true? You aren't calling isCryptoAvailable first

bradfrizzell · 2017-05-01T16:51:51Z

src/service/crypto-impl.js

-        publicKeyInfo.cryptoKey,
-        signature.subarray(5),
-        signedData));
+  verifyPkcs(key, signature, data) {


can you change the jsdoc so it documents what this stands for? Is it Public key crypto signature? i.e.

"Verifies public key crypto signature ... "

"PKCS" refers to the PKCS 1 algorithm.

bradfrizzell · 2017-05-01T17:07:00Z

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

+                          }
+                        },
+                        (err) => {
+                          dev().error(`Failed to verify signature: ${


nit: if you make a new line before the ${ can the rest all fit on one line?

This is clang-format's fault. Apparently other people within Google have also complained about this behavior. I am not totally sure what the right thing to do about it is, since I don't want to have something that the autoformatter will just revert later.

bradfrizzell

undoing approval until tests are added

bradfrizzell · 2017-05-03T20:08:31Z

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

+export function decodeSignatureHeader(headerValue) {
+  if (headerValue) {
+    const match =
+        new RegExp(


Could you add a comment in the jsdoc indicating what the expected format is?

bradfrizzell · 2017-05-03T20:09:40Z

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

+
+/**
+ * Decode the `X-Creativesize` header, in its conventional format, to a size
+ * info object.


Document expected format for headerValue here as well please

bradfrizzell · 2017-05-03T20:12:01Z

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

+export function decodeSizeHeader(headerValue) {
+  if (headerValue) {
+    dev().assert(new RegExp('[0-9]+x[0-9]+').test(headerValue));
+    const sizeArr = sizeHeader.split('x').map(Number);


is this supposed to be headerValue?

bradfrizzell · 2017-05-03T20:13:59Z

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

+   * A network connectivity failure, misbehaving signing service, or other
+   * factor beyond the ad network's control caused verification to fail.
+   */
+  NO_FAULT: 1,


NO_FAULT makes it sound like you are reporting that no fault occurred. Rename to something more accurate

What do you think it should be called?

SIGNING_SERVICE_FAILURE?

This isn't necessarily the signing service's fault; it could also be caused by the user losing network connectivity.

glevitzky

Most of my feedback is just minor formatting stuff. If you feel that any of it will make the code less readable, feel free to ignore the comment.

glevitzky · 2017-05-04T18:54:14Z

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

+                                        const message = err && err.message;
+                                        signingServiceError(
+                                            signingServiceName,
+                                            `Failed to import key (${


Nit: Not a strong preference, but I think ${var} is more readable than

${ var }

here and throughout.

I agree, but clang-format doesn't. I'm hesitant to contradict it because that might result in the line getting reformatted later.

glevitzky · 2017-05-04T18:55:02Z

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

+                return cryptoFor(this.win_)
+                    .verifyPkcs(key, signature, creative)
+                    .then(
+                        result => {


Nit: Can move this up to previous line if this doesn't screw up the formatting of the subsequent code too much.

This is because there are two callbacks here, so the rectangle rule requires that they both be on separate lines.

All formatting in this file was done by clang-format.

glevitzky · 2017-05-04T18:55:46Z

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

+                    response.headers.get('Content-Type') ==
+                    'application/jwk-set+json');
+                return response.json().then(
+                    jwkSet => {


Nit: Can move this up to previous line.

glevitzky · 2017-05-04T18:59:20Z

test/functional/test-crypto.js

+      const signaturePromise =
+          keypairPromise
+              .then(
+                  ({privateKey}) => subtle.sign(


Nit: move to previous line.

glevitzky · 2017-05-04T19:04:16Z

test/functional/test-crypto.js

+        it('should validate with the correct key and signature', () => {
+          return publicJwkPromise.then(jwk => crypto.importPkcsKey(jwk))
+              .then(
+                  key => signaturePromise.then(


Nit: Here and elsewhere in this file, we can move the callbacks to the same line as .then.

keithwrightbos

Some initial feedback

keithwrightbos · 2017-05-09T15:15:45Z

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

-    }} */
-let CreativeMetaDataDef;
+/** @typedef {{width: number, height: number}} */
+let SizeInfoDef;


Is there a base object that already meets this definition (e.g. rect?)?

If there is, we weren't using it. I just factored this out into a separate typedef to make Closure stop complaining.

keithwrightbos · 2017-05-09T15:17:16Z

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

    this.fromResumeCallback = false;
+
+    const signatureVerifier = signatureVerifierFor(this.win);
+    this.getSigningServiceNames().forEach(signingServiceName => {


I realize we currently fetch public keys in the constructor but should this be done instead in buildCallback? Should it be blocked by not in prerender state?

keithwrightbos · 2017-05-09T15:18:10Z

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

+              creativeParts.signatureInfo;
+          if (getMode().localDev) {
+            // localDev mode allows fake signature for the "fake" network.
+            if (signingServiceName == 'FAKESERVICE' &&


These if clauses can be combined into a single clause

Can they? I thought getMode().localDev invoked compiler magic that might not happen in that case. Do you know?

At worst I would expect compiler to just remove that clause. This is done throughout the codebase

keithwrightbos · 2017-05-09T15:20:11Z

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

+      creative: responseArrayBuffer,
+      signatureInfo: decodeSignatureHeader(
+          responseHeaders.get('AMP-Fast-Fetch-Signature')),
+      sizeInfo: decodeSizeHeader(responseHeaders.get('X-Creativesize')),


Is this header name correct? I would have expected the S to be capitalized

In principle, HTTP header names are case insensitive. But I'll change it anyway.

keithwrightbos · 2017-05-09T15:20:42Z

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

+ * @return {?SignatureInfoDef}
+ */
+export function decodeSignatureHeader(headerValue) {
+  if (headerValue) {


Instead of giant if, change to if (!headerValue) return null;

keithwrightbos · 2017-05-09T15:30:38Z

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

+   */
+  fetchAndAddKeys_(keys, signingServiceName, keypairId) {
+    let url = signingServerURLs[signingServiceName];
+    if (keypairId != null) {


Why not just if (keyPairId)?

keithwrightbos · 2017-05-09T15:30:50Z

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

+    if (keypairId != null) {
+      url += '?' + encodeURIComponent(keypairId);
+    }
+    return xhrFor(this.win_)


Make this.xhr_ field

keithwrightbos · 2017-05-09T15:31:35Z

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

+            {mode: 'cors', method: 'GET', ampCors: false, credentials: 'omit'})
+        .then(
+            response => {
+              if (response.status == 200) {


Same here, code is more readable if you have if (response.status != 200) {... }

keithwrightbos · 2017-05-09T15:32:07Z

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

+              if (response.status == 200) {
+                // The signing service spec requires this, but checking it at
+                // runtime in production isn't worth it.
+                dev().assert(


Why only dev? This could definitely occur in production and so we should guard against it, no?

keithwrightbos · 2017-05-09T15:34:05Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

    if (this.ampAnalyticsConfig_) {
      // Load amp-analytics extensions
-      this.extensions_./*OK*/loadExtension('amp-analytics');
+      this.extensions_./*OK*/ loadExtension('amp-analytics');


Why add space? Did lint complain?

clang-format did it.

erwinmombay · 2017-05-12T03:25:16Z

extensions/amp-a4a/0.1/test/testdata/valid_css_at_rules_amp.reserialized.js

 }
 </script>
-</body></html>`,
+</body></html>\0`,


@taymonbeal looks like closure ompiler is choking on this \0

if this file isnt source code and just for tests, prefix it with test- or suffix with _test so that it isnt globbed by the compiler task

I found a workaround.

oliy · 2017-05-15T17:23:47Z

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

+  return /** @type {?SignatureInfoDef} */ ({
+    signingServiceName: match[1],
+    keypairId: match[2],
+    signature: base64DecodeToBytes(match[3]),


Not extremely familiar with the codebase, but would errors in decoding surface here implicitly?

Decoding errors shouldn't be possible, because the SIGNATURE_FORMAT regex should only match valid base64 strings.

oliy · 2017-05-15T17:42:41Z

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

+   * A network connectivity failure, misbehaving signing service, or other
+   * factor beyond the ad network's control caused verification to fail.
+   */
+  NO_FAULT: 1,


SIGNING_SERVICE_FAILURE?

* Implement new signature verifier (ported from #9040) * Reword confusing JSDoc description * Fix another bad indent * Respond to review comments * Appease the Dread Linter

taymonbeal · 2017-08-29T19:56:59Z

Obsoleted by #11077.

taymonbeal requested review from bradfrizzell and glevitzky April 28, 2017 20:33

glevitzky requested changes Apr 28, 2017

View reviewed changes

bradfrizzell approved these changes May 1, 2017

View reviewed changes

bradfrizzell suggested changes May 1, 2017

View reviewed changes

googlebot added the cla: yes label May 3, 2017

bradfrizzell suggested changes May 3, 2017

View reviewed changes

taymonbeal mentioned this pull request May 4, 2017

A4A: Crypto operation unsuported #6676

Closed

glevitzky approved these changes May 4, 2017

View reviewed changes

keithwrightbos requested changes May 9, 2017

View reviewed changes

taymonbeal added 20 commits May 11, 2017 16:27

Implementation of new Fast Fetch signature scheme

414e84f

Clean up errors from earlier merge conflict

f9908ee

Unbroke <amp-analytics> API changes

0f4753f

Added doc comments to signature-verifier.js

8b6aac7

Added JSDoc to crypto-impl.js

eb3e755

Fixed various lint errors

9b0691a

More lint fixes

7de637e

Removed stray semicolon

590b523

Fixed JSDoc error in verifyPkcs

25e1dda

Fixed XHR service import error

a1062b6

Fixed CryptoKey type error

927ec27

Fixed some more type errors

f67907f

Fixed a few more type errors

7970d69

Turns out you're not allowed to destructure arrays

7613883

Fixed typecast syntax errors

3f82aad

Quoted isSecureContext

c41160c

Factored out SignatureInfoDef

2b5dd8d

Factored out SizeInfoDef

bf91e8d

Trying explicit typing of AdResponseDef

48e15a3

Change A4A-related tests to use new crypto API

5afcc89

taymonbeal added 9 commits May 11, 2017 16:32

Fixed test-amp-a4a.js

43dda63

Fixed AdSense impl tests

cd44ad2

Fixed DoubleClick impl tests

45de8d4

Fix lint errors

14231da

Fix a probably-false-positive-but-whatever lint error

4654f90

Add formatting information to JSDoc

4116ecf

Add trailing comma to satisfy linter

6e8b103

Cleanups to facilitate test-signature-verifier

be7e9df

Add missing semicolons

26db0d0

taymonbeal mentioned this pull request May 11, 2017

StringIndexOutOfBoundsException in Closure on Travis #9275

Closed

erwinmombay reviewed May 12, 2017

View reviewed changes

taymonbeal added 5 commits May 12, 2017 09:41

Work around Closure Compiler bug with \0

2dd34f2

Work around linter objection to Closure Compiler workaround

8c2b50a

Yet another workaround attempt

d022d25

Remove a leftover dead return statement

233264d

Collapse getMode().localDev into if condition

5f4c426

oliy reviewed May 15, 2017

View reviewed changes

oliy approved these changes May 15, 2017

View reviewed changes

taymonbeal mentioned this pull request May 15, 2017

Create XhrMock test double #9338

Closed

taymonbeal mentioned this pull request Jun 15, 2017

Refactor crypto API to just use PKCS without any extra metadata #9940

Merged

taymonbeal mentioned this pull request Jun 23, 2017

Move extractSize to a separate hook #10127

Merged

taymonbeal mentioned this pull request Jul 5, 2017

Stop overriding extractCreativeAndSignature in impls #10269

Merged

This was referenced Jul 24, 2017

Stop stubbing fetch and fetchJson in A4A tests #10594

Merged

Completely remove extractCreativeAndSignature and factor out SignatureVerifier interface #10674

Merged

taymonbeal mentioned this pull request Aug 23, 2017

Implement new signature verifier #11077

Merged

taymonbeal closed this Aug 29, 2017

taymonbeal deleted the new-signature-scheme branch August 29, 2017 19:57

Conversation

taymonbeal commented Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glevitzky 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!

bradfrizzell left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

bradfrizzell 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!

glevitzky 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!

keithwrightbos 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!

taymonbeal commented Apr 28, 2017 •

edited

Loading

bradfrizzell left a comment •

edited

Loading