Allow application certificate lookup to use entries configured with abstract ApplicationCertificateType only as fallback candidates.#3752
Conversation
Normalize loaded certificates to their concrete OPC UA certificate type and allow application certificate lookup to use entries configured with abstract ApplicationCertificateType as fallback candidates. The fallback now loads the candidate certificate first, then validates it against the concrete type required by the selected security policy. Also guard IsSupportedCertificateType against null certificate types and add regression coverage for abstract ApplicationCertificateType lookup and normalization.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3752 +/- ##
==========================================
- Coverage 72.11% 72.09% -0.02%
==========================================
Files 572 572
Lines 120749 120793 +44
Branches 20173 20178 +5
==========================================
+ Hits 87073 87089 +16
- Misses 27836 27860 +24
- Partials 5840 5844 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| CertificateType = GetCertificateType(m_certificate); | ||
| } | ||
| } | ||
| set => SetCertificate(value); |
There was a problem hiding this comment.
maybe use c# 14 field keyword and set the Certificate property instead of introducing SetCertificate meth
There was a problem hiding this comment.
Certificate Identifier now is just what it says, an id that is used to look up the certificate at any point in time from the registry. It does not cache the certificate anymore and it is not a store. Therefore this code is not needed anymore. The fix to select a valid certificate type needs to be applied when the Identifier is converted to a certificate.
There was a problem hiding this comment.
TBH the code was prepared on master378 and I moved it on master ... thanks for the highlight
3 participants
Proposed changes
Tries to rectify inconsistent behavior:
SecurityConfiguration::BuilldSupportedSecurityPolicies()advertises RSA policies if ApplicationCertificateType is set as CertificateType, while ApplicationCertificateType is not used when finding actual certificates.Use CertificateIdentifier::SetCertificate(certificate) to recalculate the CertificateType whenever the identifier receives a X509Certificate2.
Removed stale if case
if (certType == ObjectTypeIds.ApplicationCertificateType)fromSecurityConfiguration::FindApplicationCertificateAsync, sinceCertificateIdentifier.MapSecurityPolicyToCertificateTypes(securityPolicy)never returned 'ApplicationCertificateType' and it was confusing and misleading because it suggested support for abstract ApplicationCertificateType, but normal policy lookup did not actually go through it.Allow application certificate lookup to use entries configured with abstract ApplicationCertificateType as fallback candidates.
The fallback loads the candidate certificate first, then validates it against the concrete type required by the selected security policy.
'ApplicationCertificateType' is not a concrete certificate profile and should not be prefered in configuration, even if older configurations may use it. Previously, such entries could be missed during certificate lookup because the selected security policy maps to a concrete type.
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...