Summary: | [WinCairo][MiniBrowser] Add ca-bundle to display secure pages | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Basuke Suzuki <Basuke.Suzuki> | ||||||||||||
Component: | Tools / Tests | Assignee: | Basuke Suzuki <Basuke.Suzuki> | ||||||||||||
Status: | RESOLVED INVALID | ||||||||||||||
Severity: | Minor | CC: | achristensen, Basuke.Suzuki, bfulgham, commit-queue, don.olmstead, galpeter, Hironori.Fujii, lforschler, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | Windows 10 | ||||||||||||||
Bug Depends on: | 177630 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Basuke Suzuki
2017-02-16 17:18:18 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 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.
Created attachment 302677 [details]
Patch
change the order of path location trial to make the posibility of configuration with environment variable.
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.
Created attachment 302689 [details]
Patch
fix format error
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. (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. Created attachment 302858 [details]
Patch
Remove the download sequence from Cmake script.
(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 on attachment 302858 [details] Patch Clearing flags on attachment: 302858 Committed r213087: <http://trac.webkit.org/changeset/213087> All reviewed patches have been landed. Closing bug. WinCairoRequirements now includes the cacert.pem. Reopen this bug to copy the cert into application bundle. Created attachment 322139 [details]
added
Comment on attachment 322139 [details]
added
Thank you! r=me
Oh my. It was already copied by PlatformWin.cmake! I don't judge the location of bundle copy. Comment on attachment 322139 [details] added Clearing flags on attachment: 322139 Committed r222639: <http://trac.webkit.org/changeset/222639> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 177630 The code was completely rewritten and this feature was implemented in it. |