-
Notifications
You must be signed in to change notification settings - Fork 1k
Refactor string usage for secrets #3250
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
Conversation
Strings are immutable and the lifetime of secrets can not be controlled. Use byte[] or char[] instead of strings for secrets, which can be erased after use.
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.
Pull Request Overview
This pull request refactors string usage for secrets in the OPC UA stack to improve security by preventing secrets from being exposed in memory as immutable strings. The changes replace string parameters and properties for passwords and other sensitive data with byte arrays or char arrays, which can be cleared after use.
Key Changes:
- Refactored user identity tokens and password handling to use byte arrays instead of strings
- Updated certificate password handling throughout the codebase to use char arrays
- Modified user database implementations to work with UTF-8 byte arrays for passwords
- Added proper memory clearing for sensitive data after use
Reviewed Changes
Copilot reviewed 69 out of 70 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| UserIdentity.cs | Refactored to use byte arrays for passwords and implement IDisposable |
| UserNameIdentityToken.cs | Created separate file with byte array password handling |
| IssuedIdentityToken.cs | Extracted to separate file with secure token data handling |
| LinqUserDatabase.cs | Updated to use byte arrays for password operations |
| Various certificate classes | Changed password parameters from string to char[] or ReadOnlySpan |
| Test files | Updated to use new UTF-8 string literals and byte array APIs |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Libraries/Opc.Ua.Server/RoleBasedUserManagement/UserDatabase/LinqUserDatabase.cs
Show resolved
Hide resolved
Libraries/Opc.Ua.Server/RoleBasedUserManagement/UserDatabase/JsonUserDatabase.cs
Show resolved
Hide resolved
Libraries/Opc.Ua.Server/RoleBasedUserManagement/UserDatabase/IUserDatabase.cs
Show resolved
Hide resolved
mregen
left a comment
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.
👍🏼
Libraries/Opc.Ua.Security.Certificates/Org.BouncyCastle/PEMReader.cs
Outdated
Show resolved
Hide resolved
| /// <exception cref="CryptographicException"></exception> | ||
| public static ECDsa ImportECDsaPrivateKeyFromPEM(byte[] pemDataBlob, string password = null) | ||
| public static ECDsa ImportECDsaPrivateKeyFromPEM( | ||
| byte[] pemDataBlob, |
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.
also use span here for pemDataBlob?
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.
Lets handle passwords first. The blob is hopefully password protected. But we should look next at clearing any type of secret info, including PEM/DER/PFX. Let's make that a follow up.
| /// <summary> | ||
| /// An interface to open a certificate store. | ||
| /// </summary> | ||
| public interface IOpenStore |
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.
Maybe we should name it ICertificateStoreOpener?
Strings are immutable and the lifetime of secrets can not be controlled. Use byte[] or char[] instead of strings for secrets, which can be erased after use.
Ported as cherry pick with additional changes and fixes.
Proposed changes
Describe the changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.
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...