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

Support client cert negotation #33264

Merged
merged 19 commits into from
Jun 15, 2021
Merged

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented Jun 3, 2021

Fixes #23948 @avparuch

Developers can now opt-in to using delayed client certificate negotiation via ClientCertificateMode.DelayCertificate on HttpsConnectionAdapterOptions. Note negotiation is always enabled if you're using the UseHttps overload that takes ServerOptionsSelectionCallback since there are no HttpsConnectionAdapterOptions passed there.

  • POST mitigations
    • It looks like small bodies (~under 4kb) received with the initial request work because kestrel buffers it as part of parsing the headers.
    • Larger bodies cause System.InvalidOperationException: Received data during renegotiation.
    • The caller must buffer the request body before requesting the client cert. I've added a sample showing this.

@Tratcher Tratcher requested a review from halter73 June 3, 2021 22:00
@Tratcher Tratcher self-assigned this Jun 3, 2021
@Tratcher Tratcher added api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 3, 2021
@Tratcher Tratcher added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 5, 2021
@Tratcher Tratcher marked this pull request as ready for review June 8, 2021 20:38
@Tratcher Tratcher added this to the 6.0-preview6 milestone Jun 8, 2021
@Tratcher
Copy link
Member Author

Tratcher commented Jun 8, 2021

Ok, that last commit changed things around a bit. I realized that ClientCertificateMode.DelayCertificate didn't work if used in the SNI from config code path. I was able to flow it through the internal code paths as a tuple. Thoughts?

@Tratcher
Copy link
Member Author

Ok, that last commit changed things around a bit. I realized that ClientCertificateMode.DelayCertificate didn't work if used in the SNI from config code path. I was able to flow it through the internal code paths as a tuple. Thoughts?

I recommend refactoring these internals as part of #33452, but it shouldn't change the functionality in this PR.


// Look for TLS connections that don't already have a client cert, and requests that could have a body.
if (tlsFeature != null && tlsFeature.ClientCertificate == null && bodyFeature.CanHaveBody
&& !connectionItems.Items.TryGetValue("tls.clientcert.negotiated", out var _))
Copy link
Member

Choose a reason for hiding this comment

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

Why not?

Suggested change
&& !connectionItems.Items.TryGetValue("tls.clientcert.negotiated", out var _))
&& (tlsFeature is not ClientCertBufferingFeature))

Copy link
Member Author

Choose a reason for hiding this comment

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

ClientCertBufferingFeature gets stored in the HttpContext's FeatureCollection, not the connection's FeatureCollection. It's reverted before the next request. Items persist across requests.

if (ClientCertificate != null
|| ClientCertificateMode != ClientCertificateMode.DelayCertificate
// Delayed client cert negotiation is not allowed on HTTP/2 (or HTTP/3, but that's implemented elsewhere).
|| _sslStream.NegotiatedApplicationProtocol == SslApplicationProtocol.Http2)
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a test to verify we aren't renegotiating when NegotiatedApplicationProtocol is HTTP/2?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in the HTTP/2 HttpClient interop tests. That's one of the only places we test HTTP/2 over TLS.

@Tratcher Tratcher added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 15, 2021
@ghost
Copy link

ghost commented Jun 15, 2021

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@Tratcher
Copy link
Member Author

Awkward, the build passed but it never reported status here:
https://dev.azure.com/dnceng/public/_build/results?buildId=1189136&view=results

@dotnet/aspnet-build would you be so kind as to merge this?

@Tratcher Tratcher disabled auto-merge June 15, 2021 23:51
@wtgodbe
Copy link
Member

wtgodbe commented Jun 15, 2021

Yeah, github reporting has been weird today

@wtgodbe wtgodbe merged commit c28dc4c into dotnet:main Jun 15, 2021
@Tratcher Tratcher deleted the tratcher/clientcert branch June 18, 2021 22:06
@BrennanConroy BrennanConroy added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jun 21, 2021
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kestrel support for "path based" TLS renegotiation.
6 participants