A4A protect emitLifecycleEvent/onAmpRender from exiting promise chain; log signing service to stackdrive with service name #6504

keithwrightbos · 2016-12-06T20:55:48Z

Couple of changes to A4A flow:

Created generalized protectFunctionWrapper_ which ideally would use spread when calling optional on error handler but linter does not allow. Function will be used for any abstract function implementations whose failure is not expected to trigger abort (emitLifecycleEvent & onAmpCreativeRender)
Modify crypto key fetch to associate service name with key promise allowing for later validation error logging to include service name
Modify crypto validation to report to stackdriver via user().error() and to ensure error is only reported when failure is expected (when public key and signature start with same version info)

…rotect

…ing; fix a4a integration test to be hermetic

taymonbeal · 2016-12-06T21:06:20Z

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

+ * @visibileForTesting
+ * @private
+ */
+export const protectFunctionWrapper_ = (fn, opt_this, opt_onError) => {


Would probably make more sense for this to be export function protectFunctionWrapper_(fn, opt_this, opt_onError) {.

Also, old-style optional parameters are no longer used; rename them and give explicit defaults (which can be undefined if you want the same semantics) instead.

taymonbeal · 2016-12-06T21:06:25Z

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

@@ -482,8 +529,14 @@ export class AmpA4A extends AMP.BaseElement {
                  if (isValid) {
                    return creative;
                  }
-                  return Promise.reject(
-                      'Key failed to validate creative\'s signature.');
+                  // Only report is signature is expected to match given


Grammar nit: also, "... to match given ..." => "... to match, given that ..."

taymonbeal · 2016-12-06T21:09:57Z

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

-                      'Key failed to validate creative\'s signature.');
+                  // Only report is signature is expected to match given
+                  // multiple key providers could have been specified.
+                  if (!verifyHashVersion(signature, keyInfo)) {


Can you explain why this won't produce false positives? If you've got a locally built AMP runtime, this will run against both the dev and prod keys, and if the creative was signed with the prod key verifyHashVersion will return false for the dev key. Unless I've misunderstood something?

Good catch, I had the inverse. Removed !.

taymonbeal · 2016-12-06T21:10:58Z

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

+                  if (!verifyHashVersion(signature, keyInfo)) {
+                    user().error(TAG, this.element.getAttribute('type'),
+                        `Key failed to validate creative\'s signature for
+                        service ${keyInfo.serviceName}.`, keyInfo.cryptoKey);


That newline and leading whitespace won't be stripped, they'll appear in the error message.

Also, no need to escape a single quote in a non-single-quoted string.

taymonbeal · 2016-12-06T21:13:20Z

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

@@ -90,19 +91,25 @@ describe('integration test: a4a', () => {
  beforeEach(() => {
    sandbox = sinon.sandbox.create();
    xhrMock = sandbox.stub(Xhr.prototype, 'fetch');
+    // Expect key set fetches for signing services.
+    const fetchJsonMock = sandbox.stub(Xhr.prototype, 'fetchJson');


I really want a less fragile way to do test doubles for AJAX requests than these mocks. No need to fix that in this PR though, just something for the future.

Ack, I agree. I originally had fetch_ but it caused more extensive issues with the test than I wanted to address in this PR.

taymonbeal · 2016-12-06T21:28:47Z

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

+ * onError handler (if none provided, error is swallowed).
+ * @param {!Function} fn to protect
+ * @param {T=} opt_this An optional object to use as the 'this' object
+*     when calling the function.


Should probably note explicitly that this defaults to undefined if omitted.

Isn't that the usual semantic of omitted params in JS?

In terms of language semantics, yes. I was referring more to the API for this function; if a parameter is optional, it should be stated what happens if it's omitted (the answer in this case is that this is bound to the value undefined in the wrapped function, but I've seen other callback-accepting functions that do things differently).

Done (updated function documentation)

taymonbeal · 2016-12-06T21:31:31Z

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

@@ -125,6 +126,38 @@ export const LIFECYCLE_STAGES = {
 };


+/**
+ * Utility function that ensures any error thrown is handled by optional


Can we be clear on whether the caller of a function wrapped with this can use its return value? If so, there should be explicit returns for the case where there's no opt_onError or opt_onError throws, and it should be documented what's returned in these cases. If not, the tests should be changed not to rely on this behavior.

taymonbeal · 2016-12-06T21:33:10Z

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

@@ -125,6 +126,38 @@ export const LIFECYCLE_STAGES = {
 };


+/**
+ * Utility function that ensures any error thrown is handled by optional
+ * onError handler (if none provided, error is swallowed).


Make that "if none provided or handler throws"?

taymonbeal · 2016-12-06T21:33:27Z

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

+ *    and arguments provided to function call.
+ * @return {!Function} protected function
+ * @template T
+ * @visibileForTesting


taymonbeal · 2016-12-06T21:37:48Z

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

+        expect(errorCalls).to.equal(0);
+      });
+
+      it('handles error properly', done => {


I'm having a hard time figuring out how/whether this test works as intended (largely because of the done callback, whose semantics in synchronous code I can't figure out from the Chai API documentation and which should probably be avoided when possible). If protectFunctionWrapper_'s API is such that callers can rely on its return value, then I think it would be better to return an explicit value from the handler and assert that the top-level expression equals that. If not, then I think you should do something analogous to the errorCalls variable in the test above this one.

Also, probably-soon-to-be-irrelevant style nit: parentheses are recommended around arrow function arguments.

Done, agree that usage of done was unnecessary.

tdrl · 2016-12-07T18:59:18Z

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

+ * @return {!Function} protected function
+ * @template T
+ * @visibileForTesting
+ * @private


It looks weird to have it marked both @private and @visibleForTesting, but I guess there's never been a scope visibility thing enabled, so no problem. :-)

I agree. Renamed to protectFunctionWrapper and removed @Private

tdrl · 2016-12-07T19:04:07Z

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

@@ -482,8 +529,14 @@ export class AmpA4A extends AMP.BaseElement {
                  if (isValid) {
                    return creative;
                  }
-                  return Promise.reject(
-                      'Key failed to validate creative\'s signature.');
+                  // Only report is signature is expected to match given


Grammar nit: also, "... to match given ..." => "... to match, given that ..."

tdrl · 2016-12-07T19:13:48Z

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

@@ -686,40 +739,46 @@ export class AmpA4A extends AMP.BaseElement {
    const jwkSetPromises = this.getSigningServiceNames().map(serviceName => {
      dev().assert(getMode().localDev || !endsWith(serviceName, '-dev'));
      const url = signingServerURLs[serviceName];
+      const currServiceName = serviceName;


Why rename serviceName here? Why not just make it currServiceName in the loop index?

Ah because then it could be re-assigned within the map callback prior to the xhr callback such that it references the LAST value in the object. It's possible map does not actually cause such an issue (would certainly be the case if this was a for loop) but I'm old school and want to ensure we're safe in case someone changes this to a loop at some point.

tdrl · 2016-12-07T19:20:16Z

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

@@ -90,19 +91,25 @@ describe('integration test: a4a', () => {
  beforeEach(() => {
    sandbox = sinon.sandbox.create();
    xhrMock = sandbox.stub(Xhr.prototype, 'fetch');
+    // Expect key set fetches for signing services.
+    const fetchJsonMock = sandbox.stub(Xhr.prototype, 'fetchJson');
+    for (const serviceName in signingServerURLs) {


Last time I tried that, one of the AMP reviewers told me to avoid .keys(), in favor of for - in because .keys() involved allocating a list, which you want only for iteration.

…rotect

…csi_protect

keithwrightbos · 2016-12-12T13:42:50Z

@taymonbeal PTAL

taymonbeal · 2016-12-12T16:17:45Z

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

@@ -128,6 +129,42 @@ export const LIFECYCLE_STAGES = {
 };


+/**
+ * Utility function that ensures any error thrown is handled by optional
+ * onError handler (if none provided or handler throws, error is swallowed).


For an abundance of clarity, can this be "if none provided or handler throws, error is swallowed and undefined is returned"?

taymonbeal · 2016-12-12T16:21:09Z

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

+ * @visibleForTesting
+ */
+export function protectFunctionWrapper(
+    fn, opt_this = undefined, opt_onError = undefined) {


Do not prefix optional parameter names with opt_.

taymonbeal · 2016-12-12T16:24:42Z

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

+    /**
+     * Protected version of emitLifecycleEvent that ensures error does not
+     * cause promise chain to reject.
+     * @private {!function(string, !Object=)}


function (with a lowercase f) is considered a primitive type and is non-nullable by default; do not prefix it with !.

taymonbeal · 2016-12-12T16:26:33Z

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

+     */
+    this.protectedEmitLifecycleEvent_ = protectFunctionWrapper(
+      this.emitLifecycleEvent, this,
+      (err, var_args) => {


Please rename to varArgs.

taymonbeal · 2016-12-12T16:39:41Z

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

-                      'Key failed to validate creative\'s signature.');
+                  // Only report if signature is expected to match, given that
+                  // multiple key providers could have been specified.
+                  if (verifyHashVersion(signature, keyInfo)) {


What happens if the key hash in the signature doesn't match any available key? That would be an error case, but I don't see how it gets reported.

Added logging statement within promise chain when we had a signature but validation failed against all providers. This will cause multiple error statements to potentially occur (should there be one key provider that matches the signature hash but failed validation) but cannot see easy way around it.

keithwrightbos · 2016-12-13T19:22:00Z

@taymonbeal PTAL

keithwrightbos · 2016-12-13T19:24:26Z

@lannka @dvoytenko could you merge or assign another reviewer as needed? Thanks

…; log signing service to stackdrive with service name (ampproject#6504) * Initial commit * Include serviceName in crypto key info to allow for better error logging; fix a4a integration test to be hermetic * Fix test coverage * PR feedback * PR responses

keithwrightbos added 5 commits December 2, 2016 17:00

Initial commit

9af688d

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

6c6e90e

…rotect

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

56498d6

…rotect

Include serviceName in crypto key info to allow for better error logg…

74a728f

…ing; fix a4a integration test to be hermetic

Fix test coverage

5a83985

keithwrightbos added the WG: monetization label Dec 6, 2016

keithwrightbos assigned taymonbeal and tdrl Dec 6, 2016

keithwrightbos added the NEEDS REVIEW label Dec 6, 2016

taymonbeal suggested changes Dec 6, 2016

View reviewed changes

tdrl approved these changes Dec 7, 2016

View reviewed changes

erwinmombay removed the NEEDS REVIEW label Dec 7, 2016

keithwrightbos added 3 commits December 12, 2016 08:06

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

296c909

…rotect

Merge branch 'a4a_csi_protect' of github.com:google/amphtml into a4a_…

a91f4de

…csi_protect

PR feedback

f533292

taymonbeal suggested changes Dec 12, 2016

View reviewed changes

PR responses

480158f

taymonbeal approved these changes Dec 13, 2016

View reviewed changes

keithwrightbos assigned lannka Dec 13, 2016

lannka approved these changes Dec 14, 2016

View reviewed changes

lannka merged commit fcbe8f7 into ampproject:master Dec 14, 2016

taymonbeal deleted the a4a_csi_protect branch December 16, 2016 20:15

keithwrightbos mentioned this pull request Dec 29, 2016

A4A has apparent race condition in crypto validation #6780

Closed

A4A protect emitLifecycleEvent/onAmpRender from exiting promise chain; log signing service to stackdrive with service name #6504

A4A protect emitLifecycleEvent/onAmpRender from exiting promise chain; log signing service to stackdrive with service name #6504

Conversation

keithwrightbos commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taymonbeal Dec 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithwrightbos Dec 12, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithwrightbos commented Dec 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithwrightbos commented Dec 13, 2016

keithwrightbos commented Dec 13, 2016

taymonbeal Dec 6, 2016 •

edited

keithwrightbos Dec 12, 2016 •

edited