[Server] Handle ApplicationCertificate updates using events: close channels on update and block new Connections / Requests #3430
Conversation
Introduce async certificate update events (`Started`/`Completed`) in `CertificateValidator` and `ICertificateValidator`. Add `CertificateUpdateInProgress` wait handle for synchronization. Implement `CloseAllChannels` in transport listeners to force-close connections before certificate updates, and add `ForceClose` to `TcpListenerChannel`. Update server logic to close all channels before updating certificates. Refactor and update tests for new async events and certificate update sequencing. Improves security and reliability during certificate rollover.
romanett
changed the title
There was a problem hiding this comment.
Pull request overview
This PR introduces async certificate update events (CertificateUpdateStarted and CertificateUpdate) to improve reliability and safety during application certificate updates. The changes ensure that all active channels are properly closed before a certificate update occurs, and new connections are blocked during the update process using a synchronization primitive.
Key changes:
- Added async event handlers for certificate update lifecycle (started/completed)
- Implemented
CloseAllChannelsin transport listeners to force-close connections before certificate updates - Added synchronization using
ManualResetEventSlimto block new connections during updates
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| Stack/Opc.Ua.Core/Security/Certificates/ICertificateValidator.cs | Adds new events and WaitHandle property to interface |
| Stack/Opc.Ua.Core/Security/Certificates/CertificateValidator.cs | Implements async event handlers, adds synchronization primitive, changes delegate signature to Task |
| Stack/Opc.Ua.Core/Stack/Transport/ITransportListener.cs | Adds CloseAllChannels method to interface |
| Stack/Opc.Ua.Core/Stack/Tcp/TcpTransportListener.cs | Implements CloseAllChannels and waits for certificate updates before accepting connections |
| Stack/Opc.Ua.Core/Stack/Tcp/TcpListenerChannel.cs | Adds ForceClose method to immediately close channels |
| Stack/Opc.Ua.Bindings.Https/Stack/Https/HttpsTransportListener.cs | Implements CloseAllChannels (no-op) and waits for certificate updates before processing requests |
| Stack/Opc.Ua.Core/Stack/Server/ServerBase.cs | Adds OnCertificateUpdateStartedAsync handler to close all channels, changes delegate to Task |
| Libraries/Opc.Ua.Server/Server/StandardServer.cs | Registers new CertificateUpdateStarted event handler |
| Libraries/Opc.Ua.Gds.Client.Common/*.cs | Adds BadConnectionClosed to retry logic for connection attempts |
| Tests/Opc.Ua.Client.Tests/ClientFixture.cs | Adds BadConnectionClosed to retry exception handling |
| Tests/Opc.Ua.Server.Tests/CreateSessionApplicationUriValidationTests.cs | Adds BadConnectionClosed to retry exception handling |
| Tests/Opc.Ua.Core.Tests/Security/Certificates/CertificateValidatorTest.cs | Updates event handler signature from void to Task |
| Tests/Opc.Ua.Gds.Tests/PushTest.cs | Adds test verification for certificate update events and synchronization |
| Tests/Opc.Ua.Security.Certificates.Tests/Pkcs10CertificationRequestTests.cs | Refactors inline array to static readonly field |
Comments suppressed due to low confidence (4)
Tests/Opc.Ua.Security.Certificates.Tests/Pkcs10CertificationRequestTests.cs:86
- Local scope variable 'domainNames' shadows Pkcs10CertificationRequestTests.domainNames.
string[] domainNames = new[] { "localhost", "127.0.0.1" };
Tests/Opc.Ua.Security.Certificates.Tests/Pkcs10CertificationRequestTests.cs:124
- Local scope variable 'domainNames' shadows Pkcs10CertificationRequestTests.domainNames.
string[] domainNames = new[] { "localhost", "127.0.0.1" };
Tests/Opc.Ua.Security.Certificates.Tests/Pkcs10CertificationRequestTests.cs:187
- Local scope variable 'domainNames' shadows Pkcs10CertificationRequestTests.domainNames.
string[] domainNames = new[] { "localhost" };
Tests/Opc.Ua.Security.Certificates.Tests/Pkcs10CertificationRequestTests.cs:219
- Local scope variable 'domainNames' shadows Pkcs10CertificationRequestTests.domainNames.
string[] domainNames = new[] { "localhost", "testhost.local", "192.168.1.1" };
| private readonly ILogger m_logger; | ||
| private readonly ITelemetryContext m_telemetry; | ||
| private readonly Dictionary<string, X509Certificate2> m_validatedCertificates; | ||
| private readonly ManualResetEventSlim m_updateEvent = new(true); |
There was a problem hiding this comment.
The ManualResetEventSlim m_updateEvent is never disposed, which can lead to a resource leak. ManualResetEventSlim implements IDisposable and should be properly disposed when the CertificateValidator is disposed or finalized.
Consider making CertificateValidator implement IDisposable and dispose of m_updateEvent in the Dispose method, or dispose it in a finalizer if IDisposable implementation is not feasible.
| CertificateValidator.CertificateUpdate += OnCertificateUpdateAsync; | ||
| CertificateValidator.CertificateUpdateStarted += OnCertificateUpdateStartedAsync; |
There was a problem hiding this comment.
The event handlers registered for CertificateUpdate and CertificateUpdateStarted are never unsubscribed. This can lead to memory leaks if the StandardServer is disposed while the CertificateValidator remains alive, as the validator will hold references to the server through these event handlers.
Consider unsubscribing from these events in the Dispose method or OnServerStoppingAsync to prevent memory leaks.
| event CertificateUpdateEventHandler CertificateUpdateStarted; | ||
|
|
||
| /// <summary> | ||
| /// An event that signals that a certificate update is in progress. |
There was a problem hiding this comment.
The documentation comment states "An event that signals that a certificate update is in progress", but the actual behavior is the opposite. The event is signaled (set to true) when no update is in progress, and reset (set to false) during updates. The documentation should accurately describe that this WaitHandle is signaled when NO certificate update is in progress, and unsignaled during an update.
| /// An event that signals that a certificate update is in progress. | |
| /// A wait handle that is signaled when no certificate update is in progress | |
| /// and unsignaled while a certificate update is in progress. |
| /// An event that signals that a certificate update is in progress. | ||
| /// </summary> | ||
| WaitHandle CertificateUpdateInProgress { get; } |
There was a problem hiding this comment.
The property name "CertificateUpdateInProgress" has inverted semantics. The WaitHandle is signaled (set) when the update is NOT in progress, and unsignaled (reset) when an update IS in progress. This is confusing because code like "CertificateUpdateInProgress.WaitOne()" actually waits for the update to complete, not to be in progress.
Consider renaming to "CertificateUpdateCompleted" or "CertificateUpdateNotInProgress" to better reflect the actual state being signaled. Alternatively, invert the logic by initializing the event as reset (false) and setting it during updates if you want to keep the current name.
| /// An event that signals that a certificate update is in progress. | |
| /// </summary> | |
| WaitHandle CertificateUpdateInProgress { get; } | |
| /// A wait handle that is signaled when no certificate update is in progress. | |
| /// </summary> | |
| WaitHandle CertificateUpdateCompleted { get; } |
|
|
||
| await DisconnectPushClientAsync().ConfigureAwait(false); | ||
|
|
||
| var certificateUpdate = new ManualResetEvent(false); |
There was a problem hiding this comment.
This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.
| var certificateUpdate = new ManualResetEvent(false); | ||
| var certificateUpdateStarted = new ManualResetEvent(false); | ||
| Task OnCertificateUpdateStarted(object sender, EventArgs e) | ||
| { | ||
| certificateUpdateStarted.Set(); | ||
| return Task.CompletedTask; | ||
| } | ||
| Task OnCertificateUpdated(object sender, EventArgs e) | ||
| { | ||
| certificateUpdate.Set(); | ||
| return Task.CompletedTask; | ||
| } | ||
|
|
||
| var validator = m_server.Config.CertificateValidator; | ||
| try | ||
| { | ||
| validator.CertificateUpdateStarted += OnCertificateUpdateStarted; | ||
| validator.CertificateUpdate += OnCertificateUpdated; | ||
|
|
||
| if (!certificateUpdateStarted.WaitOne(TimeSpan.FromSeconds(10))) | ||
| { | ||
| NUnit.Framework.Assert.Fail("Server certificate update did not start."); | ||
| } | ||
|
|
||
| if (!certificateUpdate.WaitOne(TimeSpan.FromSeconds(30))) | ||
| { | ||
| NUnit.Framework.Assert.Fail("Server certificate update did not complete."); | ||
| } | ||
| } | ||
| finally | ||
| { | ||
| validator.CertificateUpdateStarted -= OnCertificateUpdateStarted; | ||
| validator.CertificateUpdate -= OnCertificateUpdated; | ||
| certificateUpdate.Dispose(); | ||
| certificateUpdateStarted.Dispose(); | ||
| } |
There was a problem hiding this comment.
This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.
| var certificateUpdate = new ManualResetEvent(false); | |
| var certificateUpdateStarted = new ManualResetEvent(false); | |
| Task OnCertificateUpdateStarted(object sender, EventArgs e) | |
| { | |
| certificateUpdateStarted.Set(); | |
| return Task.CompletedTask; | |
| } | |
| Task OnCertificateUpdated(object sender, EventArgs e) | |
| { | |
| certificateUpdate.Set(); | |
| return Task.CompletedTask; | |
| } | |
| var validator = m_server.Config.CertificateValidator; | |
| try | |
| { | |
| validator.CertificateUpdateStarted += OnCertificateUpdateStarted; | |
| validator.CertificateUpdate += OnCertificateUpdated; | |
| if (!certificateUpdateStarted.WaitOne(TimeSpan.FromSeconds(10))) | |
| { | |
| NUnit.Framework.Assert.Fail("Server certificate update did not start."); | |
| } | |
| if (!certificateUpdate.WaitOne(TimeSpan.FromSeconds(30))) | |
| { | |
| NUnit.Framework.Assert.Fail("Server certificate update did not complete."); | |
| } | |
| } | |
| finally | |
| { | |
| validator.CertificateUpdateStarted -= OnCertificateUpdateStarted; | |
| validator.CertificateUpdate -= OnCertificateUpdated; | |
| certificateUpdate.Dispose(); | |
| certificateUpdateStarted.Dispose(); | |
| } | |
| using (var certificateUpdate = new ManualResetEvent(false)) | |
| using (var certificateUpdateStarted = new ManualResetEvent(false)) | |
| { | |
| Task OnCertificateUpdateStarted(object sender, EventArgs e) | |
| { | |
| certificateUpdateStarted.Set(); | |
| return Task.CompletedTask; | |
| } | |
| Task OnCertificateUpdated(object sender, EventArgs e) | |
| { | |
| certificateUpdate.Set(); | |
| return Task.CompletedTask; | |
| } | |
| var validator = m_server.Config.CertificateValidator; | |
| try | |
| { | |
| validator.CertificateUpdateStarted += OnCertificateUpdateStarted; | |
| validator.CertificateUpdate += OnCertificateUpdated; | |
| if (!certificateUpdateStarted.WaitOne(TimeSpan.FromSeconds(10))) | |
| { | |
| NUnit.Framework.Assert.Fail("Server certificate update did not start."); | |
| } | |
| if (!certificateUpdate.WaitOne(TimeSpan.FromSeconds(30))) | |
| { | |
| NUnit.Framework.Assert.Fail("Server certificate update did not complete."); | |
| } | |
| } | |
| finally | |
| { | |
| validator.CertificateUpdateStarted -= OnCertificateUpdateStarted; | |
| validator.CertificateUpdate -= OnCertificateUpdated; | |
| } | |
| } |
romanett
changed the title
Proposed changes
Introduce async certificate update events (
Started/Completed) inCertificateValidatorandICertificateValidator. AddCertificateUpdateInProgresswait handle for synchronization. ImplementCloseAllChannelsin transport listeners to force-close connections before certificate updates, and addForceClosetoTcpListenerChannel. Update server logic to close all channels before updating certificates. Refactor and update tests for new async events and certificate update sequencing. Improves security and reliability during certificate rollover.The behaviour of the Server in case of a Certficate Update now follows the spec more closely:
For each CertificateGroup it may be necessary to call ApplyChanges once the Certificate Update workflow completes. ApplyChanges is required if one or more of the Methods calls returns applyChangesRequired=TRUE.
This step may cause the Server to close its Endpoints and force all Clients to reconnect. If this happens the CertificateManager may need to use the new Certificate to re-establish a Session with the Server.
Types of changes
Checklist