Implement new signature verifier#11077
Implement new signature verifier#11077keithwrightbos merged 5 commits intoampproject:masterfrom google:new-signature-verifier
Conversation
| * | ||
| * Unlike an AMP service, a signature verifier is **stateful**. It maintains a | ||
| * cache of all public keys that it has previously downloaded and imported, and | ||
| * also keeps track of which keys and signing services have already had failed |
There was a problem hiding this comment.
Nit: "have already had failed" -> "have already failed".
There was a problem hiding this comment.
"Failed" is actually an adjective here.
| } | ||
| return success; | ||
| }); | ||
| // This "recursive" call can recurse at most once. |
There was a problem hiding this comment.
For my own understanding, can you explain why this can only recurse once?
There was a problem hiding this comment.
Look seven lines up. The new signer.promise ensures that signer.keys[keypairId] can't be undefined again, by setting it to null if it is. So the second call can't go down this branch.
| return; | ||
| } | ||
|
|
||
| describe('when crypto is available', () => { |
There was a problem hiding this comment.
Can you add a test that ensures verifyCreativeAndSignature bottoms out after at most 1 recursive call? (Unless I missed it.)
There was a problem hiding this comment.
That verifyCreativeAndSignature calls itself is really an implementation detail to make the code more concise, so I think a test that spied on it directly would be overly fragile. It seems like what we really want to assert is that the verifier will not make more than two HTTP requests for a single kid; for that, see should not make more network requests retrying a nonexistent kid.
| } | ||
| return Services.xhrFor(this.win_) | ||
| .fetchJson( | ||
| url, // eslint-disable-line indent |
There was a problem hiding this comment.
This is because it should be on the previous line.
There was a problem hiding this comment.
Why should it be on the previous line when the next argument can't be? Doesn't that violate the rectangle rule?
There was a problem hiding this comment.
If the first argument is on a new line, it's only indented two spaces (it's not a line continuation).
There was a problem hiding this comment.
Style guide disagrees. Can we fix the tooling?
extensions/amp-a4a/0.1/amp-a4a.js
Outdated
| // - Chain cancelled => don't return; drop error | ||
| // - Uncaught error otherwise => don't return; percolate error up | ||
| this.adPromise_ = Services.viewerForDoc(this.getAmpDoc()).whenFirstVisible() | ||
| this.adPromise_ = this.keysetPromise_ |
There was a problem hiding this comment.
As discussed f2f, please revert this back to whenFirstVisible to ensure that keyset fetch failure does not prevent ad request. keySetPromise_ should be a dependency only on verifying responses with signature.
| // Web Cryptography isn't available. | ||
| return Promise.resolve(VerificationStatus.UNVERIFIED); | ||
| } | ||
| const signer = this.signers_[signingServiceName]; |
There was a problem hiding this comment.
Add a dev assert that signer exists
| return Promise.resolve(VerificationStatus.UNVERIFIED); | ||
| } | ||
| const signer = this.signers_[signingServiceName]; | ||
| return signer.promise.then(success => { |
There was a problem hiding this comment.
Should we limit the amount of time we're willing to wait for the public key? Presumably there is some performance trade-off (perhaps add a TODO).
| const match = signatureFormat.exec(headerValue); | ||
| if (!match) { | ||
| // TODO(@taymonbeal, #9274): replace this with real error reporting | ||
| user().error('AMP-A4A', `Invalid signature header: ${headerValue}`); |
There was a problem hiding this comment.
Do we really need to send the entire header value? Seems unnecessary... how about just the signing service name?
| }); | ||
| return VerificationStatus.OK; | ||
| } else { | ||
| return VerificationStatus.ERROR_SIGNATURE_MISMATCH; |
There was a problem hiding this comment.
I'd prefer we send a CSI ping in both cases... certainly it would be of value so we could track how often client-validation fails but perhaps that's out of scope for this PR. At last add a TODO.
| return Services.xhrFor(this.win_) | ||
| .fetchJson( | ||
| url, // eslint-disable-line indent | ||
| {mode: 'cors', method: 'GET', ampCors: false, credentials: 'omit'}) |
There was a problem hiding this comment.
We used to have a nice comment about why we do ampCors false... can it come back?
| // and there's no meaningful error recovery to be done if they | ||
| // fail, so we don't need to do them at runtime in production. | ||
| // They're included in dev mode as a debugging aid. | ||
| dev().assert( |
There was a problem hiding this comment.
I believe fetchJson already does this?
There was a problem hiding this comment.
Not quite. You could send 204 and fetchJson would let it through.
At long last, the new verifier arrives!
...sort of. This change introduces the code for the new verifier into the repo, but does not cause it to be compiled into the Fast Fetch extensions in production. The actual SignatureVerifier class is dead code and should be optimized out by the compiler. It is just here so that unit tests can be run against it, and to facilitate the next PR which links it into the Fast Fetch extensions for real, as an experiment.
Replaces #9040 (the core logic from that PR is in here). Related to #7618. Follow-up to #10674.