[SOUP] WebSocketTask: add client authentication support

[ThomasDevoogdt]

7d61be0

[SOUP] WebSocketTask: add client authentication support

https://bugs.webkit.org/show_bug.cgi?id=247608

Reviewed by Carlos Garcia Campos.

Since version WebKitGTK version 2.2, developers can register a
authenticate callback to the WebView. This gives a WebKitAuthenticationRequest
which gives a chance to provide certificates with webkit_authentication_request_authenticate.
While developing, it turned out that the websocket implementation does not handle
client certificates. The implemention in this commit is partial based on NetworkDataTaskSoup.

Canonical link: https://commits.webkit.org/256780@main

c10230a

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
✅ 🛠 🧪 unsafe-merge	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

[webkit-early-warning-system]

EWS run on previous version of this PR (hash cbd5bd9)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

[philn]

Can you remove the signed-off-by and fix the coding style errors?

[philn]

Also I'm afraid this needs to be associated with a bugzilla report.

[webkit-early-warning-system]

EWS run on previous version of this PR (hash a38e481)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

[ThomasDevoogdt]

Can you remove the signed-off-by and fix the coding style errors?

About the coding style, it was not about 'style', but about an ASSERT_NOT_REACHED
check. I've removed it since the callback won't be triggered in SOUP2.

Also I'm afraid this needs to be associated with a bugzilla report.

Yes, fixed that.

[mcatanzaro]

Huh, impressive. It's not very common that people who discover a bug manage to fix it. Good job.

[mcatanzaro]

Is it possible to modify testClientSideCertificate in Tools/TestWebKitAPI/Tests/WebKitGLib/TestSSL.cpp to ensure this doesn't break in the future? I'm not sure how hard it would be to do that, but without a test we'll break it sooner or late.

[ThomasDevoogdt]

Is it possible to modify testClientSideCertificate in Tools/TestWebKitAPI/Tests/WebKitGLib/TestSSL.cpp to ensure this doesn't break in the future? I'm not sure how hard it would be to do that, but without a test we'll break it sooner or late.

Yes, a combination of testClientSideCertificate and testWebSocketTLSErrors. Will need some time to learn how to execute and debug these tests. Do you prefer this in this PR also?

[mcatanzaro]

Yes, a combination of testClientSideCertificate and testWebSocketTLSErrors. Will need some time to learn how to execute and debug these tests. Do you prefer this in this PR also?

Yes please. Writing tests is usually the hardest part of the change in my experience, but sounds like you have a plan....

[webkit-early-warning-system]

EWS run on previous version of this PR (hash f142b66)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	🧪 gtk-wk2
	⏳ 🧪 api-ios	✅ 🧪 api-mac	🧪 api-gtk
	✅ 🛠 tv	⏳ 🧪 mac-wk1
	✅ 🛠 tv-sim	🧪 mac-wk2
	✅ 🛠 watch	⏳ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

[webkit-early-warning-system]

EWS run on previous version of this PR (hash a8c301f)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	🧪 ios-wk2	✅ 🛠 mac-AS-debug	🧪 gtk-wk2
	🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	⏳ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	⏳ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 96cbffa)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	⏳ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

[ThomasDevoogdt]

Yes please. Writing tests is usually the hardest part of the change in my experience, but sounds like you have a plan....

I spend some time getting a test working. But I'm not able to reproduce the exact same behaviors that I was solving with this PR in the first place.

  /webkit/WebKitWebView/client-side-certificate:                      PASS
undefined:1:14: CONSOLE JS ERROR TypeError: undefined is not an object (evaluating 'window.webkit.messageHandlers')
Mesa: The provided filesystem timestamp for the cache is bogus! Disabling On-disk cache.
undefined:1:14: CONSOLE JS ERROR TypeError: undefined is not an object (evaluating 'window.webkit.messageHandlers')
undefined:1:14: CONSOLE JS ERROR TypeError: undefined is not an object (evaluating 'window.webkit.messageHandlers')
undefined:1:14: CONSOLE JS ERROR TypeError: undefined is not an object (evaluating 'window.webkit.messageHandlers')
undefined:1:14: CONSOLE JS ERROR TypeError: undefined is not an object (evaluating 'window.webkit.messageHandlers')
  /webkit/WebKitWebView/web-socket-client-side-certificate:           TIMEOUT

It's clear that the test is stuck in g_main_loop_run(m_mainLoop), I was also able to pinpoint what exact line causes the timeout while developing. But no success yet. I also don't know how much time I still can spend on this topic. Is it really required to be part of this PR?

[mcatanzaro]

It's clear that the test is stuck in g_main_loop_run(m_mainLoop), I was also able to pinpoint what exact line causes the timeout while developing. But no success yet. I also don't know how much time I still can spend on this topic. Is it really required to be part of this PR?

It's much better if we have a test. Maybe you could push your test in a WIP commit so others can take a look? Your call to g_main_loop_run() should be paired with a call to g_main_loop_quit() somewhere.

[ThomasDevoogdt]

It's much better if we have a test. Maybe you could push your test in a WIP commit so others can take a look? Your call to g_main_loop_run() should be paired with a call to g_main_loop_quit() somewhere.

This is what I have: 13a124ddbfe072a8c7fb73ff76941589e3ccd49a which can be found on main...ThomasDevoogdt:WebKit:feature/soup-websocket-client-cert-test-wip.

[ThomasDevoogdt]

I'm not able to find it myself. I even don't know if the libsoup3 server (WebKitTestServer) has support for WebSocket client authentication. I do test this against a propriety Kotlin server. I hope that this PR still gets a chance to be accepted without additional tests.

[webkit-early-warning-system]

EWS run on previous version of this PR (hash a8b4c4e)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	🧪 api-ios	✅ 🧪 api-mac	❌ 🧪 api-gtk
	✅ 🛠 tv	🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 8450c03)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

[carlosgcampos]

Why did the test fail? the client certificate is requested during the connection and before the protocol upgrade libsoup handles the connection the same way.

[mcatanzaro]

Why did the test fail? the client certificate is requested during the connection and before the protocol upgrade libsoup handles the connection the same way.

See 13a124d#r89401896. It looks pretty close to working. Pretty sure the JS is just failing to call the message handler.

If it's too hard to write a test, then I think it's OK to land without a test, on the understanding that it will inevitably break if there is no test... but you seem really close. ;)

[carlosgcampos]

I'll check the test to see if I understand why it's failing.

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 6f948c0)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	❌ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	❌ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	🧪 ios-wk2	✅ 🛠 mac-AS-debug	❌ 🧪 gtk-wk2
	🧪 api-ios	⏳ 🧪 api-mac	❌ 🧪 api-gtk
	✅ 🛠 tv	⏳ 🧪 mac-wk1
	✅ 🛠 tv-sim	⏳ 🧪 mac-wk2
	✅ 🛠 watch	🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 43c205b)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	❌ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 6087d3a)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

[webkit-early-warning-system]

EWS run on current version of this PR (hash c10230a)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
✅ 🛠 🧪 unsafe-merge	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

[mcatanzaro]

It's disappointing to see you give up on the test so quickly. ;) Let's wait for Carlos to check out what's going wrong with the JS. He might need a few days to get to it.

[carlosgcampos]

Yes, I haven't forgotten it, I just need to find the time.

[ThomasDevoogdt]

Sorry, It's not that I didn't try to find it, already spend quite some time on it, but I also have to devide my time properly. I will probably come back to it, but not immediately. If besides there are still remarks related to the commit itself, let me know, will for sure adapt it until everything is fine. Anyway, also thx four your time, I do appreciate it.

[mcatanzaro]

The code changes look good to me!

I didn't mean to shame you for failure to complete the test, but rather to suggest that you put it back even though it doesn't work quite right yet, so that Carlos Garcia doesn't have to dig through your older commits to find it.

[carlosgcampos]

The patch is good, I managed to make a test, I'll open a new PR for the test.

[webkit-commit-queue]

Committed 256780@main (7d61be0): https://commits.webkit.org/256780@main

Reviewed commits have been landed. Closing PR #6245 and removing active labels.

[mcatanzaro]

Thanks for fixing this!