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

Removed hardcoded SecurityMode in SecureChannel readChunk method. #780

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ioansiran
Copy link

@ioansiran ioansiran commented Mar 19, 2025

This change removes the hardcoding of SecurityMode in readChunk method.

This hardcoding which caused any Sign Security Mode connection to fail due to the mismatch between given endpoint selection (for example auth-mode: Username, sec-mode: Sign, sec-method: Basic256Sha256).
Since this line overrides the selected security mode, the server returned an EOF and immediately closed connection, regardless of authentication method and certificate validity.

To fix the concern in the previous TODO comment, I've added a check to see if there's no valid provided security method, and set a default in that case, otherwise don't override.

I'd like to add this critical bugfix to the latest version of gopcua as this library is used in production by us at Octotronic

@magiconair
Copy link
Member

Thank you! Could you add a test as well, please? Unit or integration.

…r securityMode is provided

- removed overwrite in readChunk since it will not reach without valid securityMode
@ioansiran
Copy link
Author

Thank you! Could you add a test as well, please? Unit or integration.

Hello. I've changed the approach a bit, and enforced a valid SecurityMode when creating the SecureChannel, and removed the override when reading chunks. This made things easier to test, since we'd want to keep config consistent throughout the lifecycle

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.

None yet

2 participants