-
Notifications
You must be signed in to change notification settings - Fork 1k
Allow non OPC-UA conform SubjectName values to AddSecurityConfigurationStores #3229
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
base: master
Are you sure you want to change the base?
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.87% +0.27%
==========================================
Files 361 365 +4
Lines 79152 79506 +354
Branches 13814 13900 +86
==========================================
+ Hits 45587 46011 +424
+ Misses 29336 29323 -13
+ Partials 4229 4172 -57 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Stack/Opc.Ua.Core/Security/Certificates/CertificateIdentifier.cs
Outdated
Show resolved
Hide resolved
Stack/Opc.Ua.Core/Security/Certificates/CertificateIdentifier.cs
Outdated
Show resolved
Hide resolved
Stack/Opc.Ua.Core/Security/Certificates/CertificateIdentifier.cs
Outdated
Show resolved
Hide resolved
Stack/Opc.Ua.Core/Security/Certificates/CertificateIdentifier.cs
Outdated
Show resolved
Hide resolved
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.
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.
Stack/Opc.Ua.Core/Security/Certificates/CertificateIdentifier.cs
Outdated
Show resolved
Hide resolved
Stack/Opc.Ua.Core/Security/Certificates/CertificateIdentifier.cs
Outdated
Show resolved
Hide resolved
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 anApplicationInstance
certificate creation, the created certificate'sSubjectName
is 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 additionalApplicationInstance
certificate creation, taking as input aSubjectName
that 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
x
in the boxes that apply. You can also fill these out after creating the PR.Checklist
Put an
x
in 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...