WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
165111
CryptoAlgorithm::generateKey() should take lvalue reference to const CryptoAlgorithmParameters
https://bugs.webkit.org/show_bug.cgi?id=165111
Summary
CryptoAlgorithm::generateKey() should take lvalue reference to const CryptoAl...
Daniel Bates
Reported
2016-11-28 13:56:53 PST
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.
Attachments
Patch
(21.63 KB, patch)
2016-11-28 14:03 PST
,
Daniel Bates
no flags
Details
Formatted Diff
Diff
Patch
(21.65 KB, patch)
2016-11-28 17:03 PST
,
Daniel Bates
darin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Daniel Bates
Comment 1
2016-11-28 14:03:57 PST
Created
attachment 295528
[details]
Patch
WebKit Commit Bot
Comment 2
2016-11-28 14:05:36 PST
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
Comment 3
2016-11-28 14:06:40 PST
(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
Comment 4
2016-11-28 14:08:11 PST
(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
Comment 5
2016-11-28 17:02:33 PST
(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
Comment 6
2016-11-28 17:03:19 PST
Created
attachment 295553
[details]
Patch
WebKit Commit Bot
Comment 7
2016-11-28 17:05:11 PST
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 8
2016-11-28 19:59:46 PST
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
Comment 9
2016-11-29 10:50:09 PST
(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
Comment 10
2016-11-29 10:52:10 PST
Committed
r209077
: <
http://trac.webkit.org/changeset/209077
>
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