Skip to content

Change HttpSys default client cert mode to Allow Cert #23162

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

Merged
merged 1 commit into from
Jun 19, 2020
Merged

Change HttpSys default client cert mode to Allow Cert #23162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/Servers/HttpSys/src/FeatureContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,18 @@ async Task<X509Certificate2> ITlsConnectionFeature.GetClientCertificateAsync(Can
{
if (IsNotInitialized(Fields.ClientCertificate))
{
_clientCert = await Request.GetClientCertificateAsync(cancellationToken);
var method = _requestContext.Server.Options.ClientCertificateMethod;
if (method != ClientCertificateMethod.NoCertificate)
{
// Check if a cert was already available on the connection.
_clientCert = Request.ClientCertificate;
}

if (_clientCert == null && method == ClientCertificateMethod.AllowRenegotation)
{
_clientCert = await Request.GetClientCertificateAsync(cancellationToken);
}
Copy link
Member

@halter73 halter73 Jun 19, 2020

Choose a reason for hiding this comment

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

Why did this method have to change if this PR is merely changing the default mode?

Does this mean that ITlsConnectionFeature.GetClientCertificateAsync() was broken before when ClientCertificateMethod.AllowCertificate was set? Would it previously allow renegotiation anyway? Should we backport this part of the change?

Copy link
Member Author

@Tratcher Tratcher Jun 19, 2020

Choose a reason for hiding this comment

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

In the 3.1 fix we were primarily addressing complaints about ClientCertificate triggering IO, and sync-over-async at that. There haven't been complaints about GetClientCertificateAsync, it's rarely used. I'm changing it now more for consistency and efficiency.


SetInitialized(Fields.ClientCertificate);
}
return _clientCert;
Expand Down
4 changes: 2 additions & 2 deletions src/Servers/HttpSys/src/HttpSysOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,11 @@ public string RequestQueueName
public RequestQueueMode RequestQueueMode { get; set; }

/// <summary>
/// Indicates how client certificates should be populated. The default is to allow renegotation.
/// Indicates how client certificates should be populated. The default is to allow a certificate without renegotiation.
/// This does not change the netsh 'clientcertnegotiation' binding option which will need to be enabled for
/// ClientCertificateMethod.AllowCertificate to resolve a certificate.
/// </summary>
public ClientCertificateMethod ClientCertificateMethod { get; set; } = ClientCertificateMethod.AllowRenegotation;
public ClientCertificateMethod ClientCertificateMethod { get; set; } = ClientCertificateMethod.AllowCertificate;

/// <summary>
/// The maximum number of concurrent accepts.
Expand Down