Added RejectCertificateUriMismatch property and explicit ApplicationUri validation after certificates are loaded #3244
Conversation
…ri validation after certificates are loaded
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3244 +/- ##
==========================================
+ Coverage 57.53% 57.77% +0.23%
==========================================
Files 361 362 +1
Lines 79152 79289 +137
Branches 13814 13850 +36
==========================================
+ Hits 45543 45807 +264
+ Misses 29382 29258 -124
+ Partials 4227 4224 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| { | ||
| if (certId.Certificate != null) | ||
| { | ||
| string certApplicationUri = X509Utils.GetApplicationUriFromCertificate(certId.Certificate); |
There was a problem hiding this comment.
IfI remember correctly a cert can have multiple application uri, but we always checked / extracted only the first (long known simplification). This needs to be fixed before introducing this check. Best to fix it in all places. i think in Martins repo is a Prototype die this
There was a problem hiding this comment.
The spec mentions one ApplicationURI and multiple dNSNames if the machine has multiple names.
There was a problem hiding this comment.
This Shows Part of the Implementation we maybe need: mregen#351
I will check next week
There was a problem hiding this comment.
I see what you mean you are referring to the following method:
public static IReadOnlyList 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>();
}
`
But here there is a bit of a confusion in my opinion since while X509SubjectAltNameExtension does indeed provide the Uris property, it also provides the DomainNames property.
What adds to the fuzzy interpretation is that the comment explicitly says
" // extract the alternate domains from the subject alternate name extension."
and returns the Uris property.
In my view (which might be wrong) if I follow the spec there is a single ApplicationURI which is
a globaly unique indentifier for an OPC UA Application and the application instance certificate has the ApplicationURI in the subjectAltNameField
The spec sentence "Additional dNSNames may be specified if the machine has multiple names." points to the DomainNames property of the X509SubjectAltNameExtension.
As a simple example of my interpretation, if the application has an ApplicationUri urn:MyCompany:ServerInstanceA but can be reached via servera.local and 192.168.1.12, the app certificate contains the single ApplicationURI but lists both network addresses and clients can connect successfully using either name or address as long as the "connection path" is found in the certificate.
I have to admit my interpretation is "simple" and based on spec only and cannot find any references to multiple ApplicationUri's in the SAN from the spec point of view, since the RFC 5280 allows multiple SAN URI entries but it is more generic than the spec. The spec does mention that the subjectAltName field is completly described in RFC 5280, so this gives leave to interpretation further more.
There was a problem hiding this comment.
We cannot assume the first URI is the ApplicationURI. I would propose to remove this validation step and only validate against config
| AddAppCertToTrustedStore = true; | ||
| SendCertificateChain = true; | ||
| SuppressNonceValidationErrors = false; | ||
| RejectCertificateUriMismatch = false; |
There was a problem hiding this comment.
I would propose to set this to true and do a Version increase to 1.5.378 we should always have the Default config be the most secure
There was a problem hiding this comment.
My understanding was that replacing the ApplicationUri with the one from the certificate on the clients was convenient "feature" and most users are and have been using it involuntarily for a long time and this should stay so until it is willingly disabled.
| /// The applicationUri parameter is only used if thumbprint and subjectName don't find a match. | ||
| /// </summary> | ||
| /// <exception cref="ServiceResultException"></exception> | ||
| private async Task<bool> CheckCertificateTypeAsync( |
There was a problem hiding this comment.
I propose to rename to CheckCertificateAsync as more thant the type is checked here
| } | ||
| else if (!configuration.ApplicationUri.Equals(applicationUri, StringComparison.Ordinal)) | ||
| { | ||
| if (configuration.SecurityConfiguration.RejectCertificateUriMismatch) |
There was a problem hiding this comment.
I propose to make this check mandatory, as with ECC we can have multiple Certificates with different ApplicationUris, we need to be able to verifiy against the config reliably.
| if (!Utils.IsSupportedCertificateType(newCertificates[i].CertificateType)) | ||
| { | ||
| m_applicationCertificates.RemoveAt(i); | ||
| newCertificates.RemoveAt(i); |
There was a problem hiding this comment.
We should log an Error here
| for (int j = newCertificates.Count - 1; j > i; j--) | ||
| { | ||
| if (m_applicationCertificates[i] | ||
| .CertificateType == m_applicationCertificates[j].CertificateType) |
There was a problem hiding this comment.
wouldnt this change make more sense in the other PR? Or why was this changed here?
2 participants
Added RejectCertificateUriMismatch property and explicit ApplicationUri validation after certificates are loaded
Proposed changes
Describe the changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.
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...