-
Notifications
You must be signed in to change notification settings - Fork 950
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
Check server application Uri with the create session response #2731
base: master
Are you sure you want to change the base?
Changes from 3 commits
a3386f1
0a65ea4
c6e35ae
d4cb133
b7c787c
7d3ec37
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 |
---|---|---|
|
@@ -2338,7 +2338,6 @@ | |
|
||
if (requireEncryption) | ||
{ | ||
ValidateServerCertificateApplicationUri(serverCertificate); | ||
if (checkDomain) | ||
{ | ||
m_configuration.CertificateValidator.Validate(serverCertificateChain, m_endpoint); | ||
|
@@ -2417,24 +2416,24 @@ | |
if (!successCreateSession) | ||
{ | ||
base.CreateSession( | ||
null, | ||
clientDescription, | ||
m_endpoint.Description.Server.ApplicationUri, | ||
m_endpoint.EndpointUrl.ToString(), | ||
sessionName, | ||
clientNonce, | ||
clientCertificateChainData != null ? clientCertificateChainData : clientCertificateData, | ||
sessionTimeout, | ||
(uint)MessageContext.MaxMessageSize, | ||
out sessionId, | ||
out sessionCookie, | ||
out m_sessionTimeout, | ||
out serverNonce, | ||
out serverCertificateData, | ||
out serverEndpoints, | ||
out serverSoftwareCertificates, | ||
out serverSignature, | ||
out m_maxRequestMessageSize); | ||
null, | ||
clientDescription, | ||
m_endpoint.Description.Server.ApplicationUri, | ||
m_endpoint.EndpointUrl.ToString(), | ||
sessionName, | ||
clientNonce, | ||
clientCertificateChainData != null ? clientCertificateChainData : clientCertificateData, | ||
sessionTimeout, | ||
(uint)MessageContext.MaxMessageSize, | ||
out sessionId, | ||
out sessionCookie, | ||
out m_sessionTimeout, | ||
out serverNonce, | ||
out serverCertificateData, | ||
out serverEndpoints, | ||
out serverSoftwareCertificates, | ||
out serverSignature, | ||
out m_maxRequestMessageSize); | ||
} | ||
|
||
// save session id. | ||
|
@@ -2456,6 +2455,8 @@ | |
|
||
ValidateServerEndpoints(serverEndpoints); | ||
|
||
ValidateServerCertificateApplicationUri(m_endpoint, serverCertificate); | ||
|
||
ValidateServerSignature(serverCertificate, serverSignature, clientCertificateData, clientCertificateChainData, clientNonce); | ||
|
||
HandleSignedSoftwareCertificates(serverSoftwareCertificates); | ||
|
@@ -5352,26 +5353,44 @@ | |
!String.IsNullOrEmpty(identityPolicy.SecurityPolicyUri); | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Validates the ServerCertificate ApplicationUri to match the ApplicationUri of the Endpoint for an open call (Spec Part 4 5.4.1) | ||
/// Validates the ServerCertificate ApplicationUri to match the ApplicationUri | ||
/// of the Endpoint (Spec Part 4 5.4.1) returned by the CreateSessionResponse. | ||
/// Ensure the endpoint was matched in <see cref="ValidateServerEndpoints"/> | ||
/// with the applicationUri of the server description before the validation. | ||
/// </summary> | ||
private void ValidateServerCertificateApplicationUri( | ||
X509Certificate2 serverCertificate) | ||
private static void ValidateServerCertificateApplicationUri(ConfiguredEndpoint endpoint, X509Certificate2 serverCertificate) | ||
{ | ||
var applicationUri = m_endpoint?.Description?.Server?.ApplicationUri; | ||
//check is only neccessary if the ApplicatioUri is specified for the Endpoint | ||
if (string.IsNullOrEmpty(applicationUri)) | ||
if (serverCertificate != null) | ||
{ | ||
throw ServiceResultException.Create( | ||
StatusCodes.BadSecurityChecksFailed, | ||
"No ApplicationUri is specified for the server in the EndpointDescription."); | ||
} | ||
string certificateApplicationUri = X509Utils.GetApplicationUriFromCertificate(serverCertificate); | ||
if (!string.Equals(certificateApplicationUri, applicationUri, StringComparison.Ordinal)) | ||
{ | ||
throw ServiceResultException.Create( | ||
StatusCodes.BadSecurityChecksFailed, | ||
"Server did not return a Certificate matching the ApplicationUri specified in the EndpointDescription."); | ||
var applicationUri = endpoint?.Description?.Server?.ApplicationUri; | ||
|
||
// check that an ApplicatioUri is specified for the Endpoint | ||
if (string.IsNullOrEmpty(applicationUri)) | ||
{ | ||
throw ServiceResultException.Create( | ||
StatusCodes.BadSecurityChecksFailed, | ||
"Server did not return an ApplicationUri in the EndpointDescription."); | ||
} | ||
|
||
bool noMatch = true; | ||
var certificateApplicationUris = X509Utils.GetApplicationUrisFromCertificate(serverCertificate); | ||
foreach (var certificateApplicationUri in certificateApplicationUris) | ||
{ | ||
if (string.Equals(certificateApplicationUri, applicationUri, StringComparison.Ordinal)) | ||
{ | ||
noMatch = false; | ||
break; | ||
} | ||
} | ||
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. Why not noMatch = certificateApplicationUris .Where(x => String.Equals(certificateApplicationUri, applicationUri, StringComparison.Ordinal).Any() 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. you never know what Linq is up to... |
||
|
||
if (noMatch) | ||
{ | ||
throw ServiceResultException.Create( | ||
StatusCodes.BadSecurityChecksFailed, | ||
mregen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"Server did not return the ApplicationUri in the EndpointDescription that is used in the server certificate."); | ||
} | ||
} | ||
} | ||
|
||
|
@@ -5538,7 +5557,7 @@ | |
} | ||
} | ||
|
||
// find the matching description (TBD - check domains against certificate). | ||
// find the matching description (TBD - check domains and application URI against certificate). | ||
bool found = false; | ||
|
||
var foundDescription = FindMatchingDescription(serverEndpoints, m_endpoint.Description, true); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -672,7 +672,7 @@ private async Task<bool> CheckApplicationInstanceCertificateAsync( | |
} | ||
|
||
// check uri. | ||
string applicationUri = X509Utils.GetApplicationUriFromCertificate(certificate); | ||
string applicationUri = X509Utils.GetApplicationUrisFromCertificate(certificate).FirstOrDefault(); | ||
|
||
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. Need a better way to pick the right one. |
||
if (String.IsNullOrEmpty(applicationUri)) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,7 @@ | |
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Runtime.Serialization; | ||
using System.Security.Cryptography.X509Certificates; | ||
using Opc.Ua.Security.Certificates; | ||
|
@@ -213,7 +214,7 @@ | |
{ | ||
try | ||
{ | ||
return X509Utils.GetApplicationUriFromCertificate(Certificate); | ||
return X509Utils.GetApplicationUrisFromCertificate(Certificate).FirstOrDefault(); | ||
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. Need a better way to pick the right one. |
||
} | ||
catch (Exception e) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -126,6 +126,7 @@ | |
/// </summary> | ||
/// <param name="certificate">The certificate.</param> | ||
/// <returns>The application URI.</returns> | ||
[Obsolete("Use GetApplicationUrisFromCertificate instead. The certificate may contain more than one Uri.")] | ||
public static string GetApplicationUriFromCertificate(X509Certificate2 certificate) | ||
{ | ||
// extract the alternate domains from the subject alternate name extension. | ||
|
@@ -140,6 +141,25 @@ | |
return string.Empty; | ||
} | ||
|
||
/// <summary> | ||
/// Extracts the application URIs specified in the certificate. | ||
/// </summary> | ||
/// <param name="certificate">The certificate.</param> | ||
/// <returns>The application URIs.</returns> | ||
public static IReadOnlyList<string> GetApplicationUrisFromCertificate(X509Certificate2 certificate) | ||
{ | ||
// extract the alternate domains from the subject alternate name extension. | ||
X509SubjectAltNameExtension alternateName = X509Extensions.FindExtension<X509SubjectAltNameExtension>(certificate); | ||
|
||
// get the application uris. | ||
if (alternateName != null && alternateName.Uris != null) | ||
{ | ||
return alternateName.Uris; | ||
} | ||
|
||
return new List<string>(); | ||
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. return an empty string to be backward compatible? |
||
} | ||
|
||
/// <summary> | ||
/// Check if certificate has an application urn. | ||
/// </summary> | ||
|
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.
Need logic to pick the right one.