Bug 173646 - [GCrypt] Implement CryptoKeyEC SPKI exports
Summary: [GCrypt] Implement CryptoKeyEC SPKI exports
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:
Blocks: 133122 173648 173695 173697 173883
  Show dependency treegraph
 
Reported: 2017-06-21 06:07 PDT by Zan Dobersek
Modified: 2017-07-03 00:46 PDT (History)
3 users (show)

See Also:


Attachments
WIP patch (11.50 KB, patch)
2017-06-21 06:22 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (14.71 KB, patch)
2017-06-21 08:39 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (13.46 KB, patch)
2017-06-26 04:21 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-21 06:07:44 PDT
SSIA.
Comment 1 Zan Dobersek 2017-06-21 06:22:35 PDT
Created attachment 313512 [details]
WIP patch
Comment 2 Build Bot 2017-06-21 06:36:16 PDT
Attachment 313512 [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-21 08:39:20 PDT
Created attachment 313520 [details]
Patch

Ready for review
Comment 4 Jiewen Tan 2017-06-22 14:58:45 PDT
Comment on attachment 313520 [details]
Patch

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

Looks good to me. Please address the following comments:

> Source/WebCore/PAL/pal/crypto/tasn1/Utilities.cpp:146
> +    int ret = asn1_der_coding(root, elementName, nullptr, &length, nullptr);

I didn't see any documentation that suggests this usage. Is this a hack?

> Source/WebCore/PAL/pal/crypto/tasn1/Utilities.cpp:166
> +{

Maybe we just need the first one? Or we just don't need these two wrapper?

> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:424
> +        if (!PAL::TASN1::writeElement(ecParameters, "", "namedCurve"))

I didn't see any documentation that suggests this usage. Is the next step itself sufficient?

> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:446
> +            auto data = PAL::TASN1::encodedData(ecParameters, "");

A little bit confused that why we don't need a name for the encoded data.

> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:469
> +    auto result = PAL::TASN1::encodedData(spki, "");

Ditto.
Comment 5 Zan Dobersek 2017-06-26 04:05:41 PDT
Comment on attachment 313520 [details]
Patch

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

>> Source/WebCore/PAL/pal/crypto/tasn1/Utilities.cpp:146
>> +    int ret = asn1_der_coding(root, elementName, nullptr, &length, nullptr);
> 
> I didn't see any documentation that suggests this usage. Is this a hack?

No, this is a valid usage pattern, relying on ASN1_MEM_ERROR being returned, along with the length integer being set to the value of necessary bytes.

As used in GnuTLS:
https://gitlab.com/gnutls/gnutls/blob/master/lib/x509/x509.c#L2847

>> Source/WebCore/PAL/pal/crypto/tasn1/Utilities.cpp:166
>> +{
> 
> Maybe we just need the first one? Or we just don't need these two wrapper?

OK, we should be able to use just one, and have that one already accept `const void*` as the data pointer parameter, instead of differentiating between const char* and const uint8_t*.

>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:424
>> +        if (!PAL::TASN1::writeElement(ecParameters, "", "namedCurve"))
> 
> I didn't see any documentation that suggests this usage. Is the next step itself sufficient?

Yes. This call selects the `namedCurve` element in the `ECParameters` choice structure, the next one writes out the object identifier under it.

Again the example of GnuTLS:
https://gitlab.com/gnutls/gnutls/blob/master/lib/x509/key_encode.c#L263

In both calls, the default 0 value is used as the data size when calling the underlying asn1_write_value(). This differs from the GnuTLS usage as well as the documentation, but looking at the code the passed-in length value doesn't have any effect when writing out choice or object identifier values. I'll still drop the zero-length default and explicitly pass proper length values for every writeElement() call, following the documentation and GnuTLS code.

>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:446
>> +            auto data = PAL::TASN1::encodedData(ecParameters, "");
> 
> A little bit confused that why we don't need a name for the encoded data.

The specified name is searched for in the given asn1_node through the asn1_find_node() function. But if this is a null-escaped empty string, the original asn1_node is returned, meaning in this case that encoded data of the ecParameters structure is retrieved.

http://git.savannah.gnu.org/gitweb/?p=libtasn1.git;a=blob;f=lib/parser_aux.c;h=976ab38f1863845c03ed5730d91ebe0113348f79;hb=HEAD#l152
Comment 6 Zan Dobersek 2017-06-26 04:21:45 PDT
Created attachment 313823 [details]
Patch
Comment 7 Jiewen Tan 2017-06-29 18:13:14 PDT
Comment on attachment 313520 [details]
Patch

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

>>> Source/WebCore/PAL/pal/crypto/tasn1/Utilities.cpp:146
>>> +    int ret = asn1_der_coding(root, elementName, nullptr, &length, nullptr);
>> 
>> I didn't see any documentation that suggests this usage. Is this a hack?
> 
> No, this is a valid usage pattern, relying on ASN1_MEM_ERROR being returned, along with the length integer being set to the value of necessary bytes.
> 
> As used in GnuTLS:
> https://gitlab.com/gnutls/gnutls/blob/master/lib/x509/x509.c#L2847

OK. I guess I shouldn't only rely on its documentation.

>>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:424
>>> +        if (!PAL::TASN1::writeElement(ecParameters, "", "namedCurve"))
>> 
>> I didn't see any documentation that suggests this usage. Is the next step itself sufficient?
> 
> Yes. This call selects the `namedCurve` element in the `ECParameters` choice structure, the next one writes out the object identifier under it.
> 
> Again the example of GnuTLS:
> https://gitlab.com/gnutls/gnutls/blob/master/lib/x509/key_encode.c#L263
> 
> In both calls, the default 0 value is used as the data size when calling the underlying asn1_write_value(). This differs from the GnuTLS usage as well as the documentation, but looking at the code the passed-in length value doesn't have any effect when writing out choice or object identifier values. I'll still drop the zero-length default and explicitly pass proper length values for every writeElement() call, following the documentation and GnuTLS code.

OK.

>>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:446
>>> +            auto data = PAL::TASN1::encodedData(ecParameters, "");
>> 
>> A little bit confused that why we don't need a name for the encoded data.
> 
> The specified name is searched for in the given asn1_node through the asn1_find_node() function. But if this is a null-escaped empty string, the original asn1_node is returned, meaning in this case that encoded data of the ecParameters structure is retrieved.
> 
> http://git.savannah.gnu.org/gitweb/?p=libtasn1.git;a=blob;f=lib/parser_aux.c;h=976ab38f1863845c03ed5730d91ebe0113348f79;hb=HEAD#l152

Got you.
Comment 8 Jiewen Tan 2017-06-29 18:43:12 PDT
Comment on attachment 313823 [details]
Patch

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

r=me. Looks good to me. Please address the following comments.

> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:441
> +        if (!PAL::TASN1::writeElement(spki, "algorithm.algorithm", "1.2.840.10045.2.1", 1))

Should we use some sort of constants for "1.2.840.10045.2.1"? It should be frequently used.
Comment 9 Zan Dobersek 2017-07-03 00:44:05 PDT
(In reply to Jiewen Tan from comment #8)
> > Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:441
> > +        if (!PAL::TASN1::writeElement(spki, "algorithm.algorithm", "1.2.840.10045.2.1", 1))
> 
> Should we use some sort of constants for "1.2.840.10045.2.1"? It should be
> frequently used.

Filed bug #174091 to gather these in a single location. I would prefer this to do as a cleanup after the SPKI and PKCS#8 patches land.
Comment 10 Zan Dobersek 2017-07-03 00:46:12 PDT
Comment on attachment 313823 [details]
Patch

Clearing flags on attachment: 313823

Committed r219064: <http://trac.webkit.org/changeset/219064>
Comment 11 Zan Dobersek 2017-07-03 00:46:16 PDT
All reviewed patches have been landed.  Closing bug.