Bug 191647

Created attachment 355170 [details] PATCH

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?

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

Would you want to use the same API for DER or PEM Certificates? Could you add a test with a DER certificate?

Would you want to be able to compare certificates of different encodings and find them to be equal?

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

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.

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.

(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 on attachment 355170 [details] PATCH r=me

Comment on attachment 355170 [details] PATCH Clearing flags on attachment: 355170 Committed r238387: <https://trac.webkit.org/changeset/238387>

All reviewed patches have been landed. Closing bug.

<rdar://problem/46176215>