Skip to content

Support getting client certificate in Http/3 #35016

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
wtgodbe merged 11 commits into from
Aug 5, 2021
Merged

Support getting client certificate in Http/3 #35016

wtgodbe merged 11 commits into from
Aug 5, 2021

Conversation

@wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Aug 3, 2021 •
edited
Loading

Fixes #34756

@ghost ghost added the area-runtime label Aug 3, 2021
@wtgodbe
Copy link
Member Author

wtgodbe commented Aug 3, 2021

@Tratcher @JamesNK do you have suggestions/examples of how we test GetClientCertificateAsync elsewhere?

@JamesNK
Copy link
Member

JamesNK commented Aug 3, 2021

Add a test to Transports.Quic.Tests in connection tests file. Create a client connection with a client certificate specified, accept connection on the server, get feature from server connection, assert cert. Also create a stream on the connection and get feature plus assert again.

@Tratcher
Copy link
Member

Tratcher commented Aug 4, 2021

@Tratcher @JamesNK do you have suggestions/examples of how we test GetClientCertificateAsync elsewhere?

https://github.com/dotnet/aspnetcore/blob/main/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs#L457-L490

wtgodbe
wtgodbe commented Aug 4, 2021
View reviewed changes
src/Servers/Kestrel/Transport.Quic/test/QuicConnectionContextTests.cs Outdated

var serverConnection = await connectionListener.AcceptAndAddFeatureAsync().DefaultTimeout();

/*var tlsFeature = serverConnection.Features.Get<ITlsConnectionFeature>();
Copy link
Member Author

@wtgodbe wtgodbe Aug 4, 2021

Choose a reason for hiding this comment

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

@JamesNK the ITlsConnectionFeature and IProtocolErrorCodeFeature are not available on the serverConnection, only the serverStream. These tests also only try to get the IProtocolErrorCodeFeature from the stream - is that expected?

aspnetcore/src/Servers/Kestrel/Transport.Quic/test/QuicConnectionContextTests.cs

Line 335 in 709038b

serverStream.Features.Get<IProtocolErrorCodeFeature>().Error = (long)Http3ErrorCode.RequestRejected;

Copy link
Member

@JamesNK JamesNK Aug 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

ITlsConnectionFeature and IProtocolErrorCodeFeature should be available on the connection.

aspnetcore/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicConnectionContext.FeatureCollection.cs

Lines 29 to 33 in 45222c1

private void InitializeFeatures()
{
_currentIProtocolErrorCodeFeature = this;
_currentITlsConnectionFeature = this;
}

Have you debugged into what happens when Get is called?

Copy link
Member

@JamesNK JamesNK Aug 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If these features are not available on the connection, why does this test code work?

aspnetcore/src/Servers/Kestrel/Transport.Quic/test/QuicConnectionContextTests.cs

Line 335 in 709038b

serverStream.Features.Get<IProtocolErrorCodeFeature>().Error = (long)Http3ErrorCode.RequestRejected;

Copy link
Member Author

@wtgodbe wtgodbe Aug 4, 2021

Choose a reason for hiding this comment

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

That's getting the feature from the stream - the code above at line 524 does not have either ITlsConnectionFeature or IProtocolErrorCodeFeature available in the featureCollection of the connection.

Copy link
Member Author

@wtgodbe wtgodbe Aug 4, 2021

Choose a reason for hiding this comment

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

Have you debugged into what happens when Get is called?

This is weird - under the debugger, I get the feature and it has the ClientCertificate set, and the test passes. But when run normally, the test fails at line 526 at Assert.NotNull(tlsFeature.ClientCertificate)...

@wtgodbe wtgodbe marked this pull request as ready for review August 4, 2021 17:35
@wtgodbe wtgodbe changed the title Implement GetClientCertAsync, ApplicationProtocol Support getting client certificate in Http/3 Aug 4, 2021
This was referenced Aug 4, 2021
Open
Open
@Tratcher
Copy link
Member

Tratcher commented Aug 4, 2021 •
edited
Loading

Should we also add an end-to-end test here?
https://github.com/dotnet/aspnetcore/blob/4339b6436e7b5251c27a1cb337aebecd95205ab3/src/Servers/Kestrel/test/Interop.FunctionalTests/Http3/Http3RequestTests.cs

** Edit ** Neverminded, I'll probably have to add that later when we enable passing ClientCertificateMethod through to quic.

@JamesNK JamesNK force-pushed the wtgodbe/ClientCert branch from 4339b64 to 17f4ca1 Compare August 4, 2021 23:10
@JamesNK
Copy link
Member

JamesNK commented Aug 4, 2021

Updated the test and got it to pass. Also, I changed it to test a server stream that is accepted from the client - this is a request stream - rather than create one to the client.

Also, rebased.

@wtgodbe
Copy link
Member Author

wtgodbe commented Aug 4, 2021

Updated the test and got it to pass.

What was I doing wrong before?

JamesNK
JamesNK previously approved these changes Aug 4, 2021
View reviewed changes
@JamesNK
Copy link
Member

JamesNK commented Aug 4, 2021

Updated the test and got it to pass.

What was I doing wrong before?

I'm not sure. Might have been the rebase. I improved how features are resolved between streams and the connection in it.

@JamesNK JamesNK dismissed their stale review August 5, 2021 00:50

Linux

JamesNK
JamesNK approved these changes Aug 5, 2021
View reviewed changes
Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

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

@wtgodbe I forgot that QUIC only supports client certs on Windows at the moment. Could you place an OS condition on the test?

@wtgodbe
Copy link
Member Author

wtgodbe commented Aug 5, 2021

I forgot that QUIC only supports client certs on Windows at the moment. Could you place an OS condition on the test?

Done

@wtgodbe wtgodbe merged commit e5a0b2a into main Aug 5, 2021
@wtgodbe wtgodbe deleted the wtgodbe/ClientCert branch August 5, 2021 18:11
@ghost ghost added this to the 6.0-rc1 milestone 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 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidfowl davidfowl davidfowl left review comments

@Tratcher Tratcher Tratcher left review comments

@halter73 halter73 halter73 approved these changes

@JamesNK JamesNK JamesNK approved these changes

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

+1 more reviewer

@Kahbazi Kahbazi Kahbazi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Milestone

6.0-rc1

Development

Successfully merging this pull request may close these issues.

HTTP/3: Support getting client certificate

8 participants

@wtgodbe @JamesNK @Tratcher @halter73 @davidfowl @Kahbazi @amcasey