[webcrypto] Remove support for AES 192-bit keys (2 of 2)

[webcrypto] Remove support for AES 192-bit keys (2 of 2)

BUG=381829, 245025
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276979

[Ryan Sleevi, 2014-06-10 23:44:21]

Not LGTM.

Applies to *all* AES

https://codereview.chromium.org/328903003/diff/1/content/child/webcrypto/plat...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/328903003/diff/1/content/child/webcrypto/plat...
content/child/webcrypto/platform_crypto_nss.cc:1860: algorithm.id() ==
blink::WebCryptoAlgorithmIdAesGcm) {
Adam's comments were about AES as a whole.

if (IsAesAlgorithm(algorithm.id()) && !IsValidAesKeyLengthBytes(...)), reject

However, that you need to update this file makes me a bit nervous that we're not
being consistent in handling - that is, this isn't being treated as an Import
operation, we're instead going to create it directly, right? If it was import,
we would only need the single path to update.

https://codereview.chromium.org/328903003/diff/1/content/child/webcrypto/shar...
File content/child/webcrypto/shared_crypto.cc (right):

https://codereview.chromium.org/328903003/diff/1/content/child/webcrypto/shar...
content/child/webcrypto/shared_crypto.cc:224: return
Status::ErrorAesGcm192Unsupported();
Same comments re All AES

https://codereview.chromium.org/328903003/diff/1/content/child/webcrypto/shar...
content/child/webcrypto/shared_crypto.cc:673: }
Same comments re All AES

https://codereview.chromium.org/328903003/diff/1/content/child/webcrypto/shar...
File content/child/webcrypto/shared_crypto_unittest.cc (right):

https://codereview.chromium.org/328903003/diff/1/content/child/webcrypto/shar...
content/child/webcrypto/shared_crypto_unittest.cc:1086: if (SupportsAesGcm() &&
kKeyLength[i] != 192)
ditto re: all AES

[eroman, 2014-06-11 01:15:26]

https://codereview.chromium.org/328903003/diff/1/content/child/webcrypto/plat...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/328903003/diff/1/content/child/webcrypto/plat...
content/child/webcrypto/platform_crypto_nss.cc:1860: algorithm.id() ==
blink::WebCryptoAlgorithmIdAesGcm) {
On 2014/06/10 23:44:21, Ryan Sleevi wrote:
> Adam's comments were about AES as a whole.
> 
> if (IsAesAlgorithm(algorithm.id()) && !IsValidAesKeyLengthBytes(...)), reject
> 
> However, that you need to update this file makes me a bit nervous that we're
not
> being consistent in handling - that is, this isn't being treated as an Import
> operation, we're instead going to create it directly, right? If it was import,
> we would only need the single path to update.

I see, I'll rework this change to apply to all of AES then.

The reason there is a change in platform_crypto_nss.cc is because of how AES-KW
unwrapping is implemented -- it doesn't go through the import codepath.

[eroman, 2014-06-12 01:01:30]

PTAL.

https://codereview.chromium.org/328903003/diff/1/content/child/webcrypto/plat...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/328903003/diff/1/content/child/webcrypto/plat...
content/child/webcrypto/platform_crypto_nss.cc:1860: algorithm.id() ==
blink::WebCryptoAlgorithmIdAesGcm) {
On 2014/06/11 01:15:26, eroman wrote:
> On 2014/06/10 23:44:21, Ryan Sleevi wrote:
> > Adam's comments were about AES as a whole.
> > 
> > if (IsAesAlgorithm(algorithm.id()) && !IsValidAesKeyLengthBytes(...)),
reject
> > 
> > However, that you need to update this file makes me a bit nervous that we're
> not
> > being consistent in handling - that is, this isn't being treated as an
Import
> > operation, we're instead going to create it directly, right? If it was
import,
> > we would only need the single path to update.
> 
> I see, I'll rework this change to apply to all of AES then.
> 
> The reason there is a change in platform_crypto_nss.cc is because of how
AES-KW
> unwrapping is implemented -- it doesn't go through the import codepath.

I sent you another CL which remove the special case for AES-KW, and have based
this on top of that (so this check is gone).

[Ryan Sleevi, 2014-06-12 19:07:29]

lgtm

[eroman, 2014-06-13 02:49:06]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-06-13 02:50:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eroman@chromium.org/328903003/40001

[commit-bot: I haz the power, 2014-06-13 08:49:18]

Change committed as 276979