Enable mutual tls on server https endpoints #2849
Conversation
| @@ -307,7 +307,8 @@ public static EndpointDescription SelectEndpoint( | |||
| public static Uri GetDiscoveryUrl(string discoveryUrl) | |||
| { | |||
| // needs to add the '/discovery' back onto non-UA TCP URLs. | |||
| if (discoveryUrl.StartsWith(Utils.UriSchemeHttp, StringComparison.Ordinal)) | |||
| if (discoveryUrl.StartsWith(Utils.UriSchemeHttp, StringComparison.Ordinal) || | |||
There was a problem hiding this comment.
introcude a new helper method in Utils to find all http/https/opc.https ?
| @@ -779,6 +779,13 @@ public IApplicationConfigurationBuilderServerOptions SetAuditingEnabled(bool aud | |||
| return this; | |||
| } | |||
|
|
|||
| /// <inheritdoc/> | |||
| public IApplicationConfigurationBuilderServerOptions SetHttpsMutualTls(bool mTlsEnabled) | |||
There was a problem hiding this comment.
use consistent naming for mutualTls (e.g. httpMutualTlsEnabled)
| @@ -537,6 +642,7 @@ private static async Task<byte[]> ReadBodyAsync(HttpRequest req) | |||
| private IWebHost m_host; | |||
| private X509Certificate2 m_serverCertificate; | |||
| private X509Certificate2Collection m_serverCertificateChain; | |||
| private bool m_ClientCertificateMode; | |||
There was a problem hiding this comment.
rename to mutualTlsEnabeld?
| @@ -1509,6 +1509,7 @@ private void Initialize() | |||
| m_maxTrustListSize = 0; | |||
| m_multicastDnsEnabled = false; | |||
| m_auditingEnabled = false; | |||
| m_httpsMTls = true; | |||
There was a problem hiding this comment.
use consistent naming eg. m_httpMutualTls ?
| @@ -158,11 +159,27 @@ public void Open() | |||
| // send client certificate for servers that require TLS client authentication | |||
| if (m_settings.ClientCertificate != null) | |||
| { | |||
| // prepare the server TLS certificate | |||
| var clientCertificate = m_settings.ClientCertificate; | |||
| #if NETCOREAPP3_1_OR_GREATER || NETSTANDARD2_1 || NET472_OR_GREATER || NET5_0_OR_GREATER | |||
There was a problem hiding this comment.
what is the reason for this guard clause?
| /// <param name="chain"></param> | ||
| /// <param name="sslPolicyErrors"></param> | ||
| /// <returns></returns> | ||
| private bool ValidateClientCertificate( |
| /// </summary> | ||
| /// <param name="context">Context of the validation</param> | ||
| /// <param name="ct">Continuation token</param> | ||
| /// <returns>true if validation passes, elst false</returns> |
There was a problem hiding this comment.
nit: Continuation token -> cancellation token
elst -> else
| string path = context.Request.Path.Value?.TrimEnd('/') ?? string.Empty; | ||
| string discoveryPath = m_uri.AbsolutePath?.TrimEnd('/') + ConfiguredEndpoint.DiscoverySuffix; | ||
|
|
||
| bool isDiscoveryPath = discoveryPath.EndsWith(path, StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
ins´t it enough to thest if the request path ends with the DiscoverySuffix? this would avoid allocations, else comparsion could be done using span?
There was a problem hiding this comment.
Better ensure that request path matches the server's "configured/intended" discovery endpoint exactly (even if currently a discovery path would also match any request path ending with DiscoverySuffix later in the code)
EthanChangAED
requested review from
EWolfAtGit
and removed request for
ThomasNehring
Proposed changes
Minor https related fixes and https endpoints now require mutual TLS for establishing a TLS connection.
Related Issues
Types of changes
What types of changes does your code introduce?
Put an
xin the boxes that apply. You can also fill these out after creating the PR.Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...