Bug 173883 - [GCrypt] Key serialization support
Summary: [GCrypt] Key serialization support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on: 173646
Blocks: 133122
  Show dependency treegraph
 
Reported: 2017-06-27 12:27 PDT by Zan Dobersek
Modified: 2017-07-27 16:14 PDT (History)
4 users (show)

See Also:


Attachments
WIP patch (14.78 KB, patch)
2017-06-27 12:31 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (16.68 KB, patch)
2017-06-28 09:31 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (16.77 KB, patch)
2017-07-03 01:06 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
WIP patch (9.45 KB, patch)
2017-07-19 10:39 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (8.89 KB, patch)
2017-07-19 11:17 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (5.58 KB, patch)
2017-07-26 04:27 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2017-06-27 12:27:01 PDT
SSIA.
Comment 1 Zan Dobersek 2017-06-27 12:31:41 PDT
Created attachment 313933 [details]
WIP patch
Comment 2 Build Bot 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.
Comment 3 Zan Dobersek 2017-06-28 09:31:39 PDT
Created attachment 314031 [details]
Patch
Comment 4 Zan Dobersek 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.
Comment 5 Zan Dobersek 2017-07-03 01:06:12 PDT
Created attachment 314464 [details]
Patch

Now buildable.
Comment 6 Jiewen Tan 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.
Comment 7 Zan Dobersek 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?
Comment 8 Jiewen Tan 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.
Comment 9 Zan Dobersek 2017-07-19 06:19:55 PDT
Understood, I will simplify the implementation.
Comment 10 Zan Dobersek 2017-07-19 10:39:19 PDT
Created attachment 315933 [details]
WIP patch
Comment 11 Zan Dobersek 2017-07-19 11:17:24 PDT
Created attachment 315940 [details]
Patch
Comment 12 Jiewen Tan 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.
Comment 13 Zan Dobersek 2017-07-26 04:27:13 PDT
Created attachment 316444 [details]
Patch
Comment 14 Jiewen Tan 2017-07-26 11:43:49 PDT
Comment on attachment 316444 [details]
Patch

Looks good to me. r=me.
Comment 15 Zan Dobersek 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>
Comment 16 Zan Dobersek 2017-07-27 01:13:09 PDT
All reviewed patches have been landed.  Closing bug.