Add support for pushing to arbitrary certificate groups to the ServerPushConfigurationClient by Tyrrx · Pull Request #3585 · OPCFoundation/UA-.NETStandard

Tyrrx · 2026-02-26T10:16:35Z

Proposed changes

Extend Add/RemoveCertificate methods to support arbitrary certificate groups and add overloads for backward compatibility.
Extend ReadTrustList API to support arbitrary certificate groups and add overloads for backward compatibility.
Extend UpdateTrustList to support arbitrary certificate groups and add overloads for backward compatibility.

Related Issues

Fixes ServerPushConfigurationClient lacks overloads that accept an arbitrary certificate group id #3571

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply. You can also fill these out after creating the PR.

Bugfix (non-breaking change which fixes an issue)
Enhancement (non-breaking change which adds functionality)
Test enhancement (non-breaking change to increase test coverage)
Breaking change (fix or feature that would cause existing functionality to not work as expected, requires version increase of Nuget packages)
Documentation Update (if none of the other choices apply)

Checklist

Put an x in 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.

I have read the CONTRIBUTING doc.
I have signed the CLA.
I ran tests locally with my changes, all passed.
I fixed all failing tests in the CI pipelines.
I fixed all introduced issues with CodeQL and LGTM.
I have added tests that prove my fix is effective or that my feature works and increased code coverage.
I have added necessary documentation (if appropriate).
Any dependent changes have been merged and published in downstream modules.

Further comments

Is there a auto formatting rule set that needs to be applied?
Do the new methods need additional tests because the back-ports technically already call them?
Do the new methods need additional documentation because I think they are self explanatory?
Would it make sense to make the ServerPushConfigurationClient partial in general so that it is easier for a library consumer to implement custom logic?

… groups and add overloads for backward compatibility. Extend ReadTrustList API to support arbitrary certificate groups and add overloads for backward compatibility. Extend UpdateTrustList to support arbitrary certificate groups and add overloads for backward compatibility.

CLAassistant · 2026-02-26T10:16:44Z

All committers have signed the CLA.

romanett · 2026-02-26T17:07:55Z

@Tyrrx maybe you can add a fast path for the well known nodes, so no browse is issued for them

codecov · 2026-02-26T17:13:51Z

Codecov Report

❌ Patch coverage is 72.26027% with 81 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.97%. Comparing base (d25caff) to head (acb2ffb).
⚠️ Report is 126 commits behind head on master.

Files with missing lines	Patch %	Lines
...Gds.Client.Common/ServerPushConfigurationClient.cs	72.26%	79 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #3585       +/-   ##
===========================================
+ Coverage   51.86%   65.97%   +14.10%     
===========================================
  Files         370      459       +89     
  Lines       78618    97763    +19145     
  Branches    13650    16326     +2676     
===========================================
+ Hits        40779    64496    +23717     
+ Misses      33705    28181     -5524     
- Partials     4134     5086      +952

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This pull request adds support for operating on arbitrary certificate groups in the ServerPushConfigurationClient class, addressing issue #3571. Previously, the methods could only work with the default application certificate group. The changes enable library consumers to read, update, add, and remove certificates from any certificate group on the server.

Changes:

Extended ReadTrustListAsync, UpdateTrustListAsync, AddCertificateAsync, and RemoveCertificateAsync to accept an optional certificateGroupId parameter
Added backward-compatible overloads that default to the DefaultApplicationGroup
Implemented helper methods BrowseNodeIdAsync and FindChildByTypeDefinitionAsync to dynamically discover NodeIds for arbitrary certificate groups
Applied formatting changes throughout the file for consistency

Libraries/Opc.Ua.Gds.Client.Common/ServerPushConfigurationClient.cs

Copilot · 2026-02-26T17:16:11Z

Libraries/Opc.Ua.Gds.Client.Common/ServerPushConfigurationClient.cs


            const int maxAttempts = 5;
-            for (int attempt = 0; ; attempt++)
+            for (int attempt = 0;; attempt++)


Formatting change removes space after semicolon in for loop condition. While this doesn't affect functionality, it's typically more readable to have a space after semicolons in for loops (e.g., for (int attempt = 0; ; attempt++)). This change appears to be inconsistent with common C# formatting conventions. Consider reverting this spacing change or confirming it matches the project's .editorconfig rules.

Suggested change

for (int attempt = 0;; attempt++)

for (int attempt = 0; ; attempt++)

Copilot · 2026-02-26T17:16:12Z

Libraries/Opc.Ua.Gds.Client.Common/ServerPushConfigurationClient.cs

+        public async Task<bool> UpdateTrustListAsync(
+            TrustListDataType trustList,
+            NodeId certificateGroupId,
+            long maxTrustListSize,
+            CancellationToken ct = default)


The new public method overload is missing XML documentation for the certificateGroupId parameter. This parameter should be documented with a <param name="certificateGroupId"> tag explaining that it is the identifier for the CertificateGroup to which the trust list update should be applied.

Copilot · 2026-02-26T17:16:13Z

Libraries/Opc.Ua.Gds.Client.Common/ServerPushConfigurationClient.cs

+        /// Add certificate.
+        /// </summary>


The new public method overload is missing XML documentation for the certificateGroupId parameter. This parameter should be documented with a <param name="certificateGroupId"> tag explaining that it is the identifier for the CertificateGroup to which the certificate should be added.

Suggested change

/// Add certificate.

/// </summary>

/// Add a certificate to the specified certificate group on the server.

/// </summary>

/// <param name="certificate">The certificate to add to the certificate group.</param>

/// <param name="isTrustedCertificate">If <c>true</c>, the certificate is added as a trusted certificate; otherwise it is added as an issuer certificate.</param>

/// <param name="certificateGroupId">The identifier for the CertificateGroup to which the certificate should be added.</param>

/// <param name="ct">The token that can be used to cancel the asynchronous operation.</param>

/// <returns>A task that represents the asynchronous add-certificate operation.</returns>

Copilot · 2026-02-26T17:16:13Z

Libraries/Opc.Ua.Gds.Client.Common/ServerPushConfigurationClient.cs

+
+        /// <summary>
+        /// Remove certificate.
+        /// </summary>


The new public method overload is missing XML documentation for the certificateGroupId parameter. This parameter should be documented with a <param name="certificateGroupId"> tag explaining that it is the identifier for the CertificateGroup from which the certificate should be removed.

Suggested change

/// </summary>

/// </summary>

/// <param name="thumbprint">The thumbprint of the certificate to remove.</param>

/// <param name="isTrustedCertificate">Indicates whether the certificate is a trusted certificate.</param>

/// <param name="certificateGroupId">

/// The identifier for the CertificateGroup from which the certificate should be removed.

/// </param>

/// <param name="ct">The token to monitor for cancellation requests.</param>

/// <returns>A task that represents the asynchronous remove operation.</returns>

Copilot · 2026-02-26T17:16:13Z

Libraries/Opc.Ua.Gds.Client.Common/ServerPushConfigurationClient.cs

+            NodeId referenceTypeId,
+            CancellationToken ct = default)
+        {
+            Node node = await Session.ReadNodeAsync(searchNodeId, ct).ConfigureAwait(false);


The ReadNodeAsync call on line 1373 may fail if the searchNodeId cannot be resolved in the server's namespace (e.g., if namespace mappings differ). Consider wrapping this in a try-catch block with a more specific error message indicating that the standard node definition could not be read, which would help with debugging issues related to namespace mismatches.

Suggested change

Node node = await Session.ReadNodeAsync(searchNodeId, ct).ConfigureAwait(false);

Node node;

try

{

node = await Session.ReadNodeAsync(searchNodeId, ct).ConfigureAwait(false);

}

catch (ServiceResultException ex)

{

m_logger.LogError(

ex,

"Failed to read standard node definition for NodeId {SearchNodeId}. " +

"This may indicate a namespace mapping mismatch between client and server.",

searchNodeId);

throw;

}

Copilot · 2026-02-26T17:16:13Z

Libraries/Opc.Ua.Gds.Client.Common/ServerPushConfigurationClient.cs

                    if (IsConnected)
                    {
+                        NodeId closeMethodId =
+                            await BrowseNodeIdAsync(trustListNodeId,
+                                    Ua.MethodIds.FileType_Close, ReferenceTypeIds.HasComponent, ct)
+                                .ConfigureAwait(false);
+
                        await session.CallAsync(
-                            ExpandedNodeId.ToNodeId(
-                                Ua.ObjectIds.ServerConfiguration_CertificateGroups_DefaultApplicationGroup_TrustList,
-                                session.NamespaceUris
-                            ),
-                            ExpandedNodeId.ToNodeId(
-                                Ua.MethodIds
-                                    .ServerConfiguration_CertificateGroups_DefaultApplicationGroup_TrustList_Close,
-                                session.NamespaceUris
-                            ),
-                            ct,
-                            fileHandle)
+                                trustListNodeId,
+                                closeMethodId,
+                                ct,
+                                fileHandle)
                            .ConfigureAwait(false);


The error handler attempts to call BrowseNodeIdAsync and session.CallAsync to close the file handle, but these calls can also fail (e.g., if the connection is lost or if there's a timeout). Consider wrapping these cleanup calls in a try-catch block to prevent the original exception from being masked by a cleanup failure. This pattern is important for ensuring the original error is always propagated to the caller.

…outside of loop.

Tyrrx · 2026-02-26T20:47:40Z

@Tyrrx maybe you can add a fast path for the well known nodes, so no browse is issued for them

Yes, I also thought about that and I added it here: 293f281. But now the problem is that the actual browse methods are not covered by the tests anymore. Is it possible to introduce a custom group in the sample server and if so, how?

…izing `NodeId` resolution.

…ility

romanett · 2026-02-27T05:15:44Z

@Tyrrx thank you for the fast fix, for the Server it is not that easy. However the GDS has this functionality to specifiy additional certificate groups in the configuration. Maybe you can add a tests utilizing that in the GDS.Tests project?

Libraries/Opc.Ua.Gds.Client.Common/ServerPushConfigurationClient.cs

marcschier

Can you add tests too, please?

Tyrrx · 2026-02-27T08:45:01Z

@Tyrrx thank you for the fast fix, for the Server it is not that easy. However the GDS has this functionality to specifiy additional certificate groups in the configuration. Maybe you can add a tests utilizing that in the GDS.Tests project?

@romanett: I checked the implementation of ICertificateGroup. It seems like there is no mapping from the config to the actual node ID except for the hard coded default groups:

The actual node id is inferred by the certificate types in the group in Opc.Ua.Gds.Server.ApplicationsNodeManager.SetCertificateGroupNodes.
I tought, I could use the Opc.Ua.Gds.Server.CertificateGroupConfiguration.Id to specify the node id of a custom group as far as I see it is only used internally. Can I extend the Opc.Ua.Gds.Server.CertificateGroupConfiguration with a NodeId (for the defaults optional) or how should I proceed?

…ozenDictionary` for improved performance.

romanett · 2026-02-27T09:25:26Z

@Tyrrx i think an extenstion is just needed in applicationsNodeManager.SetCertificateGroupNodes to check the existing certificategroup.Id config.

Thank you👍.

If this doesnt work I See No harm in adding the optional NodeId config propery

Tyrrx · 2026-02-27T13:59:48Z

@romanett Okay I took a closer look and it seems like it's not done by just adding a nodeId or extending the SetCertificateGroupNodes. Or am I missing something? But we have certificate groups already and a custom certificate group should behave as a default one so I would propose to just add a flag for disabling the lookup and use it during the tests. What do you mean?

Tyrrx · 2026-02-27T14:00:37Z

Another option would be to test the private methods but I think that's worse.

romanett · 2026-02-28T12:46:52Z

@Tyrrx Copilot implemented the Support we can Test it and merge before yours, than you can base the integration Test on that.
#3589
Before Version 1.6 is Release will take some more time anyway, so your PR will end p in 1.6

romanett requested a review from Copilot February 26, 2026 17:08

Copilot started reviewing on behalf of romanett February 26, 2026 17:09 View session

Copilot AI reviewed Feb 26, 2026

View reviewed changes

Refactor trust list methods to optimize NodeId lookup by moving it …

5a6442b

…outside of loop.

Refactor trust list handling by introducing method mappings and optim…

293f281

…izing `NodeId` resolution.

Tyrrx force-pushed the feature/custom-certificate-group-support branch from b062a99 to 293f281 Compare February 26, 2026 20:52

Refactor trust list method lookup: transpose it for easier maintainab…

acb2ffb

…ility

romanett reviewed Feb 27, 2026

View reviewed changes

Libraries/Opc.Ua.Gds.Client.Common/ServerPushConfigurationClient.cs Outdated Show resolved Hide resolved

marcschier requested changes Feb 27, 2026

View reviewed changes

Optimize trust list method mapping by replacing Dictionary with `Fr…

364e57f

…ozenDictionary` for improved performance.

marcschier marked this pull request as draft February 28, 2026 06:43

	for (int attempt = 0;; attempt++)
	for (int attempt = 0; ; attempt++)

-        /// Add certificate.
-        /// </summary>
+        /// Add a certificate to the specified certificate group on the server.
+        /// </summary>
+        /// <param name="certificate">The certificate to add to the certificate group.</param>
+        /// <param name="isTrustedCertificate">If <c>true</c>, the certificate is added as a trusted certificate; otherwise it is added as an issuer certificate.</param>
+        /// <param name="certificateGroupId">The identifier for the CertificateGroup to which the certificate should be added.</param>
+        /// <param name="ct">The token that can be used to cancel the asynchronous operation.</param>
+        /// <returns>A task that represents the asynchronous add-certificate operation.</returns>

-        /// </summary>
+        /// </summary>
+        /// <param name="thumbprint">The thumbprint of the certificate to remove.</param>
+        /// <param name="isTrustedCertificate">Indicates whether the certificate is a trusted certificate.</param>
+        /// <param name="certificateGroupId">
+        /// The identifier for the CertificateGroup from which the certificate should be removed.
+        /// </param>
+        /// <param name="ct">The token to monitor for cancellation requests.</param>
+        /// <returns>A task that represents the asynchronous remove operation.</returns>

-            Node node = await Session.ReadNodeAsync(searchNodeId, ct).ConfigureAwait(false);
+            Node node;
+            try
+            {
+                node = await Session.ReadNodeAsync(searchNodeId, ct).ConfigureAwait(false);
+            }
+            catch (ServiceResultException ex)
+            {
+                m_logger.LogError(
+                    ex,
+                    "Failed to read standard node definition for NodeId {SearchNodeId}. " +
+                    "This may indicate a namespace mapping mismatch between client and server.",
+                    searchNodeId);
+                throw;
+            }

Conversation

Tyrrx commented Feb 26, 2026

Proposed changes

Related Issues

Types of changes

Checklist

Further comments

Uh oh!

CLAassistant commented Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romanett commented Feb 26, 2026

Uh oh!

codecov bot commented Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Tyrrx commented Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romanett commented Feb 27, 2026

Uh oh!

Uh oh!

marcschier left a comment

Choose a reason for hiding this comment

Uh oh!

Tyrrx commented Feb 27, 2026

Uh oh!

romanett commented Feb 27, 2026

Uh oh!

Tyrrx commented Feb 27, 2026

Uh oh!

Tyrrx commented Feb 27, 2026

Uh oh!

romanett commented Feb 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CLAassistant commented Feb 26, 2026 •

edited

Loading

codecov bot commented Feb 26, 2026 •

edited

Loading

Tyrrx commented Feb 26, 2026 •

edited

Loading