Check server application Uri with the create session response #2731
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2731 +/- ##
==========================================
- Coverage 55.01% 54.73% -0.29%
==========================================
Files 349 349
Lines 65872 66565 +693
Branches 13492 13691 +199
==========================================
+ Hits 36242 36434 +192
- Misses 25777 26215 +438
- Partials 3853 3916 +63 ☔ View full report in Codecov by Sentry. |
| return alternateName.Uris; | ||
| } | ||
|
|
||
| return new List<string>(); |
There was a problem hiding this comment.
return an empty string to be backward compatible?
|
Added Mantis for clarifactionof spec: https://mantis.opcfoundation.org/view.php?id=9795 |
| @@ -163,7 +163,7 @@ public Registration( | |||
| EventHandler<ConnectionWaitingEventArgs> onConnectionWaiting) : | |||
| this(endpointUrl, onConnectionWaiting) | |||
| { | |||
| ServerUri = X509Utils.GetApplicationUriFromCertificate(serverCertificate); | |||
| ServerUri = X509Utils.GetApplicationUrisFromCertificate(serverCertificate).FirstOrDefault(); | |||
There was a problem hiding this comment.
Need logic to pick the right one.
| noMatch = false; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Why not noMatch = certificateApplicationUris .Where(x => String.Equals(certificateApplicationUri, applicationUri, StringComparison.Ordinal).Any()
There was a problem hiding this comment.
you never know what Linq is up to...
| @@ -672,7 +672,7 @@ private async Task<bool> CheckApplicationInstanceCertificateAsync( | |||
| } | |||
|
|
|||
| // check uri. | |||
| string applicationUri = X509Utils.GetApplicationUriFromCertificate(certificate); | |||
| string applicationUri = X509Utils.GetApplicationUrisFromCertificate(certificate).FirstOrDefault(); | |||
|
|
|||
There was a problem hiding this comment.
Need a better way to pick the right one.
| @@ -213,7 +214,7 @@ public string ApplicationUri | |||
| { | |||
| try | |||
| { | |||
| return X509Utils.GetApplicationUriFromCertificate(Certificate); | |||
| return X509Utils.GetApplicationUrisFromCertificate(Certificate).FirstOrDefault(); | |||
There was a problem hiding this comment.
Need a better way to pick the right one.
|
@BoBiene @ilyanikolaeff @GregoireG-C please check if this implementation would work for you. |
In my case I get error
This solution doesn't work for me. |
|
Thanks @ilyanikolaeff, sounds like we still need a bypass flag.. |
Error same. I can provide StackTrace if you need. I'm using WinCC OA 3.11 Opc Ua server with custom certificate (Basic128Rsa15). Server returns ApplicationUri - PVSS, Certificate ApplicationUri - WinCC OA. I can't change settings or certificate because server works in production - my application just client that susbcribes to nodes changes. Btw, I'll tried manually change P.S. I'm using UaExpert as test client (latest version). It's signal error about mismatch ApplicationUri, but allow ignore it. |
|
Hi @ilyanikolaeff, I looked into integration in the validation loop but I think we run then into other issues, e.g. multiple callbacks. For the next release I would just disable the check as we need some time to come up with a better solution. Thanks for the insights! |
Proposed changes
Address two issues:
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...