Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
34583010:
[webcrypto] Add RSA key generation using NSS. (Closed)
|
Created:
7 years, 2 months ago by padolph Modified:
7 years, 1 month ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[webcrypto] Add RSA key generation using NSS.
BUG=267888
TEST=content_unittests --gtest_filter="WebCryptoImpl*"
R=bryaneyler@google.com,eroman@chromium.org,rsleevi@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232598
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : minor change, simplified loop #Patch Set 4 : re-upload after 500 server failure #
Total comments: 17
Patch Set 5 : Fixes for eroman #
Total comments: 4
Patch Set 6 : Fixes for rsleevi #
Total comments: 10
Patch Set 7 : rebase #Patch Set 8 : fixes for eroman #
Total comments: 18
Patch Set 9 : fixes for eroman #Patch Set 10 : re-upload after server 500 error #Patch Set 11 : minor comment fixes #Patch Set 12 : rebase #
Total comments: 16
Patch Set 13 : fixes for eroman and rsleevi #
Total comments: 4
Patch Set 14 : (minor) fixes for eroman #
Messages
Total messages: 29 (0 generated)
https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl.cc:19: // TODO: include all other asymmetric algorithms once they are defined, nit: use the format TODO(padolph): This doesn't mean you are signed up to do it, it just explains the "voice" that stated the TODO. This is part of the google/chromium C++ style guide. https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl.cc:102: result.completeWithError(); I think the structure of this function would be more readable if you return after setting the result (rather than having "else" clasues). So basically each time "result.completeXXX" is called, there is an accompanying return statement. https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_nss.cc:361: // TODO (padolph) Handle other asymmetric algorithm key generation nit: remove the space after TODO https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_nss.cc:379: DCHECK_LE(params->publicExponent().size(), sizeof(unsigned long)); This needs to be a runtime error (i.e. "return false"), since the publicExponent is controlled by the user (and the length is not enforced by blink). https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_nss.cc:386: // TODO (padolph): should we limit the public exponent to the 'safe' set Ryan: Can you comment on this? https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_nss.cc:407: DCHECK(false); NOTREACHED(); Also would be good to add a defensive "return false" (for release builds) https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_nss.cc:425: return false; Is it guaranteed that sec_public_key hasn't been set in this case? (If not, it will leak). https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_unittest.cc:65: const std::vector<unsigned char> public_exponent) { make this a const reference. https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_unittest.cc:744: } Can you add tests where: * exponenent is larger than unsigned long * the exponent contains leading zeros.
https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl.cc:19: // TODO: include all other asymmetric algorithms once they are defined, On 2013/10/23 20:02:46, eroman wrote: > nit: use the format TODO(padolph): > > This doesn't mean you are signed up to do it, it just explains the "voice" that > stated the TODO. This is part of the google/chromium C++ style guide. Done. https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl.cc:102: result.completeWithError(); On 2013/10/23 20:02:46, eroman wrote: > I think the structure of this function would be more readable if you return > after setting the result (rather than having "else" clasues). So basically each > time "result.completeXXX" is called, there is an accompanying return statement. Agreed for the error cases. But do you also want that for the true half of the IsAlgorithmAsymmetric test? That does not seem right to me: it makes it look like the asymmetric case is somehow special rather than a binary choice. That is, is this what you want? if (IsAlgorithmAsymmetric(algorithm)) { scoped_ptr<WebKit::WebCryptoKeyHandle> public_key_handle; scoped_ptr<WebKit::WebCryptoKeyHandle> private_key_handle; //... result.completeWithKeyPair(public_key, private_key); return; } // << end of IsAlgorithmAsymmetric clause scoped_ptr<WebKit::WebCryptoKeyHandle> handle; WebKit::WebCryptoKeyType type; // ... result.completeWithKey(key); https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_nss.cc:361: // TODO (padolph) Handle other asymmetric algorithm key generation On 2013/10/23 20:02:46, eroman wrote: > nit: remove the space after TODO Done. https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_nss.cc:379: DCHECK_LE(params->publicExponent().size(), sizeof(unsigned long)); On 2013/10/23 20:02:46, eroman wrote: > This needs to be a runtime error (i.e. "return false"), since the publicExponent > is controlled by the user (and the length is not enforced by blink). Done. https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_nss.cc:407: DCHECK(false); On 2013/10/23 20:02:46, eroman wrote: > NOTREACHED(); > Also would be good to add a defensive "return false" (for release builds) Done. https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_nss.cc:425: return false; On 2013/10/23 20:02:46, eroman wrote: > Is it guaranteed that sec_public_key hasn't been set in this case? (If not, it > will leak). Looking at the NSS source, the pubkey is destroyed if the private key generation fails, so it seems so. However, I cannot find documentation that codifies this into the API contract, so it could change I guess. Ryan, can you please comment? https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_unittest.cc:65: const std::vector<unsigned char> public_exponent) { On 2013/10/23 20:02:46, eroman wrote: > make this a const reference. Oops. Thanks for catching that. https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_unittest.cc:744: } On 2013/10/23 20:02:46, eroman wrote: > Can you add tests where: > > * exponenent is larger than unsigned long > * the exponent contains leading zeros. Done.
Quick scan https://codereview.chromium.org/34583010/diff/120001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/34583010/diff/120001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:58: DCHECK(!key_.get()); This seems unnecessary - it's a constructor, how is this invariant ever possible to be violated? explicit PrivateKeyHandle(crypto::ScopedSECKEYPrivateKey key) : key_(key.Pass()) {} https://codereview.chromium.org/34583010/diff/120001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:415: operation_flags = CKF_WRAP | CKF_UNWRAP; BUG: This should also include CKF_ENCRYPT | CKF_DECRYPT; likewise, the RsaEs should also include CKF_WRAP / CKF_UNWRAP
https://codereview.chromium.org/34583010/diff/120001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/34583010/diff/120001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:58: DCHECK(!key_.get()); On 2013/10/24 00:33:04, Ryan Sleevi wrote: > This seems unnecessary - it's a constructor, how is this invariant ever possible > to be violated? > > explicit PrivateKeyHandle(crypto::ScopedSECKEYPrivateKey key) > : key_(key.Pass()) {} Good point. Picked up through copy/paste. Will delete all. https://codereview.chromium.org/34583010/diff/120001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:415: operation_flags = CKF_WRAP | CKF_UNWRAP; On 2013/10/24 00:33:04, Ryan Sleevi wrote: > BUG: This should also include CKF_ENCRYPT | CKF_DECRYPT; likewise, the RsaEs > should also include CKF_WRAP / CKF_UNWRAP Done.
(manual email) https://codereview.chromium.org/34583010/diff/120001/content/renderer/webcryp... <https://codereview.chromium.org/34583010/diff/120001/content/renderer/webcryp...> File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/34583010/diff/120001/content/renderer/webcryp... <https://codereview.chromium.org/34583010/diff/120001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl_nss.cc:58: DCHECK(!key_.get()); On 2013/10/24 00:33:04, Ryan Sleevi wrote: > This seems unnecessary - it's a constructor, how is this invariant ever possible > to be violated? > > explicit PrivateKeyHandle(crypto::ScopedSECKEYPrivateKey key) > : key_(key.Pass()) {} Good point. Picked up through copy/paste. Will delete all. https://codereview.chromium.org/34583010/diff/120001/content/renderer/webcryp... <https://codereview.chromium.org/34583010/diff/120001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl_nss.cc:415: operation_flags = CKF_WRAP | CKF_UNWRAP; On 2013/10/24 00:33:04, Ryan Sleevi wrote: > BUG: This should also include CKF_ENCRYPT | CKF_DECRYPT; likewise, the RsaEs > should also include CKF_WRAP / CKF_UNWRAP Done. On Wed, Oct 23, 2013 at 5:33 PM, <rsleevi@chromium.org> wrote: > Quick scan > > > https://codereview.chromium.**org/34583010/diff/120001/** > content/renderer/webcrypto/**webcrypto_impl_nss.cc<https://codereview.chromium.org/34583010/diff/120001/content/renderer/webcrypto/webcrypto_impl_nss.cc> > File content/renderer/webcrypto/**webcrypto_impl_nss.cc (right): > > https://codereview.chromium.**org/34583010/diff/120001/** > content/renderer/webcrypto/**webcrypto_impl_nss.cc#**newcode58<https://codereview.chromium.org/34583010/diff/120001/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode58> > content/renderer/webcrypto/**webcrypto_impl_nss.cc:58: > DCHECK(!key_.get()); > This seems unnecessary - it's a constructor, how is this invariant ever > possible to be violated? > > explicit PrivateKeyHandle(crypto::**ScopedSECKEYPrivateKey key) > : key_(key.Pass()) {} > > https://codereview.chromium.**org/34583010/diff/120001/** > content/renderer/webcrypto/**webcrypto_impl_nss.cc#**newcode415<https://codereview.chromium.org/34583010/diff/120001/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode415> > content/renderer/webcrypto/**webcrypto_impl_nss.cc:415: operation_flags = > CKF_WRAP | CKF_UNWRAP; > BUG: This should also include CKF_ENCRYPT | CKF_DECRYPT; likewise, the > RsaEs should also include CKF_WRAP / CKF_UNWRAP > > https://codereview.chromium.**org/34583010/<https://codereview.chromium.org/3... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:118: result.completeWithKeyPair(public_key, private_key); could you add a return statement here, and then get rid of the "else"? https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:365: !params->publicExponent().size()) { Sleevi: General question: In WebCrypto's "BigInteger", what is the interpretation for an empty buffer. Is it equivalent to zero, or is it an error? https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:373: // First offset past any leading zeros in publicExponent. Could you please extract this block of code to a separate function? https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:375: for (start_index = 0; start_index < params->publicExponent().size(); In my opinion this logic would be more readable as a single loop. Something like: bool BigIntegerToLong(const uint8* data, unsigned data_size, unsigned long* result) { if (data_size == 0) return false; // Is this correct? *result = 0; for (size_t i = 0; i < data_size; ++i) { size_t reverse_i = data_size - i - 1; if (reverse_i >= sizeof(unsigned long) && data[i]) return false; // Too large for a long. *result |= data[i] << 8 * reverse_i; } return true; } https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:699: std::vector<unsigned char> public_exponent(&f4[0], &f4[0] + sizeof(f4)); [optional] You may consider using HexStringToBytes("010001") to create this std::vector.
https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:118: result.completeWithKeyPair(public_key, private_key); On 2013/10/24 22:28:59, eroman wrote: > could you add a return statement here, and then get rid of the "else"? IMO that makes the code read like asymmetric is a special case, rather than a binary choice with symmetric, as per my earlier reply. But OK, will do. https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:373: // First offset past any leading zeros in publicExponent. On 2013/10/24 22:28:59, eroman wrote: > Could you please extract this block of code to a separate function? Done. https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:375: for (start_index = 0; start_index < params->publicExponent().size(); On 2013/10/24 22:28:59, eroman wrote: > In my opinion this logic would be more readable as a single loop. Something > like: > > bool BigIntegerToLong(const uint8* data, > unsigned data_size, > unsigned long* result) { > if (data_size == 0) > return false; // Is this correct? > > *result = 0; > for (size_t i = 0; i < data_size; ++i) { > size_t reverse_i = data_size - i - 1; > > if (reverse_i >= sizeof(unsigned long) && data[i]) > return false; // Too large for a long. > > *result |= data[i] << 8 * reverse_i; > } > return true; > } Done. https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:699: std::vector<unsigned char> public_exponent(&f4[0], &f4[0] + sizeof(f4)); On 2013/10/24 22:28:59, eroman wrote: > [optional] You may consider using HexStringToBytes("010001") to create this > std::vector. Done.
(manual email) https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:118: result.completeWithKeyPair(public_key, private_key); On 2013/10/24 22:28:59, eroman wrote: > could you add a return statement here, and then get rid of the "else"? IMO that makes the code read like asymmetric is a special case, rather than a binary choice with symmetric, as per my earlier reply. But OK, will do. https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:373: // First offset past any leading zeros in publicExponent. On 2013/10/24 22:28:59, eroman wrote: > Could you please extract this block of code to a separate function? Done. https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:375: for (start_index = 0; start_index < params->publicExponent().size(); On 2013/10/24 22:28:59, eroman wrote: > In my opinion this logic would be more readable as a single loop. Something > like: > > bool BigIntegerToLong(const uint8* data, > unsigned data_size, > unsigned long* result) { > ... > } Done. https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:699: std::vector<unsigned char> public_exponent(&f4[0], &f4[0] + sizeof(f4)); On 2013/10/24 22:28:59, eroman wrote: > [optional] You may consider using HexStringToBytes("010001") to create this > std::vector. Done. On Wed, Oct 23, 2013 at 1:02 PM, <eroman@chromium.org> wrote: > > https://codereview.chromium.**org/34583010/diff/80001/** > content/renderer/webcrypto/**webcrypto_impl.cc<https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypto/webcrypto_impl.cc> > File content/renderer/webcrypto/**webcrypto_impl.cc (right): > > https://codereview.chromium.**org/34583010/diff/80001/** > content/renderer/webcrypto/**webcrypto_impl.cc#newcode19<https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypto/webcrypto_impl.cc#newcode19> > content/renderer/webcrypto/**webcrypto_impl.cc:19: // TODO: include all > other asymmetric algorithms once they are defined, > nit: use the format TODO(padolph): > > This doesn't mean you are signed up to do it, it just explains the > "voice" that stated the TODO. This is part of the google/chromium C++ > style guide. > > https://codereview.chromium.**org/34583010/diff/80001/** > content/renderer/webcrypto/**webcrypto_impl.cc#newcode102<https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypto/webcrypto_impl.cc#newcode102> > content/renderer/webcrypto/**webcrypto_impl.cc:102: > result.completeWithError(); > I think the structure of this function would be more readable if you > return after setting the result (rather than having "else" clasues). So > basically each time "result.completeXXX" is called, there is an > accompanying return statement. > > https://codereview.chromium.**org/34583010/diff/80001/** > content/renderer/webcrypto/**webcrypto_impl_nss.cc<https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypto/webcrypto_impl_nss.cc> > File content/renderer/webcrypto/**webcrypto_impl_nss.cc (right): > > https://codereview.chromium.**org/34583010/diff/80001/** > content/renderer/webcrypto/**webcrypto_impl_nss.cc#**newcode361<https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode361> > content/renderer/webcrypto/**webcrypto_impl_nss.cc:361: // TODO (padolph) > Handle other asymmetric algorithm key generation > nit: remove the space after TODO > > https://codereview.chromium.**org/34583010/diff/80001/** > content/renderer/webcrypto/**webcrypto_impl_nss.cc#**newcode379<https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode379> > content/renderer/webcrypto/**webcrypto_impl_nss.cc:379: > DCHECK_LE(params->**publicExponent().size(), sizeof(unsigned long)); > This needs to be a runtime error (i.e. "return false"), since the > publicExponent is controlled by the user (and the length is not enforced > by blink). > > https://codereview.chromium.**org/34583010/diff/80001/** > content/renderer/webcrypto/**webcrypto_impl_nss.cc#**newcode386<https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode386> > content/renderer/webcrypto/**webcrypto_impl_nss.cc:386: // TODO (padolph): > should we limit the public exponent to the 'safe' set > Ryan: Can you comment on this? > > https://codereview.chromium.**org/34583010/diff/80001/** > content/renderer/webcrypto/**webcrypto_impl_nss.cc#**newcode407<https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode407> > content/renderer/webcrypto/**webcrypto_impl_nss.cc:407: DCHECK(false); > NOTREACHED(); > Also would be good to add a defensive "return false" (for release > builds) > > https://codereview.chromium.**org/34583010/diff/80001/** > content/renderer/webcrypto/**webcrypto_impl_nss.cc#**newcode425<https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode425> > content/renderer/webcrypto/**webcrypto_impl_nss.cc:425: return false; > Is it guaranteed that sec_public_key hasn't been set in this case? (If > not, it will leak). > > https://codereview.chromium.**org/34583010/diff/80001/** > content/renderer/webcrypto/**webcrypto_impl_unittest.cc<https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypto/webcrypto_impl_unittest.cc> > File content/renderer/webcrypto/**webcrypto_impl_unittest.cc (right): > > https://codereview.chromium.**org/34583010/diff/80001/** > content/renderer/webcrypto/**webcrypto_impl_unittest.cc#**newcode65<https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypto/webcrypto_impl_unittest.cc#newcode65> > content/renderer/webcrypto/**webcrypto_impl_unittest.cc:65: const > std::vector<unsigned char> public_exponent) { > make this a const reference. > > https://codereview.chromium.**org/34583010/diff/80001/** > content/renderer/webcrypto/**webcrypto_impl_unittest.cc#**newcode744<https://codereview.chromium.org/34583010/diff/80001/content/renderer/webcrypto/webcrypto_impl_unittest.cc#newcode744> > content/renderer/webcrypto/**webcrypto_impl_unittest.cc:**744: } > Can you add tests where: > > * exponenent is larger than unsigned long > * the exponent contains leading zeros. > > https://codereview.chromium.**org/34583010/<https://codereview.chromium.org/3... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM. Ryan, can you do another sanity check of the NSS side when you get back? https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:118: result.completeWithKeyPair(public_key, private_key); On 2013/10/25 01:21:46, padolph wrote: > On 2013/10/24 22:28:59, eroman wrote: > > could you add a return statement here, and then get rid of the "else"? > > IMO that makes the code read like asymmetric is a special case, rather than a > binary choice with symmetric, as per my earlier reply. But OK, will do. If you feel strongly about it feel free to change it back. I was basing the suggestion on my own personal preference. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:219: // Convert a (big-endian) WebCrypto BigInteger, with or without leading zeros, minor nit: "Convert" --> "Converts". Google C++ style asks for passive tense in function comments (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...) https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:225: return false; // TODO(padolph): Is this correct? You can link to https://www.w3.org/Bugs/Public/show_bug.cgi?id=23655 https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:374: // TODO(padolph) Handle other asymmetric algorithm key generation minor nit: put colon after closing paren. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:386: !params->publicExponent().size() || This check is not needed, since it is OK to call .data() on an empty WebVector. BigIntegerToLong() handles the empty length. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:390: !public_exponent) { Is this check necessary, or will NSS fail later with 0 exponent? (No change necessary, just wondering). https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:406: operation_flags = CKF_ENCRYPT | CKF_DECRYPT | CKF_WRAP | CKF_UNWRAP; [optional]: since this line is the same as above, could use fall-through. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:685: // happy WebCryptoAlgorithmIdRsaEsPkcs1v1_5 key gen Could you capitalize the comment and end with a period? Also "successful" or "valid" is probably more descriptive than "happy" (although you can leave it as-is). https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:699: // bad modulus Please capitalize for consistency throughout. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:706: modulus_length = 256; // restore modulus_length for next test [optional] I think it would be clearer to inline this into the CreateRsaAlgorithm() call.
https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:219: // Convert a (big-endian) WebCrypto BigInteger, with or without leading zeros, On 2013/10/28 20:00:47, eroman wrote: > minor nit: "Convert" --> "Converts". > > Google C++ style asks for passive tense in function comments > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...) Done. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:225: return false; // TODO(padolph): Is this correct? On 2013/10/28 20:00:47, eroman wrote: > You can link to https://www.w3.org/Bugs/Public/show_bug.cgi?id=23655 Done. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:374: // TODO(padolph) Handle other asymmetric algorithm key generation On 2013/10/28 20:00:47, eroman wrote: > minor nit: put colon after closing paren. Done. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:386: !params->publicExponent().size() || On 2013/10/28 20:00:47, eroman wrote: > This check is not needed, since it is OK to call .data() on an empty WebVector. > > BigIntegerToLong() handles the empty length. Done. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:390: !public_exponent) { On 2013/10/28 20:00:47, eroman wrote: > Is this check necessary, or will NSS fail later with 0 exponent? (No change > necessary, just wondering). NSS fails correctly in this case, but I thought it was a good idea to shortcut this failure if we know ahead of time. IIRC, NSS spent considerable cycles before failing that cost is sidestepped by this check. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:406: operation_flags = CKF_ENCRYPT | CKF_DECRYPT | CKF_WRAP | CKF_UNWRAP; On 2013/10/28 20:00:47, eroman wrote: > [optional]: since this line is the same as above, could use fall-through. Done. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:685: // happy WebCryptoAlgorithmIdRsaEsPkcs1v1_5 key gen On 2013/10/28 20:00:47, eroman wrote: > Could you capitalize the comment and end with a period? Also "successful" or > "valid" is probably more descriptive than "happy" (although you can leave it > as-is). Done. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:699: // bad modulus On 2013/10/28 20:00:47, eroman wrote: > Please capitalize for consistency throughout. Done. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:706: modulus_length = 256; // restore modulus_length for next test On 2013/10/28 20:00:47, eroman wrote: > [optional] I think it would be clearer to inline this into the > CreateRsaAlgorithm() call. Done.
(manual email) *me*0 minutes ago #11 <https://codereview.chromium.org/34583010/#msg11> https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... <https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp...> File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... <https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl_nss.cc:219: // Convert a (big-endian) WebCrypto BigInteger, with or without leading zeros, On 2013/10/28 20:00:47, eroman wrote: > minor nit: "Convert" --> "Converts". > > Google C++ style asks for passive tense in function comments > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... <http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...>) Done. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... <https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl_nss.cc:225: return false; // TODO(padolph): Is this correct? On 2013/10/28 20:00:47, eroman wrote: > You can link to https://www.w3.org/Bugs/Public/show_bug.cgi?id=23655 Done. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... <https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl_nss.cc:374: // TODO(padolph) Handle other asymmetric algorithm key generation On 2013/10/28 20:00:47, eroman wrote: > minor nit: put colon after closing paren. Done. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... <https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl_nss.cc:386: !params->publicExponent().size() || On 2013/10/28 20:00:47, eroman wrote: > This check is not needed, since it is OK to call .data() on an empty WebVector. > > BigIntegerToLong() handles the empty length. Done. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... <https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl_nss.cc:390: !public_exponent) { On 2013/10/28 20:00:47, eroman wrote: > Is this check necessary, or will NSS fail later with 0 exponent? (No change > necessary, just wondering). NSS fails correctly in this case, but I thought it was a good idea to shortcut this failure if we know ahead of time. IIRC, NSS spent considerable cycles before failing that cost is sidestepped by this check. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... <https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl_nss.cc:406: operation_flags = CKF_ENCRYPT | CKF_DECRYPT | CKF_WRAP | CKF_UNWRAP; On 2013/10/28 20:00:47, eroman wrote: > [optional]: since this line is the same as above, could use fall-through. Done. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... <https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp...> File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... <https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl_unittest.cc:685: // happy WebCryptoAlgorithmIdRsaEsPkcs1v1_5 key gen On 2013/10/28 20:00:47, eroman wrote: > Could you capitalize the comment and end with a period? Also "successful" or > "valid" is probably more descriptive than "happy" (although you can leave it > as-is). Done. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... <https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl_unittest.cc:699: // bad modulus On 2013/10/28 20:00:47, eroman wrote: > Please capitalize for consistency throughout. Done. https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp... <https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl_unittest.cc:706: modulus_length = 256; // restore modulus_length for next test On 2013/10/28 20:00:47, eroman wrote: > [optional] I think it would be clearer to inline this into the > CreateRsaAlgorithm() call. Done. On Mon, Oct 28, 2013 at 1:00 PM, <eroman@chromium.org> wrote: > LGTM. > > Ryan, can you do another sanity check of the NSS side when you get back? > > > > https://codereview.chromium.**org/34583010/diff/140001/** > content/renderer/webcrypto/**webcrypto_impl.cc<https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcrypto/webcrypto_impl.cc> > File content/renderer/webcrypto/**webcrypto_impl.cc (right): > > https://codereview.chromium.**org/34583010/diff/140001/** > content/renderer/webcrypto/**webcrypto_impl.cc#newcode118<https://codereview.chromium.org/34583010/diff/140001/content/renderer/webcrypto/webcrypto_impl.cc#newcode118> > content/renderer/webcrypto/**webcrypto_impl.cc:118: > result.completeWithKeyPair(**public_key, private_key); > On 2013/10/25 01:21:46, padolph wrote: > >> On 2013/10/24 22:28:59, eroman wrote: >> > could you add a return statement here, and then get rid of the >> > "else"? > > IMO that makes the code read like asymmetric is a special case, rather >> > than a > >> binary choice with symmetric, as per my earlier reply. But OK, will >> > do. > > If you feel strongly about it feel free to change it back. I was basing > the suggestion on my own personal preference. > > https://codereview.chromium.**org/34583010/diff/250001/** > content/renderer/webcrypto/**webcrypto_impl_nss.cc<https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcrypto/webcrypto_impl_nss.cc> > File content/renderer/webcrypto/**webcrypto_impl_nss.cc (right): > > https://codereview.chromium.**org/34583010/diff/250001/** > content/renderer/webcrypto/**webcrypto_impl_nss.cc#**newcode219<https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode219> > content/renderer/webcrypto/**webcrypto_impl_nss.cc:219: // Convert a > (big-endian) WebCrypto BigInteger, with or without leading zeros, > minor nit: "Convert" --> "Converts". > > Google C++ style asks for passive tense in function comments > (http://google-styleguide.**googlecode.com/svn/trunk/** > cppguide.xml?showone=Function_**Comments#Function_Comments<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Function_Comments#Function_Comments> > ) > > https://codereview.chromium.**org/34583010/diff/250001/** > content/renderer/webcrypto/**webcrypto_impl_nss.cc#**newcode225<https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode225> > content/renderer/webcrypto/**webcrypto_impl_nss.cc:225: return false; // > TODO(padolph): Is this correct? > You can link to https://www.w3.org/Bugs/**Public/show_bug.cgi?id=23655<https://www.w3.org/Bug... > > https://codereview.chromium.**org/34583010/diff/250001/** > content/renderer/webcrypto/**webcrypto_impl_nss.cc#**newcode374<https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode374> > content/renderer/webcrypto/**webcrypto_impl_nss.cc:374: // TODO(padolph) > > Handle other asymmetric algorithm key generation > minor nit: put colon after closing paren. > > https://codereview.chromium.**org/34583010/diff/250001/** > content/renderer/webcrypto/**webcrypto_impl_nss.cc#**newcode386<https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode386> > content/renderer/webcrypto/**webcrypto_impl_nss.cc:386: > !params->publicExponent().**size() || > This check is not needed, since it is OK to call .data() on an empty > WebVector. > > BigIntegerToLong() handles the empty length. > > https://codereview.chromium.**org/34583010/diff/250001/** > content/renderer/webcrypto/**webcrypto_impl_nss.cc#**newcode390<https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode390> > content/renderer/webcrypto/**webcrypto_impl_nss.cc:390: !public_exponent) > { > Is this check necessary, or will NSS fail later with 0 exponent? (No > change necessary, just wondering). > > https://codereview.chromium.**org/34583010/diff/250001/** > content/renderer/webcrypto/**webcrypto_impl_nss.cc#**newcode406<https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode406> > content/renderer/webcrypto/**webcrypto_impl_nss.cc:406: operation_flags = > CKF_ENCRYPT | CKF_DECRYPT | CKF_WRAP | CKF_UNWRAP; > [optional]: since this line is the same as above, could use > fall-through. > > https://codereview.chromium.**org/34583010/diff/250001/** > content/renderer/webcrypto/**webcrypto_impl_unittest.cc<https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcrypto/webcrypto_impl_unittest.cc> > File content/renderer/webcrypto/**webcrypto_impl_unittest.cc (right): > > https://codereview.chromium.**org/34583010/diff/250001/** > content/renderer/webcrypto/**webcrypto_impl_unittest.cc#**newcode685<https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcrypto/webcrypto_impl_unittest.cc#newcode685> > content/renderer/webcrypto/**webcrypto_impl_unittest.cc:**685: // happy > WebCryptoAlgorithmIdRsaEsPkcs1**v1_5 key gen > Could you capitalize the comment and end with a period? Also > "successful" or "valid" is probably more descriptive than "happy" > (although you can leave it as-is). > > https://codereview.chromium.**org/34583010/diff/250001/** > content/renderer/webcrypto/**webcrypto_impl_unittest.cc#**newcode699<https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcrypto/webcrypto_impl_unittest.cc#newcode699> > content/renderer/webcrypto/**webcrypto_impl_unittest.cc:**699: // bad > modulus > Please capitalize for consistency throughout. > > https://codereview.chromium.**org/34583010/diff/250001/** > content/renderer/webcrypto/**webcrypto_impl_unittest.cc#**newcode706<https://codereview.chromium.org/34583010/diff/250001/content/renderer/webcrypto/webcrypto_impl_unittest.cc#newcode706> > content/renderer/webcrypto/**webcrypto_impl_unittest.cc:**706: > modulus_length = 256; // restore modulus_length for next test > [optional] I think it would be clearer to inline this into the > CreateRsaAlgorithm() call. > > https://codereview.chromium.**org/34583010/<https://codereview.chromium.org/3... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks for the manual emails, it helps! The latest upload seems to have failed, could you re-upload please?
Re-uploaded. Sorry, did the last one while in a meeting and did not check the result. I get those server 500 errors relatively frequently. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/34583010/440001
Sorry for I got bad news for ya. Compile failed with a clobber build on win_x64_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
The issue with compile on Windows shoudl now be resolved (https://src.chromium.org/viewvc/chrome?revision=232050&view=revision), feel free to re-submit. Note that there will be some merge conflicts to resolve due to https://src.chromium.org/viewvc/chrome?revision=232003&view=revision.
LGTM after some clarification on: https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:18: const WebKit::WebCryptoAlgorithmId algorithm_id = algorithm.id(); [optional]: seems equally short to inline. https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:111: WebKit::WebCryptoKey public_key = NullKey(); [optional]: I recommend using WebKit::WebCryptoKey::createNull() in place of NullKey(). That way if my changelist lands before yours (which removes NullKey()) you won't have to rebase. https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:121: DCHECK_EQ(true, public_key.extractable()); Shouldn't this be comparing against "extractable" not "true"? https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:439: // Make the public key always extractable. I don't see anything about this in the spec. My interpretation would have been to have the "extractable" boolean apply to both keys, since that is how the usages works too. @sleevi: what do you think? https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:442: crypto::ScopedSECKEYPublicKey(sec_public_key).Pass()), Is the .Pass() needed ? https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:707: // TODO(padolph): check key.extractable and key.usage_mask Any reason not to do that as part of this changelist? https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:751: // TODO(padolph): check key.extractable and key.usage_mask same question throughout. I guess in some sense the DCHECK()s in webcrypto_impl.cc are also exercising this.
https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:439: // Make the public key always extractable. On 2013/10/31 22:18:36, eroman wrote: > I don't see anything about this in the spec. > > My interpretation would have been to have the "extractable" boolean apply to > both keys, since that is how the usages works too. > > @sleevi: what do you think? File a spec bug. Current spec indicates both keys respect the extractable attribute.
https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:439: // Make the public key always extractable. On 2013/10/31 22:23:20, Ryan Sleevi wrote: > On 2013/10/31 22:18:36, eroman wrote: > > I don't see anything about this in the spec. > > > > My interpretation would have been to have the "extractable" boolean apply to > > both keys, since that is how the usages works too. > > > > @sleevi: what do you think? > > File a spec bug. Current spec indicates both keys respect the extractable > attribute. Done: https://www.w3.org/Bugs/Public/show_bug.cgi?id=23695 @padolph: For this changelist can you use the same "extractable" for both public and private keys?
https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:18: const WebKit::WebCryptoAlgorithmId algorithm_id = algorithm.id(); On 2013/10/31 22:18:36, eroman wrote: > [optional]: seems equally short to inline. Done. https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:111: WebKit::WebCryptoKey public_key = NullKey(); On 2013/10/31 22:18:36, eroman wrote: > [optional]: I recommend using WebKit::WebCryptoKey::createNull() in place of > NullKey(). That way if my changelist lands before yours (which removes > NullKey()) you won't have to rebase. Done. https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:121: DCHECK_EQ(true, public_key.extractable()); On 2013/10/31 22:18:36, eroman wrote: > Shouldn't this be comparing against "extractable" not "true"? This is related to the spec bug you and Ryan discussed in the comments in this CL in webcrypto_impl_nss.cc. I'll switch it to the (IMO errant) behavior and add a comment referencing the bug. https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:439: // Make the public key always extractable. On 2013/10/31 22:30:07, eroman wrote: > On 2013/10/31 22:23:20, Ryan Sleevi wrote: > > On 2013/10/31 22:18:36, eroman wrote: > > > I don't see anything about this in the spec. > > > > > > My interpretation would have been to have the "extractable" boolean apply to > > > both keys, since that is how the usages works too. > > > > > > @sleevi: what do you think? > > > > File a spec bug. Current spec indicates both keys respect the extractable > > attribute. > > Done: https://www.w3.org/Bugs/Public/show_bug.cgi?id=23695 > > @padolph: For this changelist can you use the same "extractable" for both public > and private keys? Done. https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:442: crypto::ScopedSECKEYPublicKey(sec_public_key).Pass()), On 2013/10/31 22:18:36, eroman wrote: > Is the .Pass() needed ? No. Done. https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:707: // TODO(padolph): check key.extractable and key.usage_mask On 2013/10/31 22:18:36, eroman wrote: > Any reason not to do that as part of this changelist? Oops, meant to. Thanks. https://codereview.chromium.org/34583010/diff/630002/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:751: // TODO(padolph): check key.extractable and key.usage_mask On 2013/10/31 22:18:36, eroman wrote: > same question throughout. I guess in some sense the DCHECK()s in > webcrypto_impl.cc are also exercising this. Those TODO's were to remind me to add these after the rebase. I'll get these in.
lgtm https://codereview.chromium.org/34583010/diff/840001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/34583010/diff/840001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:701: WebKit::WebCryptoKey public_key = WebCryptoImpl::NullKey(); Can you use WebCryptoKey::createNull() throughout? Thanks! https://codereview.chromium.org/34583010/diff/840001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:754: EXPECT_TRUE(!public_key.isNull()); Or alternately, EXPECT_FALSE(public_key.isNull());
https://codereview.chromium.org/34583010/diff/840001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/34583010/diff/840001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:701: WebKit::WebCryptoKey public_key = WebCryptoImpl::NullKey(); On 2013/11/01 21:07:26, eroman wrote: > Can you use WebCryptoKey::createNull() throughout? Thanks! Done. https://codereview.chromium.org/34583010/diff/840001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:754: EXPECT_TRUE(!public_key.isNull()); On 2013/11/01 21:07:26, eroman wrote: > Or alternately, EXPECT_FALSE(public_key.isNull()); Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/34583010/900001
Retried try job too often on win_rel for step(s) chrome_frame_net_tests, chrome_frame_tests, chrome_frame_unittests, nacl_integration, sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/34583010/900001
Message was sent while issue was closed.
Change committed as 232598 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
