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

HttpSys ClientCertificate property renegotiates #33586

Closed
Tratcher opened this issue Jun 16, 2021 · 1 comment · Fixed by #34012
Closed

HttpSys ClientCertificate property renegotiates #33586

Tratcher opened this issue Jun 16, 2021 · 1 comment · Fixed by #34012
Assignees
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions bug This issue describes a behavior which is not expected - a bug. feature-httpsys help wanted Up for grabs. We would accept a PR to help resolve this issue
Milestone

Comments

@Tratcher
Copy link
Member

ITlsConnectionFeature.ClientCertificate is used to get the client certificate for the current connection. ITlsConnectionFeature.GetClientCertificateAsync is used to renegotiate the TLS session to request a certificate if you don't already have one. HttpSys has had a behavior in the past where it triggered the renegotiate even from the ClientCertificate property. ClientCertificateMethod.AllowRenegotation was added to control that, but it also disables it for GetClientCertificateAsync.

When implementing GetClientCertificateAsync renegotiation for Kestrel it became clear that the pattern developers want is for ClientCertificate to return the current certificate, if any, and for GetClientCertificateAsync to renegotiate for a cert if enabled. This allows them to do conditional logic like buffer the request body before starting the renegotiation.

Proposal: Remove the renegotiate logic from the ClientCertificate property and clean up the SetInitialized logic so that GetClientCertificateAsync can still renegotiate after ClientCertificate is called.

else if (method == ClientCertificateMethod.AllowRenegotation)
{
_clientCert = Request.GetClientCertificateAsync().Result; // TODO: Sync over async;
}

@Tratcher Tratcher added bug This issue describes a behavior which is not expected - a bug. area-runtime feature-httpsys labels Jun 16, 2021
@BrennanConroy BrennanConroy added this to the Next sprint planning milestone Jun 16, 2021
@ghost
Copy link

ghost commented Jun 16, 2021

Thanks for contacting us.

We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@BrennanConroy BrennanConroy added the help wanted Up for grabs. We would accept a PR to help resolve this issue label Jun 16, 2021
@Tratcher Tratcher self-assigned this Jul 1, 2021
Tratcher added a commit to Tratcher/aspnetcore that referenced this issue Jul 1, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 5, 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 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions bug This issue describes a behavior which is not expected - a bug. feature-httpsys help wanted Up for grabs. We would accept a PR to help resolve this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants