Allow non OPC-UA conform SubjectName values to AddSecurityConfigurationStores#3229
Conversation
Added Test for the special case.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3229 +/- ##
==========================================
+ Coverage 57.59% 57.83% +0.24%
==========================================
Files 361 365 +4
Lines 79152 79506 +354
Branches 13814 13900 +86
==========================================
+ Hits 45587 45983 +396
- Misses 29336 29356 +20
+ Partials 4229 4167 -62 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where certificates with non-OPC-UA conform SubjectName values (simple strings without CN and O attributes) could cause matching problems when creating multiple ApplicationInstance certificates. The fix ensures proper certificate matching and selection even when SubjectName formats are non-standard.
- Improved certificate matching logic to handle both CN-prefixed and non-CN SubjectNames differently
- Enhanced certificate selection to prioritize CA-signed certificates and use duration-based selection for duplicates
- Added default O attribute ("OPC Foundation") when creating certificates without organization information
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| CertificateIdentifier.cs | Enhanced Find method with better matching logic and longest duration selection algorithm |
| CertificateFactory.cs | Added default O attribute injection for certificates missing organization info |
| CertificateStoreTest.cs | Added comprehensive test coverage for certificate finding scenarios |
| ApplicationInstanceTests.cs | Added test for handling SubjectName substring matching edge cases |
| CertificateValidatorTest.cs | Updated test assertions to expect O attribute in certificate subjects |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
…possible cases of prioritization depending on cert validity
|
@romanett I have added comments and tried to clarify the picking algorithm. During this I reached 6 criteria of selection. I am sure this is not exhaustive and better criteria can be added but I think the fewer the better. |
Proposed changes
When a certificate SubjectName is provided in a non OPC-UA accepted format (simple-string with no CN and O attributes) as input to
AddSecurityConfigurationStores(), in the context of anApplicationInstancecertificate creation, the created certificate'sSubjectNameis populated with a default CN=simple-string and omits the O attribute (which it should also include).A subsequent
public static X509Certificate2 Find()call, in the context of an additionalApplicationInstancecertificate creation, taking as input aSubjectNamethat is a substring of the previously created certificate with CN=simple-string, would match the previously created application certificate and throw due to a call toX509Certificate2Collection.Find(X509FindType.FindBySubjectName, subjectName, false)that seams to perform a FuzzySearch on the collection.This PR attempts to remedy the above miss-behavior even in this special case were the provided SubjectName is not in the OPC-UA accepted format and does not contain the CN and O attributes.
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...