Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix handling of Subject Names with specials Characters / and = using " as escaping character #2591

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Fix handling of Subject Names with specials Characters / and = using " as escaping character #2591

wants to merge 3 commits into from

Conversation

romanett
Copy link
Contributor

Proposed changes

As per OPC UA Spec a Subject Name can be provided to methods with a special escaping sequence:
If the value contains a ‘/’ or a ‘=’ then it shall be enclosed in double quotes (‘”’).

https://reference.opcfoundation.org/GDS/v105/docs/7.8.3

This fix removes this special escaping sequence when parsing a subject Name.

Related Issues

Types of changes

What types of changes does your code introduce?

  • Bugfix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which adds functionality)
  • Test enhancement (non-breaking change to increase test coverage)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected, requires version increase of Nuget packages)
  • Documentation Update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc.
  • I have signed the CLA.
  • I ran tests locally with my changes, all passed.
  • I fixed all failing tests in the CI pipelines.
  • I fixed all introduced issues with CodeQL and LGTM.
  • I have added tests that prove my fix is effective or that my feature works and increased code coverage.
  • I have added necessary documentation (if appropriate).
  • Any dependent changes have been merged and published in downstream modules.

Further comments

@romanett romanett linked an issue Apr 20, 2024 that may be closed by this pull request
GDS Server (Client) FinishRequest fails for special SubjectName #2586
Open
5 tasks
@codecov Codecov
Copy link

codecov bot commented Apr 20, 2024 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.74%. Comparing base (884ddf3) to head (c7c80db).

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2591      +/-   ##
==========================================
+ Coverage   54.70%   54.74%   +0.03%     
==========================================
  Files         342      342              
  Lines       65041    65042       +1     
  Branches    13331    13331              
==========================================
+ Hits        35581    35605      +24     
+ Misses      25612    25595      -17     
+ Partials     3848     3842       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@romanett romanett self-assigned this Apr 20, 2024
@romanett romanett requested a review from mregen April 20, 2024 15:15
@KircMax
Copy link
Contributor

KircMax commented Apr 20, 2024

Thanks for looking into this!
I am reading the Spec. a bit differently than it is implemented here I think, but maybe I am just wrong...:
"The value may be any printable character except for ‘”’. If the value contains a ‘/’ or a ‘=’ then it shall be enclosed in double quotes (‘”’). "
--> I read it that those strings from your test are actually to be interpreted as invalid strings since they contain a '"' ...
I'd expect the whole string to be enclosed ->
not: "CN=UA Yellow Green Server,DC=dogblueberry,O=OPC"="Foundation"
but instead: "CN=UA Yellow Green Server,DC=dogblueberry,O="OPC=Foundation""
So only the value of O= would be enclosed by quotes ... But as I said I might be reading it wrong, just wanted to hint this.

@romanett
Copy link
Contributor Author

@KircMax Thanks for the hint. However for your case we seem to handle it correctly already w.o. the patch. Can you provide the complete subject name contained in the Certificate Request created by your server? You can read it out e.g. with Bounce Castle:

var pkcs10CertificationRequest = new Org.BouncyCastle.Pkcs.Pkcs10CertificationRequest(certificateRequestByteArray);
var info = pkcs10CertificationRequest.GetCertificationRequestInfo();
var Subjectname = info.Subject;

@romanett romanett marked this pull request as draft April 26, 2024 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mregen mregen Awaiting requested review from mregen

At least 1 approving review is required to merge this pull request.

Assignees

@romanett romanett

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GDS Server (Client) FinishRequest fails for special SubjectName
2 participants
@romanett @KircMax