Bug 223591

Summary: [WinCairo] Enable Service Worker tests
Product: WebKit Reporter: Christopher Reid <chris.reid>
Component: PlatformAssignee: Christopher Reid <chris.reid>
Status: RESOLVED FIXED    
Severity: Normal CC: Basuke.Suzuki, don.olmstead, Hironori.Fujii, ross.kirsling, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
none
typo fix
none
typo fix
none
patch none

Description Christopher Reid 2021-03-22 12:23:09 PDT
They are skipped currently.
Comment 1 Christopher Reid 2021-03-22 15:42:45 PDT
Created attachment 423951 [details]
patch
Comment 2 Christopher Reid 2021-03-22 15:53:31 PDT
Created attachment 423953 [details]
patch
Comment 3 Fujii Hironori 2021-03-22 19:59:07 PDT
Comment on attachment 423953 [details]
patch

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

> Source/WebKit/ChangeLog:8
> +        WPT HTTP tests were failing because SSL Connet tests weren't triggering authentication challenges.

Connet → Connect

I don't understand why SSL connection error should trigger authentication challenges.
Auth dialog will pop up if SSL connection error happens?
Comment 4 Christopher Reid 2021-03-22 21:18:38 PDT
(In reply to Fujii Hironori from comment #3)
> Comment on attachment 423953 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423953&action=review
> 
> > Source/WebKit/ChangeLog:8
> > +        WPT HTTP tests were failing because SSL Connet tests weren't triggering authentication challenges.
> 
> Connet → Connect
> 
> I don't understand why SSL connection error should trigger authentication
> challenges.
> Auth dialog will pop up if SSL connection error happens?

What was happening was with only the isSSLCertVerificationError() check, connections to the WPT HTTP server were failing in both MiniBrowser and WKTR with CURLE_SSL_CONNECT_ERROR.

After adding the isSSLConnectError check, now MiniBrowser will trigger a dialog to accept the cert and WKTR will automatically accept the cert with TestController::didReceiveAuthenticationChallenge.
Comment 5 Christopher Reid 2021-03-22 21:19:40 PDT
Created attachment 423986 [details]
typo fix
Comment 6 Christopher Reid 2021-03-22 21:21:14 PDT
Created attachment 423987 [details]
typo fix
Comment 7 Fujii Hironori 2021-03-22 21:41:01 PDT
It sounds weird to me.
I tested my local wpt server with WinCairo MiniBrowser.

1. Start wpt server
   python.exe .\Tools\Scripts\run-webkit-httpd
2. Start MiniBrowser
3. Go to https://localhost:9443/
4. "Server Trust Evaluation Request" dialog pops up
5. Click "OK"
6. The page is shown
Comment 8 Christopher Reid 2021-03-22 23:11:52 PDT
(In reply to Fujii Hironori from comment #7)
> It sounds weird to me.
> I tested my local wpt server with WinCairo MiniBrowser.
> 
> 1. Start wpt server
>    python.exe .\Tools\Scripts\run-webkit-httpd
> 2. Start MiniBrowser
> 3. Go to https://localhost:9443/
> 4. "Server Trust Evaluation Request" dialog pops up
> 5. Click "OK"
> 6. The page is shown

Hm yeah that is weird, I'm a bit surprised all those are working.

For me first off, to even resolve localhost I need to disable c-ares with curl because of https://github.com/WebKitForWindows/WebKitRequirements/pull/51.

Running run-webkit-httpd directly fails with various errors.

For now to get around those erors I run the wpt httpd server directly with `python .\LayoutTests\imported\w3c\web-platform-tests\wpt.py serve --config C:\git\webkit\LayoutTests\imported\w3c\web-platform-tests\config2.json`

Where config2.json is a config copied from the one generated when running `python ./Tools/Scripts/run-webkit-tests --no-build --no-show-results --no-new-test-results --clobber-old-results --debug --wincairo --results-directory layout-test-results --debug-rwt-logging --no-retry-failures --force http/wpt`

This is the config.json file I use:
> {"browser_host": "localhost", "alternate_hosts": {"alt": "127.0.0.1"}, "ports": {"http": [8800, 8801], "https": [9443, 9444], "ws": [49001]}, "aliases": [{"url-path": "/resources/testharnessreport.js", "local-dir": "../../../resources/"}, {"url-path": "/resources/testharness.css", "local-dir": "../../../resources/"}, {"url-path": "/webkit-test-resources/", "local-dir": "../../../resources/"}, {"url-path": "/WebKit/", "local-dir": "../../../http/wpt/"}], "check_subdomains": false, "log_level": "debug", "bind_hostname": true, "ssl": {"type": "openssl", "encrypt_after_connect": false, "openssl": {"openssl_binary": "openssl", "base_path": "C:\\git\\webkit\\layout-test-results\\_wpt_certs", "force_regenerate": false}, "pregenerated": {"host_key_path": null, "host_cert_path": null}, "none": {}}}

Then running minibrowser without these changes and with DEBUG_CURL and CURL_LOG_FILE env vars I get:
> * Connection #0 to host webkit.org left intact
> *   Trying 127.0.0.1:9443...
> * Connected to localhost (127.0.0.1) port 9443 (#0)
> * ALPN, offering h2
> * ALPN, offering http/1.1
> * successfully imported windows ca store
> * error:1404B418:SSL routines:ST_CONNECT:tlsv1 alert unknown ca
> * Closing connection 0
Comment 9 Fujii Hironori 2021-03-22 23:44:54 PDT
I tested your original patch on my Windows PC, and reverting "|| resourceError.isSSLConnectError()" and tested again, and compare the results.
However, those two test trial end up to the same result.

> python.exe ./Tools/Scripts/run-webkit-tests --debug --wincairo --no-new-test-results --no-retry-failures http/tests/workers/service http/wpt/service-workers


> 89 tests ran as expected, 7 didn't:
> 
> Expected to fail, but passed: (4)
>   http/tests/workers/service/service-worker-request-with-body.https.html
>   http/wpt/service-workers/check-service-worker-header.https.html
>   http/wpt/service-workers/cors-preflight-star.any-serviceworker.html
>   http/wpt/service-workers/service-worker-crashing-while-fetching.https.html
> 
> 
> Regressions: Unexpected timeouts (3)
>   http/tests/workers/service/service-worker-download-async-delegates.https.html [ Timeout ]
>   http/tests/workers/service/service-worker-download.https.html [ Timeout ]
>   http/tests/workers/service/service-worker-resource-timing.https.html [ Timeout ]
Comment 10 Christopher Reid 2021-03-24 11:46:31 PDT
Hm could it be somehow using TLS1.2 to connect instead of 1.3?
It looks like the error is CURLE_PEER_FAILED_VERIFICATION on 1.2 and CURLE_SSL_CONNECT_ERROR on 1.3.


> >curl.exe https://localhost:9444 --verbose --tlsv1.3
> *   Trying 127.0.0.1:9444...
> * Connected to localhost (127.0.0.1) port 9444 (#0)
> * ALPN, offering h2
> * ALPN, offering http/1.1
> Client sending ClientHello
> * (304) (OUT), TLS handshake, Client hello (1):
> Client receiving ServerHello
> * (304) (IN), TLS handshake, Server hello (2):
> Client receiving EncryptedExtensions
> * (304) (IN), TLS handshake, Unknown (8):
> Client receiving CertificateRequest
> * (304) (IN), TLS handshake, Certificate (11):
> * error:1404B418:SSL routines:ST_CONNECT:tlsv1 alert unknown ca
> * Closing connection 0
> curl: (35) error:1404B418:SSL routines:ST_CONNECT:tlsv1 alert unknown ca
> 
> >curl.exe https://localhost:9444 --verbose --tls-max 1.2
> *   Trying 127.0.0.1:9444...
> * Connected to localhost (127.0.0.1) port 9444 (#0)
> * ALPN, offering h2
> * ALPN, offering http/1.1
> Client sending ClientHello
> * (304) (OUT), TLS handshake, Client hello (1):
> Client receiving ServerHello
> * (304) (IN), TLS handshake, Server hello (2):
> Client recv returned -6
> * TLSv1.2 (IN), TLS handshake, Certificate (11):
> * TLSv1.2 (OUT), TLS alert, unknown CA (560):
> * SSL certificate problem: unable to get local issuer certificate
> * Closing connection 0
> curl: (60) SSL certificate problem: unable to get local issuer certificate
> More details here: https://curl.se/docs/sslcerts.html
> 
> curl failed to verify the legitimacy of the server and therefore could not
> establish a secure connection to it. To learn more about this situation and
> how to fix it, please visit the web page mentioned above.
Comment 11 Fujii Hironori 2021-03-24 14:25:45 PDT
Interesting. I confirmed the same result on Linux with badssl.com.

> curl https://self-signed.badssl.com/ --verbose --tls-max 1.2
> curl https://self-signed.badssl.com/ --verbose --tlsv1.3

I have some questions:
1. Is this the expected behavior of cURL?
2. If so, can we get the detailed error reason code?
3. Why does WinCairo behave differently on your PC and mine?
Comment 12 Christopher Reid 2021-03-24 23:33:49 PDT
(In reply to Fujii Hironori from comment #11)
> Interesting. I confirmed the same result on Linux with badssl.com.
> 
> > curl https://self-signed.badssl.com/ --verbose --tls-max 1.2
> > curl https://self-signed.badssl.com/ --verbose --tlsv1.3
> 
> I have some questions:
> 1. Is this the expected behavior of cURL?
> 2. If so, can we get the detailed error reason code?

I couldn't find anything to indicate if it's intended or not. TLS 1.3 spec seems like it's stricter with error codes but these types of cert errors are both fatal on 1.2 and 1.3. It does seem inconcistent and I feel like it could be unintended. I'll make an issue on curl to check.

> 3. Why does WinCairo behave differently on your PC and mine?

That's still a big unknown. I feel like you should hit this issue if you're using TLS 1.3. One guess is if you're using system SSL then I don't think that has TLS 1.3 support.
Comment 13 Christopher Reid 2021-03-24 23:56:30 PDT
(In reply to Christopher Reid from comment #12)
>  I'll make an issue on curl to check.

Created https://github.com/curl/curl/issues/6790
Comment 14 Fujii Hironori 2021-03-25 13:14:18 PDT
(In reply to Christopher Reid from comment #13)
> Created https://github.com/curl/curl/issues/6790

Oops. It's my mistake. I didn't check the connection error messages.
badssl.com and my localhost wpt server doesn't accept TLS v1.3. They are normal connection errors.

> PS C:\tools\curl-7.75.0-win64-mingw\bin> .\curl.exe https://localhost:9444 --verbose --tlsv1.3
> *   Trying ::1:9444...
> *   Trying 127.0.0.1:9444...
> * Connected to localhost (127.0.0.1) port 9444 (#0)
> * ALPN, offering h2
> * ALPN, offering http/1.1
> * successfully set certificate verify locations:
> *  CAfile: C:\tools\curl-7.75.0-win64-mingw\bin\curl-ca-bundle.crt
> *  CApath: none
> * TLSv1.3 (OUT), TLS handshake, Client hello (1):
> * TLSv1.3 (IN), TLS alert, handshake failure (552):
> * error:14094410:SSL routines:ssl3_read_bytes:sslv3 alert handshake failure
> * Closing connection 0
> curl: (35) error:14094410:SSL routines:ssl3_read_bytes:sslv3 alert handshake failure

> PS C:\tools\curl-7.75.0-win64-mingw\bin> .\curl.exe https://localhost:9444 --verbose
> *   Trying ::1:9444...
> *   Trying 127.0.0.1:9444...
> * Connected to localhost (127.0.0.1) port 9444 (#0)
> * ALPN, offering h2
> * ALPN, offering http/1.1
> * successfully set certificate verify locations:
> *  CAfile: C:\tools\curl-7.75.0-win64-mingw\bin\curl-ca-bundle.crt
> *  CApath: none
> * TLSv1.3 (OUT), TLS handshake, Client hello (1):
> * TLSv1.3 (IN), TLS handshake, Server hello (2):
> * TLSv1.2 (IN), TLS handshake, Certificate (11):
> * TLSv1.2 (OUT), TLS alert, unknown CA (560):
> * SSL certificate problem: unable to get local issuer certificate
> * Closing connection 0
> curl: (60) SSL certificate problem: unable to get local issuer certificate
> More details here: https://curl.se/docs/sslcerts.html
> 
> curl failed to verify the legitimacy of the server and therefore could not
> establish a secure connection to it. To learn more about this situation and
> how to fix it, please visit the web page mentioned above.
Comment 15 Christopher Reid 2021-03-25 13:44:03 PDT
(In reply to Fujii Hironori from comment #14)
> (In reply to Christopher Reid from comment #13)
> > Created https://github.com/curl/curl/issues/6790
> 
> Oops. It's my mistake. I didn't check the connection error messages.
> badssl.com and my localhost wpt server doesn't accept TLS v1.3. They are
> normal connection errors.


Ah okay that should explain the different wincairo behaviors with WPT.

I was a bit mistaken in the curl ticket when I said a self generated cert returned CURLE_PEER_FAILED_VERIFICATION on TLS 1.3. 
That was only true for curl + OpenSSL. 

When compiling out curl + libressl on TLS 1.3 on ubuntu it has the same `tlsv1 alert unknown ca` error that I see on wincairo. curl + libressl on TLS 1.2 is okay and curl + OpenSSL is okay for both TLS 1.2 and 1.3. So maybe there's still an issue in libcurl or libressl.

I'm trying to dig in to libressl a bit to see what's going on before reopening the ticket or making a libressl ticket.
X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY seems like the first error that's hit when validating the cert but that is in the x509_verify.c file which doesn't seem to care about the TLS version so I think I'm still missing something.
Comment 16 Christopher Reid 2021-03-30 13:32:43 PDT
I now created https://github.com/libressl-portable/openbsd/issues/122 since it looks like a libressl/openbsd issue unless the deviation is expected.
Comment 17 Christopher Reid 2021-03-31 15:42:07 PDT
Note: I commented in the openbsd ticket that I think the call to `SSLerror(ctx->ssl, SSL_AD_REASON_OFFSET + alert_desc);` in tls13_alert_sent_cb is wrong and it should only be in the tls13_alert_received_cb function.
Comment 18 Fujii Hironori 2021-03-31 19:13:37 PDT
WPT server doesn't seem to support TLS v1.3 if it runs by Python 2.7, but Python 3.
WinCairo is still using Python 2.7. So, the libressl issue doesn't have to block this at the moment.
I think you can land the patch by reverting the part if you want.
Comment 19 Christopher Reid 2021-04-02 14:04:31 PDT
Created attachment 425052 [details]
patch

Reverted the SSL Connect check and rebaselined the tests with python 2.7
Comment 20 EWS 2021-04-05 16:56:08 PDT
Committed r275454: <https://commits.webkit.org/r275454>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425052 [details].
Comment 21 Radar WebKit Bug Importer 2021-04-05 16:57:14 PDT
<rdar://problem/76242396>
Comment 22 Christopher Reid 2021-04-13 14:56:49 PDT
FYI I merged the libressl TLS 1.3 fix for the next requirements release https://github.com/WebKitForWindows/WebKitRequirements/pull/52