Secure channel enhancements 2025 11 #3408
Conversation
romanett
left a comment
romanett
left a comment
There was a problem hiding this comment.
Moving AES + HMAC Creation from Token into each call into the transport channel does propose a significant performance hit. This can be observed /benchmarked with theses tests / benchmarks: https://github.com/romanett/UA-.NETStandard/blob/master/Tests/Opc.Ua.Client.Tests/SecurityPolicyBenchmarks.cs
Before merging this needs to be adressed.
| public static async Task Main(string[] args) | ||
| { | ||
| //foreach (var group in Enum.GetValues(typeof(RSADiffieHellmanGroup))) | ||
| //{ |
| ManualResetEvent quitEvent = ConsoleUtils.CtrlCHandler(quitCTS); | ||
|
|
||
| // insert security tester. | ||
| var tester = new SecurityTestClient.RunTest(config, telemetry); |
There was a problem hiding this comment.
Put behind a command line argument? And into the UAClient?
| { | ||
| return [.. policies.Where(u => u.TokenType != UserTokenType.UserName)]; | ||
| } | ||
| //if (description.SecurityPolicyUri == SecurityPolicies.Aes256_Sha256_RsaPss && |
There was a problem hiding this comment.
Lets remove if not needed
| = GetSoftwareCertificates(); | ||
| SignedSoftwareCertificateCollection clientSoftwareCertificates = new(); | ||
|
|
||
| // during debugging send the sesson transfer token on all activations. |
| { | ||
| if (!m_disposed) | ||
| { | ||
| if (disposing) |
There was a problem hiding this comment.
Who will be cleaning this up now?
| // don't keep signature if secure channel enhancements are not used. | ||
| m_oscRequestSignature = (SecurityPolicy.SecureChannelEnhancements) ? signature : null; | ||
|
|
||
| CryptoTrace.Start(ConsoleColor.Magenta, $"ProcessOpenSecureChannelRequest ({(State != TcpChannelState.Opening ? "RENEW" : "OPEN")})"); |
There was a problem hiding this comment.
This should use logging infra, e.g. Logger.Trace. If it is for debugging, it should be behind #if DEBUG.
There was a problem hiding this comment.
And it seems to use console, so that needs to change too. But of course, best with m_logger.LogTrace
There was a problem hiding this comment.
This is not trace - this is for debugging security stuff that should never go in a log will be compiled out of production code.
(it already has the #if DEBUG block)
logger not available in low level crypto functions and makes no sense to change multiple levels of APIs to pass in a reference.
There was a problem hiding this comment.
Got it.
IMO it would be great to have a set of unit test for this code that tests all invariants that you assert via the debug output. Because no one else will likely enable it or ensure they show what is right/wrong. Just so we don't break it later.
There was a problem hiding this comment.
Pull request overview
This PR implements new SecurityProfile support for OPC UA 1.05.07, introducing enhanced security features and refactoring the cryptographic utilities. The changes enable secure channel enhancements including channel-bound signatures, session transfer tokens, chained symmetric key derivation, and support for new security policies (RSA-DH with AES-GCM/ChaCha20Poly1305).
Key changes:
- Introduced
SecurityPolicyInfoclass to centralize security policy metadata and algorithms - Added secure channel enhancement support with channel thumbprints and improved signature handling
- Refactored ECC utilities into a more general
CryptoUtilsclass - Extended console reference applications to support unit testing scenarios
Reviewed changes
Copilot reviewed 50 out of 50 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| UA Reference.sln | Updated Visual Studio version metadata |
| Stack/Opc.Ua.Types/Encoders/BinaryDecoder.cs | Reformatted null-coalescing operator for readability |
| Stack/Opc.Ua.Types/Diagnostics/TelemetryUtils.cs | Commented out debug assertion for NullLogger |
| Stack/Opc.Ua.Core/Types/Utils/Utils.cs | Added IPv4/IPv6 address detection in GetHostName |
| Stack/Opc.Ua.Core/Stack/Types/X509IdentityToken.cs | Refactored to use SecurityPolicyInfo for signing/verification |
| Stack/Opc.Ua.Core/Stack/Types/UserNameIdentityToken.cs | Updated encryption logic to use SecurityPolicyInfo |
| Stack/Opc.Ua.Core/Stack/Transport/NullChannel.cs | Added channel certificate properties |
| Stack/Opc.Ua.Core/Stack/Transport/ITransportChannel.cs | Added channel thumbprint and certificate properties to interface |
| Stack/Opc.Ua.Core/Stack/Tcp/UaSCBinaryTransportChannel.cs | Implemented channel certificate properties |
| Stack/Opc.Ua.Core/Stack/Tcp/UaSCBinaryClientChannel.cs | Added secure channel enhancement support with signature tracking |
| Stack/Opc.Ua.Core/Stack/Tcp/UaSCBinaryChannel.cs | Added channel certificate properties and crypto tracing |
| Stack/Opc.Ua.Core/Stack/Tcp/UaSCBinaryChannel.Symmetric.cs | Refactored symmetric encryption to use SecurityPolicyInfo |
| Stack/Opc.Ua.Core/Stack/Tcp/UaSCBinaryChannel.Asymmetric.cs | Refactored asymmetric operations using SecurityPolicyInfo |
| Stack/Opc.Ua.Core/Stack/Tcp/TcpServerChannel.cs | Enhanced OpenSecureChannel handling with signatures |
| Stack/Opc.Ua.Core/Stack/Tcp/ChannelToken.cs | Simplified token structure, removed crypto objects |
| Stack/Opc.Ua.Core/Stack/Server/SecureChannelContext.cs | Added channel certificate fields |
| Stack/Opc.Ua.Core/Security/Constants/SecurityPolicyInfo.cs | New comprehensive security policy metadata class |
| Stack/Opc.Ua.Core/Security/Constants/SecurityPolicies.cs | Added new security policies and info lookup methods |
| Stack/Opc.Ua.Core/Security/Constants/AdditionalParameterNames.cs | New constants for ephemeral key parameters |
| Stack/Opc.Ua.Core/Security/Certificates/Nonce.cs | Extended with RSA-DH support and HKDF key derivation |
| Stack/Opc.Ua.Core/Security/Certificates/EncryptedSecret.cs | Moved from EccUtils with enhanced encryption support |
| Libraries/Opc.Ua.Server/Session/Session.cs | Updated signature verification to use channel enhancements |
| Libraries/Opc.Ua.Server/Server/StandardServer.cs | Implemented channel-bound signature creation |
| Applications/ConsoleReferenceServer/Program.cs | Modified default logging settings |
Comments suppressed due to low confidence (9)
Stack/Opc.Ua.Core/Stack/Tcp/UaSCBinaryTransportChannel.cs:1
- A GUID is generated but never used. This variable should be removed as it serves no purpose.
Stack/Opc.Ua.Core/Stack/Tcp/UaSCBinaryChannel.Symmetric.cs:1 - Excessive trailing whitespace on this line should be removed.
Stack/Opc.Ua.Core/Stack/Tcp/UaSCBinaryChannel.Asymmetric.cs:1 - Unnecessary semicolon after return statement should be removed.
Stack/Opc.Ua.Core/Stack/Transport/ITransportChannel.cs:1 - Corrected spelling of 'establsih' to 'establish'
Stack/Opc.Ua.Core/Stack/Transport/ITransportChannel.cs:1 - Corrected spelling of 'establsih' to 'establish'
Stack/Opc.Ua.Core/Stack/Server/SecureChannelContext.cs:1 - Corrected spelling of 'establsih' to 'establish'
Stack/Opc.Ua.Core/Stack/Server/SecureChannelContext.cs:1 - Corrected spelling of 'establsih' to 'establish'
Stack/Opc.Ua.Core/Stack/Server/SecureChannelContext.cs:1 - Corrected spelling of 'establsih' to 'establish'
Stack/Opc.Ua.Core/Stack/Server/SecureChannelContext.cs:1 - Corrected spelling of 'establsih' to 'establish'
| public const string ECC_brainpoolP384r1_ChaChaPoly = ECC_brainpoolP384r1 + "_ChaChaPoly"; | ||
|
|
||
| /// <summary> | ||
| /// The URI for the ECC_curve25519 security policy.brainpoolP384r1_AesGcm |
There was a problem hiding this comment.
The documentation comment contains an erroneous suffix 'brainpoolP384r1_AesGcm' that appears to be copy-paste error. The comment should only describe ECC_curve25519.
| /// The URI for the ECC_curve25519 security policy.brainpoolP384r1_AesGcm | |
| /// The URI for the ECC_curve25519 security policy. |
| var trucated = new byte[m_nonceLength]; | ||
| Array.Copy(bytes, 0, trucated, 0, m_nonceLength); | ||
| bytes = trucated; |
There was a problem hiding this comment.
Corrected spelling of 'trucated' to 'truncated'
| var trucated = new byte[m_nonceLength]; | |
| Array.Copy(bytes, 0, trucated, 0, m_nonceLength); | |
| bytes = trucated; | |
| var truncated = new byte[m_nonceLength]; | |
| Array.Copy(bytes, 0, truncated, 0, m_nonceLength); | |
| bytes = truncated; |
| // create server nonce. | ||
| var serverNonceObject = Nonce.CreateNonce( | ||
| context.ChannelContext.EndpointDescription.SecurityPolicyUri); | ||
| var serverNonceObject = Nonce.CreateNonce(0); |
There was a problem hiding this comment.
Creating a nonce with length 0 is incorrect. The CreateNonce method expects a minimum nonce length (s_minNonceLength is 32). This will result in a 32-byte nonce instead of 0 bytes, which may not be the intended behavior. Should use the security policy's SecureChannelNonceLength instead.
| var serverNonceObject = Nonce.CreateNonce(0); | |
| var serverNonceLength = context.ChannelContext.EndpointDescription.SecurityPolicyUri.SecureChannelNonceLength; | |
| var serverNonceObject = Nonce.CreateNonce(serverNonceLength); |
| /// </summary> | ||
| /// <returns>A new ephemeral key</returns> | ||
| EphemeralKeyType GetNewEccKey(); | ||
| EphemeralKeyType GetNewEphmeralKey(); |
There was a problem hiding this comment.
Corrected spelling of 'Ephmeeral' to 'Ephemeral' in method name
| EphemeralKeyType GetNewEphmeralKey(); | |
| EphemeralKeyType GetNewEphemeralKey(); |
| /// </summary> | ||
| /// <returns>A new ephemeral key</returns> | ||
| public virtual EphemeralKeyType GetNewEccKey() | ||
| public virtual EphemeralKeyType GetNewEphmeralKey() |
There was a problem hiding this comment.
Corrected spelling of 'Ephmeeral' to 'Ephemeral' in method name
| public virtual EphemeralKeyType GetNewEphmeralKey() | |
| public virtual EphemeralKeyType GetNewEphemeralKey() |
| session.SetEccUserTokenSecurityPolicy(policyUri); | ||
| EphemeralKeyType key = session.GetNewEccKey(); | ||
| session.SetUserTokenSecurityPolicy(policyUri); | ||
| EphemeralKeyType key = session.GetNewEphmeralKey(); |
There was a problem hiding this comment.
Corrected spelling of 'Ephmeeral' to 'Ephemeral' in method name
| EphemeralKeyType key = session.GetNewEphmeralKey(); | |
| EphemeralKeyType key = session.GetNewEphemeralKey(); |
| Value = new ExtensionObject(key) | ||
| }); | ||
|
|
||
| m_logger.LogWarning("Returning new EphmeralKey: {PublicKey}.", CryptoTrace.KeyToString(key.PublicKey)); |
There was a problem hiding this comment.
Corrected spelling of 'EphmeralKey' to 'EphemeralKey'
| Value = StatusCodes.BadSecurityPolicyRejected | ||
| }); | ||
|
|
||
| m_logger.LogWarning("Rejecting request for new EphmeralKey using {SecurityPolicyUri}.", policyUri); |
There was a problem hiding this comment.
Corrected spelling of 'EphmeralKey' to 'EphemeralKey'
| AdditionalParametersType response = null; | ||
|
|
||
| EphemeralKeyType key = session.GetNewEccKey(); | ||
| EphemeralKeyType key = session.GetNewEphmeralKey(); |
There was a problem hiding this comment.
Corrected spelling of 'Ephmeeral' to 'Ephemeral' in method name
| .Add(new KeyValuePair { Key = "ECDHKey", Value = new ExtensionObject(key) }); | ||
| .Add(new KeyValuePair { Key = AdditionalParameterNames.ECDHKey, Value = new ExtensionObject(key) }); | ||
|
|
||
| m_logger.LogWarning("Returning new EphmeralKey: {PublicKey}.", CryptoTrace.KeyToString(key.PublicKey)); |
There was a problem hiding this comment.
Corrected spelling of 'EphmeralKey' to 'EphemeralKey'
Proposed changes
New SecurityProfile implementations for 1.05.07.
ConsoleReferenceClient and ConsoleReferenceServer have been extended to be unit tests.
Need to discuss how to create a proper unit test.