Bug 124059 - Implement JWK key import for HMAC and AES-CBC
Summary: Implement JWK key import for HMAC and AES-CBC
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
Depends on:
Blocks: 122679
  Show dependency treegraph
Reported: 2013-11-08 11:42 PST by Alexey Proskuryakov
Modified: 2013-11-08 14:03 PST (History)
2 users (show)

See Also:

proposed patch (82.62 KB, patch)
2013-11-08 12:07 PST, Alexey Proskuryakov
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2013-11-08 11:42:04 PST
Starting with a simple case - JWK or another complex format will be needed to import RSA and EC keys.
Comment 1 Alexey Proskuryakov 2013-11-08 11:49:41 PST
Note that this is governed by multiple unfinished specs, those being WebCrypto, JWK/JWA, and Netflix key wrap proposal. Those don't agree about things yet.

In particular,
- Netflix proposes a JWK "use":"wrap" value, which is explicitly unnecessary per JWK,
- Netflix proposes an "extractable" JWK key, which is not in any formally published spec yet,
- we need algorithms that are not specified anywhere yet,
- WebCrypto and JWA appear to have vague but different requirements on key length for each algorithm (they are sometimes permissive).

It will get worse once we get to key unwrapping.
Comment 2 Alexey Proskuryakov 2013-11-08 12:07:24 PST
Created attachment 216421 [details]
proposed patch
Comment 3 WebKit Commit Bot 2013-11-08 12:10:04 PST
Attachment 216421 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/crypto/subtle/aes-cbc-import-jwk-expected.txt', u'LayoutTests/crypto/subtle/aes-cbc-import-jwk.html', u'LayoutTests/crypto/subtle/hmac-import-jwk-expected.txt', u'LayoutTests/crypto/subtle/hmac-import-jwk.html', u'LayoutTests/crypto/subtle/import-jwk-expected.txt', u'LayoutTests/crypto/subtle/import-jwk.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/JSCryptoKeySerializationJWK.cpp', u'Source/WebCore/bindings/js/JSCryptoKeySerializationJWK.h', u'Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp', u'Source/WebCore/crypto/CryptoAlgorithm.cpp', u'Source/WebCore/crypto/CryptoAlgorithm.h', u'Source/WebCore/crypto/CryptoKeyData.h', u'Source/WebCore/crypto/CryptoKeyFormat.h', u'Source/WebCore/crypto/CryptoKeySerialization.h', u'Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp', u'Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.h', u'Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp', u'Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.h', u'Source/WebCore/crypto/keys/CryptoKeyDataOctetSequence.cpp', u'Source/WebCore/crypto/keys/CryptoKeyDataOctetSequence.h', u'Source/WebCore/crypto/keys/CryptoKeySerializationRaw.cpp', u'Source/WebCore/crypto/keys/CryptoKeySerializationRaw.h']" exit_code: 1
Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:72:  CryptoAlgorithmAES_CBC::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/js/JSCryptoKeySerializationJWK.cpp:195:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:53:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:56:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:59:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:435:  Missing space before ( in switch(  [whitespace/parens] [5]
Total errors found: 6 in 24 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Alexey Proskuryakov 2013-11-08 12:59:23 PST
Committed <http://trac.webkit.org/changeset/158943>.
Comment 5 Darin Adler 2013-11-08 14:03:45 PST
Comment on attachment 216421 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216421&action=review

> Source/WebCore/bindings/js/JSCryptoKeySerializationJWK.h:46
> +WTF_MAKE_NONCOPYABLE(JSCryptoKeySerializationJWK);

We normally indent these.

> Source/WebCore/bindings/js/JSCryptoKeySerializationJWK.h:60
> +    virtual bool reconcileAlgorithm(std::unique_ptr<CryptoAlgorithm>&, std::unique_ptr<CryptoAlgorithmParameters>&) const OVERRIDE;
> +
> +    virtual void reconcileUsages(CryptoKeyUsage&) const OVERRIDE;
> +    virtual void reconcileExtractable(bool&) const OVERRIDE;
> +
> +    virtual std::unique_ptr<CryptoKeyData> keyData() const OVERRIDE;

Can we make these private?

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:435
>> +    switch(keyFormat) {
> Missing space before ( in switch(  [whitespace/parens] [5]

Yes, not our usual format.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp:84
> +    RefPtr<CryptoKeyHMAC> result = CryptoKeyHMAC::create(keyDataOctetSequence.octetSequence(), hmacParameters.hash, extractable, usage);
> +    promise->fulfill(result.release());

Would read better without the local variable, I think.