Bug 173646 – [GCrypt] Implement CryptoKeyEC SPKI exports

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

SSIA.

[Zan Dobersek]

Created attachment 313512 [details]
WIP patch

[Build Bot]

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.

[Zan Dobersek]

Created attachment 313520 [details]
Patch

Ready for review

[Jiewen Tan]

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.

[Zan Dobersek]

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

[Zan Dobersek]

Created attachment 313823 [details]
Patch

[Jiewen Tan]

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.

[Jiewen Tan]

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.

[Zan Dobersek]

(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.

[Zan Dobersek]

Comment on attachment 313823 [details]
Patch

Clearing flags on attachment: 313823

Committed r219064: <http://trac.webkit.org/changeset/219064>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.