Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23710003:
Added NetworkingPrivateCrypto and its unit test. (Closed)
|
Created:
7 years, 3 months ago by mef Modified:
7 years, 3 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded NetworkingPrivateCrypto for crypto part of networking private api and its unit test.
BUG=267667
TEST=unit_tests --gtest_filter=NetworkingPrivateCryptoTest.*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221505
Patch Set 1 #
Total comments: 52
Patch Set 2 : Refactor to follow Ryan's suggestions. #
Total comments: 22
Patch Set 3 : Address code review comments.. #Patch Set 4 : Address codereview comments. #Patch Set 5 : Use VFY_VerifyDataDirect as VFY_VerifyData is deprecated. #
Total comments: 10
Patch Set 6 : Code cleanup and more unit tests. #
Total comments: 44
Patch Set 7 : Address code review comments. #Patch Set 8 : Change kTrustedCAPublicKeyDER to byte array instead of base64 string. #
Total comments: 3
Patch Set 9 : Fixed lint warnings. #
Total comments: 6
Messages
Total messages: 28 (0 generated)
Ryan, could you take a look at my port of crypto stuff for networking private api? I have a bunch of questions, and would really appreciate your comments. thanks, -m https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/networking_private/networking_private_crypto.cc (right): https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:87: std::string subject_name = cl[0]->subject().GetDisplayName(); Alternatively I can get subject name directly from cert_ below. Would it make code cleaner? https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:128: crypto::PrivateKeyInfoCodec trusted_ca_key_codec(true); crypto::PrivateKeyInfoCodec is causing linking errors on Windows. Is there some alternative/easier way to create public key from its modulus and exponent? https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:144: verified = CERT_VerifySignedDataWithPublicKey( Is it ok to add these to third_party/nss/nss/exports_win.def? https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:166: 0x1a, 0x05, 0x00, 0x04, 0x14}, }; Is there a better way to add ASN1 prefix in front of hash data? https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:210: SECStatus encrypted = PK11_PubEncryptPKCS1( Is it ok to add this to third_party/nss/nss/exports_win.def? https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:246: crypto::RSAPrivateKey::CreateFromPrivateKeyInfo(private_key_data)); This returns a failure from ASN.1 parser, any suggestions on troubleshooting / fixing it? The NetworkingPrivateCrypto::DecryptByteString function is only used for testing, so I can change private_key to use different format. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/networking_private/networking_private_crypto.h (right): https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:55: SECKEYPublicKey* ca_public_key_; These member variables are here only to ease the cleanup (in destructor). Is there some better way to employ something like scoped_ptr for NSS objects? https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc (right): https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:117: EXPECT_TRUE(key->ExportPublicKey(&pubkey_data)); The exported pubkey_data isn't liked by SECKEY_ImportDERPublicKey inside of EncryptByteString.
Quick publishing some drafts while I switch to non-Canary, as it's too broken atm. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/networking_private/networking_private_crypto.cc (right): https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:87: std::string subject_name = cl[0]->subject().GetDisplayName(); On 2013/08/28 18:05:59, mef wrote: > Alternatively I can get subject name directly from cert_ below. Would it make > code cleaner? The above is really only intended for display purposes, not for any security checks (as the class mentions) My biggest concern here is that it feels that the order of operations here is incorrect. You should first verify the signature on the certificate (so that you know you can trust the fields - including subject), and then perform the remaining sanity checks. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:128: crypto::PrivateKeyInfoCodec trusted_ca_key_codec(true); On 2013/08/28 18:05:59, mef wrote: > crypto::PrivateKeyInfoCodec is causing linking errors on Windows. > Is there some alternative/easier way to create public key from its modulus and > exponent? Correct, it's not exported, because it's not part of the "public" API. Instead of storing the CAModulus and CAExponent, just store the CA SPKI. This makes it easier for all utilities to work with (eg: easy paste into command-line for use with tools like OpenSSL). That also fully avoids your dependency here on RSAPrivateKey - you can just import the DER directly. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:144: verified = CERT_VerifySignedDataWithPublicKey( On 2013/08/28 18:05:59, mef wrote: > Is it ok to add these to third_party/nss/nss/exports_win.def? Use http://mxr.mozilla.org/nss/source/lib/nss/nss.def#699 (CERT_VerifySignedDataWithPublicKeyInfo) and entirely avoid the import phase. And yes, it's fine to add either of these functions to nss.def (as they're in the upstream NSS.def). I would just add the one you need - which should be the PublicKeyInfo one. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:155: cert_public_key_ = CERT_ExtractPublicKey(cert_); Please go through the comments for "we" and look to remove it. In most cases, it should be very simple, and will make the comments much cleaner. https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3... https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:166: 0x1a, 0x05, 0x00, 0x04, 0x14}, }; On 2013/08/28 18:05:59, mef wrote: > Is there a better way to add ASN1 prefix in front of hash data? Yeah, don't use this API - you're totally using the wrong abstraction :) VFY_VerifyDigestDirect https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:210: SECStatus encrypted = PK11_PubEncryptPKCS1( On 2013/08/28 18:05:59, mef wrote: > Is it ok to add this to third_party/nss/nss/exports_win.def? Yes, this is part of nss.def.
https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/networking_private/networking_private_crypto.cc (right): https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:98: subject_name.substr(subject_mac_pos + 1).c_str())) { why not just use std::string::compare? https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:104: if (!GetDERFromPEM(certificate, "CERTIFICATE", &cert_data)) { Why are you parsing the certificate twice like this? First into the OS handle (which won't work in the sandbox), and then into an NSS handle. Pick one format and stick with it. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:255: unsigned output_length = 0; Blergh, poor API spec if we're calling it 'unsigned' rather than 'unsigned int'. Sad times. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/networking_private/networking_private_crypto.h (right): https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:22: // Verify that the destination described by |certificate| is valid. nit: line break between dtor. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:26: // trusted CA. comment nit: drop our. Explain exactly what the requirements are without the use of possessive pronouns. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:28: // the public key in |certificate|. nit: "Verify that the destination" describes what it does, but then you say "VerifyCredentials" for the name. It seems like the comment and name should be in alignment. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:36: // RSAPublicKey format. |data| is some string of bytes smaller than the nit: comment style - awkward line wrap. nit: Use StringPiece rather than string? https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:40: // success. Returns false on failure. nit: Returns true on success, storing the encrypted result in |encrypted_output|. drop the "Returns false" https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:46: // PEM-encoded private key. |encrypted_data| is data encrypted with nit: "PEM-encoded private key" is an undefined format. Please clarify. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:49: // success. Returns false on failure. nit: same comments as above apply here. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:55: SECKEYPublicKey* ca_public_key_; On 2013/08/28 18:05:59, mef wrote: > These member variables are here only to ease the cleanup (in destructor). Is > there some better way to employ something like scoped_ptr for NSS objects? Using the NSS objects directly here presumes !Android, which is a big presumption, however accurate. It would be better to use a pimpl/context type that you keep hidden here. eg: private: struct [some name here]; scoped_ptr<[some name here]> context_; The compiler should (IIRC) be able to undo the double pointer indirection. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:58: SECKEYPublicKey* enc_public_key_; nit: Naming enc_ is not a clear abbreviation. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc (right): https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:17: nit: unnecessary line break https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:20: "CERTIFICATE-----" why this line break? https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:28: "13y/" or this, etc https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:51: const char* name = "eureka8997"; All of these should be: "static const char kFoo[] = "string" https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:72: "KbYlSqaF/l8CXmzDJTyO7tDOFK59bV9auE4KljrQ==", Give these strings meaningful symbolic names in the test static const char kFoo[] = "data";
Ryan, thanks a lot for your suggestions! PTAL, hopefully it is not as terrible as before. thanks, -m https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/networking_private/networking_private_crypto.cc (right): https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:87: std::string subject_name = cl[0]->subject().GetDisplayName(); On 2013/08/28 18:31:26, Ryan Sleevi wrote: > On 2013/08/28 18:05:59, mef wrote: > > Alternatively I can get subject name directly from cert_ below. Would it make > > code cleaner? > > The above is really only intended for display purposes, not for any security > checks (as the class mentions) > > My biggest concern here is that it feels that the order of operations here is > incorrect. You should first verify the signature on the certificate (so that you > know you can trust the fields - including subject), and then perform the > remaining sanity checks. Done. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:98: subject_name.substr(subject_mac_pos + 1).c_str())) { On 2013/08/28 19:02:37, Ryan Sleevi wrote: > why not just use std::string::compare? Done. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:104: if (!GetDERFromPEM(certificate, "CERTIFICATE", &cert_data)) { On 2013/08/28 19:02:37, Ryan Sleevi wrote: > Why are you parsing the certificate twice like this? First into the OS handle > (which won't work in the sandbox), and then into an NSS handle. > > Pick one format and stick with it. Done. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:128: crypto::PrivateKeyInfoCodec trusted_ca_key_codec(true); On 2013/08/28 18:31:26, Ryan Sleevi wrote: > On 2013/08/28 18:05:59, mef wrote: > > crypto::PrivateKeyInfoCodec is causing linking errors on Windows. > > Is there some alternative/easier way to create public key from its modulus and > > exponent? > > Correct, it's not exported, because it's not part of the "public" API. > > Instead of storing the CAModulus and CAExponent, just store the CA SPKI. This > makes it easier for all utilities to work with (eg: easy paste into command-line > for use with tools like OpenSSL). > > That also fully avoids your dependency here on RSAPrivateKey - you can just > import the DER directly. Done. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:144: verified = CERT_VerifySignedDataWithPublicKey( On 2013/08/28 18:31:26, Ryan Sleevi wrote: > On 2013/08/28 18:05:59, mef wrote: > > Is it ok to add these to third_party/nss/nss/exports_win.def? > > Use http://mxr.mozilla.org/nss/source/lib/nss/nss.def#699 > (CERT_VerifySignedDataWithPublicKeyInfo) and entirely avoid the import phase. > > And yes, it's fine to add either of these functions to nss.def (as they're in > the upstream NSS.def). I would just add the one you need - which should be the > PublicKeyInfo one. Done. I did go with CERT_VerifySignedDataWithPublicKey as neither of them is already exported and I have the SECKEYPublicKey handy. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:155: cert_public_key_ = CERT_ExtractPublicKey(cert_); On 2013/08/28 18:31:26, Ryan Sleevi wrote: > Please go through the comments for "we" and look to remove it. > > In most cases, it should be very simple, and will make the comments much > cleaner. > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3... Done. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:166: 0x1a, 0x05, 0x00, 0x04, 0x14}, }; On 2013/08/28 18:31:26, Ryan Sleevi wrote: > On 2013/08/28 18:05:59, mef wrote: > > Is there a better way to add ASN1 prefix in front of hash data? > > Yeah, don't use this API - you're totally using the wrong abstraction :) > > VFY_VerifyDigestDirect Done. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:255: unsigned output_length = 0; On 2013/08/28 19:02:37, Ryan Sleevi wrote: > Blergh, poor API spec if we're calling it 'unsigned' rather than 'unsigned int'. > Sad times. Done. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/networking_private/networking_private_crypto.h (right): https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:22: // Verify that the destination described by |certificate| is valid. On 2013/08/28 19:02:37, Ryan Sleevi wrote: > nit: line break between dtor. Done. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:26: // trusted CA. On 2013/08/28 19:02:37, Ryan Sleevi wrote: > comment nit: drop our. Explain exactly what the requirements are without the use > of possessive pronouns. Done. This comment was copied from original code. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:28: // the public key in |certificate|. On 2013/08/28 19:02:37, Ryan Sleevi wrote: > nit: "Verify that the destination" describes what it does, but then you say > "VerifyCredentials" for the name. It seems like the comment and name should be > in alignment. Done. This comment was copied from original code. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:36: // RSAPublicKey format. |data| is some string of bytes smaller than the On 2013/08/28 19:02:37, Ryan Sleevi wrote: > nit: comment style - awkward line wrap. > nit: Use StringPiece rather than string? Done. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:40: // success. Returns false on failure. On 2013/08/28 19:02:37, Ryan Sleevi wrote: > nit: Returns true on success, storing the encrypted result in > |encrypted_output|. > > drop the "Returns false" Done. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:46: // PEM-encoded private key. |encrypted_data| is data encrypted with On 2013/08/28 19:02:37, Ryan Sleevi wrote: > nit: "PEM-encoded private key" is an undefined format. Please clarify. Done. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:49: // success. Returns false on failure. On 2013/08/28 19:02:37, Ryan Sleevi wrote: > nit: same comments as above apply here. Done. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:55: SECKEYPublicKey* ca_public_key_; On 2013/08/28 19:02:37, Ryan Sleevi wrote: > On 2013/08/28 18:05:59, mef wrote: > > These member variables are here only to ease the cleanup (in destructor). Is > > there some better way to employ something like scoped_ptr for NSS objects? > > Using the NSS objects directly here presumes !Android, which is a big > presumption, however accurate. > > It would be better to use a pimpl/context type that you keep hidden here. eg: > > private: > struct [some name here]; > scoped_ptr<[some name here]> context_; > > The compiler should (IIRC) be able to undo the double pointer indirection. Done. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:58: SECKEYPublicKey* enc_public_key_; On 2013/08/28 19:02:37, Ryan Sleevi wrote: > nit: Naming > > enc_ is not a clear abbreviation. Done. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc (right): https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:20: "CERTIFICATE-----" On 2013/08/28 19:02:37, Ryan Sleevi wrote: > why this line break? Done. Clang format. I think it tries to break at non-letters. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:28: "13y/" On 2013/08/28 19:02:37, Ryan Sleevi wrote: > or this, etc Same as above. Is there an easy way to re-align? https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:51: const char* name = "eureka8997"; On 2013/08/28 19:02:37, Ryan Sleevi wrote: > All of these should be: > "static const char kFoo[] = "string" Done. https://codereview.chromium.org/23710003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:72: "KbYlSqaF/l8CXmzDJTyO7tDOFK59bV9auE4KljrQ==", On 2013/08/28 19:02:37, Ryan Sleevi wrote: > Give these strings meaningful symbolic names in the test > > static const char kFoo[] = "data"; Done.
https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... File chrome/browser/extensions/api/networking_private/networking_private_crypto.cc (right): https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:32: "o6pZFdHL1m68xg3IZ0ys/4khyYfVf6YUeeq4C35EiAKpLFGwIDAQAB"; Please fix this by hand. Clang format is crap here. https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:55: CERTCertificate* cert; D'oh. I missed that all of these could be locally scoped. see src/crypto/scoped_nss_types.h for the helper functions to be used here. https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:117: char* common_name = CERT_GetCommonName(&ctx.cert->subject); BUG: This can return NULL. https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:118: std::string subject_name = common_name; Use ctor form, not assignment form. https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:146: verified = VFY_VerifyDigestDirect(&digest_item, Sorry for the misdirect. VFY_VerifyData will let you avoid the HASH_HashBuf part. VFY_VerifyData(unsigned_data.c_str(), unsigned_data.size(), public_key, &signature_item, SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION, NULL); https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:207: if (!private_key) { nit: if (!private_key || !private_key->public_key()) https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... File chrome/browser/extensions/api/networking_private/networking_private_crypto.h (right): https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:50: DISALLOW_COPY_AND_ASSIGN(NetworkingPrivateCrypto); This should be private: https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... File chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc (right): https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:40: "-----END CERTIFICATE-----\n"; Please ignore clang-format here and format it a bit more intelligently by hand if needed. vim/emacs should make this easy, hopefully. Things like "\n" on line 18 are really awkward, as are all the embedded "\n" at the 64-char mark. https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:42: // const char* mac_address = "4C:AA:16:A5:AC:DF"; nit: outdated comment? https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:73: static const char kPublicKey[] = "MIGJAoGBANTjeoILNkSKHVkd3my/" nit: place this on a newline, and reflow/reformat. https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:109: EXPECT_TRUE(decrypted == plain); Why not EXPECT_EQ ?
Hi Ryan, PTAL. thanks, -m https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... File chrome/browser/extensions/api/networking_private/networking_private_crypto.cc (right): https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:32: "o6pZFdHL1m68xg3IZ0ys/4khyYfVf6YUeeq4C35EiAKpLFGwIDAQAB"; On 2013/08/28 21:52:29, Ryan Sleevi wrote: > Please fix this by hand. Clang format is crap here. Done. Is there actually a better way to define binary constants than base64? https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:55: CERTCertificate* cert; On 2013/08/28 21:52:29, Ryan Sleevi wrote: > D'oh. I missed that all of these could be locally scoped. > > see src/crypto/scoped_nss_types.h for the helper functions to be used here. Done. Nice! https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:117: char* common_name = CERT_GetCommonName(&ctx.cert->subject); On 2013/08/28 21:52:29, Ryan Sleevi wrote: > BUG: This can return NULL. Done. https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:118: std::string subject_name = common_name; On 2013/08/28 21:52:29, Ryan Sleevi wrote: > Use ctor form, not assignment form. Done. https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:146: verified = VFY_VerifyDigestDirect(&digest_item, On 2013/08/28 21:52:29, Ryan Sleevi wrote: > Sorry for the misdirect. VFY_VerifyData will let you avoid the HASH_HashBuf > part. > > VFY_VerifyData(unsigned_data.c_str(), unsigned_data.size(), public_key, > &signature_item, SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION, NULL); Done. https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:207: if (!private_key) { On 2013/08/28 21:52:29, Ryan Sleevi wrote: > nit: if (!private_key || !private_key->public_key()) Done. https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... File chrome/browser/extensions/api/networking_private/networking_private_crypto.h (right): https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:50: DISALLOW_COPY_AND_ASSIGN(NetworkingPrivateCrypto); On 2013/08/28 21:52:29, Ryan Sleevi wrote: > This should be private: Done. https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... File chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc (right): https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:40: "-----END CERTIFICATE-----\n"; On 2013/08/28 21:52:29, Ryan Sleevi wrote: > Please ignore clang-format here and format it a bit more intelligently by hand > if needed. > > vim/emacs should make this easy, hopefully. Things like "\n" on line 18 are > really awkward, as are all the embedded "\n" at the 64-char mark. Done. https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:42: // const char* mac_address = "4C:AA:16:A5:AC:DF"; On 2013/08/28 21:52:29, Ryan Sleevi wrote: > nit: outdated comment? Done. https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:73: static const char kPublicKey[] = "MIGJAoGBANTjeoILNkSKHVkd3my/" On 2013/08/28 21:52:29, Ryan Sleevi wrote: > nit: place this on a newline, and reflow/reformat. Done. https://codereview.chromium.org/23710003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:109: EXPECT_TRUE(decrypted == plain); On 2013/08/28 21:52:29, Ryan Sleevi wrote: > Why not EXPECT_EQ ? Done.
Almost there! https://codereview.chromium.org/23710003/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto.cc (right): https://codereview.chromium.org/23710003/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:55: ScopedCERTCertificate; Clang-format messing up again. May be worth filing a bug typedef scoped_ptr_malloc< CERTCertificate, crypto::NSSDestroyer<CERTCertificate, CERT_DestroyCertificate> > ScopedCERTCertificate; https://codereview.chromium.org/23710003/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:119: // Verify that hash(unsigned_data) == public(signed_data) This comment doesn't really make sense, especially since it's technically 'signed_data' and 'signature' that you're using here // Verify that the signature matches signed_data https://codereview.chromium.org/23710003/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto.h (right): https://codereview.chromium.org/23710003/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:28: const std::string& unsigned_data, naming: "signed_data" and "unsigned_data" should be renamed "signature" and "data" (or signed_data), respectively. 3) |signature| is a valid signature for |data|, using the public key in |certificate| https://codereview.chromium.org/23710003/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:49: private: style nit: Line break between 48 & 49 https://codereview.chromium.org/23710003/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc (right): https://codereview.chromium.org/23710003/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:17: "-----BEGIN CERTIFICATE-----\n" Did you mean to include \n everywhere? A little weird with lines 69 & 83 as well.
Hi Ryan, PTAL. Greg & Chris, please review the port of crypto functionality from chromeos_public//src/platform/shill/shims/crypto_util.cc to NSS for use in Chrome on Windows & Mac. thanks, -m https://codereview.chromium.org/23710003/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto.cc (right): https://codereview.chromium.org/23710003/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:55: ScopedCERTCertificate; On 2013/08/29 21:30:43, Ryan Sleevi wrote: > Clang-format messing up again. May be worth filing a bug > > typedef scoped_ptr_malloc< > CERTCertificate, > crypto::NSSDestroyer<CERTCertificate, > CERT_DestroyCertificate> > > ScopedCERTCertificate; Done. https://codereview.chromium.org/23710003/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:119: // Verify that hash(unsigned_data) == public(signed_data) On 2013/08/29 21:30:43, Ryan Sleevi wrote: > This comment doesn't really make sense, especially since it's technically > 'signed_data' and 'signature' that you're using here > > // Verify that the signature matches signed_data Done. https://codereview.chromium.org/23710003/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto.h (right): https://codereview.chromium.org/23710003/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:28: const std::string& unsigned_data, On 2013/08/29 21:30:43, Ryan Sleevi wrote: > naming: "signed_data" and "unsigned_data" should be renamed "signature" and > "data" (or signed_data), respectively. > > 3) |signature| is a valid signature for |data|, using the public key in > |certificate| Done. https://codereview.chromium.org/23710003/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:49: private: On 2013/08/29 21:30:43, Ryan Sleevi wrote: > style nit: Line break between 48 & 49 Done. https://codereview.chromium.org/23710003/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc (right): https://codereview.chromium.org/23710003/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:17: "-----BEGIN CERTIFICATE-----\n" On 2013/08/29 21:30:43, Ryan Sleevi wrote: > Did you mean to include \n everywhere? A little weird with lines 69 & 83 as > well. Done. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc (right): https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:148: EXPECT_TRUE(crypto.EncryptByteString(public_key, plain, &encrypted_output)); Should 0 length data be encrypted correctly?
https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto.cc (right): https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:26: const char kTrustedCAPublicKeyDER[] = What is the origin of this CA key, and why is it hard coded here? ...and just a reminder that this code review is publicly viewable (which should only really matter if we're doing it wrong in the first place :-) ). https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:34: bool GetDERFromPEM(const std::string& pem_data, Document what this function does, and what the parameters are. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:51: typedef scoped_ptr_malloc< Document what this typedef is used for, and why it's in the crypto namespace. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:53: crypto::NSSDestroyer<CERTCertificate, If this is in the crypto namespace, you don't need to have crypto:: here. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:87: base::Base64Decode(kTrustedCAPublicKeyDER, &ca_key_der); Couldn't you just do this once, and store it for next time? https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:97: LOG(ERROR) << "Certificate is not issued by trusted CA."; "by the trusted CA"? https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto.h (right): https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:14: class NetworkingPrivateCrypto { Document what the function of this class is, and why it exists. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc (right): https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:15: TEST(NetworkingPrivateCryptoTest, VerifyCredentials) { Document what this test is supposed to test. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:16: static const char kCertData[] = You might want to store the certificates as test data files in chrome/test/data/networking_private_crypto, and use something like this to load them. Your call though: having all the data in the source file isn't too bad yet, but if we added more tests, it might get crazy... base::FilePath GetTestDir() { base::FilePath test_dir; PathService::Get(chrome::DIR_TEST_DATA, &test_dir); test_dir = test_dir.AppendASCII("networking_private_crypto"); return test_dir; } std::string GetTestData(const base::FilePath::StringType& file) { std::string result; base::FilePath test_path = GetTestDir(); test_path = test_path.Append(file); file_util::ReadFileToString(test_path, &result); return result; } https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:104: TEST(NetworkingPrivateCryptoTest, EncryptString) { Document what this test is supposed to test. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:151: // EXPECT_TRUE( Remove commented out code before submit...
https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto.cc (right): https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:33: namespace { nit: line break between 33/34. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:34: bool GetDERFromPEM(const std::string& pem_data, nit: Add comment. // Parses |pem_data| for a PEM block of |pem_type|. // Returns true if a |pem_type| block is found, storing the decoded result in |der_output|. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:56: } // namespace crypto Don't stick this into crypto - you don't need to. This can just go in line 47/48. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:85: // Check that the certificate is signed by trusted CA. A little judicious use of vertical whitespace would help here. 84/85 and 77/78. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:87: base::Base64Decode(kTrustedCAPublicKeyDER, &ca_key_der); Since this is a fixed constant, it'd be much better just to store it in DER form in the constant - eg: don't base64 it. Be sure to update the type to a 'const unsigned char kTrustedCAPublicKeyDER[]', otherwise you'll get the compilers yakking at you for unsigned vs signed char constants. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:91: ca_key_der.size()}; style nit: this style of initializing a SECItem is pretty uncommon. We normally go for explicit intialization of the fields. Same throughout this file. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:157: if (encrypted_length < data.size() + 13) { Why are you using "13" here. This deserves a comment. Also note that RSAES is defined as operating on messages up to a length of k - 11, where k is the octet length of the RSA modulus (SECKEY_PublicKeyStrength - which returns the length in *bytes*) RFC 3447 7.2.1 defines the inputs. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto.h (right): https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:32: // in RSAPublicKey format. |data| is some string of bytes smaller than the comment nit: |public_key| is a DER-encoded RSAPublicKey. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:36: // success. comment nit: You can avoid the double returns (which reads a little weird) "Returns true on success, storing the encrypted result in |encrypted_output|" Same for 44/45. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:46: bool DecryptByteString(const std::string& private_key_pem, If this is only used for unittesting, make it private and friend test the tests. Better to not expose interfaces unless necessary.
Hi guys, thank you for your comments, PTAL! thanks, -m https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto.cc (right): https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:26: const char kTrustedCAPublicKeyDER[] = On 2013/08/30 18:38:17, Greg Spencer (Chromium) wrote: > What is the origin of this CA key, and why is it hard coded here? > > ...and just a reminder that this code review is publicly viewable (which should > only really matter if we're doing it wrong in the first place :-) ). Actually I'm not sure. I took it from original code, but I have no idea how it got there. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:33: namespace { On 2013/08/30 18:40:26, Ryan Sleevi wrote: > nit: line break between 33/34. Done. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:34: bool GetDERFromPEM(const std::string& pem_data, On 2013/08/30 18:40:26, Ryan Sleevi wrote: > nit: Add comment. > > // Parses |pem_data| for a PEM block of |pem_type|. > // Returns true if a |pem_type| block is found, storing the decoded result in > |der_output|. Done. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:34: bool GetDERFromPEM(const std::string& pem_data, On 2013/08/30 18:38:17, Greg Spencer (Chromium) wrote: > Document what this function does, and what the parameters are. Done. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:51: typedef scoped_ptr_malloc< On 2013/08/30 18:38:17, Greg Spencer (Chromium) wrote: > Document what this typedef is used for, and why it's in the crypto namespace. Removed per Ryan's suggestion. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:53: crypto::NSSDestroyer<CERTCertificate, On 2013/08/30 18:38:17, Greg Spencer (Chromium) wrote: > If this is in the crypto namespace, you don't need to have crypto:: here. Done. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:56: } // namespace crypto On 2013/08/30 18:40:26, Ryan Sleevi wrote: > Don't stick this into crypto - you don't need to. This can just go in line > 47/48. Done. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:87: base::Base64Decode(kTrustedCAPublicKeyDER, &ca_key_der); On 2013/08/30 18:40:26, Ryan Sleevi wrote: > Since this is a fixed constant, it'd be much better just to store it in DER form > in the constant - eg: don't base64 it. > > Be sure to update the type to a 'const unsigned char kTrustedCAPublicKeyDER[]', > otherwise you'll get the compilers yakking at you for unsigned vs signed char > constants. I was thinking about that, but couldn't think of an easy way to represent byte data. Should I use python to base64 decode and generate array of bytes like { 0x01, 0xab, etc } or is there a better way? https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:87: base::Base64Decode(kTrustedCAPublicKeyDER, &ca_key_der); On 2013/08/30 18:38:17, Greg Spencer (Chromium) wrote: > Couldn't you just do this once, and store it for next time? I could, but realistically this is not performance-bound function, and it is not going to be called more than 5 times during the session. Storing it somewhere would require a singleton, which live forever, which is not that great either. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:91: ca_key_der.size()}; On 2013/08/30 18:40:26, Ryan Sleevi wrote: > style nit: this style of initializing a SECItem is pretty uncommon. We normally > go for explicit intialization of the fields. > > Same throughout this file. D'Oh, I've found this style of initializing and converted all other cases to it. :-( https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:97: LOG(ERROR) << "Certificate is not issued by trusted CA."; On 2013/08/30 18:38:17, Greg Spencer (Chromium) wrote: > "by the trusted CA"? Done. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:157: if (encrypted_length < data.size() + 13) { On 2013/08/30 18:40:26, Ryan Sleevi wrote: > Why are you using "13" here. This deserves a comment. > > Also note that RSAES is defined as operating on messages up to a length of k - > 11, where k is the octet length of the RSA modulus (SECKEY_PublicKeyStrength - > which returns the length in *bytes*) > > RFC 3447 7.2.1 defines the inputs. Done. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto.h (right): https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:14: class NetworkingPrivateCrypto { On 2013/08/30 18:38:17, Greg Spencer (Chromium) wrote: > Document what the function of this class is, and why it exists. Done. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:32: // in RSAPublicKey format. |data| is some string of bytes smaller than the On 2013/08/30 18:40:26, Ryan Sleevi wrote: > comment nit: |public_key| is a DER-encoded RSAPublicKey. Done. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:36: // success. On 2013/08/30 18:40:26, Ryan Sleevi wrote: > comment nit: You can avoid the double returns (which reads a little weird) > "Returns true on success, storing the encrypted result in |encrypted_output|" > > Same for 44/45. Done. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:46: bool DecryptByteString(const std::string& private_key_pem, On 2013/08/30 18:40:26, Ryan Sleevi wrote: > If this is only used for unittesting, make it private and friend test the tests. > Better to not expose interfaces unless necessary. Done. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc (right): https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:15: TEST(NetworkingPrivateCryptoTest, VerifyCredentials) { On 2013/08/30 18:38:17, Greg Spencer (Chromium) wrote: > Document what this test is supposed to test. Done. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:16: static const char kCertData[] = On 2013/08/30 18:38:17, Greg Spencer (Chromium) wrote: > You might want to store the certificates as test data files in > chrome/test/data/networking_private_crypto, and use something like this to load > them. Your call though: having all the data in the source file isn't too bad > yet, but if we added more tests, it might get crazy... > > base::FilePath GetTestDir() { > base::FilePath test_dir; > PathService::Get(chrome::DIR_TEST_DATA, &test_dir); > test_dir = test_dir.AppendASCII("networking_private_crypto"); > return test_dir; > > } > > std::string GetTestData(const base::FilePath::StringType& file) { > std::string result; > base::FilePath test_path = GetTestDir(); > test_path = test_path.Append(file); > file_util::ReadFileToString(test_path, &result); > return result; > } This test data is directly copied from chromeos_public//.../network_DestinationVerification.py and it might make sense to preserve it in the same form. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:104: TEST(NetworkingPrivateCryptoTest, EncryptString) { On 2013/08/30 18:38:17, Greg Spencer (Chromium) wrote: > Document what this test is supposed to test. Done. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:151: // EXPECT_TRUE( On 2013/08/30 18:38:17, Greg Spencer (Chromium) wrote: > Remove commented out code before submit... Done.
https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto.cc (right): https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:87: base::Base64Decode(kTrustedCAPublicKeyDER, &ca_key_der); On 2013/08/30 20:07:17, mef wrote: > On 2013/08/30 18:38:17, Greg Spencer (Chromium) wrote: > > Couldn't you just do this once, and store it for next time? > I could, but realistically this is not performance-bound function, and it is not > going to be called more than 5 times during the session. > Storing it somewhere would require a singleton, which live forever, which is not > that great either. I like Ryan's idea better of just storing the DER encoded form in the first place.
Me too. Could you point me at example of how it is usually represented in source code? thanks, -m On Fri, Aug 30, 2013 at 4:11 PM, <gspencer@chromium.org> wrote: > > https://codereview.chromium.**org/23710003/diff/33001/** > chrome/browser/extensions/api/**networking_private/networking_** > private_crypto.cc<https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions/api/networking_private/networking_private_crypto.cc> > File > chrome/browser/extensions/api/**networking_private/networking_** > private_crypto.cc > (right): > > https://codereview.chromium.**org/23710003/diff/33001/** > chrome/browser/extensions/api/**networking_private/networking_** > private_crypto.cc#newcode87<https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions/api/networking_private/networking_private_crypto.cc#newcode87> > chrome/browser/extensions/api/**networking_private/networking_** > private_crypto.cc:87: > base::Base64Decode(**kTrustedCAPublicKeyDER, &ca_key_der); > On 2013/08/30 20:07:17, mef wrote: > >> On 2013/08/30 18:38:17, Greg Spencer (Chromium) wrote: >> > Couldn't you just do this once, and store it for next time? >> I could, but realistically this is not performance-bound function, and >> > it is not > >> going to be called more than 5 times during the session. >> Storing it somewhere would require a singleton, which live forever, >> > which is not > >> that great either. >> > > I like Ryan's idea better of just storing the DER encoded form in the > first place. > > https://codereview.chromium.**org/23710003/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Never mind, PTAL. https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto.cc (right): https://codereview.chromium.org/23710003/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:87: base::Base64Decode(kTrustedCAPublicKeyDER, &ca_key_der); On 2013/08/30 20:11:17, Greg Spencer (Chromium) wrote: > On 2013/08/30 20:07:17, mef wrote: > > On 2013/08/30 18:38:17, Greg Spencer (Chromium) wrote: > > > Couldn't you just do this once, and store it for next time? > > I could, but realistically this is not performance-bound function, and it is > not > > going to be called more than 5 times during the session. > > Storing it somewhere would require a singleton, which live forever, which is > not > > that great either. > > I like Ryan's idea better of just storing the DER encoded form in the first > place. Done.
https://codereview.chromium.org/23710003/diff/45001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto.cc (right): https://codereview.chromium.org/23710003/diff/45001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:26: const unsigned char kTrustedCAPublicKeyDER[] = { Chris, can you comment on where this key came from, and why it's hard coded? I'm concerned that (for instance), if somehow the CA were to be compromised, or we want to switch to another CA, that we would be prevented from updating to a new CA because of this hard coded public key.
https://codereview.chromium.org/23710003/diff/45001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto.cc (right): https://codereview.chromium.org/23710003/diff/45001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:26: const unsigned char kTrustedCAPublicKeyDER[] = { On 2013/08/30 20:40:31, Greg Spencer (Chromium) wrote: > Chris, can you comment on where this key came from, and why it's hard coded? > > I'm concerned that (for instance), if somehow the CA were to be compromised, or > we want to switch to another CA, that we would be prevented from updating to a > new CA because of this hard coded public key. That's already the case. It's a manufacturing key.
On 2013/08/30 20:47:52, Ryan Sleevi wrote: > https://codereview.chromium.org/23710003/diff/45001/chrome/browser/extensions... > File > chrome/browser/extensions/api/networking_private/networking_private_crypto.cc > (right): > > https://codereview.chromium.org/23710003/diff/45001/chrome/browser/extensions... > chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:26: > const unsigned char kTrustedCAPublicKeyDER[] = { > On 2013/08/30 20:40:31, Greg Spencer (Chromium) wrote: > > Chris, can you comment on where this key came from, and why it's hard coded? > > > > I'm concerned that (for instance), if somehow the CA were to be compromised, > or > > we want to switch to another CA, that we would be prevented from updating to a > > new CA because of this hard coded public key. > > That's already the case. It's a manufacturing key. OK, so when other devices come out, we'll add more keys here?
On 2013/08/30 21:02:27, Greg Spencer (Chromium) wrote: > On 2013/08/30 20:47:52, Ryan Sleevi wrote: > > > https://codereview.chromium.org/23710003/diff/45001/chrome/browser/extensions... > > File > > chrome/browser/extensions/api/networking_private/networking_private_crypto.cc > > (right): > > > > > https://codereview.chromium.org/23710003/diff/45001/chrome/browser/extensions... > > > chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:26: > > const unsigned char kTrustedCAPublicKeyDER[] = { > > On 2013/08/30 20:40:31, Greg Spencer (Chromium) wrote: > > > Chris, can you comment on where this key came from, and why it's hard coded? > > > > > > I'm concerned that (for instance), if somehow the CA were to be compromised, > > or > > > we want to switch to another CA, that we would be prevented from updating to > a > > > new CA because of this hard coded public key. > > > > That's already the case. It's a manufacturing key. > > OK, so when other devices come out, we'll add more keys here? Come to think about it, would it make sense to pass Trusted CA public info from the extension? Considering that this is private api and extension is signed this shouldn't present a security issue, right? It's not backwards compatible with ChromeOS implementation, but if new implementation could be used on ChromeOS, then it shouldn't be a problem.
On 2013/09/03 13:47:39, mef wrote: > On 2013/08/30 21:02:27, Greg Spencer (Chromium) wrote: > > On 2013/08/30 20:47:52, Ryan Sleevi wrote: > > > > > > https://codereview.chromium.org/23710003/diff/45001/chrome/browser/extensions... > > > File > > > > chrome/browser/extensions/api/networking_private/networking_private_crypto.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/23710003/diff/45001/chrome/browser/extensions... > > > > > > chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:26: > > > const unsigned char kTrustedCAPublicKeyDER[] = { > > > On 2013/08/30 20:40:31, Greg Spencer (Chromium) wrote: > > > > Chris, can you comment on where this key came from, and why it's hard > coded? > > > > > > > > I'm concerned that (for instance), if somehow the CA were to be > compromised, > > > or > > > > we want to switch to another CA, that we would be prevented from updating > to > > a > > > > new CA because of this hard coded public key. > > > > > > That's already the case. It's a manufacturing key. > > > > OK, so when other devices come out, we'll add more keys here? > > Come to think about it, would it make sense to pass Trusted CA public info from > the extension? > Considering that this is private api and extension is signed this shouldn't > present a security issue, right? > It's not backwards compatible with ChromeOS implementation, but if new > implementation could be used on ChromeOS, then it shouldn't be a problem. I'm fine either way. The only time we'd roll this CA is if there was a compromise in the manufacturing process. We're working on a more robust auth scheme anyways, so this should only apply for existing devices.
LGTM from my point of view.
On 2013/09/03 19:10:57, Greg Spencer (Chromium) wrote: > LGTM from my point of view. Greg, thanks! Ryan, I agree that it makes sense to keep things simple for now.
I'm not terribly familiar with the coding style for this part of the world, but there seem to be some lint errors that the tools are complaining about. I thought the commented out namespaces around the unit tests looked a little funny. As they say, the proof of the pudding is in the eating. Have you hooked this up to a device verification process to make sure that works? https://codereview.chromium.org/23710003/diff/45001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto.cc (right): https://codereview.chromium.org/23710003/diff/45001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.cc:26: const unsigned char kTrustedCAPublicKeyDER[] = { On 2013/08/30 20:47:52, Ryan Sleevi wrote: > On 2013/08/30 20:40:31, Greg Spencer (Chromium) wrote: > > Chris, can you comment on where this key came from, and why it's hard coded? > > > > I'm concerned that (for instance), if somehow the CA were to be compromised, > or > > we want to switch to another CA, that we would be prevented from updating to a > > new CA because of this hard coded public key. > > That's already the case. It's a manufacturing key. This is a key that was given to us by that team. It's tied to their device manufacturing process, as rsleevi mentioned. This key isn't rooted in a valid trust chain tied to a root CA as far as I know. Given that constraint, I think this is "as good as it gets."
Hi Chris, thanks for your comments! I've fixed all lint warnings that I could (shortening long #ifdef lines causes pre-submit warnings). The pudding appears to be edible as I've used this implementation with my device and I was able to successfully verify its credentials and encrypt WPA password. thanks, -m
On 2013/09/04 16:38:13, mef wrote: > Hi Chris, thanks for your comments! I've fixed all lint warnings that I could > (shortening long #ifdef lines causes pre-submit warnings). > > The pudding appears to be edible as I've used this implementation with my device > and I was able to successfully verify its credentials and encrypt WPA password. > > thanks, > -m SGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/23710003/39001
LGTM, mod nits. https://codereview.chromium.org/23710003/diff/39001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto.h (right): https://codereview.chromium.org/23710003/diff/39001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:32: // maximum length permissable for encryption with a key of |public_key| size. s/for encryption/for PKCS#1 v1.5/ Mostly because OAEP has different length requirements than PKCS#1 v1.5 (aka RSAES) https://codereview.chromium.org/23710003/diff/39001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc (right): https://codereview.chromium.org/23710003/diff/39001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:8: #include "chrome/browser/extensions/api/networking_private/networking_private_crypto.h" This should be the first header in the file, followed by a newline See the notes on http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... about _test https://codereview.chromium.org/23710003/diff/39001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:12: // Based on chromeos_public//.../network_DestinationVerification.py Don't include this URL. It doesn't help any, and is more likely to get out of date than anything.
Message was sent while issue was closed.
Change committed as 221505
Message was sent while issue was closed.
Hi Ryan, I've addressed your comments as part of networking private api CL: https://codereview.chromium.org/22295002/. thanks, -m https://codereview.chromium.org/23710003/diff/39001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto.h (right): https://codereview.chromium.org/23710003/diff/39001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:32: // maximum length permissable for encryption with a key of |public_key| size. On 2013/09/05 18:38:07, Ryan Sleevi wrote: > s/for encryption/for PKCS#1 v1.5/ > > Mostly because OAEP has different length requirements than PKCS#1 v1.5 (aka > RSAES) Done. https://codereview.chromium.org/23710003/diff/39001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc (right): https://codereview.chromium.org/23710003/diff/39001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:8: #include "chrome/browser/extensions/api/networking_private/networking_private_crypto.h" On 2013/09/05 18:38:07, Ryan Sleevi wrote: > This should be the first header in the file, followed by a newline > > See the notes on > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... > about _test Done. https://codereview.chromium.org/23710003/diff/39001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:12: // Based on chromeos_public//.../network_DestinationVerification.py On 2013/09/05 18:38:07, Ryan Sleevi wrote: > Don't include this URL. It doesn't help any, and is more likely to get out of > date than anything. Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
