-
Notifications
You must be signed in to change notification settings - Fork 523
Implement client certificate authentication #385
Changes from all commits
bd30f28
bed8c67
8e910ba
ba63c89
85eb6ab
592d802
2cdd659
efec0fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
namespace Microsoft.AspNet.Server.Kestrel.Https | ||
{ | ||
public enum ClientCertificateMode | ||
{ | ||
NoCertificate, | ||
AllowCertificate, | ||
RequireCertificate | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,29 +3,36 @@ | |
|
||
using System; | ||
using System.Net.Security; | ||
using System.Security.Authentication; | ||
using System.Security.Cryptography.X509Certificates; | ||
using System.Threading.Tasks; | ||
using Microsoft.AspNet.Http.Features; | ||
using Microsoft.AspNet.Http.Features.Internal; | ||
using Microsoft.AspNet.Server.Kestrel.Filter; | ||
|
||
namespace Microsoft.AspNet.Server.Kestrel.Https | ||
{ | ||
public class HttpsConnectionFilter : IConnectionFilter | ||
{ | ||
private readonly X509Certificate2 _cert; | ||
private readonly HttpsConnectionFilterOptions _options; | ||
private readonly IConnectionFilter _previous; | ||
|
||
public HttpsConnectionFilter(X509Certificate2 cert, IConnectionFilter previous) | ||
public HttpsConnectionFilter(HttpsConnectionFilterOptions options, IConnectionFilter previous) | ||
{ | ||
if (cert == null) | ||
if (options == null) | ||
{ | ||
throw new ArgumentNullException(nameof(cert)); | ||
throw new ArgumentNullException(nameof(options)); | ||
} | ||
if (previous == null) | ||
{ | ||
throw new ArgumentNullException(nameof(previous)); | ||
} | ||
if (options.ServerCertificate == null) | ||
{ | ||
throw new ArgumentException("The server certificate parameter is required."); | ||
} | ||
|
||
_cert = cert; | ||
_options = options; | ||
_previous = previous; | ||
} | ||
|
||
|
@@ -35,8 +42,71 @@ public async Task OnConnection(ConnectionFilterContext context) | |
|
||
if (string.Equals(context.Address.Scheme, "https", StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
var sslStream = new SslStream(context.Connection); | ||
await sslStream.AuthenticateAsServerAsync(_cert); | ||
X509Certificate2 clientCertificate = null; | ||
SslStream sslStream; | ||
if (_options.ClientCertificateMode == ClientCertificateMode.NoCertificate) | ||
{ | ||
sslStream = new SslStream(context.Connection); | ||
await sslStream.AuthenticateAsServerAsync(_options.ServerCertificate, clientCertificateRequired: false, | ||
enabledSslProtocols: _options.SslProtocols, checkCertificateRevocation: _options.CheckCertificateRevocation); | ||
} | ||
else | ||
{ | ||
sslStream = new SslStream(context.Connection, leaveInnerStreamOpen: false, | ||
userCertificateValidationCallback: (sender, certificate, chain, sslPolicyErrors) => | ||
{ | ||
if (certificate == null) | ||
{ | ||
return _options.ClientCertificateMode != ClientCertificateMode.RequireCertificate; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: extra empty line |
||
if (_options.ClientCertificateValidation == null) | ||
{ | ||
if (sslPolicyErrors != SslPolicyErrors.None) | ||
{ | ||
return false; | ||
} | ||
} | ||
|
||
X509Certificate2 certificate2 = certificate as X509Certificate2; | ||
if (certificate2 == null) | ||
{ | ||
#if DOTNET5_4 | ||
// conversion X509Certificate to X509Certificate2 not supported | ||
// https://github.com/dotnet/corefx/issues/4510 | ||
return false; | ||
#else | ||
certificate2 = new X509Certificate2(certificate); | ||
#endif | ||
} | ||
|
||
if (_options.ClientCertificateValidation != null) | ||
{ | ||
if (!_options.ClientCertificateValidation(certificate2, chain, sslPolicyErrors)) | ||
{ | ||
return false; | ||
} | ||
} | ||
|
||
clientCertificate = certificate2; | ||
return true; | ||
}); | ||
await sslStream.AuthenticateAsServerAsync(_options.ServerCertificate, clientCertificateRequired: true, | ||
enabledSslProtocols: _options.SslProtocols, checkCertificateRevocation: _options.CheckCertificateRevocation); | ||
} | ||
|
||
var previousPrepareRequest = context.PrepareRequest; | ||
context.PrepareRequest = features => | ||
{ | ||
previousPrepareRequest?.Invoke(features); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also set IHttpRequestFeature.Scheme = "https" |
||
if (clientCertificate != null) | ||
{ | ||
features.Set<ITlsConnectionFeature>(new TlsConnectionFeature { ClientCertificate = clientCertificate }); | ||
} | ||
|
||
features.Get<IHttpRequestFeature>().Scheme = "https"; | ||
}; | ||
context.Connection = sslStream; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.Net.Security; | ||
using System.Security.Authentication; | ||
using System.Security.Cryptography.X509Certificates; | ||
|
||
namespace Microsoft.AspNet.Server.Kestrel.Https | ||
{ | ||
public class HttpsConnectionFilterOptions | ||
{ | ||
public HttpsConnectionFilterOptions() | ||
{ | ||
ClientCertificateMode = ClientCertificateMode.NoCertificate; | ||
SslProtocols = SslProtocols.Tls12 | SslProtocols.Tls11 | SslProtocols.Tls; | ||
} | ||
|
||
public X509Certificate2 ServerCertificate { get; set; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not required for this PR, but I think this eventually needs to support different certs for different ports. Something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll also need to think about ALPN if/when SslStream ever supports it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't think its happening in a rush? https://github.com/dotnet/corefx/issues/2928 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Woops, I meant SNI, but yes, same dependency. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Tratcher Depending on how much flexibility you want to allow, each option could be based on the ServerAddress.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wow. At that point it might make more sense to start a separate instance for each port/config. |
||
public ClientCertificateMode ClientCertificateMode { get; set; } | ||
public Func<X509Certificate2, X509Chain, SslPolicyErrors, bool> ClientCertificateValidation { get; set; } | ||
public SslProtocols SslProtocols { get; set; } | ||
public bool CheckCertificateRevocation { get; set; } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,16 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.IO; | ||
using Microsoft.AspNet.Http.Features; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ? |
||
|
||
namespace Microsoft.AspNet.Server.Kestrel.Filter | ||
{ | ||
public class ConnectionFilterContext | ||
{ | ||
public ServerAddress Address { get; set; } | ||
public Stream Connection { get; set; } | ||
public Stream Connection { get; set; } | ||
public Action<IFeatureCollection> PrepareRequest { get; set; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At first I wasn't sure, but now I think I'm sold on making PrepareRequest an action over adding a state object to the context. |
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I remove this in favor of the HttpsConnectionFilterOptions overload?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet. This will be the most common configuration.