Completely remove extractCreativeAndSignature and factor out SignatureVerifier interface#10674
Completely remove extractCreativeAndSignature and factor out SignatureVerifier interface#10674keithwrightbos merged 4 commits intoampproject:masterfrom google:verifier-interface
Conversation
| urlBuilt: '1', | ||
| adRequestStart: '2', | ||
| adRequestEnd: '3', | ||
| extractCreativeAndSignature: '4', |
There was a problem hiding this comment.
Should 5...9 be renumbered as 4...8?
There was a problem hiding this comment.
Wouldn't that screw up our monitoring?
There was a problem hiding this comment.
Good answer to my question :) Nevermind.
| */ | ||
| verify(creative, headers, lifecycleCallback) { | ||
| if (!this.isAvailable_()) { | ||
| return Promise.resolve( |
There was a problem hiding this comment.
I think log messages here and at line 204 might be helpful in the future. Or alternatively, in the switch statement in the previous file where NO_FAULT is handled.
There was a problem hiding this comment.
What kind of log messages do you want?
There was a problem hiding this comment.
I was referring to https://github.com/ampproject/amphtml/pull/10674/files#diff-b139e3c4641f6d160f2832cba8be0761R666, where for KEY_NOT_FOUND and SIGNATURE_MISMATCH you do:
user().error(
TAG, this.element.getAttribute('type'),
'Signature verification failed');
...but for NO_FAULT, there is no user().error(...). My concern is that we don't want a silent failure if there is a network issue. So maybe in the NO_FAULT case of that switch include the same error message. Or you could log "Signature verification failed because signature verifier is unavailable" either in the switch or here in verify().
There was a problem hiding this comment.
Those error calls were copied over from this code's previous home in amp-a4a.js. At some point they need to be cleaned up.
Do user().error logs go to Stackdriver, or is this just for the Dev Tools? The Dev Tools will already log a failed network request. I'm not sure it makes sense to log connectivity failures to StackDriver. If we're worried about CORS, maybe it would make sense to downsample them or something?
There was a problem hiding this comment.
Ah - since they were just copied code and not new to this PR, I'm fine to wait until later to clean up.
I'm not 100% sure of the answer to your Stackdriver question, but I think they do. I agree with your point about the Dev Tools.
| // service, keyInfoPromise, can verify the signature, then the | ||
| // creative is valid AMP. | ||
| if (verified) { | ||
| return Promise.reject('noop'); |
| // Note: the 'keyInfo &&' check here is not strictly | ||
| // necessary, because we checked that above. But | ||
| // Closure type compiler can't seem to recognize that, so | ||
| // this guarantees it to the compiler. |
There was a problem hiding this comment.
Instead of "keyInfo &&" could you cast to reassure Closure that it's non-null?
| * @param {!Object} jwk An object which is hopefully an RSA JSON Web Key. The | ||
| * caller should verify that it is an object before calling this function. | ||
| * @return {!Promise<!PublicKeyInfoDef>} | ||
| * @private |
| * the public key. | ||
| */ | ||
| verifySignature(data, signature, publicKeyInfo) { | ||
| verifySignature_(data, signature, publicKeyInfo) { |
| * @private | ||
| */ | ||
| importPublicKey(serviceName, jwk) { | ||
| importPublicKey_(signingServiceName, jwk) { |
| keysetBody = | ||
| `{"keys":[${validCSSAmp.publicKey},${ | ||
| validCSSAmp.publicKey | ||
| },${validCSSAmp.publicKey}]}`; |
There was a problem hiding this comment.
I suspect the linter won't like this indentation...but it's a test file so probably doesn't get seen by the linter.
There was a problem hiding this comment.
The linter looks at test files, but it doesn't care what you do inside template literals. clang-format is terrible at this. I'll make it less ugly.
| * @param {!Headers} headers | ||
| * @param {function(string, !Object)} lifecycleCallback called for each AMP | ||
| * lifecycle event triggered during verification | ||
| * @return {!Promise<?VerificationFailure>} resolves to `null` on success, or, |
There was a problem hiding this comment.
Nit: I would have created a VerificationFailure.NONE to indicate success as opposed to giving null special meaning but up to you...
| return Promise.reject('redundant'); | ||
| } | ||
| if (!keyInfo) { | ||
| return Promise.reject('Promise resolved to null key.'); |
There was a problem hiding this comment.
When would this happen in practice? Should we log back to stackdriver to track occurrence?
There was a problem hiding this comment.
This would happen if the JSON object from the signing service could not be successfully converted into a crypto key. We do log to user().error when that happens, but at import time, not at verification time.
This will facilitate the introduction of the new verifier in the next PR.
One of the PRs that #9040 is being split into. Related to #7618. Follow-up to #10594.