Bug 165111 – CryptoAlgorithm::generateKey() should take lvalue reference to const CryptoAlgorithmParameters

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

CryptoAlgorithm::generateKey() and its overrides always expect to receive a non-null pointer to a CryptoAlgorithmParameter object and never take ownership of it. Currently the CryptoAlgorithmParameter object is passed as a lvalue reference to a std::unique_ptr<CryptoAlgorithmParameter> const. It is sufficient to pass it as a lvalue reference to a CryptoAlgorithmParameter const.

[Daniel Bates]

Created attachment 295528 [details]
Patch

[WebKit Commit Bot]

Attachment 295528 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:91:  CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:80:  CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:66:  CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:91:  CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:71:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [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]

(In reply to comment #0)
> CryptoAlgorithm::generateKey() and its overrides always expect to receive a
> non-null pointer to a CryptoAlgorithmParameter object and never take
> ownership of it. Currently the CryptoAlgorithmParameter object is passed as
> a lvalue reference to a std::unique_ptr<CryptoAlgorithmParameter> const. It
> is sufficient to pass it as a lvalue reference to a CryptoAlgorithmParameter
> const.

Actually, it is a backward compatibility issue. Both SubtleCrypto::generateKey() and WebKitSubtleCrypto::generateKey() go through the same underlying CryptoKeyRSA::generatePair() to generate a RSA key pair. However, parameters used by those two are different. That's why I extract parameters outside of the CryptoAlgorithmParameter object. Ideally, we should pass the CryptoAlgorithmParameter object to CryptoKeyRSA::generatePair() once WebKitSubtleCrypto is deprecated.

[Jiewen Tan]

(In reply to comment #3)
> (In reply to comment #0)
> > CryptoAlgorithm::generateKey() and its overrides always expect to receive a
> > non-null pointer to a CryptoAlgorithmParameter object and never take
> > ownership of it. Currently the CryptoAlgorithmParameter object is passed as
> > a lvalue reference to a std::unique_ptr<CryptoAlgorithmParameter> const. It
> > is sufficient to pass it as a lvalue reference to a CryptoAlgorithmParameter
> > const.
> 
> Actually, it is a backward compatibility issue. Both
> SubtleCrypto::generateKey() and WebKitSubtleCrypto::generateKey() go through
> the same underlying CryptoKeyRSA::generatePair() to generate a RSA key pair.
> However, parameters used by those two are different. That's why I extract
> parameters outside of the CryptoAlgorithmParameter object. Ideally, we
> should pass the CryptoAlgorithmParameter object to
> CryptoKeyRSA::generatePair() once WebKitSubtleCrypto is deprecated.

And because CryptoKeyRSA::generatePair() will generate keys async. It will take over the ownership of the CryptoAlgorithmParameter object.

[Daniel Bates]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #0)
> > > CryptoAlgorithm::generateKey() and its overrides always expect to receive a
> > > non-null pointer to a CryptoAlgorithmParameter object and never take
> > > ownership of it. Currently the CryptoAlgorithmParameter object is passed as
> > > a lvalue reference to a std::unique_ptr<CryptoAlgorithmParameter> const. It
> > > is sufficient to pass it as a lvalue reference to a CryptoAlgorithmParameter
> > > const.
> > 
> > Actually, it is a backward compatibility issue. Both
> > SubtleCrypto::generateKey() and WebKitSubtleCrypto::generateKey() go through
> > the same underlying CryptoKeyRSA::generatePair() to generate a RSA key pair.
> > However, parameters used by those two are different. That's why I extract
> > parameters outside of the CryptoAlgorithmParameter object. Ideally, we
> > should pass the CryptoAlgorithmParameter object to
> > CryptoKeyRSA::generatePair() once WebKitSubtleCrypto is deprecated.
> 
> And because CryptoKeyRSA::generatePair() will generate keys async. It will
> take over the ownership of the CryptoAlgorithmParameter object.

Jiewen and I discussed this in person today (11/28). For now the proposed change is sufficient as key generation (CryptoAlgorithm::generateKey()) does not need to take ownership of the CryptoAlgorithmParameter object in both the old and new Web Crypto implementation. In the future we plan to change the new implementation such that key generation is asynchronous; => it will make sense for CryptoAlgorithm::generateKey() to take ownership of the CryptoAlgorithmParameter object. We will update the interface of CryptoAlgorithm::generateKey() as necessary to support such transfer of ownership.

[Daniel Bates]

Created attachment 295553 [details]
Patch

[WebKit Commit Bot]

Attachment 295553 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:91:  CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:80:  CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:66:  CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:91:  CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:71:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 295553 [details]
Patch

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

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:555
> +    algorithm->generateKey(*params.get(), extractable, keyUsages, WTFMove(callback), WTFMove(exceptionCallback), *scriptExecutionContextFromExecState(&state));

Should not need the .get() here.

[Daniel Bates]

(In reply to comment #8)
> > Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:555
> > +    algorithm->generateKey(*params.get(), extractable, keyUsages, WTFMove(callback), WTFMove(exceptionCallback), *scriptExecutionContextFromExecState(&state));
> 
> Should not need the .get() here.

Will remove .get() before landing.

[Daniel Bates]

Committed r209077: <http://trac.webkit.org/changeset/209077>