Bug 191647 - [Curl] Add API for CertificateInfo.
Summary: [Curl] Add API for CertificateInfo.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks: 187679 191646
  Show dependency treegraph
 
Reported: 2018-11-14 12:40 PST by Basuke Suzuki
Modified: 2018-11-19 18:03 PST (History)
7 users (show)

See Also:


Attachments
PATCH (18.80 KB, patch)
2018-11-16 18:36 PST, Basuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2018-11-14 12:40:48 PST
It will be used by Server Trust Evaluation UI.
Comment 1 Basuke Suzuki 2018-11-16 18:36:51 PST
Created attachment 355170 [details]
PATCH
Comment 2 Alex Christensen 2018-11-16 20:27:47 PST
Comment on attachment 355170 [details]
PATCH

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

> Source/WebKit/Shared/API/c/curl/WKCertificateInfoCurl.cpp:46
> +        auto data = reinterpret_cast<WKDataRef>(WKArrayGetItemAtIndex(certificateChainRef, i));

I'm not sure you want to make an API that takes PEM encoded certificates as WKDataRefs.  I think of PEM encoded certificates as strings that need to be parsed into data.  Does libcurl have a way to just give the raw certificate data to it, or does it only take PEM encoded data?
Comment 3 Basuke Suzuki 2018-11-16 21:03:34 PST
(In reply to Alex Christensen from comment #2)
> Comment on attachment 355170 [details]
> PATCH
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355170&action=review
> 
> > Source/WebKit/Shared/API/c/curl/WKCertificateInfoCurl.cpp:46
> > +        auto data = reinterpret_cast<WKDataRef>(WKArrayGetItemAtIndex(certificateChainRef, i));
> 
> I'm not sure you want to make an API that takes PEM encoded certificates as
> WKDataRefs.  I think of PEM encoded certificates as strings that need to be
> parsed into data.  Does libcurl have a way to just give the raw certificate
> data to it, or does it only take PEM encoded data?

curl doesn't have any support for PEM contents. We have to use OpenSSL to decode information from them. Also it can be DER format (binary) or PEM format (TEXT w/ separator and base64 contents). In that sense, it should be treated as opaque data.
Comment 4 Alex Christensen 2018-11-16 22:42:06 PST
Would you want to use the same API for DER or PEM Certificates?  Could you add a test with a DER certificate?
Comment 5 Alex Christensen 2018-11-16 22:44:05 PST
Would you want to be able to compare certificates of different encodings and find them to be equal?
Comment 6 Basuke Suzuki 2018-11-16 23:40:35 PST
(In reply to Alex Christensen from comment #4)
> Would you want to use the same API for DER or PEM Certificates?  Could you
> add a test with a DER certificate?

(In reply to Alex Christensen from comment #5)
> Would you want to be able to compare certificates of different encodings and
> find them to be equal?

This patch is for CertificateInfo which is just a container for certificates and it doesn't care what it hold inside. The requested tests will be handled by verifier tests, which will be included in https://bugs.webkit.org/show_bug.cgi?id=187679 ([Curl] Add allowSpecificHTTPSCertificateForHost support.)
Comment 7 Basuke Suzuki 2018-11-16 23:46:17 PST
Sorry, this is wrong. The openssl api only allows PEM format. DER must be converted to PEM. There's no encoding mix-up here. Only PEM.

But even if it is a PEM format, which is consisted by printable ascii characters, the contents is just base64 encoded binary with header and footer. There's no text parsing to get information from this chunk of data. I think treating this as a Data is better than String.
Comment 8 Alex Christensen 2018-11-17 07:39:02 PST
There *is* parsing, but it's probably between libcurl and openssl to do so.  The network uses certificate bytes from finding and decoding the base64 info.  It's true that the string here can only be ASCII, but it is a string that needs to be parsed by something before it can be used on the network.  I'm ok with it being a data instead of a string because it's always ascii, but I want that to be an informed choice.
Comment 9 Basuke Suzuki 2018-11-17 13:32:26 PST
(In reply to Alex Christensen from comment #8)
> There *is* parsing, but it's probably between libcurl and openssl to do so. 

I meant the word “parsing” as text processing, such as JSON parsing. Because certificate is in a binary shape, I want to use the term “decoding”. Anyway, those are completely task of OpenSSL’s task, neither libcurl nor WebKit curl layer care about its content detail so that there are no parsing or decoding. If an app want to display those information, that is the task of the app to fetch information using OpenSSL api. That’s why I want to treat it as opaque. That’s the reason I think WKDataRef is appropriate for this API.

> The network uses certificate bytes from finding and decoding the base64
> info.  It's true that the string here can only be ASCII, but it is a string
> that needs to be parsed by something before it can be used on the network. 
> I'm ok with it being a data instead of a string because it's always ascii,
> but I want that to be an informed choice.

I am not clear what you request me. What should I show you ?
Comment 10 Alex Christensen 2018-11-19 17:35:59 PST
Comment on attachment 355170 [details]
PATCH

r=me
Comment 11 WebKit Commit Bot 2018-11-19 18:02:42 PST
Comment on attachment 355170 [details]
PATCH

Clearing flags on attachment: 355170

Committed r238387: <https://trac.webkit.org/changeset/238387>
Comment 12 WebKit Commit Bot 2018-11-19 18:02:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2018-11-19 18:03:38 PST
<rdar://problem/46176215>