Bug 173883

Summary: [GCrypt] Key serialization support
Product: WebKit Reporter: Zan Dobersek <zan>
Component: New BugsAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, jiewen_tan, mcatanzaro, webkit-bug-importer
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 173646    
Bug Blocks: 133122    
Attachments:
Description Flags
WIP patch
none
Patch
none
Patch
none
WIP patch
none
Patch
none
Patch none

Zan Dobersek
Reported 2017-06-27 12:27:01 PDT
SSIA.
Attachments
WIP patch (14.78 KB, patch)
2017-06-27 12:31 PDT, Zan Dobersek 		no flags
DetailsFormatted DiffDiff
Patch (16.68 KB, patch)
2017-06-28 09:31 PDT, Zan Dobersek 		no flags
DetailsFormatted DiffDiff
Patch (16.77 KB, patch)
2017-07-03 01:06 PDT, Zan Dobersek 		no flags
DetailsFormatted DiffDiff
WIP patch (9.45 KB, patch)
2017-07-19 10:39 PDT, Zan Dobersek 		no flags
DetailsFormatted DiffDiff
Patch (8.89 KB, patch)
2017-07-19 11:17 PDT, Zan Dobersek 		no flags
DetailsFormatted DiffDiff
Patch (5.58 KB, patch)
2017-07-26 04:27 PDT, Zan Dobersek 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Zan Dobersek
Comment 1 2017-06-27 12:31:41 PDT
Created attachment 313933 [details] WIP patch
Build Bot
Comment 2 2017-06-28 07:04:31 PDT
Attachment 313933 [details] did not pass style-queue: ERROR: Source/WebCore/PAL/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Zan Dobersek
Comment 3 2017-06-28 09:31:39 PDT
Created attachment 314031 [details] Patch
Zan Dobersek
Comment 4 2017-06-28 09:32:47 PDT
(In reply to Zan Dobersek from comment #3) > Created attachment 314031 [details] > Patch This is ready for review, but the patch still relies on libtasn1 helpers that are being added in bug #173646.
Zan Dobersek
Comment 5 2017-07-03 01:06:12 PDT
Created attachment 314464 [details] Patch Now buildable.
Jiewen Tan
Comment 6 2017-07-12 11:53:58 PDT
Comment on attachment 314464 [details] Patch In general, I don't believe we need to encrypt CryptoKey objects when they are stored into the indexedDB. However, this functionality has been implemented before I take over the WebCrypto API, and it is hard to remove for backward compatibility. I recommend GTK+ not to follow this approach at least you have legitimate reasons for it.
Zan Dobersek
Comment 7 2017-07-14 23:36:02 PDT
(In reply to Jiewen Tan from comment #6) > Comment on attachment 314464 [details] > Patch > > In general, I don't believe we need to encrypt CryptoKey objects when they > are stored into the indexedDB. However, this functionality has been > implemented before I take over the WebCrypto API, and it is hard to remove > for backward compatibility. I recommend GTK+ not to follow this approach at > least you have legitimate reasons for it. If I'm not mistaken, serialization support is still required for passing keys into worker contexts. The main intent was to gain support for that. Do you plan to change how that is supported?
Jiewen Tan
Comment 8 2017-07-17 09:27:00 PDT
(In reply to Zan Dobersek from comment #7) > (In reply to Jiewen Tan from comment #6) > > Comment on attachment 314464 [details] > > Patch > > > > In general, I don't believe we need to encrypt CryptoKey objects when they > > are stored into the indexedDB. However, this functionality has been > > implemented before I take over the WebCrypto API, and it is hard to remove > > for backward compatibility. I recommend GTK+ not to follow this approach at > > least you have legitimate reasons for it. > > If I'm not mistaken, serialization support is still required for passing > keys into worker contexts. The main intent was to gain support for that. > > Do you plan to change how that is supported? Yes, that's true. But you don't have to encrypt/decrypt the binaries.
Zan Dobersek
Comment 9 2017-07-19 06:19:55 PDT
Understood, I will simplify the implementation.
Zan Dobersek
Comment 10 2017-07-19 10:39:19 PDT
Created attachment 315933 [details] WIP patch
Zan Dobersek
Comment 11 2017-07-19 11:17:24 PDT
Created attachment 315940 [details] Patch
Jiewen Tan
Comment 12 2017-07-25 15:16:04 PDT
Comment on attachment 315940 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315940&action=review Thanks for taking my last comment. r- for the following reasons. Please address it. > Source/WebCore/crypto/gcrypt/SerializedCryptoKeyWrapGCrypt.cpp:44 > bool wrapSerializedCryptoKey(const Vector<uint8_t>& masterKey, const Vector<uint8_t>& key, Vector<uint8_t>& result) I am suggesting you just return the key. Maybe a copy is needed. That's it. The key here is already serialized. You don't have to do it again.
Zan Dobersek
Comment 13 2017-07-26 04:27:13 PDT
Created attachment 316444 [details] Patch
Jiewen Tan
Comment 14 2017-07-26 11:43:49 PDT
Comment on attachment 316444 [details] Patch Looks good to me. r=me.
Zan Dobersek
Comment 15 2017-07-27 01:13:05 PDT
Comment on attachment 316444 [details] Patch Clearing flags on attachment: 316444 Committed r219976: <http://trac.webkit.org/changeset/219976>
Zan Dobersek
Comment 16 2017-07-27 01:13:09 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.