-
Notifications
You must be signed in to change notification settings - Fork 1k
[Server] Handle ApplicationCertificate updates using events: close channels on update and block new Connections / Requests #3430
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,6 +97,15 @@ public event CertificateUpdateEventHandler CertificateUpdate | |
| remove => m_CertificateUpdate -= value; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Raised before an application certificate update occurs. | ||
| /// </summary> | ||
| public event CertificateUpdateEventHandler CertificateUpdateStarted | ||
| { | ||
| add => m_CertificateUpdateStarted += value; | ||
| remove => m_CertificateUpdateStarted -= value; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Updates the validator with the current state of the configuration. | ||
| /// </summary> | ||
|
|
@@ -284,51 +293,69 @@ public virtual async Task UpdateCertificateAsync( | |
| string applicationUri = null, | ||
| CancellationToken ct = default) | ||
| { | ||
| await m_semaphore.WaitAsync(ct).ConfigureAwait(false); | ||
| m_updateEvent.Reset(); | ||
|
|
||
| try | ||
| { | ||
| m_applicationCertificates.Clear(); | ||
| // | ||
| // crash occurs if the cert is in use still and this has not run yet. | ||
| // This might be the intended design but this runs on a free task that | ||
| // might not be scheduled right away. | ||
| // | ||
| // TODO: We need a better way to disconnect all sessions when the cert is | ||
| // updated. (See caller of this method) | ||
| // | ||
| // foreach (CertificateIdentifier applicationCertificate in securityConfiguration | ||
| // .ApplicationCertificates) | ||
| // { | ||
| // applicationCertificate.DisposeCertificate(); | ||
| // } | ||
|
|
||
| foreach (CertificateIdentifier applicationCertificate in securityConfiguration | ||
| .ApplicationCertificates) | ||
| { | ||
| await applicationCertificate | ||
| .LoadPrivateKeyExAsync( | ||
| securityConfiguration.CertificatePasswordProvider, | ||
| applicationUri, | ||
| m_telemetry, | ||
| ct) | ||
| .ConfigureAwait(false); | ||
| CertificateUpdateEventHandler started_callback = m_CertificateUpdateStarted; | ||
| if (started_callback != null) | ||
| { | ||
| var args = new CertificateUpdateEventArgs( | ||
| securityConfiguration, | ||
| GetChannelValidator()); | ||
| await started_callback(this, args).ConfigureAwait(false); | ||
| } | ||
|
|
||
| await m_semaphore.WaitAsync(ct).ConfigureAwait(false); | ||
|
|
||
| try | ||
| { | ||
| m_applicationCertificates.Clear(); | ||
| // | ||
| // crash occurs if the cert is in use still and this has not run yet. | ||
| // This might be the intended design but this runs on a free task that | ||
| // might not be scheduled right away. | ||
| // | ||
| // TODO: We need a better way to disconnect all sessions when the cert is | ||
| // updated. (See caller of this method) | ||
| // | ||
| // foreach (CertificateIdentifier applicationCertificate in securityConfiguration | ||
| // .ApplicationCertificates) | ||
| // { | ||
| // applicationCertificate.DisposeCertificate(); | ||
| // } | ||
|
|
||
| foreach (CertificateIdentifier applicationCertificate in securityConfiguration | ||
| .ApplicationCertificates) | ||
| { | ||
| await applicationCertificate | ||
| .LoadPrivateKeyExAsync( | ||
| securityConfiguration.CertificatePasswordProvider, | ||
| applicationUri, | ||
| m_telemetry, | ||
| ct) | ||
| .ConfigureAwait(false); | ||
| } | ||
| } | ||
| finally | ||
| { | ||
| m_semaphore.Release(); | ||
| } | ||
| } | ||
| finally | ||
| { | ||
| m_semaphore.Release(); | ||
| } | ||
|
|
||
| await UpdateAsync(securityConfiguration, applicationUri, ct).ConfigureAwait(false); | ||
| await UpdateAsync(securityConfiguration, applicationUri, ct).ConfigureAwait(false); | ||
|
|
||
| CertificateUpdateEventHandler callback = m_CertificateUpdate; | ||
| if (callback != null) | ||
| CertificateUpdateEventHandler callback = m_CertificateUpdate; | ||
| if (callback != null) | ||
| { | ||
| var args = new CertificateUpdateEventArgs( | ||
| securityConfiguration, | ||
| GetChannelValidator()); | ||
| await callback(this, args).ConfigureAwait(false); | ||
| } | ||
| } | ||
| finally | ||
| { | ||
| var args = new CertificateUpdateEventArgs( | ||
| securityConfiguration, | ||
| GetChannelValidator()); | ||
| callback(this, args); | ||
| m_updateEvent.Set(); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -2234,6 +2261,9 @@ public static bool IsECSecureForProfile( | |
| throw new NotSupportedException("Unsupported curve type."); | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public WaitHandle CertificateUpdateInProgress => m_updateEvent.WaitHandle; | ||
|
|
||
| /// <summary> | ||
| /// Flag to protect setting by application | ||
| /// from a modification by a SecurityConfiguration. | ||
|
|
@@ -2254,13 +2284,15 @@ private enum ProtectFlags | |
| private readonly ILogger m_logger; | ||
| private readonly ITelemetryContext m_telemetry; | ||
| private readonly Dictionary<string, X509Certificate2> m_validatedCertificates; | ||
| private readonly ManualResetEventSlim m_updateEvent = new(true); | ||
|
||
| private CertificateStoreIdentifier m_trustedCertificateStore; | ||
| private CertificateIdentifierCollection m_trustedCertificateList; | ||
| private CertificateStoreIdentifier m_issuerCertificateStore; | ||
| private CertificateIdentifierCollection m_issuerCertificateList; | ||
| private CertificateStoreIdentifier m_rejectedCertificateStore; | ||
| private event CertificateValidationEventHandler m_CertificateValidation; | ||
| private event CertificateUpdateEventHandler m_CertificateUpdate; | ||
| private event CertificateUpdateEventHandler m_CertificateUpdateStarted; | ||
| private readonly List<X509Certificate2> m_applicationCertificates; | ||
| private ProtectFlags m_protectFlags; | ||
| private bool m_autoAcceptUntrustedCertificates; | ||
|
|
@@ -2350,7 +2382,7 @@ public CertificateUpdateEventArgs( | |
| /// <summary> | ||
| /// Used to handle certificate update events. | ||
| /// </summary> | ||
| public delegate void CertificateUpdateEventHandler( | ||
| public delegate Task CertificateUpdateEventHandler( | ||
| CertificateValidator sender, | ||
| CertificateUpdateEventArgs e); | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -38,6 +38,29 @@ namespace Opc.Ua | |||||||||||||
| /// </summary> | ||||||||||||||
| public interface ICertificateValidator | ||||||||||||||
| { | ||||||||||||||
| /// <summary> | ||||||||||||||
| /// Raised when an application certificate update occurs. | ||||||||||||||
| /// </summary> | ||||||||||||||
| event CertificateUpdateEventHandler CertificateUpdate; | ||||||||||||||
|
|
||||||||||||||
| /// <summary> | ||||||||||||||
| /// Raised before an application certificate update occurs. | ||||||||||||||
| /// </summary> | ||||||||||||||
| event CertificateUpdateEventHandler CertificateUpdateStarted; | ||||||||||||||
|
|
||||||||||||||
| /// <summary> | ||||||||||||||
| /// An event that signals that a certificate update is in progress. | ||||||||||||||
|
||||||||||||||
| /// 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. |
Copilot
AI
Dec 22, 2025
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.
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; } |
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.
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.