Skip to content
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

[release/6.0] add support for parsing Unified TLS hello #68425

Merged
merged 3 commits into from
May 4, 2022

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Apr 22, 2022

Customer Impact

This may prevent TLS handshake from some older clients. While the basic parsing should work, the certificate selection callback is not invoked and it breaks scenarios like Kestrel and YARP that depend on it.

This currently impacts Azure App service and their YARP setup.

Testing

Private binaries were verified by AppService. We currently don't have test coverage for this as it depends on particular client device (some IoT in this particular case)

Risk

small. This essentially adds ability to process some new variations but does not change or replace old code.

fixes #68310

Details

This covers cases when client sends TLS 1.x hello in Sslv2 frame format. As described in the linked issue, server will respond with normal response and TLS 1.x will be negotiated.

The TLS Parser is essentially dup of dotnet/yarp#1656

Both changes were verified by App Service.

Note that main will need more work because of #64322.
We may need to bring back some fragments but I would like to do more testing before that.

@wfurt wfurt requested a review from a team April 22, 2022 22:48
@wfurt wfurt self-assigned this Apr 22, 2022
@ghost
Copy link

ghost commented Apr 22, 2022

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

fixes #68310

This covers cases when client sends TLS 1.x hello in Sslv2 frame format. As described in the linked issue, server will respond with normal response and TLS 1.x will be negotiated.

The TLS Parser is essentially dup of dotnet/yarp#1656

Both changes were verified by App Service.

Note that main will need more work because of #64322.
We may need to bring back some fragments but I would like to do more testing before that.

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Security

Milestone: -

@wfurt
Copy link
Member Author

wfurt commented Apr 23, 2022

test failures should be fixed by #68332.

Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wfurt wfurt added the Servicing-consider Issue for next servicing release review label Apr 25, 2022
@karelz karelz added this to the 6.0.x milestone Apr 26, 2022
@karelz karelz requested a review from stephentoub April 26, 2022 18:41
@wfurt
Copy link
Member Author

wfurt commented Apr 27, 2022

The should be ready for another pass @stephentoub . I use more constant when possible and I removed the side check as unnecessary and possibly problematic.

@danmoseley danmoseley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 28, 2022
@danmoseley danmoseley modified the milestones: 6.0.x, 6.0.6 Apr 28, 2022
@danmoseley
Copy link
Member

approved -- once signeoff/green.

@carlossanlop
Copy link
Member

@wfurt @rzikm I merged the test PR #68332

I'll re-run the CI.

@carlossanlop
Copy link
Member

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wfurt
Copy link
Member Author

wfurt commented May 4, 2022

thanks @carlossanlop. It seems only System.IO is failing on Mac.

@carlossanlop carlossanlop merged commit ac796cb into dotnet:release/6.0 May 4, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants