Skip to content

fix(client-certificates): errors during http2 TLS handshake #32258

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

mxschmitt
Copy link
Member

@mxschmitt mxschmitt commented Aug 22, 2024

Investigation: Currently when a TLS handshake error happens on a http2 connection, the response gets corrupted. This is because:

  1. The client (browser, internalTLS) sends a http2 handshake to our proxy
  2. The target connection emits a secureConnect event
  3. We attach listeners since we received secureConnect event and send the buffered "client hello" data over
  4. The target connection emits an error event
  5. We try to send a http2 response over but the client (browser) already received some parts of the handshake and is confused. This ends up in Error: Received bad client magic byte string and net::ERR_HTTP2_PROTOCOL_ERROR.

This brought up that using the session event seems to be the right one for our use-case.

Relates playwright-community/playwright-go#480
Example error: https://github.com/playwright-community/playwright-go/actions/runs/10465991368/job/28982096640?pr=480#step:6:166

@mxschmitt mxschmitt force-pushed the fix-client-certificates-http2-errors branch from 6facfb2 to 4e3f764 Compare August 22, 2024 07:37
@mxschmitt mxschmitt marked this pull request as ready for review August 22, 2024 07:38
@mxschmitt mxschmitt force-pushed the fix-client-certificates-http2-errors branch 2 times, most recently from 9f6a6f1 to 65c02fc Compare August 22, 2024 07:39

This comment has been minimized.

This comment has been minimized.

@mxschmitt mxschmitt force-pushed the fix-client-certificates-http2-errors branch 2 times, most recently from 41649aa to bb68403 Compare August 22, 2024 12:00

This comment has been minimized.

This comment has been minimized.

@mxschmitt mxschmitt force-pushed the fix-client-certificates-http2-errors branch from bb68403 to 459966e Compare August 22, 2024 12:52

This comment has been minimized.

@mxschmitt mxschmitt merged commit 16e76cb into microsoft:main Aug 22, 2024
27 of 29 checks passed
Copy link
Contributor

Test results for "tests 1"

3 failed
❌ [firefox-page] › page/page-goto.spec.ts:182:3 › should properly cancel Cross-Origin-Opener-Policy navigation
❌ [webkit-library] › library/modernizr.spec.ts:33:3 › safari-14-1
❌ [webkit-library] › library/modernizr.spec.ts:83:3 › mobile-safari-14-1

2 flaky ⚠️ [firefox-page] › page/frame-goto.spec.ts:46:3 › should continue after client redirect
⚠️ [playwright-test] › ui-mode-test-progress.spec.ts:22:5 › should update trace live

30093 passed, 870 skipped
✔️✔️✔️

Merge workflow run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants