Add a Mac WebCrypto implementation of HMAC importKey/sign/verify

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Add a Mac WebCrypto implementation of HMAC importKey/sign/verify.

[Alexey Proskuryakov]

Created attachment 215694 [details]
proposed patch

[WebKit Commit Bot]

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.

[Anders Carlsson]

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?

[Alexey Proskuryakov]

Created attachment 215720 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 215720 [details]
patch for landing

Clearing flags on attachment: 215720

Committed r158427: <http://trac.webkit.org/changeset/158427>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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?

[Alexey Proskuryakov]

> 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.