Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
68303009:
[webcrypto] Add RSASSA-PKCS1-v1_5 sign and verify for NSS. (Closed)
|
Created:
7 years, 1 month ago by padolph Modified:
6 years, 11 months 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 RSASSA-PKCS1-v1_5 sign and verify for NSS.
BUG=245025
TEST=content_unittests --gtest_filter="WebCryptoImpl*"
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245430
Patch Set 1 #Patch Set 2 : rebase + minor test fix #
Total comments: 8
Patch Set 3 : rebase #Patch Set 4 : rebase #Patch Set 5 : fixes for eroman #
Total comments: 2
Patch Set 6 : rebase #Patch Set 7 : rewrote to use higher-level NSS functions, and added known-answer tests #Patch Set 8 : rebase #Patch Set 9 : rebase #
Total comments: 7
Patch Set 10 : rebase #Patch Set 11 : fixes for eroman #
Total comments: 9
Patch Set 12 : fixes for eroman and rebase #
Total comments: 15
Patch Set 13 : fixes for eroman, and MAYBE'ed new tests (missed in last rebase) #
Total comments: 26
Patch Set 14 : rebase #Patch Set 15 : fixes for rsleevi #Patch Set 16 : rebase plus minor fix #
Total comments: 8
Patch Set 17 : rebase #Patch Set 18 : fixes for eroman #Patch Set 19 : minor code formatting fix #
Messages
Total messages: 40 (0 generated)
Looks fine to me, passing off to @rsleevi. https://codereview.chromium.org/68303009/diff/40001/content/renderer/webcrypt... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/40001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_nss.cc:246: blink::WebCryptoAlgorithm::createNull(); [optional] Rather than assigning inside each case, how about directly returning the algorithm, and leaving createNull() as the final return value. https://codereview.chromium.org/68303009/diff/40001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_nss.cc:248: case blink::WebCryptoAlgorithmIdHmac: Doesn't look like the callers care, however note that the keygen params are not enumerated. https://codereview.chromium.org/68303009/diff/40001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_nss.cc:673: &signature_item, indentation: Add 4 more spaces. https://codereview.chromium.org/68303009/diff/40001/content/renderer/webcrypt... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/68303009/diff/40001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_unittest.cc:719: modulus_length, indentation needs to change
https://codereview.chromium.org/68303009/diff/40001/content/renderer/webcrypt... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/40001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_nss.cc:246: blink::WebCryptoAlgorithm::createNull(); On 2013/11/20 23:57:44, eroman wrote: > [optional] Rather than assigning inside each case, how about directly returning > the algorithm, and leaving createNull() as the final return value. Done. https://codereview.chromium.org/68303009/diff/40001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_nss.cc:248: case blink::WebCryptoAlgorithmIdHmac: On 2013/11/20 23:57:44, eroman wrote: > Doesn't look like the callers care, however note that the keygen params are not > enumerated. Done. https://codereview.chromium.org/68303009/diff/40001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_nss.cc:673: &signature_item, On 2013/11/20 23:57:44, eroman wrote: > indentation: Add 4 more spaces. fixed https://codereview.chromium.org/68303009/diff/40001/content/renderer/webcrypt... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/68303009/diff/40001/content/renderer/webcrypt... content/renderer/webcrypto/webcrypto_impl_unittest.cc:719: modulus_length, On 2013/11/20 23:57:44, eroman wrote: > indentation needs to change Done.
Please add known answer tests, which would spot the BUG highlighted below. https://codereview.chromium.org/68303009/diff/150001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/150001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:651: if (!DigestInternal(hash_algorithm, data, data_size, &digest)) I'm not sure this is the right approach. Why not use the SGN_ API (secdig.h & cryptohi.h), which is where the PSS support is going to be added? Why reach directly into the PK11 layer? BUG: Also, an RsaSsa signature is over a DigestInfo, but it appears that you're signing a 'raw' hash, rather than the digest info.
On 2013/11/21 01:45:57, Ryan Sleevi wrote: > Please add known answer tests, which would spot the BUG highlighted below. To do that I need to import an RSA private key. I have two options but got stuck on both: 1. pkcs8 format - Can you point me to the NSS APIs that can do import a private key in this format? I did not have much luck with my searches. Or do I need to transform the key through other formats? 2. jwk format - I was able to find info on how to import an RSA public key given the discrete bignum parameters, by encoding to ASN.1 and using an NSS DER import function. Is there an equivalent method for a private key? Any pointers will be appreciated. :-) If I can get jwk import working then I can easily use the NIST test vectors here ftp://ftp.rsa.com/pub/rsalabs/tmp/pkcs1v15sign-vectors.txt
On 2013/11/21 02:42:08, padolph wrote: > On 2013/11/21 01:45:57, Ryan Sleevi wrote: > > Please add known answer tests, which would spot the BUG highlighted below. > > To do that I need to import an RSA private key. I have two options but got stuck > on both: > 1. pkcs8 format - Can you point me to the NSS APIs that can do import a private > key in this format? I did not have much luck with my searches. Or do I need to > transform the key through other formats? http://mxr.mozilla.org/nss/source/lib/pk11wrap/secmodt.h#371 http://mxr.mozilla.org/nss/source/lib/pk11wrap/pk11pub.h#544 > 2. jwk format - I was able to find info on how to import an RSA public key given > the discrete bignum parameters, by encoding to ASN.1 and using an NSS DER import > function. Is there an equivalent method for a private key? > > Any pointers will be appreciated. :-) > > If I can get jwk import working then I can easily use the NIST test vectors here > ftp://ftp.rsa.com/pub/rsalabs/tmp/pkcs1v15sign-vectors.txt
https://codereview.chromium.org/68303009/diff/150001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/150001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:651: if (!DigestInternal(hash_algorithm, data, data_size, &digest)) On 2013/11/21 01:45:57, Ryan Sleevi wrote: > I'm not sure this is the right approach. > > Why not use the SGN_ API (secdig.h & cryptohi.h), which is where the PSS support > is going to be added? Why reach directly into the PK11 layer? > > BUG: Also, an RsaSsa signature is over a DigestInfo, but it appears that you're > signing a 'raw' hash, rather than the digest info. Done. Rewrote to use SEC_SignData (a sequence of SGN_ calls) and VFY_VerifyDataDirect (a sequence of VFY_ calls). Passes new KAT's. Please take a look.
On 2013/11/21 01:45:57, Ryan Sleevi wrote: > Please add known answer tests, which would spot the BUG highlighted below. > Done. I added KATs using all the vectors from Example 1 of ftp://ftp.rsa.com/pub/rsalabs/tmp/pkcs1v15sign-vectors.txt. This is probably too many at this level (and there are a ton more in that link). Should I thin these down, and if so by how much? Please advise.
https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:260: // TODO(padolph): Move to webcrypto_util This can be done as part of this changelist. https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:985: memcpy(result.data(), signature_item->data, signature_item->len); I believe there is now a helper for this checked in. https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:1051: SEC_OID_UNKNOWN, Forgive my crypto ignorance, but is this safe? The NSS docs say the hashAlg is optional (and if not specified will be inferred from the RSA key + signature). What if the webapp calls verify() with a SHA-1 signature, asking it to be verified as a SHA-256 signature. If this were to return "true" then we have a problem. It is entirely possible that what I am saying doesn't make sense, but I think we should be passing WebCryptoAlgorithmToNssSecOidShaTag(hash_algorithm) here. Deferring this review rsleevi as I am unqualified, but I did wanted to ping it :)
https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:260: // TODO(padolph): Move to webcrypto_util On 2013/12/06 02:20:42, eroman wrote: > This can be done as part of this changelist. Done. https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:985: memcpy(result.data(), signature_item->data, signature_item->len); On 2013/12/06 02:20:42, eroman wrote: > I believe there is now a helper for this checked in. Done. https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:1051: SEC_OID_UNKNOWN, On 2013/12/06 02:20:42, eroman wrote: > Forgive my crypto ignorance, but is this safe? > > The NSS docs say the hashAlg is optional (and if not specified will be inferred > from the RSA key + signature). > > What if the webapp calls verify() with a SHA-1 signature, asking it to be > verified as a SHA-256 signature. If this were to return "true" then we have a > problem. > > It is entirely possible that what I am saying doesn't make sense, but I think we > should be passing WebCryptoAlgorithmToNssSecOidShaTag(hash_algorithm) here. > > Deferring this review rsleevi as I am unqualified, but I did wanted to ping it > :) We know at this callsite that we have an RSA key, and the NSS docs say "If the key is an RSA key, and sig is not NULL, then hashAlg can be SEC_OID_UNKNOWN". This makes the code a little simpler and seems like NSS is OK with it. That's the only reason I took that shortcut. Also would like rsleevi's opinion on this.
(manual email) https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:260: // TODO(padolph): Move to webcrypto_util On 2013/12/06 02:20:42, eroman wrote: > This can be done as part of this changelist. Done. https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:985: memcpy(result.data(), signature_item->data, signature_item->len); On 2013/12/06 02:20:42, eroman wrote: > I believe there is now a helper for this checked in. Done. https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:1051: SEC_OID_UNKNOWN, On 2013/12/06 02:20:42, eroman wrote: > Forgive my crypto ignorance, but is this safe? > > The NSS docs say the hashAlg is optional (and if not specified will be inferred > from the RSA key + signature). > > What if the webapp calls verify() with a SHA-1 signature, asking it to be > verified as a SHA-256 signature. If this were to return "true" then we have a > problem. > > It is entirely possible that what I am saying doesn't make sense, but I think we > should be passing WebCryptoAlgorithmToNssSecOidShaTag(hash_algorithm) here. > > Deferring this review rsleevi as I am unqualified, but I did wanted to ping it > :) We know at this callsite that we have an RSA key, and the NSS docs say "If the key is an RSA key, and sig is not NULL, then hashAlg can be SEC_OID_UNKNOWN". This makes the code a little simpler and seems like NSS is OK with it. That's the only reason I took that shortcut. Also would like rsleevi's opinion on this. On Thu, Dec 5, 2013 at 6:20 PM, <eroman@chromium.org> wrote: > > https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... > File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): > > https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_nss.cc:260: // TODO(padolph): > Move to webcrypto_util > This can be done as part of this changelist. > > https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_nss.cc:985: > memcpy(result.data(), signature_item->data, signature_item->len); > I believe there is now a helper for this checked in. > > https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_nss.cc:1051: SEC_OID_UNKNOWN, > Forgive my crypto ignorance, but is this safe? > > The NSS docs say the hashAlg is optional (and if not specified will be > inferred from the RSA key + signature). > > What if the webapp calls verify() with a SHA-1 signature, asking it to > be verified as a SHA-256 signature. If this were to return "true" then > we have a problem. > > It is entirely possible that what I am saying doesn't make sense, but I > think we should be passing > WebCryptoAlgorithmToNssSecOidShaTag(hash_algorithm) here. > > Deferring this review rsleevi as I am unqualified, but I did wanted to > ping it :) > > https://codereview.chromium.org/68303009/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
OOO today, and from cellphone, but... Don't rely on NSS's auto detection. Supply explicitly the hash you expect, let NSS reject it. This will mirror PSS (without such recovery) and is more aligned with how other crypto libraries / smart cards work. I've been trying to clean this up upstream. On Dec 6, 2013 10:53 AM, "Paul Adolph" <padolph@netflix.com> wrote: > (manual email) > > > https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp. > .. > File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): > > > https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp. > .. > content/renderer/webcrypto/webcrypto_impl_nss.cc:260: // TODO(padolph): > Move to > webcrypto_util > On 2013/12/06 02:20:42, eroman wrote: > > This can be done as part of this changelist. > > Done. > > > https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp. > .. > content/renderer/webcrypto/webcrypto_impl_nss.cc:985: memcpy(result.data(), > signature_item->data, signature_item->len); > On 2013/12/06 02:20:42, eroman wrote: > > I believe there is now a helper for this checked in. > > Done. > > > https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp. > .. > content/renderer/webcrypto/webcrypto_impl_nss.cc:1051: SEC_OID_UNKNOWN, > On 2013/12/06 02:20:42, eroman wrote: > > Forgive my crypto ignorance, but is this safe? > > > > The NSS docs say the hashAlg is optional (and if not specified will be > inferred > > from the RSA key + signature). > > > > What if the webapp calls verify() with a SHA-1 signature, asking it to be > > verified as a SHA-256 signature. If this were to return "true" then we > have a > > problem. > > > > It is entirely possible that what I am saying doesn't make sense, but I > think > we > > should be passing WebCryptoAlgorithmToNssSecOidShaTag(hash_algorithm) > here. > > > > Deferring this review rsleevi as I am unqualified, but I did wanted to > ping it > > :) > > We know at this callsite that we have an RSA key, and the NSS docs say "If > the > key is an RSA key, and sig is not NULL, then hashAlg can be > SEC_OID_UNKNOWN". > This makes the code a little simpler and seems like NSS is OK with it. > That's > the only reason I took that shortcut. > > Also would like rsleevi's opinion on this. > > On Thu, Dec 5, 2013 at 6:20 PM, <eroman@chromium.org> wrote: > > > > > https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... > > File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): > > > > > https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... > > content/renderer/webcrypto/webcrypto_impl_nss.cc:260: // TODO(padolph): > > Move to webcrypto_util > > This can be done as part of this changelist. > > > > > https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... > > content/renderer/webcrypto/webcrypto_impl_nss.cc:985: > > memcpy(result.data(), signature_item->data, signature_item->len); > > I believe there is now a helper for this checked in. > > > > > https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... > > content/renderer/webcrypto/webcrypto_impl_nss.cc:1051: SEC_OID_UNKNOWN, > > Forgive my crypto ignorance, but is this safe? > > > > The NSS docs say the hashAlg is optional (and if not specified will be > > inferred from the RSA key + signature). > > > > What if the webapp calls verify() with a SHA-1 signature, asking it to > > be verified as a SHA-256 signature. If this were to return "true" then > > we have a problem. > > > > It is entirely possible that what I am saying doesn't make sense, but I > > think we should be passing > > WebCryptoAlgorithmToNssSecOidShaTag(hash_algorithm) here. > > > > Deferring this review rsleevi as I am unqualified, but I did wanted to > > ping it :) > > > > https://codereview.chromium.org/68303009/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks for the feedback Ryan, enjoy your vacation :) @Paul: Since we decided to restrict by the inner hash when verifying, could you please add an additional unit-test which checks that behavior. In other words, using one of the existing RSASSA SHA-1 signatures from the unittests, try calling verify with algorithm = CreateRsaAlgorithmWithInnerHash(blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5, blink::WebCryptoAlgorithmIdSha256); In the current patchset this will succeed, but we instead want it to fail.
(With the caveat that the RSA should have also been imported with SHA-256 as the inner hash of params, otherwise it will fail early on key mismatch and be an un-interesting test.).
On 2013/12/06 20:22:37, eroman wrote: > Thanks for the feedback Ryan, enjoy your vacation :) > > @Paul: Since we decided to restrict by the inner hash when verifying, could you > please add an additional unit-test which checks that behavior. > > In other words, using one of the existing RSASSA SHA-1 signatures from the > unittests, try calling verify with algorithm = > CreateRsaAlgorithmWithInnerHash(blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5, > blink::WebCryptoAlgorithmIdSha256); > > In the current patchset this will succeed, but we instead want it to fail. Done.
On 2013/12/06 21:33:44, eroman wrote: > (With the caveat that the RSA should have also been imported with SHA-256 as the > inner hash of params, otherwise it will fail early on key mismatch and be an > un-interesting test.). No need for this if I use a key produced by generateKey. In that case the key has WebCryptoRsaKeyGenParams and not WebCryptoRsaSsaParams, so there is no inner hash on the key algorithm to mismatch. This is a quirk of the API - should we be concerned?
https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:1051: SEC_OID_UNKNOWN, On 2013/12/06 18:52:52, padolph wrote: > On 2013/12/06 02:20:42, eroman wrote: > > Forgive my crypto ignorance, but is this safe? > > > > The NSS docs say the hashAlg is optional (and if not specified will be > inferred > > from the RSA key + signature). > > > > What if the webapp calls verify() with a SHA-1 signature, asking it to be > > verified as a SHA-256 signature. If this were to return "true" then we have a > > problem. > > > > It is entirely possible that what I am saying doesn't make sense, but I think > we > > should be passing WebCryptoAlgorithmToNssSecOidShaTag(hash_algorithm) here. > > > > Deferring this review rsleevi as I am unqualified, but I did wanted to ping it > > :) > > We know at this callsite that we have an RSA key, and the NSS docs say "If the > key is an RSA key, and sig is not NULL, then hashAlg can be SEC_OID_UNKNOWN". > This makes the code a little simpler and seems like NSS is OK with it. That's > the only reason I took that shortcut. > > Also would like rsleevi's opinion on this. > Added explicit setting of the hash algorithm, and a unit test to validate. You are correct: the unit test fails without the change. Thanks for pointing this out.
(manual email) On 2013/12/06 20:22:37, eroman wrote: > Thanks for the feedback Ryan, enjoy your vacation :) > > @Paul: Since we decided to restrict by the inner hash when verifying, could you > please add an additional unit-test which checks that behavior. > > In other words, using one of the existing RSASSA SHA-1 signatures from the > unittests, try calling verify with algorithm = > CreateRsaAlgorithmWithInnerHash(blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5, > blink::WebCryptoAlgorithmIdSha256); > > In the current patchset this will succeed, but we instead want it to fail. Done. ================ On 2013/12/06 21:33:44, eroman wrote: > (With the caveat that the RSA should have also been imported with SHA-256 as the > inner hash of params, otherwise it will fail early on key mismatch and be an > un-interesting test.). No need for this if I use a key produced by generateKey. In that case the key has WebCryptoRsaKeyGenParams and not WebCryptoRsaSsaParams, so there is no inner hash on the key algorithm to mismatch. This is a quirk of the API - should we be concerned? ================ https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:1051: SEC_OID_UNKNOWN, On 2013/12/06 18:52:52, padolph wrote: > On 2013/12/06 02:20:42, eroman wrote: > > Forgive my crypto ignorance, but is this safe? > > > > The NSS docs say the hashAlg is optional (and if not specified will be > inferred > > from the RSA key + signature). > > > > What if the webapp calls verify() with a SHA-1 signature, asking it to be > > verified as a SHA-256 signature. If this were to return "true" then we have a > > problem. > > > > It is entirely possible that what I am saying doesn't make sense, but I think > we > > should be passing WebCryptoAlgorithmToNssSecOidShaTag(hash_algorithm) here. > > > > Deferring this review rsleevi as I am unqualified, but I did wanted to ping it > > :) > > We know at this callsite that we have an RSA key, and the NSS docs say "If the > key is an RSA key, and sig is not NULL, then hashAlg can be SEC_OID_UNKNOWN". > This makes the code a little simpler and seems like NSS is OK with it. That's > the only reason I took that shortcut. > > Also would like rsleevi's opinion on this. > Added explicit setting of the hash algorithm, and a unit test to validate. You are correct: the unit test fails without the change. Thanks for pointing this out. On Fri, Dec 6, 2013 at 1:33 PM, <eroman@chromium.org> wrote: > (With the caveat that the RSA should have also been imported with SHA-256 as > the > inner hash of params, otherwise it will fail early on key mismatch and be an > un-interesting test.). > > https://codereview.chromium.org/68303009/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri, Dec 6, 2013 at 4:46 PM, Paul Adolph <padolph@netflix.com> wrote: > (manual email) > > On 2013/12/06 20:22:37, eroman wrote: > > Thanks for the feedback Ryan, enjoy your vacation :) > > > > @Paul: Since we decided to restrict by the inner hash when verifying, > could > you > > please add an additional unit-test which checks that behavior. > > > > In other words, using one of the existing RSASSA SHA-1 signatures from > the > > unittests, try calling verify with algorithm = > > > CreateRsaAlgorithmWithInnerHash(blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5, > > blink::WebCryptoAlgorithmIdSha256); > > > > In the current patchset this will succeed, but we instead want it to > fail. > > Done. > > ================ > > On 2013/12/06 21:33:44, eroman wrote: > > (With the caveat that the RSA should have also been imported with > SHA-256 as > the > > inner hash of params, otherwise it will fail early on key mismatch and > be an > > un-interesting test.). > > No need for this if I use a key produced by generateKey. In that case the > key > has WebCryptoRsaKeyGenParams and not WebCryptoRsaSsaParams, so there is no > inner > hash on the key algorithm to mismatch. This is a quirk of the API - should > we be > concerned? > Agreed it is weird and there is an issue right now. I am waiting on clarification for what normalization to do for key.algorithm: https://www.w3.org/Bugs/Public/show_bug.cgi?id=23098 Right now we are passing-through the algorithm and params specified during importKey/generateKey, which leads to an awkward asymmetry between which parameters the resultant key has. My hope is that any generation parameters be dropped, and the expected parameters on the key will be well defined. > > ================ > > > https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp. > .. > File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): > > > https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp. > .. > content/renderer/webcrypto/webcrypto_impl_nss.cc:1051: SEC_OID_UNKNOWN, > On 2013/12/06 18:52:52, padolph wrote: > > On 2013/12/06 02:20:42, eroman wrote: > > > Forgive my crypto ignorance, but is this safe? > > > > > > The NSS docs say the hashAlg is optional (and if not specified will be > > inferred > > > from the RSA key + signature). > > > > > > What if the webapp calls verify() with a SHA-1 signature, asking it to > be > > > verified as a SHA-256 signature. If this were to return "true" then we > have > a > > > problem. > > > > > > It is entirely possible that what I am saying doesn't make sense, but I > think > > we > > > should be passing WebCryptoAlgorithmToNssSecOidShaTag(hash_algorithm) > here. > > > > > > Deferring this review rsleevi as I am unqualified, but I did wanted to > ping > it > > > :) > > > > We know at this callsite that we have an RSA key, and the NSS docs say > "If the > > key is an RSA key, and sig is not NULL, then hashAlg can be > SEC_OID_UNKNOWN". > > This makes the code a little simpler and seems like NSS is OK with it. > That's > > the only reason I took that shortcut. > > > > Also would like rsleevi's opinion on this. > > > > Added explicit setting of the hash algorithm, and a unit test to validate. > You > are correct: the unit test fails without the change. Thanks for pointing > this > out. > > On Fri, Dec 6, 2013 at 1:33 PM, <eroman@chromium.org> wrote: > > (With the caveat that the RSA should have also been imported with > SHA-256 as > > the > > inner hash of params, otherwise it will fail early on key mismatch and > be an > > un-interesting test.). > > > > https://codereview.chromium.org/68303009/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> > On 2013/12/06 21:33:44, eroman wrote: > > > (With the caveat that the RSA should have also been imported with > > SHA-256 as > > the > > > inner hash of params, otherwise it will fail early on key mismatch and > > be an > > > un-interesting test.). > > > > No need for this if I use a key produced by generateKey. In that case the > > key > > has WebCryptoRsaKeyGenParams and not WebCryptoRsaSsaParams, so there is no > > inner > > hash on the key algorithm to mismatch. This is a quirk of the API - should > > we be > > concerned? > > > > Agreed it is weird and there is an issue right now. I am waiting on > clarification for what normalization to do for key.algorithm: > https://www.w3.org/Bugs/Public/show_bug.cgi?id=23098 > > Right now we are passing-through the algorithm and params specified during > importKey/generateKey, which leads to an awkward asymmetry between which > parameters the resultant key has. My hope is that any generation parameters > be dropped, and the expected parameters on the key will be well defined. Understood. Anything else required before we can commit this?
L G T M after a rebase. I was expecting Ryan to give the final approval on this one, will chat with him to coordinate once next patchset is up. https://codereview.chromium.org/68303009/diff/310001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/310001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:1035: const SECOidTag hash_alg_tag = WebCryptoAlgorithmToNssSecOidShaTag( Please check for SEC_OID_UNKNOWN and fail. Passing SEC_OID_UNKNOWN to VFY_VerifyDataDirect has a very particular meaning and we don't want to do that. https://codereview.chromium.org/68303009/diff/310001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/68303009/diff/310001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:80: #endif // #if !defined(USE_OPENSSL) This will need to be rebased https://codereview.chromium.org/68303009/diff/310001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:82: // TODO(padolph): Move to webcrypto_util This is in webcrypto_util.cc now; could be exposed in the header if needed. https://codereview.chromium.org/68303009/diff/310001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:94: DCHECK(algorithm_id == blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5 || DCHECK_EQ
https://codereview.chromium.org/68303009/diff/310001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/310001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:1035: const SECOidTag hash_alg_tag = WebCryptoAlgorithmToNssSecOidShaTag( On 2013/12/19 22:56:38, eroman wrote: > Please check for SEC_OID_UNKNOWN and fail. > > Passing SEC_OID_UNKNOWN to VFY_VerifyDataDirect has a very particular meaning > and we don't want to do that. Done. https://codereview.chromium.org/68303009/diff/310001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/68303009/diff/310001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:80: #endif // #if !defined(USE_OPENSSL) On 2013/12/19 22:56:38, eroman wrote: > This will need to be rebased Done. https://codereview.chromium.org/68303009/diff/310001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:82: // TODO(padolph): Move to webcrypto_util On 2013/12/19 22:56:38, eroman wrote: > This is in webcrypto_util.cc now; could be exposed in the header if needed. Done. https://codereview.chromium.org/68303009/diff/310001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:94: DCHECK(algorithm_id == blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5 || On 2013/12/19 22:56:38, eroman wrote: > DCHECK_EQ Not sure how to use that here. I want to ensure that algorithm_id is either WebCryptoAlgorithmIdRsaSsaPkcs1v1_5 or WebCryptoAlgorithmIdRsaOaep. DCHECK on the boolean OR result seems like a better choice.
Does anybody mind the huge test vector set for this CL? IMO it gets in the way of readability, but I don't have a good idea of which vectors are best to thin out. Please advise.
LGTM. I don't mind the big interleaving the test data, I think it is consistent with our other tests. https://codereview.chromium.org/68303009/diff/310001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/68303009/diff/310001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:94: DCHECK(algorithm_id == blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5 || On 2013/12/20 00:03:39, padolph wrote: > On 2013/12/19 22:56:38, eroman wrote: > > DCHECK_EQ > > Not sure how to use that here. I want to ensure that algorithm_id is either > WebCryptoAlgorithmIdRsaSsaPkcs1v1_5 or WebCryptoAlgorithmIdRsaOaep. DCHECK on > the boolean OR result seems like a better choice. My mistake, didn't notice the ||. https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:86: return SEC_OID_UNKNOWN; This seems like it should be a DCHECK() instead. https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1673: bool signature_match = false; For the purpose of this test, I actually suggest leaving all initializiation off of signature_match. That way failure to set it to false has the chance of being caught by our memory tools. https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1700: signature_match = false; Same here, leave off the reset https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1715: signature_match = false; remove https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1748: // TODO(padolph): Not sure this kind of test is required here, it might be Doesn't hurt to have it; but you are right, it is an invariant that should be enforced by blink side. https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1755: // TODO(padolph): Not sure this kind of test is required here, it might be Actually currently the blink side is not testing that the inner hash matches (it does for HMAC, but there isn't a systematic definition of what it means for algorithms to be consistent). I have proposed on the WG list that the spec enumerate a normalization table for what parameters identify the "algorithm". Meanwhile I will also update the blink side. Thanks for reminding me! https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1789: // algorithm has WebCryptoRsaKeyGenParams and not WebCryptoRsaSsaParams. Thus Thanks for the reminder: I will change this on the blink side, since it is definitely weird right now. I will assume a normalization step for reflected Key.algorithm, and define an equality test for key parameters to simplify it. Hopefully the spec adopts the same concept. https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:2224: signature.reset(); Can you delete this line? I believe SignInternal() should be overwriting. https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:2228: bool is_match = false; Please remove the initialization.
https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:86: return SEC_OID_UNKNOWN; On 2013/12/20 01:12:50, eroman wrote: > This seems like it should be a DCHECK() instead. Done. https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1673: bool signature_match = false; On 2013/12/20 01:12:50, eroman wrote: > For the purpose of this test, I actually suggest leaving all initializiation off > of signature_match. That way failure to set it to false has the chance of being > caught by our memory tools. Done. https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1700: signature_match = false; On 2013/12/20 01:12:50, eroman wrote: > Same here, leave off the reset Done. https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1715: signature_match = false; On 2013/12/20 01:12:50, eroman wrote: > remove Done. https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1748: // TODO(padolph): Not sure this kind of test is required here, it might be On 2013/12/20 01:12:50, eroman wrote: > Doesn't hurt to have it; but you are right, it is an invariant that should be > enforced by blink side. Done. https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1755: // TODO(padolph): Not sure this kind of test is required here, it might be On 2013/12/20 01:12:50, eroman wrote: > Actually currently the blink side is not testing that the inner hash matches (it > does for HMAC, but there isn't a systematic definition of what it means for > algorithms to be consistent). > > I have proposed on the WG list that the spec enumerate a normalization table for > what parameters identify the "algorithm". Meanwhile I will also update the blink > side. > > Thanks for reminding me! Done.
(manual email) Are we still waiting for an LGTM from Ryan? https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:86: return SEC_OID_UNKNOWN; On 2013/12/20 01:12:50, eroman wrote: > This seems like it should be a DCHECK() instead. Done. https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1673: bool signature_match = false; On 2013/12/20 01:12:50, eroman wrote: > For the purpose of this test, I actually suggest leaving all initializiation off > of signature_match. That way failure to set it to false has the chance of being > caught by our memory tools. Done. https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1700: signature_match = false; On 2013/12/20 01:12:50, eroman wrote: > Same here, leave off the reset Done. https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1715: signature_match = false; On 2013/12/20 01:12:50, eroman wrote: > remove Done. https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1748: // TODO(padolph): Not sure this kind of test is required here, it might be On 2013/12/20 01:12:50, eroman wrote: > Doesn't hurt to have it; but you are right, it is an invariant that should be > enforced by blink side. Done. https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1755: // TODO(padolph): Not sure this kind of test is required here, it might be On 2013/12/20 01:12:50, eroman wrote: > Actually currently the blink side is not testing that the inner hash matches (it > does for HMAC, but there isn't a systematic definition of what it means for > algorithms to be consistent). > > I have proposed on the WG list that the spec enumerate a normalization table for > what parameters identify the "algorithm". Meanwhile I will also update the blink > side. > > Thanks for reminding me! Done. On Thu, Dec 19, 2013 at 5:12 PM, <eroman@chromium.org> wrote: > LGTM. > > I don't mind the big interleaving the test data, I think it is consistent > with > our other tests. > > > > https://codereview.chromium.org/68303009/diff/310001/content/renderer/webcryp... > File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): > > https://codereview.chromium.org/68303009/diff/310001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:94: > DCHECK(algorithm_id == blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5 || > On 2013/12/20 00:03:39, padolph wrote: >> >> On 2013/12/19 22:56:38, eroman wrote: >> > DCHECK_EQ > > >> Not sure how to use that here. I want to ensure that algorithm_id is > > either >> >> WebCryptoAlgorithmIdRsaSsaPkcs1v1_5 or WebCryptoAlgorithmIdRsaOaep. > > DCHECK on >> >> the boolean OR result seems like a better choice. > > > My mistake, didn't notice the ||. > > https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... > File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): > > https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_nss.cc:86: return > SEC_OID_UNKNOWN; > This seems like it should be a DCHECK() instead. > > https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... > File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): > > https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:1673: bool > signature_match = false; > For the purpose of this test, I actually suggest leaving all > initializiation off of signature_match. That way failure to set it to > false has the chance of being caught by our memory tools. > > https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:1700: > signature_match = false; > Same here, leave off the reset > > https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:1715: > signature_match = false; > remove > > https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:1748: // > TODO(padolph): Not sure this kind of test is required here, it might be > Doesn't hurt to have it; but you are right, it is an invariant that > should be enforced by blink side. > > https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:1755: // > TODO(padolph): Not sure this kind of test is required here, it might be > Actually currently the blink side is not testing that the inner hash > matches (it does for HMAC, but there isn't a systematic definition of > what it means for algorithms to be consistent). > > I have proposed on the WG list that the spec enumerate a normalization > table for what parameters identify the "algorithm". Meanwhile I will > also update the blink side. > > Thanks for reminding me! > > https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:1789: // algorithm > has WebCryptoRsaKeyGenParams and not WebCryptoRsaSsaParams. Thus > Thanks for the reminder: > > I will change this on the blink side, since it is definitely weird right > now. I will assume a normalization step for reflected Key.algorithm, and > define an equality test for key parameters to simplify it. Hopefully the > spec adopts the same concept. > > https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:2224: > signature.reset(); > Can you delete this line? I believe SignInternal() should be > overwriting. > > https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:2228: bool > is_match = false; > Please remove the initialization. > > https://codereview.chromium.org/68303009/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:972: hash_alg_tag); This is unnecessary. The design of this API was meant to be forward-flexible with other RSA schemes, but it's not - because PSS has additional parameters that need to be represented. You should instead do a mapping to the NSS sign_alg_tag based on the WebCrypto inner hash algorithm and the knowledge that this will always be RSA-SSA signatures. That is, the "SEC_GetSignatureAlgorithmOidTag" is a bad API, although the code will work correctly. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:1029: unnecessary newline https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:1030: // Only public key signature verification is supported. This comment makes no sense, given the algorithm being discussed. RSA-SSA is a public-key only algorithm. "private key signature verification" isn't a term https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:1059: NULL) == SECSuccess; Was this run through git cl format? The formatting here seems... odd? That said, the "== SECSuccess" at the end is especially subtle, so it seems better to write this yoda style *signature_match = SECSuccess == VFY_VerifyDataDirect(arg, arg, arg, arg arg, arg, arg, arg); Which is valid style guide. More of a nit than anything. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1654: unnecessary line break https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1665: algorithm, true, usage_mask, &public_key, &private_key)); Tests that generate keys = tests that slow down. If the import functionality is wired, can we not use that? Alternatively, can we not add a test helper that can take a binary key and initailize a blink key for it? (eg: ImportInternal) https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1675: const char* kTestData[] = {"", "00", "010203040506070809"}; As a corpus of test data, this is not really a good test set. It fails to test any of the boundary or edge conditions for spec conformance. As such, I'm not really sure what the goal of these different inputs is. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1677: unnecessary line break https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1722: // Ensure extra long signature does not cause issues and fails. I have trouble parsing this sentence "does not cause issues" ... ok "and fails"... I would think "failure" as "an issue" // Ensure signatures that are greater than the modulus size fail. ? Also, 1024 should be a local constant (used up on line 1658) so that as/if the key size changes, this test continues to test what it's supposed to. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1733: // Ensure can't verify using a private key. Proper sentence structure // Ensure that verifying using a private key, rather than a public key, fails Similar concerns with the remaining comments in this test. Make them full descriptive sentences. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1769: // is specified by the input algorithm (desired), the verify will fail. Do not use pronouns in comments ("we"). https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1845: const TestCase kTests[] = { I'm really getting the feeling that we're mixing too much test *data* in with the tests. The wrapping here is also very much inconsistent with the preceding test data. Definitely feel we should be looking at moving this out to its own file (or a series of test files) https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:2212: unnecessary vertical whitespace.
https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:972: hash_alg_tag); On 2013/12/20 02:08:50, Ryan Sleevi wrote: > This is unnecessary. The design of this API was meant to be forward-flexible > with other RSA schemes, but it's not - because PSS has additional parameters > that need to be represented. > > You should instead do a mapping to the NSS sign_alg_tag based on the WebCrypto > inner hash algorithm and the knowledge that this will always be RSA-SSA > signatures. > > That is, the "SEC_GetSignatureAlgorithmOidTag" is a bad API, although the code > will work correctly. Done. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:1029: On 2013/12/20 02:08:50, Ryan Sleevi wrote: > unnecessary newline Done. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:1030: // Only public key signature verification is supported. On 2013/12/20 02:08:50, Ryan Sleevi wrote: > This comment makes no sense, given the algorithm being discussed. RSA-SSA is a > public-key only algorithm. > > "private key signature verification" isn't a term Done. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:1059: NULL) == SECSuccess; On 2013/12/20 02:08:50, Ryan Sleevi wrote: > Was this run through git cl format? The formatting here seems... odd? > > That said, the "== SECSuccess" at the end is especially subtle, so it seems > better to write this yoda style > > *signature_match = > SECSuccess == VFY_VerifyDataDirect(arg, arg, arg, arg > arg, arg, arg, > arg); > > Which is valid style guide. More of a nit than anything. > Rewrote yoda style and reformatted, and ran through clang-format -style=Chromium. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1654: On 2013/12/20 02:08:50, Ryan Sleevi wrote: > unnecessary line break Done. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1665: algorithm, true, usage_mask, &public_key, &private_key)); On 2013/12/20 02:08:50, Ryan Sleevi wrote: > Tests that generate keys = tests that slow down. > > If the import functionality is wired, can we not use that? > > Alternatively, can we not add a test helper that can take a binary key and > initailize a blink key for it? (eg: ImportInternal) Changed here to import known keys, and similarly changed all other instances of key generation where import can be used instead. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1675: const char* kTestData[] = {"", "00", "010203040506070809"}; On 2013/12/20 02:08:50, Ryan Sleevi wrote: > As a corpus of test data, this is not really a good test set. It fails to test > any of the boundary or edge conditions for spec conformance. As such, I'm not > really sure what the goal of these different inputs is. Me neither. I copied it from the HMAC sign/verify test, so it probably does not apply. Deleted. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1677: On 2013/12/20 02:08:50, Ryan Sleevi wrote: > unnecessary line break Done. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1722: // Ensure extra long signature does not cause issues and fails. On 2013/12/20 02:08:50, Ryan Sleevi wrote: > I have trouble parsing this sentence > > "does not cause issues" ... ok > "and fails"... I would think "failure" as "an issue" > > // Ensure signatures that are greater than the modulus size fail. > > ? > > Also, 1024 should be a local constant (used up on line 1658) so that as/if the > key size changes, this test continues to test what it's supposed to. 1024 above is the modulus size in bits. The value here is bytes, intended to be much greater than the modulus size to check for overrun, etc. It's unlikely that anyone would change the test to use a 8192-bit key, right? https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1733: // Ensure can't verify using a private key. On 2013/12/20 02:08:50, Ryan Sleevi wrote: > Proper sentence structure > > // Ensure that verifying using a private key, rather than a public key, fails > > Similar concerns with the remaining comments in this test. Make them full > descriptive sentences. Done. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1769: // is specified by the input algorithm (desired), the verify will fail. On 2013/12/20 02:08:50, Ryan Sleevi wrote: > Do not use pronouns in comments ("we"). Done. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1845: const TestCase kTests[] = { On 2013/12/20 02:08:50, Ryan Sleevi wrote: > I'm really getting the feeling that we're mixing too much test *data* in with > the tests. The wrapping here is also very much inconsistent with the preceding > test data. > > Definitely feel we should be looking at moving this out to its own file (or a > series of test files) Adjusted wrapping. I'm hesitant to create a new file for test data in this CL. But I agree we should probably eventually create a test vectors file. That might also be useful to the Layout Tests in some form. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:2212: On 2013/12/20 02:08:50, Ryan Sleevi wrote: > unnecessary vertical whitespace. Done.
(manual email) https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:972: hash_alg_tag); On 2013/12/20 02:08:50, Ryan Sleevi wrote: > This is unnecessary. The design of this API was meant to be forward-flexible > with other RSA schemes, but it's not - because PSS has additional parameters > that need to be represented. > > You should instead do a mapping to the NSS sign_alg_tag based on the WebCrypto > inner hash algorithm and the knowledge that this will always be RSA-SSA > signatures. > > That is, the "SEC_GetSignatureAlgorithmOidTag" is a bad API, although the code > will work correctly. Done. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:1029: On 2013/12/20 02:08:50, Ryan Sleevi wrote: > unnecessary newline Done. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:1030: // Only public key signature verification is supported. On 2013/12/20 02:08:50, Ryan Sleevi wrote: > This comment makes no sense, given the algorithm being discussed. RSA-SSA is a > public-key only algorithm. > > "private key signature verification" isn't a term Done. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:1059: NULL) == SECSuccess; On 2013/12/20 02:08:50, Ryan Sleevi wrote: > Was this run through git cl format? The formatting here seems... odd? > > That said, the "== SECSuccess" at the end is especially subtle, so it seems > better to write this yoda style > > *signature_match = > SECSuccess == VFY_VerifyDataDirect(arg, arg, arg, arg > arg, arg, arg, > arg); > > Which is valid style guide. More of a nit than anything. > Rewrote yoda style and reformatted, and ran through clang-format -style=Chromium. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1654: On 2013/12/20 02:08:50, Ryan Sleevi wrote: > unnecessary line break Done. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1665: algorithm, true, usage_mask, &public_key, &private_key)); On 2013/12/20 02:08:50, Ryan Sleevi wrote: > Tests that generate keys = tests that slow down. > > If the import functionality is wired, can we not use that? > > Alternatively, can we not add a test helper that can take a binary key and > initailize a blink key for it? (eg: ImportInternal) Changed here to import known keys, and similarly changed all other instances of key generation where import can be used instead. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1675: const char* kTestData[] = {"", "00", "010203040506070809"}; On 2013/12/20 02:08:50, Ryan Sleevi wrote: > As a corpus of test data, this is not really a good test set. It fails to test > any of the boundary or edge conditions for spec conformance. As such, I'm not > really sure what the goal of these different inputs is. Me neither. I copied it from the HMAC sign/verify test, so it probably does not apply. Deleted. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1677: On 2013/12/20 02:08:50, Ryan Sleevi wrote: > unnecessary line break Done. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1722: // Ensure extra long signature does not cause issues and fails. On 2013/12/20 02:08:50, Ryan Sleevi wrote: > I have trouble parsing this sentence > > "does not cause issues" ... ok > "and fails"... I would think "failure" as "an issue" > > // Ensure signatures that are greater than the modulus size fail. > > ? > > Also, 1024 should be a local constant (used up on line 1658) so that as/if the > key size changes, this test continues to test what it's supposed to. 1024 above is the modulus size in bits. The value here is bytes, intended to be much greater than the modulus size to check for overrun, etc. It's unlikely that anyone would change the test to use a 8192-bit key, right? https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1733: // Ensure can't verify using a private key. On 2013/12/20 02:08:50, Ryan Sleevi wrote: > Proper sentence structure > > // Ensure that verifying using a private key, rather than a public key, fails > > Similar concerns with the remaining comments in this test. Make them full > descriptive sentences. Done. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1769: // is specified by the input algorithm (desired), the verify will fail. On 2013/12/20 02:08:50, Ryan Sleevi wrote: > Do not use pronouns in comments ("we"). Done. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1845: const TestCase kTests[] = { On 2013/12/20 02:08:50, Ryan Sleevi wrote: > I'm really getting the feeling that we're mixing too much test *data* in with > the tests. The wrapping here is also very much inconsistent with the preceding > test data. > > Definitely feel we should be looking at moving this out to its own file (or a > series of test files) Adjusted wrapping. I'm hesitant to create a new file for test data in this CL. But I agree we should probably eventually create a test vectors file. That might also be useful to the Layout Tests in some form. https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:2212: On 2013/12/20 02:08:50, Ryan Sleevi wrote: > unnecessary vertical whitespace. Done. On Thu, Dec 19, 2013 at 6:08 PM, <rsleevi@chromium.org> wrote: > > https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... > File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): > > https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_nss.cc:972: hash_alg_tag); > This is unnecessary. The design of this API was meant to be > forward-flexible with other RSA schemes, but it's not - because PSS has > additional parameters that need to be represented. > > You should instead do a mapping to the NSS sign_alg_tag based on the > WebCrypto inner hash algorithm and the knowledge that this will always > be RSA-SSA signatures. > > That is, the "SEC_GetSignatureAlgorithmOidTag" is a bad API, although > the code will work correctly. > > https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_nss.cc:1029: > unnecessary newline > > https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_nss.cc:1030: // Only public > key signature verification is supported. > This comment makes no sense, given the algorithm being discussed. > RSA-SSA is a public-key only algorithm. > > "private key signature verification" isn't a term > > https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_nss.cc:1059: NULL) == > SECSuccess; > Was this run through git cl format? The formatting here seems... odd? > > That said, the "== SECSuccess" at the end is especially subtle, so it > seems better to write this yoda style > > *signature_match = > SECSuccess == VFY_VerifyDataDirect(arg, arg, arg, arg > arg, arg, > arg, arg); > > Which is valid style guide. More of a nit than anything. > > https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... > File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): > > https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:1654: > unnecessary line break > > https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:1665: algorithm, > true, usage_mask, &public_key, &private_key)); > Tests that generate keys = tests that slow down. > > If the import functionality is wired, can we not use that? > > Alternatively, can we not add a test helper that can take a binary key > and initailize a blink key for it? (eg: ImportInternal) > > https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:1675: const char* > kTestData[] = {"", "00", "010203040506070809"}; > As a corpus of test data, this is not really a good test set. It fails > to test any of the boundary or edge conditions for spec conformance. As > such, I'm not really sure what the goal of these different inputs is. > > https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:1677: > unnecessary line break > > https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:1722: // Ensure > extra long signature does not cause issues and fails. > I have trouble parsing this sentence > > "does not cause issues" ... ok > "and fails"... I would think "failure" as "an issue" > > // Ensure signatures that are greater than the modulus size fail. > > ? > > Also, 1024 should be a local constant (used up on line 1658) so that > as/if the key size changes, this test continues to test what it's > supposed to. > > https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:1733: // Ensure > can't verify using a private key. > Proper sentence structure > > // Ensure that verifying using a private key, rather than a public key, > fails > > Similar concerns with the remaining comments in this test. Make them > full descriptive sentences. > > https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:1769: // is > specified by the input algorithm (desired), the verify will fail. > Do not use pronouns in comments ("we"). > > https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:1845: const > TestCase kTests[] = { > I'm really getting the feeling that we're mixing too much test *data* in > with the tests. The wrapping here is also very much inconsistent with > the preceding test data. > > Definitely feel we should be looking at moving this out to its own file > (or a series of test files) > > https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:2212: > unnecessary vertical whitespace. > > https://codereview.chromium.org/68303009/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
lgtm https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:146: const std::string kPublicKeySpkiDerHex = Chromium style doesn't allow for non plain-old-data globals to avoid static initializers. This isn't generally enforced in unittests, but these are easy enough to change to const char[] that I recommend that. https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:223: if (public_key->isNull() || !public_key->handle()) I don't be4lieve it is possible (or desirable) for ImportKeyInternal to return true and give a null key. Hence I think this would be clearer as a DCHECK() rather than an if. https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:235: if (private_key->isNull() || !private_key->handle()) Same comment here. https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:38: bool IsHashAlgorithm(blink::WebCryptoAlgorithmId alg_id); This needs to be marked as CONTENT_EXPORT or it will result in a linking failure for the content_unittests (since it is used from the _unittest.cc, which may be compiled as a separate module)
https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:146: const std::string kPublicKeySpkiDerHex = On 2014/01/15 23:10:02, eroman wrote: > Chromium style doesn't allow for non plain-old-data globals to avoid static > initializers. This isn't generally enforced in unittests, but these are easy > enough to change to const char[] that I recommend that. Done. https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:223: if (public_key->isNull() || !public_key->handle()) On 2014/01/15 23:10:02, eroman wrote: > I don't be4lieve it is possible (or desirable) for ImportKeyInternal to return > true and give a null key. Hence I think this would be clearer as a DCHECK() > rather than an if. I changed the logic to use EXPECT's to be consistent with ImportSecretKeyFromRawHexString() above. https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:235: if (private_key->isNull() || !private_key->handle()) On 2014/01/15 23:10:02, eroman wrote: > Same comment here. Done. https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:38: bool IsHashAlgorithm(blink::WebCryptoAlgorithmId alg_id); On 2014/01/15 23:10:02, eroman wrote: > This needs to be marked as CONTENT_EXPORT or it will result in a linking failure > for the content_unittests (since it is used from the _unittest.cc, which may be > compiled as a separate module) Done.
(manual email) 2 minutes ago #32 https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:146: const std::string kPublicKeySpkiDerHex = On 2014/01/15 23:10:02, eroman wrote: > Chromium style doesn't allow for non plain-old-data globals to avoid static > initializers. This isn't generally enforced in unittests, but these are easy > enough to change to const char[] that I recommend that. Done. https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:223: if (public_key->isNull() || !public_key->handle()) On 2014/01/15 23:10:02, eroman wrote: > I don't be4lieve it is possible (or desirable) for ImportKeyInternal to return > true and give a null key. Hence I think this would be clearer as a DCHECK() > rather than an if. I changed the logic to use EXPECT's to be consistent with ImportSecretKeyFromRawHexString() above. https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:235: if (private_key->isNull() || !private_key->handle()) On 2014/01/15 23:10:02, eroman wrote: > Same comment here. Done. https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:38: bool IsHashAlgorithm(blink::WebCryptoAlgorithmId alg_id); On 2014/01/15 23:10:02, eroman wrote: > This needs to be marked as CONTENT_EXPORT or it will result in a linking failure > for the content_unittests (since it is used from the _unittest.cc, which may be > compiled as a separate module) Done. On Wed, Jan 15, 2014 at 3:10 PM, <eroman@chromium.org> wrote: > lgtm > > > > > https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... > File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): > > https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:146: const > std::string kPublicKeySpkiDerHex = > Chromium style doesn't allow for non plain-old-data globals to avoid > static initializers. This isn't generally enforced in unittests, but > these are easy enough to change to const char[] that I recommend that. > > https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:223: if > (public_key->isNull() || !public_key->handle()) > I don't be4lieve it is possible (or desirable) for ImportKeyInternal to > return true and give a null key. Hence I think this would be clearer as > a DCHECK() rather than an if. > > https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:235: if > (private_key->isNull() || !private_key->handle()) > Same comment here. > > https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... > File content/renderer/webcrypto/webcrypto_util.h (right): > > https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_util.h:38: bool > IsHashAlgorithm(blink::WebCryptoAlgorithmId alg_id); > This needs to be marked as CONTENT_EXPORT or it will result in a linking > failure for the content_unittests (since it is used from the > _unittest.cc, which may be compiled as a separate module) > > https://codereview.chromium.org/68303009/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/68303009/640001
Retried try job too often on win_x64_rel for step(s) base_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/68303009/910001
(This isn't going to work until I roll NSS forward too, cancelling commit).
Oh. Sorry to jump the gun. On Thu, Jan 16, 2014 at 12:25 PM, <eroman@chromium.org> wrote: > (This isn't going to work until I roll NSS forward too, cancelling commit). > > https://codereview.chromium.org/68303009/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/68303009/910001
Message was sent while issue was closed.
Change committed as 245430 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
