[GCrypt] Implement CryptoKeyEC SPKI imports

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[GCrypt] Implement CryptoKeyEC SPKI imports

[Zan Dobersek]

Created attachment 312017 [details]
Patch

[Build Bot]

Attachment 312017 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:261:  Use nullptr instead of NULL (even in *comments*).  [readability/null] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

Comment on attachment 312017 [details]
Patch

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

> Source/WebCore/PAL/pal/crypto/gcrypt/ASN1.h:1
> +/*

I wonder if we could replace a custom ANS.1 decoder with a well-maintained library. I have implemented one because I was not aware of any available Apple ASN.1 decoder/encoder at that time. However, I discovered some after. Therefore, it is one of my future effort to remove Apple port's customized ASN.1 decoder/encoder.

[Michael Catanzaro]

We should probably use Libtasn1 (LGPLv2+) at least for GTK/WPE. I believe it would be suitable for Apple as well.

[Michael Catanzaro]

On the other hand, the current implementation looks like a fairly small amount of code.

[Jiewen Tan]

(In reply to Michael Catanzaro from comment #5)
> On the other hand, the current implementation looks like a fairly small
> amount of code.

The thing is I am not sure if we are able to fix any security bugs that with the ASN.1 decoder/encoder. Actually, there aren't even any tests for the customized ASN.1 decoder in this patch.

[Jiewen Tan]

(In reply to Michael Catanzaro from comment #4)
> We should probably use Libtasn1 (LGPLv2+) at least for GTK/WPE. I believe it
> would be suitable for Apple as well.

That sounds good. Actually I haven't decided which library to use at this moment. It will be good if GTK+ and Apple can share the same decoder/encoder. It shouldn't have any platform specific issues. Widely speaking, it might also open the opportunity for other people to use a ASN.1 decoder/encoder in WebKit.

BTW, I suspect WebRTC could have one already since it added BoringSSL to the project.

[Michael Catanzaro]

(In reply to Jiewen Tan from comment #7)
> BTW, I suspect WebRTC could have one already since it added BoringSSL to the
> project.

Wow... I had no clue that libwebrtc bundled boringssl. That's not going to be acceptable for GTK/WPE. But bundling libwebrtc isn't either, so I guess our folks working on that will figure something out.

[Zan Dobersek]

Let's try with libtasn1.

[Zan Dobersek]

Created attachment 312640 [details]
Patch

[Build Bot]

Attachment 312640 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/crypto/tasn1/Utilities.cpp:100:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/PAL/pal/crypto/tasn1/Utilities.h:52:  asn1_node is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Created attachment 312760 [details]
Patch

Doesn't use RELEASE_ASSERT_NOT_REACHED() in supportedAlgorithmIdentifier().

[Build Bot]

Attachment 312760 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/crypto/tasn1/Utilities.cpp:100:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/PAL/pal/crypto/tasn1/Utilities.h:52:  asn1_node is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

Comment on attachment 312760 [details]
Patch

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

Thanks for implementing it with Libtasn1. The library seems well maintained. Please address the following comments.

Michael: Please do a sanity check if the way Zan adds Libtasn1 follows GTK+'s rule.

> Source/WebCore/PAL/pal/crypto/tasn1/Utilities.h:34
> +namespace TASN1 {

Why not ASN1?
Do we really need a separate namespace?

> Source/WebCore/PAL/pal/crypto/tasn1/WebCrypto.asn:1
> +WebCrypto { }

It would be better if we could include links to RFCs that define the following structures.

> Source/WebCore/PAL/pal/crypto/tasn1/WebCrypto.asn:16
> +  attributes           [0]  IMPLICIT Attributes OPTIONAL

Doesn't feel this attribute is useful for us. I am not certain if we should copy the structure bit by bit from the spec.

Maybe we should only focus on useful parts.

> Source/WebCore/PAL/pal/crypto/tasn1/WebCrypto.asn:25
> +Attributes ::= SET OF Attribute

Ditto.

> Source/WebCore/PAL/pal/crypto/tasn1/WebCrypto.asn:27
> +Attribute ::= SEQUENCE {

Ditto.

> Source/WebCore/PAL/pal/crypto/tasn1/WebCrypto.asn:32
> +AttributeSetValue ::= SET OF ANY

Ditto.

> Source/WebCore/PAL/pal/crypto/tasn1/WebCrypto.asn:46
> +ECParameters ::= CHOICE {

This part confused me before as OpenSSL and its forks seem using something different from the spec.
I can't tell the source hence I just copy and paste their output to be compatible with theirs.
I don't know if you encounter anything like that.

> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:200
> +    static const std::array<uint8_t, 13> s_id_ecDH { { "1.3.132.1.12" } };

Have you found any concrete examples that actually use id-ecDH? I am curious about it as I haven't found any. Therefore, I didn't implement it for Apple ports.
Actually, I think the SPKI/PKCS8 parts of the spec are somewhat too detailed. To me, it seems to be a direct copy&paste from other RFC but doesn't reflect the actual word usage.
For example, OpenSSL has a single function for generating EC key pair. Therefore, it doesn't output id_ecDH.

> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:241
> +RefPtr<CryptoKeyEC> CryptoKeyEC::platformImportSpki(CryptoAlgorithmIdentifier identifier, NamedCurve curve, Vector<uint8_t>&& keyData, bool extractable, CryptoKeyUsageBitmap usages)

One thing I am worried about is the efficiency of the library. Ideally, there should be only one pass of the data and no extra spaces are needed. Have you confirmed how do they do?

> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:288
> +        if (uncompressedPointSizeForCurve(curve) != subjectPublicKey->size() || subjectPublicKey->at(0) != 0x04)

I am curious about if the second check will be done inside gcry_sexp_build(&platformKey, nullptr, "(public-key(ecc(curve %s)(q %b)))", curveName(curve), subjectPublicKey->size(), subjectPublicKey->data());

[Michael Catanzaro]

(In reply to Jiewen Tan from comment #14)
> Michael: Please do a sanity check if the way Zan adds Libtasn1 follows
> GTK+'s rule.

It's almost right, I'd just ask for a better error message if libtasn1 is unavailable. Something like:

find_package(Libtasn1)
if (NOT LIBTASN1_FOUND)
  message(FATAL_ERROR "libtasn1
endif ()

[Michael Catanzaro]

Whoops

find_package(Libtasn1)
if (NOT LIBTASN1_FOUND)
  message(FATAL_ERROR "libtasn1 is required to enable Web Crypto API support")
endif ()

[Zan Dobersek]

Comment on attachment 312760 [details]
Patch

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

>> Source/WebCore/PAL/pal/crypto/tasn1/Utilities.h:34
>> +namespace TASN1 {
> 
> Why not ASN1?
> Do we really need a separate namespace?

We don't, I just followed the PAL::GCrypt style. TASN1 was used due to library being called libtasn1. I don't know what's the story with the letter T though.

>> Source/WebCore/PAL/pal/crypto/tasn1/WebCrypto.asn:1
>> +WebCrypto { }
> 
> It would be better if we could include links to RFCs that define the following structures.

OK.

>> Source/WebCore/PAL/pal/crypto/tasn1/WebCrypto.asn:16
>> +  attributes           [0]  IMPLICIT Attributes OPTIONAL
> 
> Doesn't feel this attribute is useful for us. I am not certain if we should copy the structure bit by bit from the spec.
> 
> Maybe we should only focus on useful parts.

It definitely isn't useful for us, but the spec doesn't demand that it's ignored during parsing.

OTOH if the attribute is removed from the ASN.1 definition, no W3C test case is affected.

>> Source/WebCore/PAL/pal/crypto/tasn1/WebCrypto.asn:46
>> +ECParameters ::= CHOICE {
> 
> This part confused me before as OpenSSL and its forks seem using something different from the spec.
> I can't tell the source hence I just copy and paste their output to be compatible with theirs.
> I don't know if you encounter anything like that.

Similarly to PrivateKeyInfo, I preferred using the same ASN.1 declarations as specified in RFCs. But obviously the only viable choice here is 'namedCurve', so the 'parameters' attribute in AlgortihmIdentifier is required to encode an object identifier representing an EC curve (in case the 'algorithm' attribute represents an EC algorithm).

>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:200
>> +    static const std::array<uint8_t, 13> s_id_ecDH { { "1.3.132.1.12" } };
> 
> Have you found any concrete examples that actually use id-ecDH? I am curious about it as I haven't found any. Therefore, I didn't implement it for Apple ports.
> Actually, I think the SPKI/PKCS8 parts of the spec are somewhat too detailed. To me, it seems to be a direct copy&paste from other RFC but doesn't reflect the actual word usage.
> For example, OpenSSL has a single function for generating EC key pair. Therefore, it doesn't output id_ecDH.

This was added to follow the spec. Checking the W3C tests, no test case uses this identifier.

>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:241
>> +RefPtr<CryptoKeyEC> CryptoKeyEC::platformImportSpki(CryptoAlgorithmIdentifier identifier, NamedCurve curve, Vector<uint8_t>&& keyData, bool extractable, CryptoKeyUsageBitmap usages)
> 
> One thing I am worried about is the efficiency of the library. Ideally, there should be only one pass of the data and no extra spaces are needed. Have you confirmed how do they do?

Only one pass of the encoded data is done, with the decoded data stored in specific asn1_node objects, either in the decoded form (i.e. object identifiers are decoded into strings) or directly copied (bit and octet strings). The elementData() helper then retrieves that data, again copying it from specific asn1_node objects.

libtasn1 provides API that exposes the data contained in ans1_node through asn1_read_node_value(). I preferred copying the data into Vector<uint8_t> objects to get up and running quickly and safely, but we can build helpers around asn1_read_node_value() as well.
https://www.gnu.org/software/libtasn1/manual/libtasn1.html#index-asn1_005fread_005fnode_005fvalue

>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:288
>> +        if (uncompressedPointSizeForCurve(curve) != subjectPublicKey->size() || subjectPublicKey->at(0) != 0x04)
> 
> I am curious about if the second check will be done inside gcry_sexp_build(&platformKey, nullptr, "(public-key(ecc(curve %s)(q %b)))", curveName(curve), subjectPublicKey->size(), subjectPublicKey->data());

No, that just copies the EC point data into the s-expression object. So here we'd require further checks that the EC point is valid for this curve, using gcry_mpi_ec_curve_point().

[Jiewen Tan]

Comment on attachment 312760 [details]
Patch

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

>>> Source/WebCore/PAL/pal/crypto/tasn1/Utilities.h:34
>>> +namespace TASN1 {
>> 
>> Why not ASN1?
>> Do we really need a separate namespace?
> 
> We don't, I just followed the PAL::GCrypt style. TASN1 was used due to library being called libtasn1. I don't know what's the story with the letter T though.

OK.

>>> Source/WebCore/PAL/pal/crypto/tasn1/WebCrypto.asn:16
>>> +  attributes           [0]  IMPLICIT Attributes OPTIONAL
>> 
>> Doesn't feel this attribute is useful for us. I am not certain if we should copy the structure bit by bit from the spec.
>> 
>> Maybe we should only focus on useful parts.
> 
> It definitely isn't useful for us, but the spec doesn't demand that it's ignored during parsing.
> 
> OTOH if the attribute is removed from the ASN.1 definition, no W3C test case is affected.

OK.

>>> Source/WebCore/PAL/pal/crypto/tasn1/WebCrypto.asn:46
>>> +ECParameters ::= CHOICE {
>> 
>> This part confused me before as OpenSSL and its forks seem using something different from the spec.
>> I can't tell the source hence I just copy and paste their output to be compatible with theirs.
>> I don't know if you encounter anything like that.
> 
> Similarly to PrivateKeyInfo, I preferred using the same ASN.1 declarations as specified in RFCs. But obviously the only viable choice here is 'namedCurve', so the 'parameters' attribute in AlgortihmIdentifier is required to encode an object identifier representing an EC curve (in case the 'algorithm' attribute represents an EC algorithm).

OK. This comment doesn't apply to SPKI. You should pay attention to it when you implement PKCS8.

>>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:200
>>> +    static const std::array<uint8_t, 13> s_id_ecDH { { "1.3.132.1.12" } };
>> 
>> Have you found any concrete examples that actually use id-ecDH? I am curious about it as I haven't found any. Therefore, I didn't implement it for Apple ports.
>> Actually, I think the SPKI/PKCS8 parts of the spec are somewhat too detailed. To me, it seems to be a direct copy&paste from other RFC but doesn't reflect the actual word usage.
>> For example, OpenSSL has a single function for generating EC key pair. Therefore, it doesn't output id_ecDH.
> 
> This was added to follow the spec. Checking the W3C tests, no test case uses this identifier.

You might want to add a specific test to check this and enable it only for GTK+ for now.

>>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:241
>>> +RefPtr<CryptoKeyEC> CryptoKeyEC::platformImportSpki(CryptoAlgorithmIdentifier identifier, NamedCurve curve, Vector<uint8_t>&& keyData, bool extractable, CryptoKeyUsageBitmap usages)
>> 
>> One thing I am worried about is the efficiency of the library. Ideally, there should be only one pass of the data and no extra spaces are needed. Have you confirmed how do they do?
> 
> Only one pass of the encoded data is done, with the decoded data stored in specific asn1_node objects, either in the decoded form (i.e. object identifiers are decoded into strings) or directly copied (bit and octet strings). The elementData() helper then retrieves that data, again copying it from specific asn1_node objects.
> 
> libtasn1 provides API that exposes the data contained in ans1_node through asn1_read_node_value(). I preferred copying the data into Vector<uint8_t> objects to get up and running quickly and safely, but we can build helpers around asn1_read_node_value() as well.
> https://www.gnu.org/software/libtasn1/manual/libtasn1.html#index-asn1_005fread_005fnode_005fvalue

To me, it seems too many copies. Since the gcrypt API operates on pointers as well, I don't think it is worth converting a pointer to a vector then back to a pointer.
Also, the API itself does a copy already. This should be enough for safety I presume.

>>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:288
>>> +        if (uncompressedPointSizeForCurve(curve) != subjectPublicKey->size() || subjectPublicKey->at(0) != 0x04)
>> 
>> I am curious about if the second check will be done inside gcry_sexp_build(&platformKey, nullptr, "(public-key(ecc(curve %s)(q %b)))", curveName(curve), subjectPublicKey->size(), subjectPublicKey->data());
> 
> No, that just copies the EC point data into the s-expression object. So here we'd require further checks that the EC point is valid for this curve, using gcry_mpi_ec_curve_point().

OK, but I don't see the further check in the codes.

[Zan Dobersek]

Comment on attachment 312760 [details]
Patch

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

>>>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:241
>>>> +RefPtr<CryptoKeyEC> CryptoKeyEC::platformImportSpki(CryptoAlgorithmIdentifier identifier, NamedCurve curve, Vector<uint8_t>&& keyData, bool extractable, CryptoKeyUsageBitmap usages)
>>> 
>>> One thing I am worried about is the efficiency of the library. Ideally, there should be only one pass of the data and no extra spaces are needed. Have you confirmed how do they do?
>> 
>> Only one pass of the encoded data is done, with the decoded data stored in specific asn1_node objects, either in the decoded form (i.e. object identifiers are decoded into strings) or directly copied (bit and octet strings). The elementData() helper then retrieves that data, again copying it from specific asn1_node objects.
>> 
>> libtasn1 provides API that exposes the data contained in ans1_node through asn1_read_node_value(). I preferred copying the data into Vector<uint8_t> objects to get up and running quickly and safely, but we can build helpers around asn1_read_node_value() as well.
>> https://www.gnu.org/software/libtasn1/manual/libtasn1.html#index-asn1_005fread_005fnode_005fvalue
> 
> To me, it seems too many copies. Since the gcrypt API operates on pointers as well, I don't think it is worth converting a pointer to a vector then back to a pointer.
> Also, the API itself does a copy already. This should be enough for safety I presume.

I tried using asn1_read_node_value() to directly access the value data, but the problem is that libtasn1 even after decoding stores data for some node types in DER format. So we'd have to manually validate that DER data before using it. We could do that just like asn1_read_value() does it, but looking at that function it's not that straightforward:
http://git.savannah.gnu.org/gitweb/?p=libtasn1.git;a=blob;f=lib/element.c;h=b09f82647f7b461e66df9801eaeb99bf826917a3;hb=HEAD#l833

I don't think it's worth implementing this complexity, at least not right from the start. I'd prefer an initial implementation that relies on copying the properly-decoded data. Furthermore, I don't think copying and the allocation of the underlying memory would introduce great overhead -- these aren't large memory areas.

>>>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:288
>>>> +        if (uncompressedPointSizeForCurve(curve) != subjectPublicKey->size() || subjectPublicKey->at(0) != 0x04)
>>> 
>>> I am curious about if the second check will be done inside gcry_sexp_build(&platformKey, nullptr, "(public-key(ecc(curve %s)(q %b)))", curveName(curve), subjectPublicKey->size(), subjectPublicKey->data());
>> 
>> No, that just copies the EC point data into the s-expression object. So here we'd require further checks that the EC point is valid for this curve, using gcry_mpi_ec_curve_point().
> 
> OK, but I don't see the further check in the codes.

Will add.

[Zan Dobersek]

Created attachment 313064 [details]
Patch

[Build Bot]

Attachment 313064 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/crypto/tasn1/Utilities.cpp:100:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/PAL/pal/crypto/tasn1/Utilities.h:52:  asn1_node is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

Comment on attachment 312760 [details]
Patch

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

>>>>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:241
>>>>> +RefPtr<CryptoKeyEC> CryptoKeyEC::platformImportSpki(CryptoAlgorithmIdentifier identifier, NamedCurve curve, Vector<uint8_t>&& keyData, bool extractable, CryptoKeyUsageBitmap usages)
>>>> 
>>>> One thing I am worried about is the efficiency of the library. Ideally, there should be only one pass of the data and no extra spaces are needed. Have you confirmed how do they do?
>>> 
>>> Only one pass of the encoded data is done, with the decoded data stored in specific asn1_node objects, either in the decoded form (i.e. object identifiers are decoded into strings) or directly copied (bit and octet strings). The elementData() helper then retrieves that data, again copying it from specific asn1_node objects.
>>> 
>>> libtasn1 provides API that exposes the data contained in ans1_node through asn1_read_node_value(). I preferred copying the data into Vector<uint8_t> objects to get up and running quickly and safely, but we can build helpers around asn1_read_node_value() as well.
>>> https://www.gnu.org/software/libtasn1/manual/libtasn1.html#index-asn1_005fread_005fnode_005fvalue
>> 
>> To me, it seems too many copies. Since the gcrypt API operates on pointers as well, I don't think it is worth converting a pointer to a vector then back to a pointer.
>> Also, the API itself does a copy already. This should be enough for safety I presume.
> 
> I tried using asn1_read_node_value() to directly access the value data, but the problem is that libtasn1 even after decoding stores data for some node types in DER format. So we'd have to manually validate that DER data before using it. We could do that just like asn1_read_value() does it, but looking at that function it's not that straightforward:
> http://git.savannah.gnu.org/gitweb/?p=libtasn1.git;a=blob;f=lib/element.c;h=b09f82647f7b461e66df9801eaeb99bf826917a3;hb=HEAD#l833
> 
> I don't think it's worth implementing this complexity, at least not right from the start. I'd prefer an initial implementation that relies on copying the properly-decoded data. Furthermore, I don't think copying and the allocation of the underlying memory would introduce great overhead -- these aren't large memory areas.

OK. Sure those aren't large memory areas, but I am just trying to see if we could optimize our codes as fast as possible.

[Jiewen Tan]

Comment on attachment 313064 [details]
Patch

GTK+ and WPE are not happy. BTW, you should add a test case to test id_ecDH. Enable it only for GTK+ now.

Also, I want to confirm if GCrypt doesn't accept any DER encoded binaries.

[Zan Dobersek]

(In reply to Jiewen Tan from comment #23)
> Comment on attachment 313064 [details]
> Patch
> 
> GTK+ and WPE are not happy. BTW, you should add a test case to test id_ecDH.
> Enable it only for GTK+ now.
> 

I'll add it in a separate bug that blocks this one.

> Also, I want to confirm if GCrypt doesn't accept any DER encoded binaries.

It does not.

[Zan Dobersek]

Created attachment 313240 [details]
Patch

Not yet requesting for a review.

[Build Bot]

Attachment 313240 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/crypto/tasn1/Utilities.cpp:100:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/PAL/pal/crypto/tasn1/Utilities.h:52:  asn1_node is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

(In reply to Zan Dobersek from comment #24)
> (In reply to Jiewen Tan from comment #23)
> > Comment on attachment 313064 [details]
> > Patch
> > 
> > GTK+ and WPE are not happy. BTW, you should add a test case to test id_ecDH.
> > Enable it only for GTK+ now.
> > 
> 
> I'll add it in a separate bug that blocks this one.
> 

Patch uploaded in bug #173543.

[Zan Dobersek]

Created attachment 313292 [details]
Patch

[Build Bot]

Attachment 313292 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/crypto/tasn1/Utilities.cpp:100:  More than one command on the same line  [whitespace/newline] [4]
ERROR: LayoutTests/platform/gtk/TestExpectations:843:  Path does not exist.  [test/expectations] [5]
ERROR: Source/WebCore/PAL/pal/crypto/tasn1/Utilities.h:52:  asn1_node is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

Comment on attachment 313292 [details]
Patch

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

Looks good to me. Could you please elaborate a little bit more on the mechanism you use in the following to me? Thanks.

> Source/WebCore/PAL/pal/crypto/tasn1/Utilities.h:52
> +    operator asn1_node() const { return m_structure; }

Sorry, I don't quite get what this is.

[Zan Dobersek]

Comment on attachment 313292 [details]
Patch

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

>> Source/WebCore/PAL/pal/crypto/tasn1/Utilities.h:52
>> +    operator asn1_node() const { return m_structure; }
> 
> Sorry, I don't quite get what this is.

It's an implicit conversion operator. We can use this to silently pass the m_structure object held in PAL::TASN1::Structure to libtasn1 API that expects an asn1_node argument.

> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:254
> +        auto algorithm = PAL::TASN1::elementData(spki, "algorithm.algorithm");

Here, for example, elementData() expects asn1_node as the first parameter, and because of that operator the asn1_node object can be passed here implicitly just by listing the PAL::TASN1::Structure object.

[Jiewen Tan]

Comment on attachment 313292 [details]
Patch

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

Thanks for the illustration. r=me if Michael doesn't have any issues with the build part.

>>> Source/WebCore/PAL/pal/crypto/tasn1/Utilities.h:52
>>> +    operator asn1_node() const { return m_structure; }
>> 
>> Sorry, I don't quite get what this is.
> 
> It's an implicit conversion operator. We can use this to silently pass the m_structure object held in PAL::TASN1::Structure to libtasn1 API that expects an asn1_node argument.

Got it.

[Zan Dobersek]

(In reply to Jiewen Tan from comment #32)
> Comment on attachment 313292 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313292&action=review
> 
> Thanks for the illustration. r=me if Michael doesn't have any issues with
> the build part.
> 

Michael's on holidays this week, so I can get someone else to look at the build integration tomorrow. His only feedback regarding this was in comment #16, and that's been addressed.

[Carlos Garcia Campos]

Comment on attachment 313292 [details]
Patch

CMake changes look good to me, and the rest was already reviewed by Jiewen and Michael, so r+

[Zan Dobersek]

Created attachment 313496 [details]
Patch for landing

Listing all three reviewers.

[Zan Dobersek]

Comment on attachment 313496 [details]
Patch for landing

Clearing flags on attachment: 313496

Committed r218626: <http://trac.webkit.org/changeset/218626>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Zan Dobersek]

*** Bug 169272 has been marked as a duplicate of this bug. ***