WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
128748
WebCrypto algorithms should be exposed via KeyAlgorithm interface
https://bugs.webkit.org/show_bug.cgi?id=128748
Summary
WebCrypto algorithms should be exposed via KeyAlgorithm interface
Alexey Proskuryakov
Reported
2014-02-13 10:20:37 PST
WebCrypto spec used to have Key.algorithm as dictionary, which is invalid per WebIDL. Now it's a KeyAlgorithm interface, and there is going to be a better specification of what's exposed for each type of key.
Attachments
Patch
(34.87 KB, patch)
2016-09-06 15:46 PDT
,
Jiewen Tan
no flags
Details
Formatted Diff
Diff
Patch
(36.58 KB, patch)
2016-09-08 10:29 PDT
,
Jiewen Tan
no flags
Details
Formatted Diff
Diff
Patch for landing
(36.62 KB, patch)
2016-09-12 15:03 PDT
,
Jiewen Tan
no flags
Details
Formatted Diff
Diff
Patch for landing
(37.74 KB, patch)
2016-09-12 19:09 PDT
,
Jiewen Tan
no flags
Details
Formatted Diff
Diff
Patch for landing
(37.76 KB, patch)
2016-09-12 19:14 PDT
,
Jiewen Tan
no flags
Details
Formatted Diff
Diff
Patch
(39.53 KB, patch)
2016-09-13 16:56 PDT
,
Jiewen Tan
no flags
Details
Formatted Diff
Diff
Patch
(43.76 KB, patch)
2016-09-13 23:37 PDT
,
Jiewen Tan
no flags
Details
Formatted Diff
Diff
Patch
(43.76 KB, patch)
2016-09-14 00:36 PDT
,
Jiewen Tan
no flags
Details
Formatted Diff
Diff
Patch
(43.81 KB, patch)
2016-09-14 11:50 PDT
,
Jiewen Tan
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1
2014-02-13 10:31:52 PST
See <
https://dvcs.w3.org/hg/webcrypto-api/rev/2fa3494f0179
> and <
https://dvcs.w3.org/hg/webcrypto-api/rev/2fa3494f0179
>.
Radar WebKit Bug Importer
Comment 2
2016-07-14 15:20:44 PDT
<
rdar://problem/27359438
>
Jiewen Tan
Comment 3
2016-09-06 15:46:05 PDT
Created
attachment 288055
[details]
Patch
WebKit Commit Bot
Comment 4
2016-09-06 15:47:45 PDT
Attachment 288055
[details]
did not pass style-queue: ERROR: Source/WebCore/crypto/CryptoKey.h:47: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:52: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:53: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:54: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:55: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 5 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Jiewen Tan
Comment 5
2016-09-08 10:29:10 PDT
Created
attachment 288274
[details]
Patch
WebKit Commit Bot
Comment 6
2016-09-08 10:31:05 PDT
Attachment 288274
[details]
did not pass style-queue: ERROR: Source/WebCore/crypto/CryptoKey.h:47: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:52: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:53: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:54: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:55: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 5 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Brent Fulgham
Comment 7
2016-09-08 18:36:37 PDT
Comment on
attachment 288274
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=288274&action=review
Very nice! r=me. Please correct the typos I mentioned before landing.
> Source/WebCore/ChangeLog:12 > + No change of behaviors.
No change in behavior, so no new tests.
> Source/WebCore/ChangeLog:17 > + Remove CryptoAlgorithmDescriptionBuilder.cpp.
Do you know why this class existed? Seems like it might have been intended to be fleshed out later, but was never completed.
> Source/WebCore/ChangeLog:25 > + Get ride of dependency on CryptoAlgorithmDescriptionBuilder.
Get rid of dependency ...
Jiewen Tan
Comment 8
2016-09-12 14:54:13 PDT
Comment on
attachment 288274
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=288274&action=review
>> Source/WebCore/ChangeLog:17 >> + Remove CryptoAlgorithmDescriptionBuilder.cpp. > > Do you know why this class existed? Seems like it might have been intended to be fleshed out later, but was never completed.
Previously, this is the base (virtual) class of JSCryptoAlgorithmBuilder, which is created to add information to the JSObject that describes algorithms binding to the key. I am not quite sure why it is needed, but I assume that Alexey did it before is try to separate codes relating to JS from non-JS ones. So, with this class, the callers, i.e. non-JS codes will not need the knowledge of underlying implementation, i.e. JS codes to add information to describe corresponding algorithms. However, in the new implementation, I will only return the KeyAlgorithm object to JS codes and the JS codes itself convert the object to a JSObject. Therefore, this base class is not needed. The actual JSCryptoAlgorithmBuilder is therefore kept to do the conversion.
Jiewen Tan
Comment 9
2016-09-12 14:59:50 PDT
Comment on
attachment 288274
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=288274&action=review
Thanks Brent for reviewing my patch.
>> Source/WebCore/ChangeLog:12 >> + No change of behaviors. > > No change in behavior, so no new tests.
Fixed.
>> Source/WebCore/ChangeLog:25 >> + Get ride of dependency on CryptoAlgorithmDescriptionBuilder. > > Get rid of dependency ...
Fixed.
Jiewen Tan
Comment 10
2016-09-12 15:03:13 PDT
Created
attachment 288613
[details]
Patch for landing
WebKit Commit Bot
Comment 11
2016-09-12 15:04:58 PDT
Attachment 288613
[details]
did not pass style-queue: ERROR: Source/WebCore/crypto/CryptoKey.h:47: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:52: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:53: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:54: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:55: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 5 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Jiewen Tan
Comment 12
2016-09-12 19:09:28 PDT
Created
attachment 288655
[details]
Patch for landing
WebKit Commit Bot
Comment 13
2016-09-12 19:12:24 PDT
Attachment 288655
[details]
did not pass style-queue: ERROR: Source/WebCore/crypto/CryptoKey.h:47: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:52: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:53: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:54: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:55: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 5 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Chris Dumez
Comment 14
2016-09-12 19:13:50 PDT
Comment on
attachment 288655
[details]
Patch for landing View in context:
https://bugs.webkit.org/attachment.cgi?id=288655&action=review
> Source/WebCore/ChangeLog:10 > + defined by the spec.
Please link to the spec that define this interface in the Changelog.
Jiewen Tan
Comment 15
2016-09-12 19:14:47 PDT
Created
attachment 288657
[details]
Patch for landing
WebKit Commit Bot
Comment 16
2016-09-12 19:18:11 PDT
Attachment 288657
[details]
did not pass style-queue: ERROR: Source/WebCore/crypto/CryptoKey.h:47: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:52: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:53: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:54: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:55: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 5 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Chris Dumez
Comment 17
2016-09-12 19:26:24 PDT
Comment on
attachment 288657
[details]
Patch for landing View in context:
https://bugs.webkit.org/attachment.cgi?id=288657&action=review
> Source/WebCore/ChangeLog:3 > + WebCrypto algorithms should be exposed via KeyAlgorithm interface
KeyAlgorithm is a dictionary, not an interface
> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:48 > + AesKeyAlgorithm& aesAlgorithm = downcast<AesKeyAlgorithm>(algorithm.get());
auto&
> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:54 > + HmacKeyAlgorithm& hmacAlgorithm = downcast<HmacKeyAlgorithm>(algorithm.get());
auto&
> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:63 > + RsaHashedKeyAlgorithm& rsaAlgorithm = downcast<RsaHashedKeyAlgorithm>(algorithm.get());
auto&
> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:73 > + RsaKeyAlgorithm& rsaAlgorithm = downcast<RsaKeyAlgorithm>(algorithm.get());
auto&
> Source/WebCore/crypto/CryptoKey.h:64 > + String name;
Why is this public?
> Source/WebCore/crypto/CryptoKey.h:66 > + KeyAlgorithm(const String& n)
RefCounted classes normally do not have a public constructor but a factory function that returns a Ref<> since we don't want those allocated on the stack.
> Source/WebCore/crypto/keys/CryptoKeyAES.h:39 > + size_t length;
Why is this public?
> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:38 > + KeyHashAlgorithm hash;
Why are those public?
> Source/WebCore/crypto/keys/CryptoKeyRSA.h:52 > + size_t modulusLength;
Why are those public?
Chris Dumez
Comment 18
2016-09-12 19:30:02 PDT
Comment on
attachment 288657
[details]
Patch for landing View in context:
https://bugs.webkit.org/attachment.cgi?id=288657&action=review
> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:81 > return builder.result();
The specification seems to say that we should return a *cached* JS Object. However, it seems you are returning a new object every time (or I am missing the caching?).
Chris Dumez
Comment 19
2016-09-12 19:35:15 PDT
Comment on
attachment 288657
[details]
Patch for landing View in context:
https://bugs.webkit.org/attachment.cgi?id=288657&action=review
> Source/WebCore/crypto/CryptoKey.h:87 > + virtual Ref<KeyAlgorithm> algorithm() const = 0;
Given that this constructs a new object and transfers ownership, I think we need to choose a better name for this method, maybe buildAlgorithm() or createAlgorithm().
> Source/WebCore/crypto/CryptoKey.h:104 > + CryptoAlgorithmIdentifier m_algorithmIdentifier;
I think subclasses should use the algorithmIdentifier() getter instead of making this member protected.
Chris Dumez
Comment 20
2016-09-12 19:50:46 PDT
Comment on
attachment 288657
[details]
Patch for landing View in context:
https://bugs.webkit.org/attachment.cgi?id=288657&action=review
> Source/WebCore/crypto/CryptoKey.h:55 > + HRSA
Where is HRSA coming from? I do not see it in the spec. And this change is not mentioned in the Changelog unless I missed it.
> Source/WebCore/crypto/CryptoKey.h:58 > +struct KeyHashAlgorithm {
Do we really need a struct for this? Why don't we use a String instead?
>> Source/WebCore/crypto/CryptoKey.h:66 >> + KeyAlgorithm(const String& n) > > RefCounted classes normally do not have a public constructor but a factory function that returns a Ref<> since we don't want those allocated on the stack.
This constructor should be protected and marked as explicit. This base class does not need a factory function since it is pure virtual, only subclasses do. The constructors in the subclasses should be protected or private, depending if the subclass is final or not.
> Source/WebCore/crypto/gnutls/CryptoKeyRSAGnuTLS.cpp:86 > + Vector<uint8_t> publicExponent;
Not needed.
> Source/WebCore/crypto/gnutls/CryptoKeyRSAGnuTLS.cpp:87 > + return adoptRef(*new RsaKeyAlgorithm(emptyString(), 0, WTFMove(publicExponent)));
return adoptRef(*new RsaKeyAlgorithm(emptyString(), 0, {}));
> Source/WebCore/crypto/keys/CryptoKeyAES.h:37 > +class AesKeyAlgorithm : public KeyAlgorithm {
Should be final
> Source/WebCore/crypto/keys/CryptoKeyAES.h:41 > + AesKeyAlgorithm(const String& n, size_t l)
Bad parameter names. Should be private and there should be a public factory function instead.
> Source/WebCore/crypto/keys/CryptoKeyAES.h:46 > + virtual ~AesKeyAlgorithm()
I don't need this destructor it seems.
> Source/WebCore/crypto/keys/CryptoKeyAES.h:50 > + KeyAlgorithmClass keyAlgorithmClass() const override { return KeyAlgorithmClass::AES; }
override -> final
> Source/WebCore/crypto/keys/CryptoKeyAES.h:72 > + Ref<KeyAlgorithm> algorithm() const override;
override -> final
> Source/WebCore/crypto/keys/CryptoKeyHMAC.cpp:71 > + KeyHashAlgorithm hashAlgorithm;
KeyHashAlgorithm hashAlgorithm { CryptoAlgorithmRegistry::singleton().nameForIdentifier(m_hash) };
> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:36 > +class HmacKeyAlgorithm : public KeyAlgorithm {
Should be final
> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:41 > + HmacKeyAlgorithm(const String& n, KeyHashAlgorithm&& h, size_t l)
Bad parameter names. Should be private.
> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:47 > + virtual ~HmacKeyAlgorithm()
I don't need this destructor it seems.
> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:51 > + KeyAlgorithmClass keyAlgorithmClass() const override { return KeyAlgorithmClass::HMAC; }
override -> final
> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:74 > + Ref<KeyAlgorithm> algorithm() const override;
override -> final
> Source/WebCore/crypto/keys/CryptoKeyRSA.h:55 > + RsaKeyAlgorithm(const String& n, size_t m, Vector<uint8_t>&& p)
Bad parameter names. Should be protected.
> Source/WebCore/crypto/keys/CryptoKeyRSA.h:61 > + virtual ~RsaKeyAlgorithm()
I don't need this destructor it seems.
> Source/WebCore/crypto/keys/CryptoKeyRSA.h:68 > +class RsaHashedKeyAlgorithm : public RsaKeyAlgorithm {
Should be final.
> Source/WebCore/crypto/keys/CryptoKeyRSA.h:70 > + KeyHashAlgorithm hash;
Why is this public?
> Source/WebCore/crypto/keys/CryptoKeyRSA.h:72 > + RsaHashedKeyAlgorithm(const String& n, size_t m, Vector<uint8_t>&& p, KeyHashAlgorithm&& h)
Those parameter names are really not great. We try to avoid abbreviations in WebKit. Should be private.
> Source/WebCore/crypto/keys/CryptoKeyRSA.h:77 > + virtual ~RsaHashedKeyAlgorithm()
I don't need this destructor it seems.
> Source/WebCore/crypto/keys/CryptoKeyRSA.h:82 > + KeyAlgorithmClass keyAlgorithmClass() const override { return KeyAlgorithmClass::HRSA; }
override -> final
> Source/WebCore/crypto/keys/CryptoKeyRSA.h:109 > + Ref<KeyAlgorithm> algorithm() const override;
override -> final
Jiewen Tan
Comment 21
2016-09-12 22:33:38 PDT
Comment on
attachment 288657
[details]
Patch for landing View in context:
https://bugs.webkit.org/attachment.cgi?id=288657&action=review
>> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:81 >> return builder.result(); > > The specification seems to say that we should return a *cached* JS Object. However, it seems you are returning a new object every time (or I am missing the caching?).
Thank you for pointing out. I forgot to implement it. Oops.
>> Source/WebCore/crypto/CryptoKey.h:55 >> + HRSA > > Where is HRSA coming from? I do not see it in the spec. And this change is not mentioned in the Changelog unless I missed it.
There are two types of RSAs, one with hash, another without. So I name the one with hash as HRSA.
>> Source/WebCore/crypto/CryptoKey.h:58 >> +struct KeyHashAlgorithm { > > Do we really need a struct for this? Why don't we use a String instead?
I am struggling with this part. The hash is supposed to be of type KeyAlgorithm as well. However, according to the current spec, only SHA1, 256, 384, 512 could serve the purpose of digest, see
https://www.w3.org/TR/WebCryptoAPI/#algorithm-overview
. Therefore, according to
https://www.w3.org/TR/WebCryptoAPI/#sha
and
https://www.w3.org/TR/WebCryptoAPI/#algorithm-normalization-normalize-an-algorithm
, the KeyAlgorithm type will only have the name attribute which is the below KeyAlgorithm virtual class. But I can't initialize a virtual class. The reason why the below KeyAlgorithm class is virtual is because I want the type-check downcast trait. Hence, I make a struct here. However, I think a String will serve the purpose as well.
>> Source/WebCore/crypto/CryptoKey.h:64 >> + String name; > > Why is this public?
I want a Struct initially, but later on realize that I would like to return a reference and use polymorphism as well. I can't think of any other approach to make it possible except inheriting RefCounted. Hence, those members are no needed to be private or protected. Do you have any better idea to achieve the same purpose with Struct only?
Chris Dumez
Comment 22
2016-09-13 07:44:59 PDT
(In reply to
comment #21
)
> Comment on
attachment 288657
[details]
> Patch for landing > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=288657&action=review
> > >> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:81 > >> return builder.result(); > > > > The specification seems to say that we should return a *cached* JS Object. However, it seems you are returning a new object every time (or I am missing the caching?). > > Thank you for pointing out. I forgot to implement it. Oops. > > >> Source/WebCore/crypto/CryptoKey.h:55 > >> + HRSA > > > > Where is HRSA coming from? I do not see it in the spec. And this change is not mentioned in the Changelog unless I missed it. > > There are two types of RSAs, one with hash, another without. So I name the > one with hash as HRSA. > > >> Source/WebCore/crypto/CryptoKey.h:58 > >> +struct KeyHashAlgorithm { > > > > Do we really need a struct for this? Why don't we use a String instead? > > I am struggling with this part. The hash is supposed to be of type > KeyAlgorithm as well. However, according to the current spec, only SHA1, > 256, 384, 512 could serve the purpose of digest, see >
https://www.w3.org/TR/WebCryptoAPI/#algorithm-overview
. Therefore, according > to
https://www.w3.org/TR/WebCryptoAPI/#sha
and >
https://www.w3.org/TR/WebCryptoAPI/#algorithm-normalization-normalize-an
- > algorithm, the KeyAlgorithm type will only have the name attribute which is > the below KeyAlgorithm virtual class. But I can't initialize a virtual > class. The reason why the below KeyAlgorithm class is virtual is because I > want the type-check downcast trait. Hence, I make a struct here. However, I > think a String will serve the purpose as well. > > >> Source/WebCore/crypto/CryptoKey.h:64 > >> + String name; > > > > Why is this public? > > I want a Struct initially, but later on realize that I would like to return > a reference and use polymorphism as well. I can't think of any other > approach to make it possible except inheriting RefCounted. Hence, those > members are no needed to be private or protected. Do you have any better > idea to achieve the same purpose with Struct only?
Given how this is currently implemented, I still do not think they should be public.
Chris Dumez
Comment 23
2016-09-13 07:47:31 PDT
(In reply to
comment #22
)
> (In reply to
comment #21
) > > Comment on
attachment 288657
[details]
> > Patch for landing > > > > View in context: > >
https://bugs.webkit.org/attachment.cgi?id=288657&action=review
> > > > >> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:81 > > >> return builder.result(); > > > > > > The specification seems to say that we should return a *cached* JS Object. However, it seems you are returning a new object every time (or I am missing the caching?). > > > > Thank you for pointing out. I forgot to implement it. Oops. > > > > >> Source/WebCore/crypto/CryptoKey.h:55 > > >> + HRSA > > > > > > Where is HRSA coming from? I do not see it in the spec. And this change is not mentioned in the Changelog unless I missed it. > > > > There are two types of RSAs, one with hash, another without. So I name the > > one with hash as HRSA. > > > > >> Source/WebCore/crypto/CryptoKey.h:58 > > >> +struct KeyHashAlgorithm { > > > > > > Do we really need a struct for this? Why don't we use a String instead? > > > > I am struggling with this part. The hash is supposed to be of type > > KeyAlgorithm as well. However, according to the current spec, only SHA1, > > 256, 384, 512 could serve the purpose of digest, see > >
https://www.w3.org/TR/WebCryptoAPI/#algorithm-overview
. Therefore, according > > to
https://www.w3.org/TR/WebCryptoAPI/#sha
and > >
https://www.w3.org/TR/WebCryptoAPI/#algorithm-normalization-normalize-an
- > > algorithm, the KeyAlgorithm type will only have the name attribute which is > > the below KeyAlgorithm virtual class. But I can't initialize a virtual > > class. The reason why the below KeyAlgorithm class is virtual is because I > > want the type-check downcast trait. Hence, I make a struct here. However, I > > think a String will serve the purpose as well. > > > > >> Source/WebCore/crypto/CryptoKey.h:64 > > >> + String name; > > > > > > Why is this public? > > > > I want a Struct initially, but later on realize that I would like to return > > a reference and use polymorphism as well. I can't think of any other > > approach to make it possible except inheriting RefCounted. Hence, those > > members are no needed to be private or protected. Do you have any better > > idea to achieve the same purpose with Struct only? > > Given how this is currently implemented, I still do not think they should be > public.
I have not looked in too much details but if you do not need ref counting, the factory could return a unique_ptr instead of a Ref, and you would not need to subclass RefCounted.
Jiewen Tan
Comment 24
2016-09-13 16:25:45 PDT
Comment on
attachment 288657
[details]
Patch for landing View in context:
https://bugs.webkit.org/attachment.cgi?id=288657&action=review
Thank you for reviewing my patch, Chris.
>> Source/WebCore/ChangeLog:3 >> + WebCrypto algorithms should be exposed via KeyAlgorithm interface > > KeyAlgorithm is a dictionary, not an interface
Fixed.
>> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:48 >> + AesKeyAlgorithm& aesAlgorithm = downcast<AesKeyAlgorithm>(algorithm.get()); > > auto&
Fixed.
>> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:54 >> + HmacKeyAlgorithm& hmacAlgorithm = downcast<HmacKeyAlgorithm>(algorithm.get()); > > auto&
Fixed.
>> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:63 >> + RsaHashedKeyAlgorithm& rsaAlgorithm = downcast<RsaHashedKeyAlgorithm>(algorithm.get()); > > auto&
Fixed.
>> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:73 >> + RsaKeyAlgorithm& rsaAlgorithm = downcast<RsaKeyAlgorithm>(algorithm.get()); > > auto&
Fixed.
>>>>> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:81 >>>>> return builder.result(); >>>> >>>> The specification seems to say that we should return a *cached* JS Object. However, it seems you are returning a new object every time (or I am missing the caching?). >>> >>> Thank you for pointing out. I forgot to implement it. Oops. >> >> Given how this is currently implemented, I still do not think they should be public. > > I have not looked in too much details but if you do not need ref counting, the factory could return a unique_ptr instead of a Ref, and you would not need to subclass RefCounted.
Fixed.
>>> Source/WebCore/crypto/CryptoKey.h:66 >>> + KeyAlgorithm(const String& n) >> >> RefCounted classes normally do not have a public constructor but a factory function that returns a Ref<> since we don't want those allocated on the stack. > > This constructor should be protected and marked as explicit. This base class does not need a factory function since it is pure virtual, only subclasses do. The constructors in the subclasses should be protected or private, depending if the subclass is final or not.
Now, I will be using a unique_ptr, in this case, probably there is no need to use a factory function there?
>> Source/WebCore/crypto/CryptoKey.h:87 >> + virtual Ref<KeyAlgorithm> algorithm() const = 0; > > Given that this constructs a new object and transfers ownership, I think we need to choose a better name for this method, maybe buildAlgorithm() or createAlgorithm().
Changed it to buildAlgorithm().
>> Source/WebCore/crypto/CryptoKey.h:104 >> + CryptoAlgorithmIdentifier m_algorithmIdentifier; > > I think subclasses should use the algorithmIdentifier() getter instead of making this member protected.
Fixed. I am kind of thinking both of them are okay.
>> Source/WebCore/crypto/gnutls/CryptoKeyRSAGnuTLS.cpp:86 >> + Vector<uint8_t> publicExponent; > > Not needed.
Fixed.
>> Source/WebCore/crypto/gnutls/CryptoKeyRSAGnuTLS.cpp:87 >> + return adoptRef(*new RsaKeyAlgorithm(emptyString(), 0, WTFMove(publicExponent))); > > return adoptRef(*new RsaKeyAlgorithm(emptyString(), 0, {}));
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyAES.h:37 >> +class AesKeyAlgorithm : public KeyAlgorithm { > > Should be final
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyAES.h:39 >> + size_t length; > > Why is this public?
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyAES.h:41 >> + AesKeyAlgorithm(const String& n, size_t l) > > Bad parameter names. Should be private and there should be a public factory function instead.
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyAES.h:46 >> + virtual ~AesKeyAlgorithm() > > I don't need this destructor it seems.
Removed.
>> Source/WebCore/crypto/keys/CryptoKeyAES.h:50 >> + KeyAlgorithmClass keyAlgorithmClass() const override { return KeyAlgorithmClass::AES; } > > override -> final
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyAES.h:72 >> + Ref<KeyAlgorithm> algorithm() const override; > > override -> final
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:36 >> +class HmacKeyAlgorithm : public KeyAlgorithm { > > Should be final
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:38 >> + KeyHashAlgorithm hash; > > Why are those public?
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:41 >> + HmacKeyAlgorithm(const String& n, KeyHashAlgorithm&& h, size_t l) > > Bad parameter names. Should be private.
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:47 >> + virtual ~HmacKeyAlgorithm() > > I don't need this destructor it seems.
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:51 >> + KeyAlgorithmClass keyAlgorithmClass() const override { return KeyAlgorithmClass::HMAC; } > > override -> final
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:74 >> + Ref<KeyAlgorithm> algorithm() const override; > > override -> final
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:52 >> + size_t modulusLength; > > Why are those public?
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:55 >> + RsaKeyAlgorithm(const String& n, size_t m, Vector<uint8_t>&& p) > > Bad parameter names. Should be protected.
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:61 >> + virtual ~RsaKeyAlgorithm() > > I don't need this destructor it seems.
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:68 >> +class RsaHashedKeyAlgorithm : public RsaKeyAlgorithm { > > Should be final.
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:70 >> + KeyHashAlgorithm hash; > > Why is this public?
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:72 >> + RsaHashedKeyAlgorithm(const String& n, size_t m, Vector<uint8_t>&& p, KeyHashAlgorithm&& h) > > Those parameter names are really not great. We try to avoid abbreviations in WebKit. > Should be private.
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:77 >> + virtual ~RsaHashedKeyAlgorithm() > > I don't need this destructor it seems.
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:82 >> + KeyAlgorithmClass keyAlgorithmClass() const override { return KeyAlgorithmClass::HRSA; } > > override -> final
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:109 >> + Ref<KeyAlgorithm> algorithm() const override; > > override -> final
Fixed.
Jiewen Tan
Comment 25
2016-09-13 16:56:11 PDT
Created
attachment 288747
[details]
Patch
WebKit Commit Bot
Comment 26
2016-09-13 16:57:51 PDT
Attachment 288747
[details]
did not pass style-queue: ERROR: Source/WebCore/crypto/CryptoKey.h:47: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:52: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:53: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:54: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:55: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 5 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Chris Dumez
Comment 27
2016-09-13 18:28:04 PDT
Comment on
attachment 288747
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=288747&action=review
Looks like the patch does not build on ews.
> Source/WebCore/crypto/CryptoKey.idl:50 > + [CachedAttribute, CustomGetter] readonly attribute object algorithm;
CachedAttribute does not do anything if you have a CustomGetter. You would have to do the caching yourself in the custom bindings. An easy way to write a test for this is: shouldBeTrue("key.algorithm === key.algorithm"); key.algorithm.foo = 1; gc(); shouldBe("key.algorithm.foo", "1");
Chris Dumez
Comment 28
2016-09-13 18:44:08 PDT
Comment on
attachment 288747
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=288747&action=review
> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:33 > +#include "CryptoKeyRSA.h"
Based on the build error, I think you'll need to include: #include <heap/HeapInlines.h>
> Source/WebCore/crypto/CryptoKey.h:60 > + KeyAlgorithm(const String& name)
This one should be explicit.
> Source/WebCore/crypto/CryptoKey.h:70 > + String name() const { return m_name; }
should return a const String&.
> Source/WebCore/crypto/CryptoKey.h:74 > +
nit: extra line here.
>> Source/WebCore/crypto/CryptoKey.idl:50 >> + [CachedAttribute, CustomGetter] readonly attribute object algorithm; > > CachedAttribute does not do anything if you have a CustomGetter. You would have to do the caching yourself in the custom bindings. > > An easy way to write a test for this is: > shouldBeTrue("key.algorithm === key.algorithm"); > key.algorithm.foo = 1; > gc(); > shouldBe("key.algorithm.foo", "1");
Never mind, it does have the impact of generating the m_algorithm member and it will be fine as long as you do the caching in the custom bindings (which I think you do). We should still add the layout test to confirm this works (and keeps working in the future).
> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:38 > + explicit HmacKeyAlgorithm(const String& name, const String& hash, size_t length)
explicit is not useful here.
> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:47 > + String hash() const { return m_hash; }
should return a const String&.
> Source/WebCore/crypto/keys/CryptoKeyRSA.h:52 > + explicit RsaKeyAlgorithm(const String& name, size_t modulusLength, Vector<uint8_t>&& publicExponent)
explicit is not really useful here since there is more than 1 mandatory parameter.
> Source/WebCore/crypto/keys/CryptoKeyRSA.h:62 > + Vector<uint8_t> publicExponent() const { return m_publicExponent; }
nit: Should return a const Vector<uint8_t>&.
> Source/WebCore/crypto/keys/CryptoKeyRSA.h:79 > + String hash() const { return m_hash; }
nit: should return a const String&.
Chris Dumez
Comment 29
2016-09-13 18:46:56 PDT
Comment on
attachment 288747
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=288747&action=review
> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:43 > + if (m_algorithm)
Yeah, your caching code in this function looks good. [CachedAttribute] should work.
Chris Dumez
Comment 30
2016-09-13 18:51:10 PDT
Comment on
attachment 288747
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=288747&action=review
> Source/WebCore/ChangeLog:7 > + Reviewed by Brent Fulgham.
Oh, and please add my name since I reviewed too :) Reviewed by Brent Fulgham and Chris Dumez.
Jiewen Tan
Comment 31
2016-09-13 19:22:39 PDT
Comment on
attachment 288747
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=288747&action=review
>> Source/WebCore/ChangeLog:7 >> + Reviewed by Brent Fulgham. > > Oh, and please add my name since I reviewed too :) > Reviewed by Brent Fulgham and Chris Dumez.
Since you haven't r+, I didn't add you. Fixed now. :)
>> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:33 >> +#include "CryptoKeyRSA.h" > > Based on the build error, I think you'll need to include: > #include <heap/HeapInlines.h>
Fixed.
>> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:43 >> + if (m_algorithm) > > Yeah, your caching code in this function looks good. [CachedAttribute] should work.
:)
>> Source/WebCore/crypto/CryptoKey.h:60 >> + KeyAlgorithm(const String& name) > > This one should be explicit.
Fixed.
>> Source/WebCore/crypto/CryptoKey.h:70 >> + String name() const { return m_name; } > > should return a const String&.
Fixed.
>> Source/WebCore/crypto/CryptoKey.h:74 >> + > > nit: extra line here.
Fixed.
>>> Source/WebCore/crypto/CryptoKey.idl:50 >>> + [CachedAttribute, CustomGetter] readonly attribute object algorithm; >> >> CachedAttribute does not do anything if you have a CustomGetter. You would have to do the caching yourself in the custom bindings. >> >> An easy way to write a test for this is: >> shouldBeTrue("key.algorithm === key.algorithm"); >> key.algorithm.foo = 1; >> gc(); >> shouldBe("key.algorithm.foo", "1"); > > Never mind, it does have the impact of generating the m_algorithm member and it will be fine as long as you do the caching in the custom bindings (which I think you do). We should still add the layout test to confirm this works (and keeps working in the future).
Testcases are added.
>> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:38 >> + explicit HmacKeyAlgorithm(const String& name, const String& hash, size_t length) > > explicit is not useful here.
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:47 >> + String hash() const { return m_hash; } > > should return a const String&.
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:52 >> + explicit RsaKeyAlgorithm(const String& name, size_t modulusLength, Vector<uint8_t>&& publicExponent) > > explicit is not really useful here since there is more than 1 mandatory parameter.
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:62 >> + Vector<uint8_t> publicExponent() const { return m_publicExponent; } > > nit: Should return a const Vector<uint8_t>&.
Fixed.
>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:79 >> + String hash() const { return m_hash; } > > nit: should return a const String&.
Fixed.
Jiewen Tan
Comment 32
2016-09-13 23:37:32 PDT
Created
attachment 288775
[details]
Patch
WebKit Commit Bot
Comment 33
2016-09-13 23:39:16 PDT
Attachment 288775
[details]
did not pass style-queue: ERROR: Source/WebCore/crypto/CryptoKey.h:47: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:52: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:53: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:54: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:55: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 5 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Jiewen Tan
Comment 34
2016-09-14 00:36:31 PDT
Created
attachment 288779
[details]
Patch
WebKit Commit Bot
Comment 35
2016-09-14 00:37:30 PDT
Attachment 288779
[details]
did not pass style-queue: ERROR: Source/WebCore/crypto/CryptoKey.h:47: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:52: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:53: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:54: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:55: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 5 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Chris Dumez
Comment 36
2016-09-14 09:12:45 PDT
Looks like the patch does not build on GTK/EFL.
Chris Dumez
Comment 37
2016-09-14 09:22:56 PDT
Comment on
attachment 288779
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=288779&action=review
r=me with comments.
> Source/WebCore/crypto/CryptoKey.h:60 > + explicit KeyAlgorithm(const String& name)
nit: This should be protected, not public.
> Source/WebCore/crypto/gnutls/CryptoKeyRSAGnuTLS.cpp:86 > + return std::make_unique<RsaKeyAlgorithm>(emptyString(), 0, { });
Looks like EFL / GTK do not like the { } here :/
> LayoutTests/crypto/subtle/crypto-key-algorithm-gc.html:11 > +description("Test that CryptoKey.algoithm preserves custom properties.");
typo: algoithm
> LayoutTests/crypto/subtle/crypto-key-algorithm-gc.html:16 > + key = result;
Is this async? If so, what guarantees that this completion handler has been called *before* startTest() is executed?
> LayoutTests/crypto/subtle/crypto-key-algorithm-gc.html:36 > + shouldBe('key.algorithm.foo', '"bar"');
shouldBeEqualToString('key.algorithm.foo', 'bar');
> LayoutTests/crypto/subtle/crypto-key-usages-gc.html:11 > +description("Test that CryptoKey.algoithm preserves custom properties.");
Typo but anyway, it should be 'usages'
> LayoutTests/crypto/subtle/crypto-key-usages-gc.html:16 > + key = result;
Same comment as earlier about the potential race.
> LayoutTests/crypto/subtle/crypto-key-usages-gc.html:21 > + shouldBeTrue("key.algorithm === key.algorithm");
key.usages === key.usages
Jiewen Tan
Comment 38
2016-09-14 11:48:48 PDT
Comment on
attachment 288779
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=288779&action=review
>> Source/WebCore/crypto/CryptoKey.h:60 >> + explicit KeyAlgorithm(const String& name) > > nit: This should be protected, not public.
Fixed.
>> Source/WebCore/crypto/gnutls/CryptoKeyRSAGnuTLS.cpp:86 >> + return std::make_unique<RsaKeyAlgorithm>(emptyString(), 0, { }); > > Looks like EFL / GTK do not like the { } here :/
Created a Vector instance instead.
>> LayoutTests/crypto/subtle/crypto-key-algorithm-gc.html:11 >> +description("Test that CryptoKey.algoithm preserves custom properties."); > > typo: algoithm
Fixed.
>> LayoutTests/crypto/subtle/crypto-key-algorithm-gc.html:16 >> + key = result; > > Is this async? If so, what guarantees that this completion handler has been called *before* startTest() is executed?
Fixed.
>> LayoutTests/crypto/subtle/crypto-key-algorithm-gc.html:36 >> + shouldBe('key.algorithm.foo', '"bar"'); > > shouldBeEqualToString('key.algorithm.foo', 'bar');
Fixed.
>> LayoutTests/crypto/subtle/crypto-key-usages-gc.html:11 >> +description("Test that CryptoKey.algoithm preserves custom properties."); > > Typo but anyway, it should be 'usages'
Fixed.
>> LayoutTests/crypto/subtle/crypto-key-usages-gc.html:16 >> + key = result; > > Same comment as earlier about the potential race.
Fixed.
>> LayoutTests/crypto/subtle/crypto-key-usages-gc.html:21 >> + shouldBeTrue("key.algorithm === key.algorithm"); > > key.usages === key.usages
Fixed.
Jiewen Tan
Comment 39
2016-09-14 11:50:49 PDT
Created
attachment 288841
[details]
Patch
WebKit Commit Bot
Comment 40
2016-09-14 11:52:51 PDT
Attachment 288841
[details]
did not pass style-queue: ERROR: Source/WebCore/crypto/CryptoKey.h:47: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:52: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:53: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:54: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/crypto/CryptoKey.h:55: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 5 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Jiewen Tan
Comment 41
2016-09-14 17:21:40 PDT
Committed
r205941
: <
http://trac.webkit.org/changeset/205941
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug