Bug 168486 – [WinCairo][MiniBrowser] Add ca-bundle to display secure pages

• Status: RESOLVED
• Resolution: INVALID
• Component: Tools / Tests

[Basuke Suzuki]

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\

[Fujii Hironori]

(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

[Basuke Suzuki]

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.

[Basuke Suzuki]

Created attachment 302677 [details]
Patch

change the order of path location trial to make the posibility of configuration with environment variable.

[WebKit Commit Bot]

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.

[Basuke Suzuki]

Created attachment 302689 [details]
Patch

fix format error

[Alex Christensen]

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.

[Basuke Suzuki]

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

[Basuke Suzuki]

Created attachment 302858 [details]
Patch

Remove the download sequence from Cmake script.

[Alex Christensen]

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

[WebKit Commit Bot]

Comment on attachment 302858 [details]
Patch

Clearing flags on attachment: 302858

Committed r213087: <http://trac.webkit.org/changeset/213087>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Basuke Suzuki]

WinCairoRequirements now includes the cacert.pem. Reopen this bug to copy the cert into application bundle.

[Basuke Suzuki]

Created attachment 322139 [details]
added

[Brent Fulgham]

Comment on attachment 322139 [details]
added

Thank you! r=me

[Basuke Suzuki]

Oh my. It was already copied by PlatformWin.cmake!
I don't judge the location of bundle copy.

[WebKit Commit Bot]

Comment on attachment 322139 [details]
added

Clearing flags on attachment: 322139

Committed r222639: <http://trac.webkit.org/changeset/222639>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 177630

[Basuke Suzuki]

The code was completely rewritten and this feature was implemented in it.

[Radar WebKit Bug Importer]

<rdar://problem/58615544>