Bug 123598 - Add a Mac WebCrypto implementation of HMAC importKey/sign/verify
Summary: Add a Mac WebCrypto implementation of HMAC importKey/sign/verify
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks: 122679
  Show dependency treegraph
 
Reported: 2013-10-31 17:05 PDT by Alexey Proskuryakov
Modified: 2013-11-01 10:27 PDT (History)
8 users (show)

See Also:


Attachments
proposed patch (43.20 KB, patch)
2013-10-31 17:09 PDT, Alexey Proskuryakov
andersca: review+
Details | Formatted Diff | Diff
patch for landing (43.04 KB, patch)
2013-10-31 23:43 PDT, Alexey Proskuryakov
no flags 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-10-31 17:05:19 PDT
Add a Mac WebCrypto implementation of HMAC importKey/sign/verify.
Comment 1 Alexey Proskuryakov 2013-10-31 17:09:16 PDT
Created attachment 215694 [details]
proposed patch
Comment 2 WebKit Commit Bot 2013-10-31 17:10:13 PDT
Attachment 215694 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/crypto/subtle/hmac-sign-verify-expected.txt', u'LayoutTests/crypto/subtle/hmac-sign-verify.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp', u'Source/WebCore/crypto/SubtleCrypto.idl', u'Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp', u'Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.h', u'Source/WebCore/crypto/keys/CryptoKeyHMAC.cpp', u'Source/WebCore/crypto/keys/CryptoKeyHMAC.h', u'Source/WebCore/crypto/mac/CryptoAlgorithmHMACMac.cpp', u'Source/WebCore/crypto/mac/CryptoAlgorithmRegistryMac.cpp']" exit_code: 1
Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.h:45:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/crypto/keys/CryptoKeyHMAC.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Anders Carlsson 2013-10-31 17:21:36 PDT
Comment on attachment 215694 [details]
proposed patch

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

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:235
> +    std::unique_ptr<PromiseWrapper> promiseWrapper = PromiseWrapper::create(globalObject(), promise);

I’d use auto for the variable declaration here.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:238
> +    algorithm->verify(*parameters, *key.get(), signature, data, std::move(promiseWrapper), ec);

Can’t you just do *key here?

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:337
> +    std::unique_ptr<PromiseWrapper> promiseWrapper = PromiseWrapper::create(globalObject(), promise);

Auto?

> Source/WebCore/crypto/keys/CryptoKeyHMAC.cpp:52
> +    std::unique_ptr<CryptoAlgorithmDescriptionBuilder> hashDescriptionBuilder = builder.createEmptyClone();

Auto?
Comment 4 Alexey Proskuryakov 2013-10-31 23:43:44 PDT
Created attachment 215720 [details]
patch for landing
Comment 5 WebKit Commit Bot 2013-11-01 00:14:07 PDT
Comment on attachment 215720 [details]
patch for landing

Clearing flags on attachment: 215720

Committed r158427: <http://trac.webkit.org/changeset/158427>
Comment 6 WebKit Commit Bot 2013-11-01 00:14:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Darin Adler 2013-11-01 09:57:20 PDT
Comment on attachment 215720 [details]
patch for landing

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

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:144
> +        if (usageString == "encrypt")
> +            result |= CryptoKeyUsageEncrypt;
> +        else if (usageString == "decrypt")
> +            result |= CryptoKeyUsageDecrypt;
> +        else if (usageString == "sign")
> +            result |= CryptoKeyUsageSign;
> +        else if (usageString == "verify")
> +            result |= CryptoKeyUsageVerify;
> +        else if (usageString == "deriveKey")
> +            result |= CryptoKeyUsageDeriveKey;
> +        else if (usageString == "deriveBits")
> +            result |= CryptoKeyUsageDeriveBits;
> +        else if (usageString == "wrapKey")
> +            result |= CryptoKeyUsageWrapKey;
> +        else if (usageString == "unwrapKey")
> +            result |= CryptoKeyUsageUnwrapKey;

Are repeated strings allowed? Are unknown strings supposed to be silently ignored?
Comment 8 Alexey Proskuryakov 2013-11-01 10:27:59 PDT
> Are repeated strings allowed? 

WebCrypto would have to specify that, which it doesn't (neither does it specify pretty much anything about "usages" yet).

> Are unknown strings supposed to be silently ignored?

Good point, WebIDL says that a typeError should be raised when a JS value is not one of enum values. I'm not sure if that makes sense for WebCrypto, because unknown values have no effect on the behavior. If we don't implement a WebCrypto operation, it doesn't matter if it's allowed. The only way it's observable is via inspecting key.usages attribute, which is probably not useful outside of debugging. It can also become observable for keys that are stored permanently, if we add support for new operations.

Our generated bindings code currently doesn't raise exceptions when setting an attribute to an unknown enum value. It silently ignores setting an attribute to an unknown enum value. Ugh.