Bug 185782

Summary: [Curl] Allow passing contents of Root CA data directly.
Product: WebKit Reporter: Basuke Suzuki <Basuke.Suzuki>
Component: WebCore Misc.Assignee: Basuke Suzuki <Basuke.Suzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, annulen, Basuke.Suzuki, commit-queue, don.olmstead, ews-watchlist, galpeter, pvollan, rniwa, webkit-bug-importer, youennf, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
PATCH
youennf: review+
PATCH
none
FIX
none
FIX
none
PATCH
none
FIX none

Description Basuke Suzuki 2018-05-18 13:16:34 PDT
Currently the data must be in a file and set by its path. This patch allow application to set root CA data by passing binary data directly, or adding special prefix ":memory:" via regular setCACertPath() method.
Comment 1 Basuke Suzuki 2018-05-18 14:46:39 PDT
Created attachment 340739 [details]
PATCH
Comment 2 youenn fablet 2018-05-24 10:25:49 PDT
Comment on attachment 340739 [details]
PATCH

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

> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:85
> +        caCertData.append(caCertString.data(), caCertString.length());

This create a string temporarily.
Maybe there is a way to use StringView so that you can directly create a Vector<char> from it.

> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:86
> +        setCACertData(WTFMove(caCertData));

Early return.
Comment 3 Basuke Suzuki 2018-06-15 10:41:23 PDT
Created attachment 342825 [details]
PATCH

After Youenn's review, we've decided remove :onmemory: hack into path data. Client now should prepare data by itself. It is the right direction as a library interface.
Comment 4 Basuke Suzuki 2018-06-15 13:51:36 PDT
Created attachment 342841 [details]
FIX
Comment 5 Basuke Suzuki 2018-06-15 13:53:01 PDT
Created attachment 342842 [details]
FIX
Comment 6 Alex Christensen 2018-06-15 14:01:36 PDT
Comment on attachment 342842 [details]
FIX

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

> Source/WebCore/platform/network/curl/CurlSSLHandle.h:110
>      String m_caCertPath;
> +    Vector<char> m_caCertData;

It seems like you have one or the other but never both.  If that's the case, you could use Variant<String, Vector<uint8_t>> m_caCertPathOrData.  This would reduce future "I forgot about the case where the cert root info was in the other form.
Comment 7 Basuke Suzuki 2018-06-15 14:10:47 PDT
Is it okay to start using variant?? I thought C++ features other than optional were still on hold state.
Comment 8 Basuke Suzuki 2018-06-18 10:43:41 PDT
Created attachment 342950 [details]
PATCH

Switch the set of member variables into Variant.
Comment 9 EWS Watchlist 2018-06-18 10:45:10 PDT
Attachment 342950 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/curl/CurlSSLHandle.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Basuke Suzuki 2018-06-18 11:14:00 PDT
Created attachment 342954 [details]
FIX
Comment 11 Basuke Suzuki 2018-06-19 10:22:59 PDT
Can anybody review this? The main feature was already r+ by Youenn. After that, I removed unnecessary hack and started using Variant from Alex's suggestion.
Comment 12 Yusuke Suzuki 2018-06-19 19:22:48 PDT
Comment on attachment 342954 [details]
FIX

r=me
Comment 13 WebKit Commit Bot 2018-06-19 19:58:33 PDT
Comment on attachment 342954 [details]
FIX

Clearing flags on attachment: 342954

Committed r233002: <https://trac.webkit.org/changeset/233002>
Comment 14 WebKit Commit Bot 2018-06-19 19:58:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2018-06-19 19:59:19 PDT
<rdar://problem/41274600>