Bug 168486 - [WinCairo][MiniBrowser] Add ca-bundle to display secure pages
Summary: [WinCairo][MiniBrowser] Add ca-bundle to display secure pages
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Windows 10
: P2 Minor
Assignee: Basuke Suzuki
URL:
Keywords:
Depends on: 177630
Blocks:
  Show dependency treegraph
 
Reported: 2017-02-16 17:18 PST by Basuke Suzuki
Modified: 2017-09-28 17:02 PDT (History)
9 users (show)

See Also:


Attachments
Patch (1.24 KB, patch)
2017-02-24 11:14 PST, Basuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (2.84 KB, patch)
2017-02-24 11:44 PST, Basuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (2.84 KB, patch)
2017-02-24 14:20 PST, Basuke Suzuki
achristensen: review-
Details | Formatted Diff | Diff
Patch (1.60 KB, patch)
2017-02-27 11:43 PST, Basuke Suzuki
no flags Details | Formatted Diff | Diff
added (1.30 KB, patch)
2017-09-28 16:20 PDT, 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 2017-02-16 17:18:18 PST
The MiniBrowser of WinCairo doesn't have ca bundle and cannot display https pages. There are many solutions:

1. this is up to the user. they have to prepare ca-bundle and set the environment variable to point it.
2. download the latest ca-bundle from somewhere we can trust and place it in WebKit bundle directory while building.
3. disable certificate validation when no ca-bundle is supplied.

for 2, the location will be WebKitBuild\Release\bin64\WebKit.resources\certificates\
Comment 1 Fujii Hironori 2017-02-19 18:51:58 PST
(In reply to comment #0)
> 3. disable certificate validation when no ca-bundle is supplied.

You can disable it by a following env var.
set WEBKIT_IGNORE_SSL_ERRORS=1
https://trac.webkit.org/changeset/30359
Comment 2 Basuke Suzuki 2017-02-24 11:14:16 PST
Created attachment 302675 [details]
Patch

Added download phase on CMake if the build is WinCairo.
The source of the ca-bundle is Curl website. It is chosen because of its good maintenance of the latest ca-bundle.
Comment 3 Basuke Suzuki 2017-02-24 11:44:21 PST
Created attachment 302677 [details]
Patch

change the order of path location trial to make the posibility of configuration with environment variable.
Comment 4 WebKit Commit Bot 2017-02-24 14:05:53 PST
Attachment 302677 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:104:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Basuke Suzuki 2017-02-24 14:20:34 PST
Created attachment 302689 [details]
Patch

fix format error
Comment 6 Alex Christensen 2017-02-27 07:06:08 PST
Comment on attachment 302689 [details]
Patch

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

> Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:102
> +    char* envPath = getenv("CURL_CA_BUNDLE_PATH");

Preferring this if it exists seems ok.

> Tools/MiniBrowser/win/CMakeLists.txt:41
> +    file(DOWNLOAD ${MiniBrowser_CABUNDLE_URL} "${MiniBrowser_CERTIFICATES_DIR}/cacert.pem" SHOW_PROGRESS)

I'm not sure it's a good idea to download this file every time you build.  It could slow things down, it would require network access while building or fail, it would ping the curl server every time a developer runs cmake and developers might worry that they are being tracked, and CMake's file(DOWNLOAD ...) seems to ignore ssl errors, so it's unclear how secure it really is.  It would be nice to print out instructions for how to get the cacert.pem, but I don't think this is a good idea to commit to the repository as-is.
Comment 7 Basuke Suzuki 2017-02-27 10:48:53 PST
(In reply to comment #6)

> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302689&action=review
> 
> > Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:102
> > +    char* envPath = getenv("CURL_CA_BUNDLE_PATH");
> 
> Preferring this if it exists seems ok.
> 

> I'm not sure it's a good idea to download this file every time you build. 
> It could slow things down, it would require network access while building or
> fail, it would ping the curl server every time a developer runs cmake and
> developers might worry that they are being tracked,

That's right. Actually that is exactly annoying thing by update-webkit-dependency do for me. I agree to avoid this kind of download on build process.

> CMake's
> file(DOWNLOAD ...) seems to ignore ssl errors, so it's unclear how secure it
> really is.

OMG, this must be the biggest reason to deny this patch.


> It would be nice to print out instructions for how to get the
> cacert.pem, but I don't think this is a good idea to commit to the
> repository as-is.

My idea is not downloading it separately, but include it inside WinCairoRequirements, because that library must maintain regularly. But at this moment, it seems not easy to make change to WinCairoRequirements.

Until then, I just want to remove the download patch.
Comment 8 Basuke Suzuki 2017-02-27 11:43:10 PST
Created attachment 302858 [details]
Patch

Remove the download sequence from Cmake script.
Comment 9 Alex Christensen 2017-02-27 12:25:09 PST
(In reply to comment #7)
> My idea is not downloading it separately, but include it inside
> WinCairoRequirements, because that library must maintain regularly. But at
> this moment, it seems not easy to make change to WinCairoRequirements.
That sounds like a good idea.  It should be easy.  Just fork https://github.com/peavo/WinCairoRequirements and off you go.
Comment 10 WebKit Commit Bot 2017-02-27 12:53:35 PST
Comment on attachment 302858 [details]
Patch

Clearing flags on attachment: 302858

Committed r213087: <http://trac.webkit.org/changeset/213087>
Comment 11 WebKit Commit Bot 2017-02-27 12:53:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Basuke Suzuki 2017-09-28 14:57:53 PDT
WinCairoRequirements now includes the cacert.pem. Reopen this bug to copy the cert into application bundle.
Comment 13 Basuke Suzuki 2017-09-28 16:20:00 PDT
Created attachment 322139 [details]
added
Comment 14 Brent Fulgham 2017-09-28 16:26:24 PDT
Comment on attachment 322139 [details]
added

Thank you! r=me
Comment 15 Basuke Suzuki 2017-09-28 16:31:51 PDT
Oh my. It was already copied by PlatformWin.cmake!
I don't judge the location of bundle copy.
Comment 16 WebKit Commit Bot 2017-09-28 16:53:01 PDT
Comment on attachment 322139 [details]
added

Clearing flags on attachment: 322139

Committed r222639: <http://trac.webkit.org/changeset/222639>
Comment 17 WebKit Commit Bot 2017-09-28 16:53:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 WebKit Commit Bot 2017-09-28 17:02:25 PDT
Re-opened since this is blocked by bug 177630