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.

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

Parsing basicConstraints extension without CA tag but with pathLength specified. #18

Merged

Conversation

mchldbrtl
Copy link
Contributor

I found myself in a situation where an OPCUA server is offering me a certificate with the basicConstraints extension specifying pathLength but not CA field. This is probably due to a malformed certificate but it is still correctly parsed by popular tools like openssl etc etc.
The client then fails because the function readBasicConstraint2_5_29_19 expects the CA field to be encountered first and the pathLength second, possibly.
Honestly, the literature and the real world examples are often misleading about the correct structure of this extension.
I don't know if my suggestion is fully compliant with the specs but I hope it should provide a less stringent and more accepted approach for parsing basicConstraints extensions.

@CLAassistant
Copy link

CLAassistant commented Aug 23, 2023

CLA assistant check
All committers have signed the CLA.

@mchldbrtl mchldbrtl force-pushed the patch-readBasicConstraint2_5_29_19 branch from 9422f2e to 927a5eb Compare August 23, 2023 09:14
@mchldbrtl mchldbrtl marked this pull request as ready for review August 23, 2023 09:34
@erossignon
Copy link
Member

erossignon commented Aug 23, 2023

Hi mchldbrtl thank you for bringing this enhancement.

Please make sure to add a unit test that specifically stresses the code that you have added.

This will be a prerequisite before the PR could be accepted.

@mchldbrtl mchldbrtl force-pushed the patch-readBasicConstraint2_5_29_19 branch from 98f95f8 to eab74ef Compare September 4, 2023 10:01
@mchldbrtl mchldbrtl force-pushed the patch-readBasicConstraint2_5_29_19 branch from eab74ef to cf5ccd1 Compare September 4, 2023 10:31
@erossignon
Copy link
Member

Thank you!
Is this ready to merge ?

@mchldbrtl
Copy link
Contributor Author

mchldbrtl commented Sep 4, 2023

Hi! It is ready indeed. Still, the appveyor CI fails in the very early stage with error:

The build phase is set to "MSBuild" mode (default), but no Visual Studio project or solution files were found in the root directory. If you are not building Visual Studio project switch build mode to "Script" and provide your custom build command.

Are you aware of the failures? I saw that happens for other jobs too. Thanks!

@erossignon erossignon merged commit c62228d into node-opcua:master Sep 5, 2023
26 of 27 checks passed
@mchldbrtl mchldbrtl deleted the patch-readBasicConstraint2_5_29_19 branch September 6, 2023 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants