Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
195893034:
[webcrypto] Add JWK symmetric key RSAES-PKCS1-v1_5 wrap / unwrap for NSS. (Closed)
|
Created:
6 years, 9 months ago by padolph Modified:
6 years, 9 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 JWK symmetric key RSAES-PKCS1-v1_5 wrap / unwrap for NSS.
BUG=245025
TEST=content_unittests --gtest_filter="SharedCryptoTest*"
Based on https://codereview.chromium.org/195983010/
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258223
Patch Set 1 #
Total comments: 13
Patch Set 2 : fixes for eroman #Patch Set 3 : rebase #
Total comments: 2
Patch Set 4 : fixes for eroman #
Total comments: 6
Patch Set 5 : rebase and upload only HEAD~ #Patch Set 6 : upload HEAD~2 #
Messages
Total messages: 12 (0 generated)
https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/jwk.cc File content/child/webcrypto/jwk.cc (right): https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/jwk.... content/child/webcrypto/jwk.cc:169: if (alg1.paramsType() == I am not sure that I understand the change made here, can you explain? https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/shar... File content/child/webcrypto/shared_crypto.cc (left): https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/shar... content/child/webcrypto/shared_crypto.cc:29: bool KeyUsageAllowsAnyOf(const blink::WebCryptoKey& key, this probably needs to be rebased, these diffs shouldn't be here https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/shar... File content/child/webcrypto/shared_crypto.cc (right): https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/shar... content/child/webcrypto/shared_crypto.cc:649: if (!KeyUsageAllows(wrapping_key, blink::WebCryptoKeyUsageWrapKey)) hah! these checks are mainly a precaution, since the blink side should have already verified usages before calling in. https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/shar... File content/child/webcrypto/shared_crypto_unittest.cc (left): https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/shar... content/child/webcrypto/shared_crypto_unittest.cc:2642: TEST_F(SharedCryptoTest, MAYBE(AesKwJwkSymkeyUnwrapErrors)) { ditto: these diffs should not be in here https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/shar... File content/child/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/shar... content/child/webcrypto/shared_crypto_unittest.cc:2936: TEST_F(SharedCryptoTest, MAYBE(RsaEsJwkSymkeyUnwrapKnownAnswer)) { Is this test related to the code changes? Seems like this is testing unwrap whereas this change is about wrap(). Or is this a carry-over from the rebase? This isn't a bad test by any means, I just want to understand where it fits in. https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/shar... content/child/webcrypto/shared_crypto_unittest.cc:2959: blink::WebCryptoKeyUsageDecrypt | blink::WebCryptoKeyUsageUnwrapKey, Would it be more interesting if the unwrapping key only had Unwrap usage? That way if the code relies on calling decrypt directly will fail.
https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/jwk.cc File content/child/webcrypto/jwk.cc (right): https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/jwk.... content/child/webcrypto/jwk.cc:169: if (alg1.paramsType() == On 2014/03/18 06:18:39, eroman wrote: > I am not sure that I understand the change made here, can you explain? Without this change the RsaEsJwkSymkeyWrapUnwrapRoundTrip test fails. The use case is to call GenerateKey() to create a key, call WrapKey() to wrap it, then UnwrapKey() to get it back. During the unwrap, since this test uses JWK format, this function is called to compare the key algorithm extracted from the JWK to the algorithm passed into the UnwrapKey() method. In this case the algorithm passed into UnwrapKey() is the same instance that was used to generate the symkey; namely, one produced by CreateAesCbcKeyGenAlgorithm(). Prior to this change, ImportAlgorithmsConsistent() failed because the paramsType of a key produced by CreateAesCbcKeyGenAlgorithm() is different from the paramsType of a key produced by CreateAlgorithm() (as jwk.c does with CreateImportAlgorithm()). I think the paramsType check is not relevant in this case, since there is no inner hash. Indeed I think using generated symmetric keys is a typical use case for ephemeral keys, and wrap/JWK import/export on such keys is sometimes used for persistence or exchange. I confess I have not followed your changes around 'key' algorithms too closely, so there might be a better way to address this. https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/shar... File content/child/webcrypto/shared_crypto.cc (left): https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/shar... content/child/webcrypto/shared_crypto.cc:29: bool KeyUsageAllowsAnyOf(const blink::WebCryptoKey& key, On 2014/03/18 06:18:39, eroman wrote: > this probably needs to be rebased, these diffs shouldn't be here 'git cl rebase' does not work for me. I get the error update-index --refresh: command returned error: 1 third_party/WebKit: needs update I searched the forums and a few people also see this problem, but no solutions are offered as far as I found. For this CL, instead of 'cl rebase' I did a 'cl patch' from the other CL, and when I uploaded I did 'git cl upload HEAD~'. I'm not sure why this diff got uploaded. In any event, this diff will go away once that other CL lands and I rebase from that. https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/shar... File content/child/webcrypto/shared_crypto.cc (right): https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/shar... content/child/webcrypto/shared_crypto.cc:649: if (!KeyUsageAllows(wrapping_key, blink::WebCryptoKeyUsageWrapKey)) On 2014/03/18 06:18:39, eroman wrote: > hah! these checks are mainly a precaution, since the blink side should have > already verified usages before calling in. Was hoping I could sneak this fix in unnoticed ;-) https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/shar... File content/child/webcrypto/shared_crypto_unittest.cc (left): https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/shar... content/child/webcrypto/shared_crypto_unittest.cc:2642: TEST_F(SharedCryptoTest, MAYBE(AesKwJwkSymkeyUnwrapErrors)) { On 2014/03/18 06:18:39, eroman wrote: > ditto: these diffs should not be in here Will rebase once the other CL lands. https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/shar... File content/child/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/shar... content/child/webcrypto/shared_crypto_unittest.cc:2936: TEST_F(SharedCryptoTest, MAYBE(RsaEsJwkSymkeyUnwrapKnownAnswer)) { On 2014/03/18 06:18:39, eroman wrote: > Is this test related to the code changes? Seems like this is testing unwrap > whereas this change is about wrap(). Or is this a carry-over from the rebase? > > This isn't a bad test by any means, I just want to understand where it fits in. This CL covers both RSAES-PKCS1-v1_5 wrap and unwrap. This is a new test for this CL. https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/shar... content/child/webcrypto/shared_crypto_unittest.cc:2959: blink::WebCryptoKeyUsageDecrypt | blink::WebCryptoKeyUsageUnwrapKey, On 2014/03/18 06:18:39, eroman wrote: > Would it be more interesting if the unwrapping key only had Unwrap usage? That > way if the code relies on calling decrypt directly will fail. Good idea. Yes, I'll make a change to use separate keys for the decrypt and unwrap phases..
https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/jwk.cc File content/child/webcrypto/jwk.cc (right): https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/jwk.... content/child/webcrypto/jwk.cc:169: if (alg1.paramsType() == On 2014/03/18 17:52:19, padolph wrote: > On 2014/03/18 06:18:39, eroman wrote: > > I am not sure that I understand the change made here, can you explain? > > Without this change the RsaEsJwkSymkeyWrapUnwrapRoundTrip test fails. The use > case is to call GenerateKey() to create a key, call WrapKey() to wrap it, then > UnwrapKey() to get it back. > > During the unwrap, since this test uses JWK format, this function is called to > compare the key algorithm extracted from the JWK to the algorithm passed into > the UnwrapKey() method. In this case the algorithm passed into UnwrapKey() is > the same instance that was used to generate the symkey; namely, one produced by > CreateAesCbcKeyGenAlgorithm(). > > Prior to this change, ImportAlgorithmsConsistent() failed because the paramsType > of a key produced by CreateAesCbcKeyGenAlgorithm() is different from the > paramsType of a key produced by CreateAlgorithm() (as jwk.c does with > CreateImportAlgorithm()). > > I think the paramsType check is not relevant in this case, since there is no > inner hash. Indeed I think using generated symmetric keys is a typical use case > for ephemeral keys, and wrap/JWK import/export on such keys is sometimes used > for persistence or exchange. > > I confess I have not followed your changes around 'key' algorithms too closely, > so there might be a better way to address this. I see thanks for explaining. That sounds like a problem with the test then: UnwrapKey() should get called with an "import algorithm" rather than a "generate algorithm". I will point out the changes I suggest in that file. From a high level perspective, Blink will guarantee that the WebCryptoAlgorithm passed in is appropriate for the algorithm+operation. So we can be certain that only import algorithms will be given to unwrap key, and hence no need for our unit-tests to check otherwise. To see what types are expected you can look at the data tables in the spec and see the name of the parameters. For instance: https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#aes-cbc I realize that interpreting the WebCryptoAlgorithm is pretty tedious and magical. I have a refactor which does the fan-out at the blink layer, so the interfaces on the chromium side don't even take WebCryptoAlgorithm anymore, they take the parameters directly (i.e. what platform_crypto.h is today). But it is a very large change and I wouldn't dare push it right now as we are actively working on things, as it will be a merging nightmare :) https://codereview.chromium.org/195893034/diff/30001/content/child/webcrypto/... File content/child/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/195893034/diff/30001/content/child/webcrypto/... content/child/webcrypto/shared_crypto_unittest.cc:2978: const blink::WebCryptoAlgorithm algorithm = CreateAesCbcKeyGenAlgorithm(256); Call this gen_algorithm instead. And then at the callsite where it calls Unwrap, pass in CreateAlgorithm(AesCBC).
On 2014/03/19 04:04:00, eroman wrote: > https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/jwk.cc > File content/child/webcrypto/jwk.cc (right): > > https://codereview.chromium.org/195893034/diff/1/content/child/webcrypto/jwk.... > content/child/webcrypto/jwk.cc:169: if (alg1.paramsType() == > On 2014/03/18 17:52:19, padolph wrote: > > On 2014/03/18 06:18:39, eroman wrote: > > > I am not sure that I understand the change made here, can you explain? > > > > Without this change the RsaEsJwkSymkeyWrapUnwrapRoundTrip test fails. The use > > case is to call GenerateKey() to create a key, call WrapKey() to wrap it, then > > UnwrapKey() to get it back. > > > > During the unwrap, since this test uses JWK format, this function is called to > > compare the key algorithm extracted from the JWK to the algorithm passed into > > the UnwrapKey() method. In this case the algorithm passed into UnwrapKey() is > > the same instance that was used to generate the symkey; namely, one produced > by > > CreateAesCbcKeyGenAlgorithm(). > > > > Prior to this change, ImportAlgorithmsConsistent() failed because the > paramsType > > of a key produced by CreateAesCbcKeyGenAlgorithm() is different from the > > paramsType of a key produced by CreateAlgorithm() (as jwk.c does with > > CreateImportAlgorithm()). > > > > I think the paramsType check is not relevant in this case, since there is no > > inner hash. Indeed I think using generated symmetric keys is a typical use > case > > for ephemeral keys, and wrap/JWK import/export on such keys is sometimes used > > for persistence or exchange. > > > > I confess I have not followed your changes around 'key' algorithms too > closely, > > so there might be a better way to address this. > > I see thanks for explaining. > > That sounds like a problem with the test then: > > UnwrapKey() should get called with an "import algorithm" rather than a > "generate algorithm". I will point out the changes I suggest in that file. > > From a high level perspective, Blink will guarantee that the WebCryptoAlgorithm > passed in is appropriate for the algorithm+operation. So we can be certain that > only import algorithms will be given to unwrap key, and hence no need for our > unit-tests to check otherwise. Ok, understood, thanks. We should probably make sure to cover this subtlety in the Javascript tests then.
https://codereview.chromium.org/195893034/diff/30001/content/child/webcrypto/... File content/child/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/195893034/diff/30001/content/child/webcrypto/... content/child/webcrypto/shared_crypto_unittest.cc:2978: const blink::WebCryptoAlgorithm algorithm = CreateAesCbcKeyGenAlgorithm(256); On 2014/03/19 04:04:00, eroman wrote: > Call this gen_algorithm instead. And then at the callsite where it calls Unwrap, > pass in CreateAlgorithm(AesCBC). Done.
L G T M. However there are still unexpected diffs showing up. I would like to review this with only the changes that are to be committed. https://codereview.chromium.org/195893034/diff/40001/content/child/webcrypto/... File content/child/webcrypto/shared_crypto.cc (left): https://codereview.chromium.org/195893034/diff/40001/content/child/webcrypto/... content/child/webcrypto/shared_crypto.cc:414: if (!KeyUsageAllows(wrapping_key, blink::WebCryptoKeyUsageUnwrapKey)) What is the motivation for removing this line? Is this another rebase issue? https://codereview.chromium.org/195893034/diff/40001/content/child/webcrypto/... File content/child/webcrypto/shared_crypto.cc (right): https://codereview.chromium.org/195893034/diff/40001/content/child/webcrypto/... content/child/webcrypto/shared_crypto.cc:745: return Status::ErrorUnsupported(); // TODO(padolph) Is there anything stopping WrapKeyExportAndEncrypt() from working for the other wrap formats? Seems to me like it is generic enough that it can be allowed (short of not having tests for them yet).
https://codereview.chromium.org/195893034/diff/40001/content/child/webcrypto/... File content/child/webcrypto/shared_crypto.cc (left): https://codereview.chromium.org/195893034/diff/40001/content/child/webcrypto/... content/child/webcrypto/shared_crypto.cc:414: if (!KeyUsageAllows(wrapping_key, blink::WebCryptoKeyUsageUnwrapKey)) On 2014/03/19 18:40:38, eroman wrote: > What is the motivation for removing this line? Is this another rebase issue? It is redundant. UnwrapKey() already does this and this function is only called by UnwrapKey(). This follows the pattern we've been using in these types of refactorings. Should I just make this a DCHECK()? https://codereview.chromium.org/195893034/diff/40001/content/child/webcrypto/... File content/child/webcrypto/shared_crypto.cc (right): https://codereview.chromium.org/195893034/diff/40001/content/child/webcrypto/... content/child/webcrypto/shared_crypto.cc:745: return Status::ErrorUnsupported(); // TODO(padolph) On 2014/03/19 18:40:38, eroman wrote: > Is there anything stopping WrapKeyExportAndEncrypt() from working for the other > wrap formats? Seems to me like it is generic enough that it can be allowed > (short of not having tests for them yet). As per Ryan, yes we can use WrapKeyExportAndEncrypt() for public keys (spki) but not for private keys. In the latter case we need to keep secret keying material below the NSS line, and NSS apparently has APIs to support this. My only reason for not including spki here was the lack of tests for it, and making test data for the wrap/unwrap case is a bit painful. So I wanted to defer it to a later CL.
On 2014/03/19 18:40:37, eroman wrote: > L G T M. However there are still unexpected diffs showing up. I would like to > review this with only the changes that are to be committed. These are showing up because of my git cl patch. I uploaded a new patch set that skips over that. Please take a look.
lgtm https://codereview.chromium.org/195893034/diff/40001/content/child/webcrypto/... File content/child/webcrypto/shared_crypto.cc (left): https://codereview.chromium.org/195893034/diff/40001/content/child/webcrypto/... content/child/webcrypto/shared_crypto.cc:414: if (!KeyUsageAllows(wrapping_key, blink::WebCryptoKeyUsageUnwrapKey)) On 2014/03/19 20:01:17, padolph wrote: > On 2014/03/19 18:40:38, eroman wrote: > > What is the motivation for removing this line? Is this another rebase issue? > > It is redundant. UnwrapKey() already does this and this function is only called > by UnwrapKey(). This follows the pattern we've been using in these types of > refactorings. > > Should I just make this a DCHECK()? Makes sense. No DCHECK() necessary then. https://codereview.chromium.org/195893034/diff/40001/content/child/webcrypto/... File content/child/webcrypto/shared_crypto.cc (right): https://codereview.chromium.org/195893034/diff/40001/content/child/webcrypto/... content/child/webcrypto/shared_crypto.cc:745: return Status::ErrorUnsupported(); // TODO(padolph) On 2014/03/19 20:01:17, padolph wrote: > On 2014/03/19 18:40:38, eroman wrote: > > Is there anything stopping WrapKeyExportAndEncrypt() from working for the > other > > wrap formats? Seems to me like it is generic enough that it can be allowed > > (short of not having tests for them yet). > > As per Ryan, yes we can use WrapKeyExportAndEncrypt() for public keys (spki) but > not for private keys. In the latter case we need to keep secret keying material > below the NSS line, and NSS apparently has APIs to support this. > > My only reason for not including spki here was the lack of tests for it, and > making test data for the wrap/unwrap case is a bit painful. So I wanted to defer > it to a later CL. Acknowledged. Fine as is then.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/195893034/80001
Message was sent while issue was closed.
Change committed as 258223 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
