Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1522813002:
Add public key and signature verification to browser-side API keys (Closed)
DescriptionAdd public key and signature verification to browser-side API keys
This adds the ability for experimental API keys to be cryptographically verified
in the browser. There is currently a single fixed signing key, whose public key
is embeddded in the browser code.
This makes use of Ed25519 cryptographic signature verification in boringssl, so
boringssl is added as a dependency to content/common for all platforms (except
iOS)
BUG=543215
Committed: https://crrev.com/592c59e49f7a1c1ce7fb469dd1f7ef0d98482ad8
Cr-Commit-Position: refs/heads/master@{#369329}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Update tests #Patch Set 4 : Rebase; update tests for consistency with rest of framework #Patch Set 5 : Rebase against nonbroken issue 1521063003 #
Total comments: 2
Patch Set 6 : Add TODOs for future work #
Total comments: 34
Patch Set 7 : Rebase against parent issue #Patch Set 8 : Addressing comments from PS#6 #Patch Set 9 : Rebase #Patch Set 10 : Rebase #Patch Set 11 : Switch to Ed25519 for signature verification #
Total comments: 8
Patch Set 12 : Remove pk length param; fix broken test #
Total comments: 15
Patch Set 13 : Addressing feedback from PS#12 #
Total comments: 4
Patch Set 14 : Addressing comments from PS#13 #
Messages
Total messages: 25 (7 generated)
iclelland@chromium.org changed reviewers: + chasej@chromium.org
+r chasej, PTAL, thanks! (Sorry for the flood of reviews today)
iclelland@chromium.org changed reviewers: + davidben@chromium.org
+r davidben, especially because of the use of crypto for validating the signatures.
LGTM, with nits. https://codereview.chromium.org/1522813002/diff/80001/content/browser/experim... File content/browser/experiments/api_key.cc (right): https://codereview.chromium.org/1522813002/diff/80001/content/browser/experim... content/browser/experiments/api_key.cc:117: crypto::SignatureVerifier::SHA256, crypto::SignatureVerifier::SHA256, Nit: add a TODO to indicate that this will eventually use a provided SPKI (SubjectPublicKeyInfo), rather hard-coding the hash algorithm.
https://codereview.chromium.org/1522813002/diff/80001/content/browser/experim... File content/browser/experiments/api_key.cc (right): https://codereview.chromium.org/1522813002/diff/80001/content/browser/experim... content/browser/experiments/api_key.cc:117: crypto::SignatureVerifier::SHA256, crypto::SignatureVerifier::SHA256, On 2015/12/17 14:13:58, chasej wrote: > Nit: add a TODO to indicate that this will eventually use a provided SPKI > (SubjectPublicKeyInfo), rather hard-coding the hash algorithm. Done.
https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... File content/browser/experiments/api_key.cc (right): https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:32: "-----END PUBLIC KEY-----"; Here, use this: static const uint8_t kPublicKey[] = { 0x30, 0x82, 0x01, 0x22, 0x30, 0x0d, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01, 0x05, 0x00, 0x03, 0x82, 0x01, 0x0f, 0x00, 0x30, 0x82, 0x01, 0x0a, 0x02, 0x82, 0x01, 0x01, 0x00, 0xdf, 0xc0, 0xb0, 0x04, 0x77, 0x33, 0xf1, 0x8a, 0x02, 0x9c, 0xf5, 0x70, 0x84, 0x7d, 0x69, 0x8f, 0x39, 0xb3, 0xfd, 0x32, 0x85, 0xbb, 0x58, 0x20, 0x23, 0x4f, 0xa0, 0xce, 0xe8, 0x01, 0x13, 0xe7, 0xac, 0x02, 0x98, 0xe6, 0xe0, 0x8f, 0x4d, 0xd4, 0xf2, 0x77, 0xfc, 0x62, 0xda, 0xfa, 0xaa, 0x73, 0xa7, 0xf5, 0x69, 0x83, 0x2c, 0xd6, 0xa9, 0x78, 0x91, 0xa0, 0x63, 0x3f, 0x44, 0x26, 0x40, 0x57, 0x95, 0x3e, 0x90, 0x36, 0xa8, 0xba, 0x4c, 0x40, 0xdf, 0x71, 0x9d, 0x78, 0x0d, 0xc1, 0x52, 0xb7, 0x18, 0x04, 0x86, 0xac, 0xa4, 0x64, 0x71, 0xeb, 0x15, 0xd7, 0xdf, 0xa1, 0xcb, 0x2c, 0xf2, 0x45, 0xe9, 0x87, 0x63, 0xc5, 0xb1, 0x01, 0xc9, 0x8a, 0x6b, 0xcd, 0xe7, 0x17, 0x84, 0xd5, 0x08, 0xaa, 0x5e, 0x55, 0x5b, 0x70, 0xf4, 0x21, 0x1e, 0x1f, 0xad, 0x4e, 0x45, 0xcc, 0xfc, 0xbf, 0x5c, 0x1e, 0x62, 0x6e, 0x51, 0x71, 0x64, 0x01, 0xb8, 0xeb, 0xcd, 0x88, 0x45, 0x12, 0xf9, 0x29, 0xd1, 0x8b, 0xe0, 0xe5, 0x85, 0x00, 0xfb, 0xb3, 0x68, 0xa3, 0x8c, 0xab, 0xba, 0x23, 0x78, 0xa0, 0x3d, 0xcb, 0xc1, 0xae, 0xdb, 0xd8, 0x6e, 0x46, 0x49, 0xca, 0x00, 0xbb, 0x92, 0x73, 0x1a, 0xa4, 0xf8, 0x99, 0xf9, 0x41, 0xfd, 0xf3, 0x59, 0xc9, 0x0b, 0x4e, 0x9c, 0x32, 0x3e, 0x8a, 0x28, 0x5c, 0x34, 0x35, 0xd7, 0xab, 0xec, 0xd1, 0xbf, 0x6c, 0xb1, 0xa9, 0x51, 0x4a, 0xe0, 0x52, 0x46, 0x1e, 0x10, 0xb6, 0x3c, 0xe8, 0x43, 0xa4, 0xb5, 0x4b, 0x96, 0x6a, 0xc3, 0xed, 0xab, 0xbd, 0x77, 0xd8, 0x85, 0x56, 0x7a, 0xe6, 0x28, 0x91, 0xd7, 0x3c, 0xf3, 0xea, 0x70, 0x57, 0x22, 0xe5, 0x26, 0xf4, 0x70, 0x33, 0x20, 0x81, 0x78, 0xdc, 0xb1, 0x3a, 0x6a, 0x41, 0xbd, 0x13, 0xf1, 0x7d, 0xdd, 0x96, 0x0d, 0x9b, 0x02, 0x03, 0x01, 0x00, 0x01, }; and pass kPublicKey and sizeof(kPublicKey) into the function. We at least avoid storing a base64-encoded string in the binary (smaller) and no need to pull in a PEM parser. If you like, you can call this kPublicKeyDER or kPublicKeySPKI to be explicit about what it is. (It's a DER-encoded SubjectPublicKeyInfo. It's literally the base64-decode of the PEM version. :-) ) https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:36: */ Nit: This can use // for comments https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:37: const size_t kSaltLength = ((2048 + 6) >> 3) - (256 >> 3) - 2; So, I'm not all that familiar with RSA-PSS, but I asked agl about salt length guidance and he says: """ Equal to the hash length or something? It's one of those cases where the spec is stupidly over general. I don't think it really matters. """ QUIC seems to use the length of the hash, judging by ProofVerifierChromium. https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:95: bool ApiKey::ValidateSignature(const char* publicKeyPEM) const { public_key_pem https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:105: if (!base::IsStringASCII(signatureText)) { Does base::Base64Decode not check this for you? https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:112: net::PEMTokenizer pem_tokenizer(publicKeyPEM, {"PUBLIC KEY"}); I don't thiiink we get initializer lists yet? (Hope we do soon!) But it doesn't matter since we can avoid involving PEM in this at all. :-) https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:116: std::string publicKey = pem_tokenizer.data(); public_key https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:117: std::string dataInUTF8 = data; data_in_utf8, but it looks like you can just data. I'm guessing this is ported over from Blink code that had to deal with WebString. https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:118: crypto::SignatureVerifier sv; verifier https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:120: // https://crbug.com/570684 Commented on the bug, but this seems just fine. Also I'm not sure SubjectPublicKeyInfo is quite what you want. :-) https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:121: sv.VerifyInitRSAPSS( Check for failure. Shouldn't happen, but still. https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:123: kSaltLength, reinterpret_cast<const uint8*>(signature.data()), uint8_t for new code (I should go and fix //crypto's interfaces since that's small...) https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... File content/browser/experiments/api_key.h (right): https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.h:46: bool ValidateSignature(const char* publicKeyPEM) const; public_key_pem https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.h:48: static bool ValidateSignature(const std::string& signatureText, signature_text https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.h:50: const char* publicKeyPEM); public_key_pem https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... File content/browser/experiments/api_key_unittest.cc (right): https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key_unittest.cc:16: /* Nit: I think // is more common, though I don't terribly care either. https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key_unittest.cc:66: "e.com|Frobulate|1458766277"; FWIW, there's much much smaller signature schemes we could do. RSA gets pretty unreasonably large. An ECDSA signature with P-256 will be around 71-ish bytes (give or take; the encoding is stupidly variable-length) and is widely implemented. SignatureVerifier can do ECDSA, though the API is a little funky. This is Blink and //content, so you could also use BoringSSL APIs directly. (Long term I'm hoping we switch to just using those APIs directly rather than maintain wrapper upon wrapper.) There's also Ed25519 which is a very modern scheme with 64-byte signatures. We've got support in BoringSSL, although it's currently compiled out of Chromium (and Android) because nothing's using it right now. But we could certainly enable it. We expect we'll want to eventually.
https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... File content/browser/experiments/api_key.cc (right): https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:32: "-----END PUBLIC KEY-----"; On 2015/12/18 22:23:17, davidben wrote: > Here, use this: > > static const uint8_t kPublicKey[] = { > 0x30, 0x82, 0x01, 0x22, 0x30, 0x0d, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, > 0xf7, 0x0d, 0x01, 0x01, 0x01, 0x05, 0x00, 0x03, 0x82, 0x01, 0x0f, 0x00, > 0x30, 0x82, 0x01, 0x0a, 0x02, 0x82, 0x01, 0x01, 0x00, 0xdf, 0xc0, 0xb0, > 0x04, 0x77, 0x33, 0xf1, 0x8a, 0x02, 0x9c, 0xf5, 0x70, 0x84, 0x7d, 0x69, > 0x8f, 0x39, 0xb3, 0xfd, 0x32, 0x85, 0xbb, 0x58, 0x20, 0x23, 0x4f, 0xa0, > 0xce, 0xe8, 0x01, 0x13, 0xe7, 0xac, 0x02, 0x98, 0xe6, 0xe0, 0x8f, 0x4d, > 0xd4, 0xf2, 0x77, 0xfc, 0x62, 0xda, 0xfa, 0xaa, 0x73, 0xa7, 0xf5, 0x69, > 0x83, 0x2c, 0xd6, 0xa9, 0x78, 0x91, 0xa0, 0x63, 0x3f, 0x44, 0x26, 0x40, > 0x57, 0x95, 0x3e, 0x90, 0x36, 0xa8, 0xba, 0x4c, 0x40, 0xdf, 0x71, 0x9d, > 0x78, 0x0d, 0xc1, 0x52, 0xb7, 0x18, 0x04, 0x86, 0xac, 0xa4, 0x64, 0x71, > 0xeb, 0x15, 0xd7, 0xdf, 0xa1, 0xcb, 0x2c, 0xf2, 0x45, 0xe9, 0x87, 0x63, > 0xc5, 0xb1, 0x01, 0xc9, 0x8a, 0x6b, 0xcd, 0xe7, 0x17, 0x84, 0xd5, 0x08, > 0xaa, 0x5e, 0x55, 0x5b, 0x70, 0xf4, 0x21, 0x1e, 0x1f, 0xad, 0x4e, 0x45, > 0xcc, 0xfc, 0xbf, 0x5c, 0x1e, 0x62, 0x6e, 0x51, 0x71, 0x64, 0x01, 0xb8, > 0xeb, 0xcd, 0x88, 0x45, 0x12, 0xf9, 0x29, 0xd1, 0x8b, 0xe0, 0xe5, 0x85, > 0x00, 0xfb, 0xb3, 0x68, 0xa3, 0x8c, 0xab, 0xba, 0x23, 0x78, 0xa0, 0x3d, > 0xcb, 0xc1, 0xae, 0xdb, 0xd8, 0x6e, 0x46, 0x49, 0xca, 0x00, 0xbb, 0x92, > 0x73, 0x1a, 0xa4, 0xf8, 0x99, 0xf9, 0x41, 0xfd, 0xf3, 0x59, 0xc9, 0x0b, > 0x4e, 0x9c, 0x32, 0x3e, 0x8a, 0x28, 0x5c, 0x34, 0x35, 0xd7, 0xab, 0xec, > 0xd1, 0xbf, 0x6c, 0xb1, 0xa9, 0x51, 0x4a, 0xe0, 0x52, 0x46, 0x1e, 0x10, > 0xb6, 0x3c, 0xe8, 0x43, 0xa4, 0xb5, 0x4b, 0x96, 0x6a, 0xc3, 0xed, 0xab, > 0xbd, 0x77, 0xd8, 0x85, 0x56, 0x7a, 0xe6, 0x28, 0x91, 0xd7, 0x3c, 0xf3, > 0xea, 0x70, 0x57, 0x22, 0xe5, 0x26, 0xf4, 0x70, 0x33, 0x20, 0x81, 0x78, > 0xdc, 0xb1, 0x3a, 0x6a, 0x41, 0xbd, 0x13, 0xf1, 0x7d, 0xdd, 0x96, 0x0d, > 0x9b, 0x02, 0x03, 0x01, 0x00, 0x01, > }; > > and pass kPublicKey and sizeof(kPublicKey) into the function. We at least avoid > storing a base64-encoded string in the binary (smaller) and no need to pull in a > PEM parser. > > If you like, you can call this kPublicKeyDER or kPublicKeySPKI to be explicit > about what it is. (It's a DER-encoded SubjectPublicKeyInfo. It's literally the > base64-decode of the PEM version. :-) ) Thanks, done. I wanted something paste-able from an OpenSSL-generated private key, but you're right that this will take up less space (code+data), and be generally faster. https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:36: */ On 2015/12/18 22:23:17, davidben wrote: > Nit: This can use // for comments Done. https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:37: const size_t kSaltLength = ((2048 + 6) >> 3) - (256 >> 3) - 2; On 2015/12/18 22:23:16, davidben wrote: > So, I'm not all that familiar with RSA-PSS, but I asked agl about salt length > guidance and he says: > """ > Equal to the hash length or something? > It's one of those cases where the spec is stupidly over general. > I don't think it really matters. > """ > > QUIC seems to use the length of the hash, judging by ProofVerifierChromium. I used the max length, and ensured that this was interoperable with the Python cryptography.hazmat package. I've replaced this with the constant `-2`, which, as it turns out, implies the same calculation (https://commondatastorage.googleapis.com/chromium-boringssl-docs/evp.h.html#E...). If you prefer, though, I can reduce it to 256 bits to match the hash. https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:95: bool ApiKey::ValidateSignature(const char* publicKeyPEM) const { On 2015/12/18 22:23:16, davidben wrote: > public_key_pem Done. https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:105: if (!base::IsStringASCII(signatureText)) { On 2015/12/18 22:23:16, davidben wrote: > Does base::Base64Decode not check this for you? Yes. Yes I suppose it does, in a roundabout way (not to mention also filtering out the other 63 invalid ASCII characters). Thanks :) https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:112: net::PEMTokenizer pem_tokenizer(publicKeyPEM, {"PUBLIC KEY"}); On 2015/12/18 22:23:17, davidben wrote: > I don't thiiink we get initializer lists yet? (Hope we do soon!) But it doesn't > matter since we can avoid involving PEM in this at all. :-) Acknowledged. PEMTokenizer is gone. https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:116: std::string publicKey = pem_tokenizer.data(); On 2015/12/18 22:23:16, davidben wrote: > public_key Done. https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:117: std::string dataInUTF8 = data; On 2015/12/18 22:23:17, davidben wrote: > data_in_utf8, but it looks like you can just data. I'm guessing this is ported > over from Blink code that had to deal with WebString. It's ported from my original attempt at this code, which all lived in blink (until I realized that it made more sense over here :) ) https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:118: crypto::SignatureVerifier sv; On 2015/12/18 22:23:17, davidben wrote: > verifier Done. https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:120: // https://crbug.com/570684 On 2015/12/18 22:23:17, davidben wrote: > Commented on the bug, but this seems just fine. Also I'm not sure > SubjectPublicKeyInfo is quite what you want. :-) Removed, thanks :) https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:121: sv.VerifyInitRSAPSS( On 2015/12/18 22:23:17, davidben wrote: > Check for failure. Shouldn't happen, but still. Done. https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:123: kSaltLength, reinterpret_cast<const uint8*>(signature.data()), On 2015/12/18 22:23:17, davidben wrote: > uint8_t for new code (I should go and fix //crypto's interfaces since that's > small...) Done. https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... File content/browser/experiments/api_key.h (right): https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.h:46: bool ValidateSignature(const char* publicKeyPEM) const; On 2015/12/18 22:23:17, davidben wrote: > public_key_pem Done. Actually changed to uint8_t* public_key (drop the PEM. It's cleaner.) https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.h:48: static bool ValidateSignature(const std::string& signatureText, On 2015/12/18 22:23:17, davidben wrote: > signature_text Done. https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key.h:50: const char* publicKeyPEM); On 2015/12/18 22:23:17, davidben wrote: > public_key_pem Done. https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... File content/browser/experiments/api_key_unittest.cc (right): https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key_unittest.cc:16: /* On 2015/12/18 22:23:17, davidben wrote: > Nit: I think // is more common, though I don't terribly care either. Ack. I deliberately chose /* */ here so that anyone who wants to test this, or generate their own sample keys, say to update tests, could more easily copy the key data out of this file, without having to strip the // from every line. https://codereview.chromium.org/1522813002/diff/100001/content/browser/experi... content/browser/experiments/api_key_unittest.cc:66: "e.com|Frobulate|1458766277"; On 2015/12/18 22:23:17, davidben wrote: > FWIW, there's much much smaller signature schemes we could do. RSA gets pretty > unreasonably large. > > An ECDSA signature with P-256 will be around 71-ish bytes (give or take; the > encoding is stupidly variable-length) and is widely implemented. > SignatureVerifier can do ECDSA, though the API is a little funky. This is Blink > and //content, so you could also use BoringSSL APIs directly. (Long term I'm > hoping we switch to just using those APIs directly rather than maintain wrapper > upon wrapper.) > > There's also Ed25519 which is a very modern scheme with 64-byte signatures. > We've got support in BoringSSL, although it's currently compiled out of Chromium > (and Android) because nothing's using it right now. But we could certainly > enable it. We expect we'll want to eventually. That's a great idea, and I appreciate you enabling that in boringssl for it; I'll work on another CL to switch that out.
Description was changed from ========== Add public key and signature verification to browser-side API keys This adds the ability for experimental API keys to be cryptographically verified in the browser. There is currently a single fixed signing key, whose public key is embeddded in the browser code. BUG=543215 ========== to ========== Add public key and signature verification to browser-side API keys This adds the ability for experimental API keys to be cryptographically verified in the browser. There is currently a single fixed signing key, whose public key is embeddded in the browser code. This makes use of Ed25519 cryptographic signature verification in boringssl, so boringssl is added as a dependency to content/common for all platforms (except iOS) BUG=543215 ==========
On 2015/12/22 05:45:28, iclelland wrote: > That's a great idea, and I appreciate you enabling that in boringssl for it; > I'll work on another CL to switch that out. It was easy in the end to add the change to this CL. davidben, PTA(nother)L, thanks!
https://codereview.chromium.org/1522813002/diff/200001/content/common/experim... File content/common/experiments/api_key.cc (right): https://codereview.chromium.org/1522813002/diff/200001/content/common/experim... content/common/experiments/api_key.cc:119: size_t public_key_length) { Remove the public_key_length parameter? I don't see it used with the new algorithm for signature validation. That should allow the constant kPublicKeyLength to be removed as well. https://codereview.chromium.org/1522813002/diff/200001/content/common/experim... File content/common/experiments/api_key_unittest.cc (right): https://codereview.chromium.org/1522813002/diff/200001/content/common/experim... content/common/experiments/api_key_unittest.cc:35: const size_t kTestPublicKeyLength = sizeof(kTestPublicKey); This can be removed, once the key length parameter is removed from the validation methods on APIKey. https://codereview.chromium.org/1522813002/diff/200001/content/common/experim... content/common/experiments/api_key_unittest.cc:180: EXPECT_FALSE(key->IsValid(base::Time::FromDoubleT(kInvalidTimestamp))); Should this be passing kValidTimestamp? Otherwise, this isn't reliably testing the signature validation. That is, IsValid() could return false due to the timestamp, not necessarily because the signature was deemed invalid. https://codereview.chromium.org/1522813002/diff/200001/content/common/experim... content/common/experiments/api_key_unittest.cc:181: } Do we need more tests for ApiKey.IsValid()? I know it's a simple method, but the result is now a combination of conditions. The only test for IsValid() is the ValidateSignatureOnWrongKey above. This is an open question, I could be convinced either way (i.e. more tests could certainly be overkill).
https://codereview.chromium.org/1522813002/diff/200001/content/common/experim... File content/common/experiments/api_key.cc (right): https://codereview.chromium.org/1522813002/diff/200001/content/common/experim... content/common/experiments/api_key.cc:119: size_t public_key_length) { On 2016/01/11 16:11:25, chasej wrote: > Remove the public_key_length parameter? I don't see it used with the new > algorithm for signature validation. That should allow the constant > kPublicKeyLength to be removed as well. No, it's a holdover from the previous (RSAPSS) code. Removed. https://codereview.chromium.org/1522813002/diff/200001/content/common/experim... File content/common/experiments/api_key_unittest.cc (right): https://codereview.chromium.org/1522813002/diff/200001/content/common/experim... content/common/experiments/api_key_unittest.cc:35: const size_t kTestPublicKeyLength = sizeof(kTestPublicKey); On 2016/01/11 16:11:25, chasej wrote: > This can be removed, once the key length parameter is removed from the > validation methods on APIKey. Done. https://codereview.chromium.org/1522813002/diff/200001/content/common/experim... content/common/experiments/api_key_unittest.cc:180: EXPECT_FALSE(key->IsValid(base::Time::FromDoubleT(kInvalidTimestamp))); On 2016/01/11 16:11:25, chasej wrote: > Should this be passing kValidTimestamp? Otherwise, this isn't reliably testing > the signature validation. That is, IsValid() could return false due to the > timestamp, not necessarily because the signature was deemed invalid. Thanks, that was a typo introduced back in PS#4. Fixed. https://codereview.chromium.org/1522813002/diff/200001/content/common/experim... content/common/experiments/api_key_unittest.cc:181: } On 2016/01/11 16:11:25, chasej wrote: > Do we need more tests for ApiKey.IsValid()? I know it's a simple method, but > the result is now a combination of conditions. The only test for IsValid() is > the ValidateSignatureOnWrongKey above. This is an open question, I could be > convinced either way (i.e. more tests could certainly be overkill). IsValid is deliberately kept very simple, and by design uses the real public key(s). Without shipping a valid API key in tests, which is subject to change all the time, and will make the tests fragile here, I think the best thing to do is to provide exhaustive unit tests for the various Validate*() methods that it calls, and then ensure that IsValid() is obviously correct. Once I've added proper key management, it should be possible to "preload" the validator with keys -- they won't be hard-coded like they are now. At that point, it will be much easier to actually use IsValid() in tests, and I'll be happy to test all permutations of valid/not-valid cases that we need.
https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... File content/common/experiments/api_key.cc (right): https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... content/common/experiments/api_key.cc:8: #include <openssl/curve25519.h> Nit: I believe this should be #include <openssl/curve25519.h> #include <vector> since it's a C header. (Actually, I want to switch us to including BoringSSL headers as #include "third_party/blah", but I'll do that separately.) https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... content/common/experiments/api_key.cc:123: // TODO: verify signature length is 64 That seems pretty important to do. Otherwise you'll read past the end of some buffer. :-) https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... File content/common/experiments/api_key.h (right): https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... content/common/experiments/api_key.h:59: bool ValidateSignature(const uint8_t* public_key) const; Passing in a bare pointer without some indication of length seems iffy. I would suggest one of: - Use a base::StringPiece and check that the length is 32 in the function. - Make the parameter a const uint8_t public_key[32] which at least tells the reader they're responsible for passing in something of suitable size. (This doesn't make a copy. C is weird.) https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... File content/common/experiments/api_key_unittest.cc (right): https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... content/common/experiments/api_key_unittest.cc:29: */ Nit: I think // C++-style comments are more common. https://codereview.chromium.org/1522813002/diff/220001/content/content_common... File content/content_common.gypi (right): https://codereview.chromium.org/1522813002/diff/220001/content/content_common... content/content_common.gypi:638: '../third_party/boringssl/boringssl.gyp:boringssl', corresponding BUILD.gn change?
https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... File content/common/experiments/api_key.cc (right): https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... content/common/experiments/api_key.cc:8: #include <openssl/curve25519.h> On 2016/01/11 20:18:57, davidben wrote: > Nit: I believe this should be > > #include <openssl/curve25519.h> > > #include <vector> > > since it's a C header. Thanks, done. > (Actually, I want to switch us to including BoringSSL > headers as #include "third_party/blah", but I'll do that separately.) (Yeah, I tried that, but DEPS said no. :( ) https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... content/common/experiments/api_key.cc:123: // TODO: verify signature length is 64 On 2016/01/11 20:18:57, davidben wrote: > That seems pretty important to do. Otherwise you'll read past the end of some > buffer. :-) Done. https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... File content/common/experiments/api_key.h (right): https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... content/common/experiments/api_key.h:59: bool ValidateSignature(const uint8_t* public_key) const; On 2016/01/11 20:18:57, davidben wrote: > Passing in a bare pointer without some indication of length seems iffy. I would > suggest one of: > - Use a base::StringPiece and check that the length is 32 in the function. > - Make the parameter a const uint8_t public_key[32] which at least tells the > reader they're responsible for passing in something of suitable size. (This > doesn't make a copy. C is weird.) I can certainly switch to StringPiece if that makes you feel better about it :) The code is only called by this class, with a reference to the included public key, and by tests, with a pointer to the public key in *that* class. It's not a public method. In the future, though, it will need to deal with data provided by the /chrome/ layer, so I may as well switch it now. (I had a length parameter, but it wasn't being used in any meaningful way, so it was removed. Also, annotating the parameter size in the declaration looks like a fun hack, but it's really no better than a code comment, I think.) https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... File content/common/experiments/api_key_unittest.cc (right): https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... content/common/experiments/api_key_unittest.cc:29: */ On 2016/01/11 20:18:57, davidben wrote: > Nit: I think // C++-style comments are more common. Switched. When this was a PEM-formatted RSA key, I wanted to make sure that these lines could be cut-and-pasted into a text file by anyone. Now that it's formatted this way, it doesn't matter so much. https://codereview.chromium.org/1522813002/diff/220001/content/content_common... File content/content_common.gypi (right): https://codereview.chromium.org/1522813002/diff/220001/content/content_common... content/content_common.gypi:638: '../third_party/boringssl/boringssl.gyp:boringssl', On 2016/01/11 20:18:57, davidben wrote: > corresponding BUILD.gn change? Thanks, done -- looks like libcontent.so was picking up the dependency transitively somehow, but I added it to BUILD.gn to make it explicit now.
lgtm https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... File content/common/experiments/api_key.cc (right): https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... content/common/experiments/api_key.cc:8: #include <openssl/curve25519.h> On 2016/01/12 14:52:49, iclelland wrote: > (Yeah, I tried that, but DEPS said no. :( ) Yeah, part of my motivation for switching it to the other one is so we can actually start putting third_party/boringssl into DEPS files. Right now it just doesn't enforce anything because it thinks it's a system header. (Not as something for you to do now. This is a thing that's in my queue.) https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... File content/common/experiments/api_key.h (right): https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... content/common/experiments/api_key.h:59: bool ValidateSignature(const uint8_t* public_key) const; On 2016/01/12 14:52:49, iclelland wrote: > On 2016/01/11 20:18:57, davidben wrote: > > Passing in a bare pointer without some indication of length seems iffy. I > would > > suggest one of: > > - Use a base::StringPiece and check that the length is 32 in the function. > > - Make the parameter a const uint8_t public_key[32] which at least tells the > > reader they're responsible for passing in something of suitable size. (This > > doesn't make a copy. C is weird.) > > I can certainly switch to StringPiece if that makes you feel better about it :) > The code is only called by this class, with a reference to the included public > key, and by tests, with a pointer to the public key in *that* class. It's not a > public method. > > In the future, though, it will need to deal with data provided by the /chrome/ > layer, so I may as well switch it now. > > (I had a length parameter, but it wasn't being used in any meaningful way, so it > was removed. Also, annotating the parameter size in the declaration looks like a > fun hack, but it's really no better than a code comment, I think.) I'm fine with whatever. StringPiece or code comment or the funny array thing. Also occurs to me that const uint8_t (&public_key)[32] would let the compiler check it. *shrug* I just wanted there to be some indication as to what the size is supposed to be. Being protected, it's part of your class's API for (test-only) subclasses. https://codereview.chromium.org/1522813002/diff/240001/content/common/experim... File content/common/experiments/api_key.cc (right): https://codereview.chromium.org/1522813002/diff/240001/content/common/experim... content/common/experiments/api_key.cc:123: // Public key must be 32 bytes long for Ed25519 Nit: Period at end. https://codereview.chromium.org/1522813002/diff/240001/content/common/experim... content/common/experiments/api_key.cc:124: DCHECK(public_key.length() == 32); Nit: DCHECK_EQ. Or perhaps something that works at runtime (CHECK or return false) if you're planning that this be callable by other files.
https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... File content/common/experiments/api_key.cc (right): https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... content/common/experiments/api_key.cc:8: #include <openssl/curve25519.h> On 2016/01/13 20:50:39, davidben wrote: > On 2016/01/12 14:52:49, iclelland wrote: > > (Yeah, I tried that, but DEPS said no. :( ) > > Yeah, part of my motivation for switching it to the other one is so we can > actually start putting third_party/boringssl into DEPS files. Right now it just > doesn't enforce anything because it thinks it's a system header. > > (Not as something for you to do now. This is a thing that's in my queue.) Acknowledged. https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... File content/common/experiments/api_key.h (right): https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... content/common/experiments/api_key.h:59: bool ValidateSignature(const uint8_t* public_key) const; On 2016/01/13 20:50:39, davidben wrote: > On 2016/01/12 14:52:49, iclelland wrote: > > On 2016/01/11 20:18:57, davidben wrote: > > > Passing in a bare pointer without some indication of length seems iffy. I > > would > > > suggest one of: > > > - Use a base::StringPiece and check that the length is 32 in the function. > > > - Make the parameter a const uint8_t public_key[32] which at least tells the > > > reader they're responsible for passing in something of suitable size. (This > > > doesn't make a copy. C is weird.) > > > > I can certainly switch to StringPiece if that makes you feel better about it > :) > > The code is only called by this class, with a reference to the included public > > key, and by tests, with a pointer to the public key in *that* class. It's not > a > > public method. > > > > In the future, though, it will need to deal with data provided by the /chrome/ > > layer, so I may as well switch it now. > > > > (I had a length parameter, but it wasn't being used in any meaningful way, so > it > > was removed. Also, annotating the parameter size in the declaration looks like > a > > fun hack, but it's really no better than a code comment, I think.) > > I'm fine with whatever. StringPiece or code comment or the funny array thing. > Also occurs to me that const uint8_t (&public_key)[32] would let the compiler > check it. > > *shrug* I just wanted there to be some indication as to what the size is > supposed to be. Being protected, it's part of your class's API for (test-only) > subclasses. I'm good with StringPiece (other than all of the reinterpret_casts needed to use it to uint_8 data). It may evolve into a PublicKey class at some point, but for now it's just good to be able to pass around a single *thing* with an intrinsic length property. https://codereview.chromium.org/1522813002/diff/240001/content/common/experim... File content/common/experiments/api_key.cc (right): https://codereview.chromium.org/1522813002/diff/240001/content/common/experim... content/common/experiments/api_key.cc:123: // Public key must be 32 bytes long for Ed25519 On 2016/01/13 20:50:39, davidben wrote: > Nit: Period at end. Done. https://codereview.chromium.org/1522813002/diff/240001/content/common/experim... content/common/experiments/api_key.cc:124: DCHECK(public_key.length() == 32); On 2016/01/13 20:50:39, davidben wrote: > Nit: DCHECK_EQ. Or perhaps something that works at runtime (CHECK or return > false) if you're planning that this be callable by other files. Done, thanks.
On 2016/01/13 21:30:11, iclelland wrote: > https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... > File content/common/experiments/api_key.cc (right): > > https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... > content/common/experiments/api_key.cc:8: #include <openssl/curve25519.h> > On 2016/01/13 20:50:39, davidben wrote: > > On 2016/01/12 14:52:49, iclelland wrote: > > > (Yeah, I tried that, but DEPS said no. :( ) > > > > Yeah, part of my motivation for switching it to the other one is so we can > > actually start putting third_party/boringssl into DEPS files. Right now it > just > > doesn't enforce anything because it thinks it's a system header. > > > > (Not as something for you to do now. This is a thing that's in my queue.) > > Acknowledged. > > https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... > File content/common/experiments/api_key.h (right): > > https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... > content/common/experiments/api_key.h:59: bool ValidateSignature(const uint8_t* > public_key) const; > On 2016/01/13 20:50:39, davidben wrote: > > On 2016/01/12 14:52:49, iclelland wrote: > > > On 2016/01/11 20:18:57, davidben wrote: > > > > Passing in a bare pointer without some indication of length seems iffy. I > > > would > > > > suggest one of: > > > > - Use a base::StringPiece and check that the length is 32 in the function. > > > > - Make the parameter a const uint8_t public_key[32] which at least tells > the > > > > reader they're responsible for passing in something of suitable size. > (This > > > > doesn't make a copy. C is weird.) > > > > > > I can certainly switch to StringPiece if that makes you feel better about it > > :) > > > The code is only called by this class, with a reference to the included > public > > > key, and by tests, with a pointer to the public key in *that* class. It's > not > > a > > > public method. > > > > > > In the future, though, it will need to deal with data provided by the > /chrome/ > > > layer, so I may as well switch it now. > > > > > > (I had a length parameter, but it wasn't being used in any meaningful way, > so > > it > > > was removed. Also, annotating the parameter size in the declaration looks > like > > a > > > fun hack, but it's really no better than a code comment, I think.) > > > > I'm fine with whatever. StringPiece or code comment or the funny array thing. > > Also occurs to me that const uint8_t (&public_key)[32] would let the compiler > > check it. > > > > *shrug* I just wanted there to be some indication as to what the size is > > supposed to be. Being protected, it's part of your class's API for (test-only) > > subclasses. > > I'm good with StringPiece (other than all of the reinterpret_casts needed to use > it to uint_8 data). It may evolve into a PublicKey class at some point, but for > now it's just good to be able to pass around a single *thing* with an intrinsic > length property. > > https://codereview.chromium.org/1522813002/diff/240001/content/common/experim... > File content/common/experiments/api_key.cc (right): > > https://codereview.chromium.org/1522813002/diff/240001/content/common/experim... > content/common/experiments/api_key.cc:123: // Public key must be 32 bytes long > for Ed25519 > On 2016/01/13 20:50:39, davidben wrote: > > Nit: Period at end. > > Done. > > https://codereview.chromium.org/1522813002/diff/240001/content/common/experim... > content/common/experiments/api_key.cc:124: DCHECK(public_key.length() == 32); > On 2016/01/13 20:50:39, davidben wrote: > > Nit: DCHECK_EQ. Or perhaps something that works at runtime (CHECK or return > > false) if you're planning that this be callable by other files. > > Done, thanks. SLGTM.
https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... File content/common/experiments/api_key.h (right): https://codereview.chromium.org/1522813002/diff/220001/content/common/experim... content/common/experiments/api_key.h:59: bool ValidateSignature(const uint8_t* public_key) const; On 2016/01/13 21:30:11, iclelland wrote: > I'm good with StringPiece (other than all of the reinterpret_casts needed to use > it to uint_8 data). It may evolve into a PublicKey class at some point, but for > now it's just good to be able to pass around a single *thing* with an intrinsic > length property. Yeah, having to add those casts drives me crazy. :-) (I will never forgive you, C, for signed char...) There's https://crbug.com/559302 but it's still unresolved.
The CQ bit was checked by iclelland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chasej@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1522813002/#ps260001 (title: "Addressing comments from PS#13")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1522813002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1522813002/260001
Message was sent while issue was closed.
Description was changed from ========== Add public key and signature verification to browser-side API keys This adds the ability for experimental API keys to be cryptographically verified in the browser. There is currently a single fixed signing key, whose public key is embeddded in the browser code. This makes use of Ed25519 cryptographic signature verification in boringssl, so boringssl is added as a dependency to content/common for all platforms (except iOS) BUG=543215 ========== to ========== Add public key and signature verification to browser-side API keys This adds the ability for experimental API keys to be cryptographically verified in the browser. There is currently a single fixed signing key, whose public key is embeddded in the browser code. This makes use of Ed25519 cryptographic signature verification in boringssl, so boringssl is added as a dependency to content/common for all platforms (except iOS) BUG=543215 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Add public key and signature verification to browser-side API keys This adds the ability for experimental API keys to be cryptographically verified in the browser. There is currently a single fixed signing key, whose public key is embeddded in the browser code. This makes use of Ed25519 cryptographic signature verification in boringssl, so boringssl is added as a dependency to content/common for all platforms (except iOS) BUG=543215 ========== to ========== Add public key and signature verification to browser-side API keys This adds the ability for experimental API keys to be cryptographically verified in the browser. There is currently a single fixed signing key, whose public key is embeddded in the browser code. This makes use of Ed25519 cryptographic signature verification in boringssl, so boringssl is added as a dependency to content/common for all platforms (except iOS) BUG=543215 Committed: https://crrev.com/592c59e49f7a1c1ce7fb469dd1f7ef0d98482ad8 Cr-Commit-Position: refs/heads/master@{#369329} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/592c59e49f7a1c1ce7fb469dd1f7ef0d98482ad8 Cr-Commit-Position: refs/heads/master@{#369329} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
