-
Notifications
You must be signed in to change notification settings - Fork 923
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
GDS: Trust own CA & add revoked Certificates to CRL #2333
Conversation
…rtificategroup - makes it possible for the user to change the implementation of CertificateGroup - all tests still passing
…pplications get the updated CA
…licationInsance fixes issue 2327
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.
nice work, thanks!
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2333 +/- ##
==========================================
- Coverage 53.82% 51.83% -2.00%
==========================================
Files 331 313 -18
Lines 63952 61358 -2594
Branches 13120 12623 -497
==========================================
- Hits 34420 31802 -2618
- Misses 25803 26053 +250
+ Partials 3729 3503 -226 ☔ View full report in Codecov by Sentry. |
@mregen I pushed the commits without a valid git email, do I have to recreate this pull-request, or can we somehow get around this? |
@romanett can you try to directly add a dummy commit from github? e.g. add/remove some blanks. this may pick up you as a verified user? |
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.
I'd approve if the comment is resolved.
lgtm, nice work! I could have merged without the latest sync to master. |
Hi @romanett , due to flaky builds I'm unable to merge. Could you merge your GDS PRs with master to get the updated ci build definition? Hopefully I can merge then... |
Proposed changes
With this Pull Request i improve the reusability of the Code by changing reverences to CertificateGroup to the Interface ICertificateGroup, through this change users can extend the functionality of CertificateGroup by providing an own implementation.
Furthermore I fix two issues of the GDS:
The GDS does not trust its own CA Certificate, therefore it rejects connection attempts from clients with a CA signed certificate (e.g. after a sucessful registration & a new CA signed Application certificate received -> Client tries to connect with the new certificate to get the TrustList -> gets rejected)
-> new Method AddOwnCertificateToTrustedStoreAsync added to Class ApplicationInstance
-> you can call this after starting the GDS Server with CA-Certificate of the GDS as parameter
The GDS does not revoke Certificate in the TrustedList Store CRL, so OPC UA Applications can´t get revoked certs
-> this is fixed by adding a call to UpdateAuthorityCertInTrustedList to the method RevokeCertificateAsync
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...