Skip to content

Check SNI in h3 #10184

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.

Sign up for GitHub

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

Jump to bottom
Merged
maskit merged 1 commit into from
Aug 14, 2023
Merged

Check SNI in h3 #10184

maskit merged 1 commit into from
Aug 14, 2023

Conversation

@lzx404243
Copy link
Collaborator

@lzx404243 lzx404243 commented Aug 14, 2023

This closes #10089.

@lzx404243 lzx404243 added this to the 10.0.0 milestone Aug 14, 2023
@lzx404243 lzx404243 self-assigned this Aug 14, 2023
@lzx404243
Copy link
Collaborator Author

lzx404243 commented Aug 14, 2023 •
edited
Loading

@maskit What do you think about placing the SNI check in Http3SessionAccept::accept()? On one hand, it makes sense as this is right before the start of a H3 session and once we know that client didn't send a SNI we can reject it and don't need to start the session at all. The downside for this is we can't send a H3 response from here. We just rejects it.

@lzx404243 lzx404243 requested a review from maskit August 14, 2023 18:03
@maskit
Copy link
Member

maskit commented Aug 14, 2023 •
edited
Loading

Sounds reasonable and I'm fine with it, however, I wonder how other implementations behave.

Also, I should have known this but what happens after ATS rejects a connection at accept()?

@lzx404243
Copy link
Collaborator Author

lzx404243 commented Aug 14, 2023

what happens after ATS rejects a connection at accept()?

netvc->do_io_close() will be called which supposedly closes the quic connection.

However, I noticed in the Autest that the curl/proxy verifier client times out. And doing a packet capture on the rejection case, I don't see ATS closes the quic connection(no CONNECTION_CLOSE frame etc). It seems there is some silent drop going on here.

@maskit
Copy link
Member

maskit commented Aug 14, 2023

Ok, I think this PR is fine. I don't think I implemented a way to initiate connection close on H3 layer.

@maskit maskit merged commit 335054e into apache:master Aug 14, 2023
cmcfarlen pushed a commit to cmcfarlen/trafficserver that referenced this pull request Jun 3, 2024
@zwoop
Merge remote-tracking branch 'asf/master'
2ab0df2 
* asf/master: (221 commits)
  LSan: Fix leak of test_Metrics (apache#10179)
  LSan: Fix memory leak of test_EventSystem (apache#10178)
  LSan: Fix memory leak of test_X509HostnameValidator (apache#10161)
  Remove cqtx log field (apache#10111)
  Require ATS plugins to be compiled with C++17. (apache#10007)
  Fix conf_remap plugin build on macOS (apache#10177)
  libswoc: Update to 1.5.4 (apache#10155)
  Makes cmake build again, on macOS (apache#10172)
  Check SNI in h3 (apache#10184)
  Remove autoconf headers during CMake configuration (apache#10173)
  test_QUICLossDetector.cc: Add back get_hrtime() (apache#10185)
  ink_ink_get_hrtime -> ink_get_hrtime (apache#10182)
  mgmt: make libconfigmanager a true static library (apache#10181)
  Make sure that the thread local time is updated timely (apache#10163)
  Unrequire remap rules for OCSP (apache#10146)
  cache_range test performance improvement (apache#10170)
  Clean up certifier plugin debug messages. (apache#9975)
  cmake: add check for clock_gettime (apache#10169)
  Remove Http3NoError allocations (apache#10165)
  Fix Throttler initialization. (apache#10154)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maskit maskit maskit approved these changes

Assignees

@lzx404243 lzx404243

Labels

HTTP/3

Projects

None yet

Milestone

10.0.0

Development

Successfully merging this pull request may close these issues.

Check SNI extension is used on H3

2 participants

@lzx404243 @maskit