mirror of
https://github.com/OPCFoundation/UA-.NETStandard.git
synced 2026-10-17 05:11:13 +08:00
Fix reverse-connect certificate leak and stale-connection retry (#4386)
Two related robustness fixes on the reverse-connect / UA-SC handshake path, both surfaced by the Sessions suite. ## 1. Certificate handle leak in the asymmetric header parse `UaSCBinaryChannel.ReadAsymmetricMessageHeader` parses the sender certificate chain (`Utils.ParseCertificateChainBlob`, allocating fresh `Certificate` handles) and then runs several validations that can throw — invalid/missing sender thumbprint, no matching receiver certificate, receiver thumbprint mismatch, missing receiver thumbprint. The chain is returned through an `out` parameter, so a throw never reaches the caller's `using (senderCertificateChain)` in `ReadAsymmetricMessageAsync`, and the freshly parsed chain is abandoned. For a self-signed leaf that is exactly the intermittent `created == disposed + 1` seen on the Release leak gate under coverage instrumentation. This is the third sibling of the two concurrent-reconnect ownership leaks closed in #4356. **Fix:** wrap the post-allocation region in a `try/catch` that disposes the chain before rethrowing (validation logic unchanged). **Test:** `ReadAsymmetricMessageHeaderDisposesSenderChainWhenReceiverThumbprintMismatches` crafts a header with a valid sender certificate and a mismatched receiver thumbprint; without the fix it reproduces `created=3, disposed=2` and trips the assembly leak gate, with the fix it passes. ## 2. Stale reverse-connection retry A reverse connection the `ReverseConnectManager` delivers can already have been closed by the server before the secure-channel handshake runs: a server rotates and times out the outbound connections it offers, so the one matched here — especially after an `updateBeforeConnect` discovery round consumed an earlier one — may be stale. That surfaced as an intermittent `BadConnectionClosed` ("Remote side closed the connection") failing the whole connect (e.g. `ReverseConnect2Async(true,true,…)`). **Fix:** wrap reverse-connect session creation in `DefaultSessionFactory` in a bounded retry (3 attempts) that requests a freshly delivered connection when an attempt fails with a connection-drop status (`BadConnectionClosed` / `BadNotConnected` / `BadSecureChannelClosed`). Non-transient failures and cancellation still surface immediately; the bound prevents an unbounded loop against a genuinely unreachable server. ## 3. Test-host crash: NULL channel map in the inactivity timer `TcpTransportListener.DetectInactiveChannels` (a fire-and-forget inactivity-timer callback) iterated `m_channels` without holding `m_lock`, while `CloseAsync` disposes the timer and nulls `m_channels` under the lock. Disposing an `ITimer` does not join an already-dispatched callback, so a late callback dereferenced the now-null map and threw an unhandled `NullReferenceException` on a thread-pool thread, crashing the process. This intermittently aborted the `test-macOS-latest-Client` host. **Fix:** snapshot `m_channels` and return early when null (mirroring the existing guarded read on the accept path). Deterministic regression test invokes the callback on a listener whose channel map is null (its state before `OpenAsync` / after `CloseAsync`) and asserts it does not throw. ## Verification - `Opc.Ua.Core.Tests` Stack.Transport/Client/Server: 2178 passed, leak gate green. - Coverage-instrumented `Opc.Ua.Sessions.Tests` (the CI leg): 775 passed, 0 failed, leak gate green. - Reverse-connect suite (`ReverseConnect*`): 52 passed with the retry. - `Opc.Ua.Core` and `Opc.Ua.Client` build warning-free (net10.0, net48, netstandard2.1, net472). Note: the reverse-connect failure is a rare timing flake (0 reproductions in 88 local attempts), so fix #2 addresses the root cause identified from the ADO log and is validated by construction plus regression rather than by a captured local repro. ## Related - Follows #4356 (concurrent channel reconnect certificate leaks). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Marc <marcschier@hotmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -30,6 +30,7 @@
|
||||
using System;
|
||||
using System.Threading;
|
||||
using System.Threading.Tasks;
|
||||
using Microsoft.Extensions.Logging;
|
||||
using Opc.Ua.Security.Certificates;
|
||||
|
||||
namespace Opc.Ua.Client
|
||||
@@ -235,19 +236,99 @@ namespace Opc.Ua.Client
|
||||
}
|
||||
} while (connection == null);
|
||||
|
||||
return await CreateAsync(
|
||||
configuration,
|
||||
// A reverse connection the manager delivers can already have been
|
||||
// closed by the server before the secure-channel handshake runs: a
|
||||
// server rotates and times out the outbound connections it offers,
|
||||
// so the one matched here - especially after a discovery round
|
||||
// (updateBeforeConnect) consumed an earlier one - may be stale. That
|
||||
// surfaces as BadConnectionClosed / BadNotConnected on first use.
|
||||
// Rather than failing the whole connect, request a freshly delivered
|
||||
// connection and retry a bounded number of times.
|
||||
return await CreateReverseConnectSessionWithRetryAsync(
|
||||
connection,
|
||||
endpoint,
|
||||
false,
|
||||
checkDomain,
|
||||
sessionName,
|
||||
sessionTimeout,
|
||||
userIdentity,
|
||||
preferredLocales,
|
||||
(resolved, token) => CreateAsync(
|
||||
configuration,
|
||||
resolved,
|
||||
endpoint,
|
||||
false,
|
||||
checkDomain,
|
||||
sessionName,
|
||||
sessionTimeout,
|
||||
userIdentity,
|
||||
preferredLocales,
|
||||
token),
|
||||
token => reverseConnectManager.WaitForConnectionAsync(
|
||||
endpoint.EndpointUrl!,
|
||||
endpoint.ReverseConnect?.ServerUri,
|
||||
token),
|
||||
kMaxReverseConnectAttempts,
|
||||
Telemetry?.CreateLogger<DefaultSessionFactory>(),
|
||||
endpoint.EndpointUrl,
|
||||
ct).ConfigureAwait(false);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Creates a session from a reverse connection, retrying with a freshly
|
||||
/// delivered connection when an attempt fails because the delivered
|
||||
/// connection was already closed/dropped by the server. Bounded by
|
||||
/// <paramref name="maxAttempts"/> so a genuinely unreachable server still
|
||||
/// fails; non-transient failures and cancellation surface immediately.
|
||||
/// </summary>
|
||||
internal static async Task<ISession> CreateReverseConnectSessionWithRetryAsync(
|
||||
ITransportWaitingConnection initialConnection,
|
||||
Func<ITransportWaitingConnection, CancellationToken, Task<ISession>> createSessionAsync,
|
||||
Func<CancellationToken, Task<ITransportWaitingConnection>> resolveFreshConnectionAsync,
|
||||
int maxAttempts,
|
||||
ILogger? logger,
|
||||
Uri? endpointUrl,
|
||||
CancellationToken ct)
|
||||
{
|
||||
ITransportWaitingConnection connection = initialConnection;
|
||||
for (int attempt = 1; ; attempt++)
|
||||
{
|
||||
try
|
||||
{
|
||||
return await createSessionAsync(connection, ct).ConfigureAwait(false);
|
||||
}
|
||||
catch (ServiceResultException sre) when (
|
||||
attempt < maxAttempts &&
|
||||
!ct.IsCancellationRequested &&
|
||||
IsStaleReverseConnectionStatus(sre.StatusCode))
|
||||
{
|
||||
if (logger != null)
|
||||
{
|
||||
logger.LogWarning(
|
||||
"Reverse connection to {EndpointUrl} was stale ({StatusCode}); " +
|
||||
"retrying with a fresh connection (attempt {Attempt} of {Max}).",
|
||||
endpointUrl,
|
||||
sre.StatusCode,
|
||||
attempt,
|
||||
maxAttempts);
|
||||
}
|
||||
|
||||
connection = await resolveFreshConnectionAsync(ct).ConfigureAwait(false);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Maximum number of reverse-connect attempts (initial plus retries)
|
||||
/// before a stale reverse connection is surfaced as a failure.
|
||||
/// </summary>
|
||||
private const int kMaxReverseConnectAttempts = 3;
|
||||
|
||||
/// <summary>
|
||||
/// Whether a status code from a reverse-connect attempt indicates the
|
||||
/// delivered connection was already closed/dropped by the server and a
|
||||
/// fresh connection should be requested instead of failing.
|
||||
/// </summary>
|
||||
private static bool IsStaleReverseConnectionStatus(StatusCode statusCode)
|
||||
{
|
||||
return statusCode == StatusCodes.BadConnectionClosed
|
||||
|| statusCode == StatusCodes.BadNotConnected
|
||||
|| statusCode == StatusCodes.BadSecureChannelClosed;
|
||||
}
|
||||
|
||||
/// <inheritdoc/>
|
||||
public virtual async Task<ITransportChannel> CreateChannelAsync(
|
||||
ApplicationConfiguration configuration,
|
||||
|
||||
@@ -1339,10 +1339,22 @@ namespace Opc.Ua.Bindings
|
||||
/// </summary>
|
||||
private void DetectInactiveChannels(object? state = null)
|
||||
{
|
||||
// This is a fire-and-forget timer callback: CloseAsync disposes the
|
||||
// timer and nulls m_channels under m_lock, but a callback already
|
||||
// dispatched can still run afterwards. Snapshot the field and bail
|
||||
// out when the listener has closed rather than dereferencing null,
|
||||
// which would surface as an unhandled NullReferenceException that
|
||||
// crashes the process.
|
||||
ConcurrentDictionary<uint, TcpListenerChannel>? activeChannels = m_channels;
|
||||
if (activeChannels == null)
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
var channels = new List<TcpListenerChannel>();
|
||||
|
||||
bool cleanup = false;
|
||||
foreach (KeyValuePair<uint, TcpListenerChannel> chEntry in m_channels!)
|
||||
foreach (KeyValuePair<uint, TcpListenerChannel> chEntry in activeChannels)
|
||||
{
|
||||
if (chEntry.Value.ElapsedSinceLastActiveTime > m_quotas.ChannelLifetime)
|
||||
{
|
||||
|
||||
@@ -1162,79 +1162,92 @@ namespace Opc.Ua.Bindings
|
||||
"The asymmetric security header could not be parsed.");
|
||||
}
|
||||
|
||||
// verify sender certificate chain.
|
||||
if (certificateData.Length > 0)
|
||||
// Once the sender chain is parsed below it owns freshly allocated
|
||||
// certificate handles. Every remaining validation can throw, and an
|
||||
// out parameter is not handed back to the caller's using-block when
|
||||
// the method throws - so dispose the chain here on any failure
|
||||
// instead of abandoning its certificate handles.
|
||||
try
|
||||
{
|
||||
senderCertificateChain = Utils.ParseCertificateChainBlob(
|
||||
certificateData,
|
||||
Telemetry);
|
||||
// verify sender certificate chain.
|
||||
if (certificateData.Length > 0)
|
||||
{
|
||||
senderCertificateChain = Utils.ParseCertificateChainBlob(
|
||||
certificateData,
|
||||
Telemetry);
|
||||
|
||||
try
|
||||
{
|
||||
string thumbprint =
|
||||
senderCertificateChain[0].Thumbprint
|
||||
?? throw ServiceResultException.Create(
|
||||
StatusCodes.BadCertificateInvalid,
|
||||
"Invalid certificate thumbprint.");
|
||||
}
|
||||
catch (Exception e)
|
||||
{
|
||||
throw ServiceResultException.Create(
|
||||
StatusCodes.BadCertificateInvalid,
|
||||
e,
|
||||
"The sender's certificate could not be parsed.");
|
||||
}
|
||||
}
|
||||
else if (securityPolicyUri != SecurityPolicies.None)
|
||||
{
|
||||
throw ServiceResultException.Create(
|
||||
StatusCodes.BadCertificateInvalid,
|
||||
"The sender's certificate was not specified.");
|
||||
}
|
||||
|
||||
// verify receiver thumbprint.
|
||||
if (thumbprintData.Length > 0)
|
||||
{
|
||||
// TODO: client should use the proider too!
|
||||
if (m_serverCertificates != null)
|
||||
{
|
||||
// Replace the channel-owned instance certificate (and its
|
||||
// issuer chain) with independent handles on the registry's
|
||||
// current entry.
|
||||
using (CertificateEntry? receiverEntry =
|
||||
m_serverCertificates.AcquireApplicationCertificateBySecurityPolicy(securityPolicyUri))
|
||||
try
|
||||
{
|
||||
ServerCertificate?.Dispose();
|
||||
ServerCertificate = receiverEntry?.Certificate.AddRef();
|
||||
ServerCertificateChain?.Dispose();
|
||||
ServerCertificateChain = receiverEntry == null
|
||||
? null
|
||||
: BuildServerCertificateChain(receiverEntry);
|
||||
_ = senderCertificateChain[0].Thumbprint
|
||||
?? throw ServiceResultException.Create(
|
||||
StatusCodes.BadCertificateInvalid,
|
||||
"Invalid certificate thumbprint.");
|
||||
}
|
||||
catch (Exception e)
|
||||
{
|
||||
throw ServiceResultException.Create(
|
||||
StatusCodes.BadCertificateInvalid,
|
||||
e,
|
||||
"The sender's certificate could not be parsed.");
|
||||
}
|
||||
receiverCertificate = ServerCertificate;
|
||||
}
|
||||
|
||||
if (receiverCertificate == null)
|
||||
else if (securityPolicyUri != SecurityPolicies.None)
|
||||
{
|
||||
throw ServiceResultException.Create(
|
||||
StatusCodes.BadCertificateInvalid,
|
||||
"The receiver has no matching certificate for the selected profile.");
|
||||
"The sender's certificate was not specified.");
|
||||
}
|
||||
|
||||
if (!receiverCertificate.Thumbprint.Equals(
|
||||
GetThumbprintString(thumbprintData),
|
||||
StringComparison.OrdinalIgnoreCase))
|
||||
// verify receiver thumbprint.
|
||||
if (thumbprintData.Length > 0)
|
||||
{
|
||||
// TODO: client should use the provider too!
|
||||
if (m_serverCertificates != null)
|
||||
{
|
||||
// Replace the channel-owned instance certificate (and its
|
||||
// issuer chain) with independent handles on the registry's
|
||||
// current entry.
|
||||
using (CertificateEntry? receiverEntry =
|
||||
m_serverCertificates.AcquireApplicationCertificateBySecurityPolicy(securityPolicyUri))
|
||||
{
|
||||
ServerCertificate?.Dispose();
|
||||
ServerCertificate = receiverEntry?.Certificate.AddRef();
|
||||
ServerCertificateChain?.Dispose();
|
||||
ServerCertificateChain = receiverEntry == null
|
||||
? null
|
||||
: BuildServerCertificateChain(receiverEntry);
|
||||
}
|
||||
receiverCertificate = ServerCertificate;
|
||||
}
|
||||
|
||||
if (receiverCertificate == null)
|
||||
{
|
||||
throw ServiceResultException.Create(
|
||||
StatusCodes.BadCertificateInvalid,
|
||||
"The receiver has no matching certificate for the selected profile.");
|
||||
}
|
||||
|
||||
if (!receiverCertificate.Thumbprint.Equals(
|
||||
GetThumbprintString(thumbprintData),
|
||||
StringComparison.OrdinalIgnoreCase))
|
||||
{
|
||||
throw ServiceResultException.Create(
|
||||
StatusCodes.BadCertificateInvalid,
|
||||
"The receiver's certificate thumbprint is not valid.");
|
||||
}
|
||||
}
|
||||
else if (securityPolicyUri != SecurityPolicies.None)
|
||||
{
|
||||
throw ServiceResultException.Create(
|
||||
StatusCodes.BadCertificateInvalid,
|
||||
"The receiver's certificate thumbprint is not valid.");
|
||||
"The receiver's certificate thumbprint was not specified.");
|
||||
}
|
||||
}
|
||||
else if (securityPolicyUri != SecurityPolicies.None)
|
||||
catch
|
||||
{
|
||||
throw ServiceResultException.Create(
|
||||
StatusCodes.BadCertificateInvalid,
|
||||
"The receiver's certificate thumbprint was not specified.");
|
||||
senderCertificateChain?.Dispose();
|
||||
senderCertificateChain = null;
|
||||
throw;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -28,13 +28,5 @@
|
||||
* ======================================================================*/
|
||||
|
||||
using System;
|
||||
using System.Runtime.CompilerServices;
|
||||
|
||||
[assembly: CLSCompliant(false)]
|
||||
|
||||
// The event data builder of WoT Binding Section 6.1 is an implementation
|
||||
// detail of the OPC UA channel, and its rules - the nesting, the state
|
||||
// Variable's Name member and the collision handling - are exactly what a test
|
||||
// has to pin. Exposing it publicly to test it would make an internal shape
|
||||
// part of the API.
|
||||
[assembly: InternalsVisibleTo("Opc.Ua.WotCon.Bindings.Tests")]
|
||||
|
||||
@@ -30,6 +30,7 @@
|
||||
using System;
|
||||
using System.Threading;
|
||||
using System.Threading.Tasks;
|
||||
using Microsoft.Extensions.Logging;
|
||||
using Moq;
|
||||
using NUnit.Framework;
|
||||
using Opc.Ua.Tests;
|
||||
@@ -377,5 +378,135 @@ namespace Opc.Ua.Client.Tests
|
||||
Assert.DoesNotThrowAsync(async () => await task.ConfigureAwait(false));
|
||||
factory.Verify();
|
||||
}
|
||||
|
||||
private static readonly Uri s_reverseEndpointUrl = new("opc.tcp://localhost:4840");
|
||||
|
||||
[Test]
|
||||
public async Task ReverseConnectRetryReturnsFirstSessionWhenConnectionIsHealthyAsync()
|
||||
{
|
||||
ITransportWaitingConnection connection = new Mock<ITransportWaitingConnection>().Object;
|
||||
ISession session = new Mock<ISession>().Object;
|
||||
int resolveCalls = 0;
|
||||
|
||||
ISession result = await DefaultSessionFactory
|
||||
.CreateReverseConnectSessionWithRetryAsync(
|
||||
connection,
|
||||
(conn, ct) => Task.FromResult(session),
|
||||
ct =>
|
||||
{
|
||||
resolveCalls++;
|
||||
return Task.FromResult(connection);
|
||||
},
|
||||
maxAttempts: 3,
|
||||
logger: null,
|
||||
endpointUrl: s_reverseEndpointUrl,
|
||||
ct: CancellationToken.None)
|
||||
.ConfigureAwait(false);
|
||||
|
||||
Assert.That(result, Is.SameAs(session));
|
||||
Assert.That(resolveCalls, Is.Zero, "a healthy connection must not be re-fetched");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task ReverseConnectRetryRefetchesConnectionAfterStaleFailureAsync()
|
||||
{
|
||||
ITransportWaitingConnection firstConnection = new Mock<ITransportWaitingConnection>().Object;
|
||||
ITransportWaitingConnection secondConnection = new Mock<ITransportWaitingConnection>().Object;
|
||||
ISession session = new Mock<ISession>().Object;
|
||||
int createCalls = 0;
|
||||
int resolveCalls = 0;
|
||||
|
||||
ISession result = await DefaultSessionFactory
|
||||
.CreateReverseConnectSessionWithRetryAsync(
|
||||
firstConnection,
|
||||
(conn, ct) =>
|
||||
{
|
||||
createCalls++;
|
||||
if (createCalls == 1)
|
||||
{
|
||||
Assert.That(conn, Is.SameAs(firstConnection));
|
||||
throw new ServiceResultException(StatusCodes.BadConnectionClosed);
|
||||
}
|
||||
|
||||
Assert.That(conn, Is.SameAs(secondConnection),
|
||||
"the retry must use the freshly delivered connection");
|
||||
return Task.FromResult(session);
|
||||
},
|
||||
ct =>
|
||||
{
|
||||
resolveCalls++;
|
||||
return Task.FromResult(secondConnection);
|
||||
},
|
||||
maxAttempts: 3,
|
||||
// Exercise the warning-log branch with a real logger.
|
||||
logger: m_telemetry.CreateLogger<DefaultSessionFactory>(),
|
||||
endpointUrl: s_reverseEndpointUrl,
|
||||
ct: CancellationToken.None)
|
||||
.ConfigureAwait(false);
|
||||
|
||||
Assert.That(result, Is.SameAs(session));
|
||||
Assert.That(createCalls, Is.EqualTo(2));
|
||||
Assert.That(resolveCalls, Is.EqualTo(1));
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void ReverseConnectRetryPropagatesNonTransientFailureWithoutRefetch()
|
||||
{
|
||||
ITransportWaitingConnection connection = new Mock<ITransportWaitingConnection>().Object;
|
||||
int resolveCalls = 0;
|
||||
|
||||
ServiceResultException ex = Assert.ThrowsAsync<ServiceResultException>(async () =>
|
||||
await DefaultSessionFactory
|
||||
.CreateReverseConnectSessionWithRetryAsync(
|
||||
connection,
|
||||
(conn, ct) =>
|
||||
throw new ServiceResultException(StatusCodes.BadIdentityTokenRejected),
|
||||
ct =>
|
||||
{
|
||||
resolveCalls++;
|
||||
return Task.FromResult(connection);
|
||||
},
|
||||
maxAttempts: 3,
|
||||
logger: null,
|
||||
endpointUrl: s_reverseEndpointUrl,
|
||||
ct: CancellationToken.None)
|
||||
.ConfigureAwait(false))!;
|
||||
|
||||
Assert.That(ex.StatusCode, Is.EqualTo((uint)StatusCodes.BadIdentityTokenRejected));
|
||||
Assert.That(resolveCalls, Is.Zero,
|
||||
"a non-transient failure must not trigger a reverse-connection re-fetch");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void ReverseConnectRetryStopsAfterMaxAttemptsAndSurfacesStaleFailure()
|
||||
{
|
||||
ITransportWaitingConnection connection = new Mock<ITransportWaitingConnection>().Object;
|
||||
int createCalls = 0;
|
||||
int resolveCalls = 0;
|
||||
|
||||
ServiceResultException ex = Assert.ThrowsAsync<ServiceResultException>(async () =>
|
||||
await DefaultSessionFactory
|
||||
.CreateReverseConnectSessionWithRetryAsync(
|
||||
connection,
|
||||
(conn, ct) =>
|
||||
{
|
||||
createCalls++;
|
||||
throw new ServiceResultException(StatusCodes.BadConnectionClosed);
|
||||
},
|
||||
ct =>
|
||||
{
|
||||
resolveCalls++;
|
||||
return Task.FromResult(connection);
|
||||
},
|
||||
maxAttempts: 3,
|
||||
logger: null,
|
||||
endpointUrl: s_reverseEndpointUrl,
|
||||
ct: CancellationToken.None)
|
||||
.ConfigureAwait(false))!;
|
||||
|
||||
Assert.That(ex.StatusCode, Is.EqualTo((uint)StatusCodes.BadConnectionClosed));
|
||||
Assert.That(createCalls, Is.EqualTo(3), "initial attempt plus two bounded retries");
|
||||
Assert.That(resolveCalls, Is.EqualTo(2), "a fresh connection is fetched before each retry");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -31,6 +31,7 @@
|
||||
|
||||
using System;
|
||||
using System.Net;
|
||||
using System.Reflection;
|
||||
using System.Threading.Tasks;
|
||||
using Microsoft.Extensions.Time.Testing;
|
||||
using NUnit.Framework;
|
||||
@@ -249,5 +250,24 @@ namespace Opc.Ua.Core.Tests.Stack.Transport
|
||||
|
||||
Assert.That(listener.UriScheme, Is.EqualTo(Utils.UriSchemeOpcTcp));
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task DetectInactiveChannelsIsANoOpWhenChannelMapIsNullAsync()
|
||||
{
|
||||
// The channel map is null before OpenAsync and again after CloseAsync
|
||||
// nulls it. The inactivity timer is fire-and-forget, so a callback
|
||||
// dispatched around close can still run in that window. It must return
|
||||
// quietly rather than dereferencing the null map - which previously
|
||||
// surfaced as an unhandled NullReferenceException in the timer callback
|
||||
// that crashed the test host.
|
||||
await using var listener = new TcpTransportListener(m_telemetry, new FakeTimeProvider());
|
||||
|
||||
MethodInfo detect = typeof(TcpTransportListener).GetMethod(
|
||||
"DetectInactiveChannels",
|
||||
BindingFlags.Instance | BindingFlags.NonPublic)!;
|
||||
Assert.That(detect, Is.Not.Null);
|
||||
|
||||
Assert.DoesNotThrow(() => detect.Invoke(listener, [null]));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -194,6 +194,123 @@ namespace Opc.Ua.Core.Tests.Stack.Transport
|
||||
roundTrip.StatusCode.Code, Is.EqualTo((uint)StatusCodes.BadTcpEndpointUrlInvalid));
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void ReadAsymmetricMessageHeaderDisposesSenderChainWhenReceiverThumbprintMismatches()
|
||||
{
|
||||
var factory = new RecordingByteTransportFactory();
|
||||
using var channel = new TestClientChannel(
|
||||
m_buffers,
|
||||
factory,
|
||||
m_quotas,
|
||||
null,
|
||||
BuildEndpoint(MessageSecurityMode.None, SecurityPolicies.None),
|
||||
m_telemetry,
|
||||
new FakeTimeProvider());
|
||||
|
||||
using Certificate sender = CreateSmallCertificate();
|
||||
using Certificate receiver = CreateSmallCertificate();
|
||||
|
||||
// A valid sender certificate forces ReadAsymmetricMessageHeader to
|
||||
// parse (and therefore allocate) the sender chain, while the bogus
|
||||
// receiver thumbprint makes the subsequent validation throw after
|
||||
// that allocation.
|
||||
byte[] header = BuildAsymmetricHeader(
|
||||
SecurityPolicies.Basic256Sha256,
|
||||
sender.RawData,
|
||||
new byte[TcpMessageLimits.CertificateThumbprintSize]);
|
||||
|
||||
// Baseline after the sender/receiver handles already exist: only the
|
||||
// chain the header parser allocates internally may move the counters.
|
||||
long createdBefore = Certificate.InstancesCreated;
|
||||
long disposedBefore = Certificate.InstancesDisposed;
|
||||
|
||||
ServiceResultException ex = Assert.Throws<ServiceResultException>(
|
||||
() => channel.CallReadAsymmetricMessageHeader(
|
||||
new ArraySegment<byte>(header), receiver))!;
|
||||
Assert.That(ex.StatusCode, Is.EqualTo((uint)StatusCodes.BadCertificateInvalid));
|
||||
|
||||
Assert.That(
|
||||
Certificate.InstancesCreated - createdBefore,
|
||||
Is.EqualTo(Certificate.InstancesDisposed - disposedBefore),
|
||||
"the parsed sender certificate chain must be disposed when asymmetric " +
|
||||
"header validation fails instead of being abandoned as a leaked handle.");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void ReadAsymmetricMessageHeaderDisposesSenderChainWhenReceiverCertificateMissing()
|
||||
{
|
||||
var factory = new RecordingByteTransportFactory();
|
||||
using var channel = new TestClientChannel(
|
||||
m_buffers,
|
||||
factory,
|
||||
m_quotas,
|
||||
null,
|
||||
BuildEndpoint(MessageSecurityMode.None, SecurityPolicies.None),
|
||||
m_telemetry,
|
||||
new FakeTimeProvider());
|
||||
|
||||
using Certificate sender = CreateSmallCertificate();
|
||||
|
||||
byte[] header = BuildAsymmetricHeader(
|
||||
SecurityPolicies.Basic256Sha256,
|
||||
sender.RawData,
|
||||
new byte[TcpMessageLimits.CertificateThumbprintSize]);
|
||||
|
||||
long createdBefore = Certificate.InstancesCreated;
|
||||
long disposedBefore = Certificate.InstancesDisposed;
|
||||
|
||||
// No receiver certificate and no server certificate registry (client
|
||||
// side): the parser reaches the "receiver has no matching certificate"
|
||||
// failure after the sender chain is allocated.
|
||||
ServiceResultException ex = Assert.Throws<ServiceResultException>(
|
||||
() => channel.CallReadAsymmetricMessageHeader(
|
||||
new ArraySegment<byte>(header), receiverCertificate: null))!;
|
||||
Assert.That(ex.StatusCode, Is.EqualTo((uint)StatusCodes.BadCertificateInvalid));
|
||||
|
||||
Assert.That(
|
||||
Certificate.InstancesCreated - createdBefore,
|
||||
Is.EqualTo(Certificate.InstancesDisposed - disposedBefore),
|
||||
"the parsed sender chain must be disposed when the receiver certificate is missing.");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void ReadAsymmetricMessageHeaderDisposesSenderChainWhenReceiverThumbprintMissing()
|
||||
{
|
||||
var factory = new RecordingByteTransportFactory();
|
||||
using var channel = new TestClientChannel(
|
||||
m_buffers,
|
||||
factory,
|
||||
m_quotas,
|
||||
null,
|
||||
BuildEndpoint(MessageSecurityMode.None, SecurityPolicies.None),
|
||||
m_telemetry,
|
||||
new FakeTimeProvider());
|
||||
|
||||
using Certificate sender = CreateSmallCertificate();
|
||||
using Certificate receiver = CreateSmallCertificate();
|
||||
|
||||
// A secured policy with no receiver thumbprint on the wire reaches the
|
||||
// "receiver's certificate thumbprint was not specified" failure after
|
||||
// the sender chain is allocated.
|
||||
byte[] header = BuildAsymmetricHeader(
|
||||
SecurityPolicies.Basic256Sha256,
|
||||
sender.RawData,
|
||||
Array.Empty<byte>());
|
||||
|
||||
long createdBefore = Certificate.InstancesCreated;
|
||||
long disposedBefore = Certificate.InstancesDisposed;
|
||||
|
||||
ServiceResultException ex = Assert.Throws<ServiceResultException>(
|
||||
() => channel.CallReadAsymmetricMessageHeader(
|
||||
new ArraySegment<byte>(header), receiver))!;
|
||||
Assert.That(ex.StatusCode, Is.EqualTo((uint)StatusCodes.BadCertificateInvalid));
|
||||
|
||||
Assert.That(
|
||||
Certificate.InstancesCreated - createdBefore,
|
||||
Is.EqualTo(Certificate.InstancesDisposed - disposedBefore),
|
||||
"the parsed sender chain must be disposed when the receiver thumbprint is absent.");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void VerifyMessageTypeWithWrongTypeThrowsBadTcpMessageTypeInvalid()
|
||||
{
|
||||
@@ -542,6 +659,29 @@ namespace Opc.Ua.Core.Tests.Stack.Transport
|
||||
return TrimTo(buffer, size);
|
||||
}
|
||||
|
||||
private byte[] BuildAsymmetricHeader(
|
||||
string securityPolicyUri, byte[] senderCertificate, byte[] receiverThumbprint)
|
||||
{
|
||||
// Matches the field order ReadAsymmetricMessageHeader decodes: it
|
||||
// first skips the message-type and size UInt32s, then reads the
|
||||
// secure channel id, the security policy uri, the sender
|
||||
// certificate blob, and the receiver certificate thumbprint.
|
||||
byte[] buffer = new byte[senderCertificate.Length + 1024];
|
||||
int size;
|
||||
using (var stream = new MemoryStream(buffer, 0, buffer.Length))
|
||||
using (var encoder = new BinaryEncoder(stream, m_context, false))
|
||||
{
|
||||
encoder.WriteUInt32(null, TcpMessageType.Open); // message type
|
||||
encoder.WriteUInt32(null, 0); // size placeholder
|
||||
encoder.WriteUInt32(null, 0); // secure channel id
|
||||
encoder.WriteString(null, securityPolicyUri);
|
||||
encoder.WriteByteString(null, senderCertificate);
|
||||
encoder.WriteByteString(null, receiverThumbprint);
|
||||
size = encoder.Close();
|
||||
}
|
||||
return TrimTo(buffer, size);
|
||||
}
|
||||
|
||||
private byte[] BuildChunk(uint messageType, Action<BinaryEncoder> writeBody)
|
||||
{
|
||||
byte[] buffer = new byte[256];
|
||||
@@ -689,6 +829,21 @@ namespace Opc.Ua.Core.Tests.Stack.Transport
|
||||
ReadAndVerifyMessageTypeAndSize(decoder, expectedMessageType, count);
|
||||
}
|
||||
|
||||
public (uint ChannelId, CertificateCollection? SenderChain, string SecurityPolicyUri)
|
||||
CallReadAsymmetricMessageHeader(
|
||||
ArraySegment<byte> buffer, Certificate? receiverCertificate)
|
||||
{
|
||||
using var decoder = new BinaryDecoder(buffer, Quotas.MessageContext);
|
||||
Certificate? receiver = receiverCertificate;
|
||||
ReadAsymmetricMessageHeader(
|
||||
decoder,
|
||||
ref receiver,
|
||||
out uint channelId,
|
||||
out CertificateCollection? senderChain,
|
||||
out string securityPolicyUri);
|
||||
return (channelId, senderChain, securityPolicyUri);
|
||||
}
|
||||
|
||||
protected override void HandleWriteComplete(
|
||||
BufferCollection? buffers,
|
||||
object? state,
|
||||
|
||||
Reference in New Issue
Block a user