feat(opcuaclient): opt-in server-certificate host-name check; NAT fixture keeps its PKI (#816, #818)

#816 finding: the SDK checks that the server certificate names the host only when a session is created with checkDomain:true. The driver, the AdminUI browser and Client.Shared all called the 8-argument DefaultSessionFactory.CreateAsync, which forwards checkDomain:false — so no host-name check ever ran (why a trusted NAT cert for *.invalid browsed fine over an IP in #813). When enabled the SDK (Session.OpenAsync -> ValidateDomains -> FindDomain) compares ConfiguredEndpoint.EndpointUrl.IdnHost — the CONFIGURED host, because OpcUaEndpointSelection/EndpointSelector rewrote the advertised one — against the cert's DNS/IP SANs; secured sessions only, and the auto-accept handler accepts a mismatch. Policy: certificateValidation.validateServerHostName, tri-state. Unset = OpcUaCertificateValidationOptions.DefaultValidateServerHostName = false, the pre-#816 behaviour, so nothing changes on upgrade; true = validate the configured host; false = explicit NAT/port-publish opt-out. Threaded into the driver, the AdminUI browser (same JSON) and the AdminUI form (a select that keeps "unset" unset), Client.Shared ConnectionSettings.ValidateServerHostName and the CLI --validate-host-name. A driver refusal names the opt-out. #818: the NAT fixture's /app/pki is on the opc-plc-nat-pki volume (verified: same thumbprint across --force-recreate), its PUBLIC cert is committed as Docker/nat/opc-plc-nat-server.der, and NatPublishedEndpointBrowseTests run every secure browse on a throwaway PKI (internal browser seam): refusal with nothing trusted, positive with the cert trusted (unset/false host check), and a BadCertificateHostNameInvalid refusal with the check on. The trusted cert is OPCUA_NAT_SERVER_CERT or the committed one (skip on thumbprint mismatch); OPCUA_NAT_SERVER_CERT_TRUSTED is retired. CI exports its throwaway NAT container's public cert. ServerHostNameValidationTests pin the driver against the plain opc-plc. MIN_EXECUTED not raised (needs a hosted run): +6 in v2-integration (440 -> 446), documented in scripts/ci/README.md.
2026-11-11 01:33:00 -05:00
parent bde6cce2fc
commit ae91e6b7f7
30 changed files with 648 additions and 59 deletions
@@ -57,6 +57,13 @@ public abstract class CommandBase : ICommand
    [CommandOption("failover-urls", 'F', Description = "Comma-separated failover endpoint URLs for redundancy")]
    public string? FailoverUrls { get; init; }

+    /// <summary>
+    /// Gets a value indicating whether the server certificate must name the host given in <see cref="Url" /> (#816).
+    /// </summary>
+    [CommandOption("validate-host-name",
+        Description = "Require the server certificate to list the --url host (secured connections only; off by default — leave off behind NAT)")]
+    public bool ValidateHostName { get; init; }
+
    /// <summary>
    /// Gets a value indicating whether verbose Serilog output should be enabled for troubleshooting.
    /// </summary>
@@ -88,7 +95,8 @@ public abstract class CommandBase : ICommand
            Username = Username,
            Password = Password,
            SecurityMode = securityMode,
-            AutoAcceptCertificates = true
+            AutoAcceptCertificates = true,
+            ValidateServerHostName = ValidateHostName
        };

        return settings;
@@ -18,6 +18,7 @@ internal sealed class DefaultSessionFactory : ISessionFactory
        string sessionName,
        uint sessionTimeoutMs,
        UserIdentity identity,
+        bool validateServerHostName,
        CancellationToken ct)
    {
        var endpointConfig = EndpointConfiguration.Create(config);
@@ -26,7 +27,10 @@ internal sealed class DefaultSessionFactory : ISessionFactory
        var created = await new Opc.Ua.Client.DefaultSessionFactory(ClientTelemetry.Context).CreateAsync(
            config,
            configuredEndpoint,
-            false,
+            false,                       // updateBeforeConnect
+            // #816: the overload without checkDomain hard-codes false. The endpoint URL here is the operator's host
+            // (EndpointSelector rewrote the advertised one), so that is the host the certificate is checked against.
+            validateServerHostName,      // checkDomain
            sessionName,
            sessionTimeoutMs,
            identity,
@@ -15,6 +15,9 @@ internal interface ISessionFactory
    /// <param name="sessionName">The session name.</param>
    /// <param name="sessionTimeoutMs">Session timeout in milliseconds.</param>
    /// <param name="identity">The user identity.</param>
+    /// <param name="validateServerHostName">
+    ///     The SDK's <c>checkDomain</c>: require the server certificate to name the endpoint URL's host (#816).
+    /// </param>
    /// <param name="ct">Cancellation token.</param>
    /// <returns>A session adapter wrapping the created session.</returns>
    Task<ISessionAdapter> CreateSessionAsync(
@@ -23,5 +26,6 @@ internal interface ISessionFactory
        string sessionName,
        uint sessionTimeoutMs,
        UserIdentity identity,
+        bool validateServerHostName,
        CancellationToken ct = default);
 }
@@ -40,6 +40,15 @@ public sealed class ConnectionSettings
    /// </summary>
    public bool AutoAcceptCertificates { get; set; } = true;

+    /// <summary>
+    ///     Gitea #816 — require the server certificate to list the host of the URL being connected (the operator's
+    ///     host: discovery keeps it in place of the server-advertised one) among its subject-alternative names;
+    ///     a mismatch fails the connect with <c>BadCertificateHostNameInvalid</c>. Defaults to <c>false</c>, the
+    ///     behaviour before #816 (the SDK overload used never checked). Secured connections only. Leave it off behind
+    ///     NAT / a port publish / when connecting by an IP the certificate does not name.
+    /// </summary>
+    public bool ValidateServerHostName { get; set; }
+
    /// <summary>
    ///     Path to the certificate store. Defaults to <see cref="string.Empty"/>; the
    ///     consuming application configuration factory resolves the canonical path via
@@ -777,6 +777,7 @@ public sealed class OpcUaClientService : IOpcUaClientService
            SecurityMode = settings.SecurityMode,
            SessionTimeoutSeconds = settings.SessionTimeoutSeconds,
            AutoAcceptCertificates = settings.AutoAcceptCertificates,
+            ValidateServerHostName = settings.ValidateServerHostName,
            CertificateStorePath = settings.CertificateStorePath,
            Username = settings.Username,
            Password = settings.Password
@@ -794,7 +795,7 @@ public sealed class OpcUaClientService : IOpcUaClientService

            var sessionTimeoutMs = (uint)(settings.SessionTimeoutSeconds * 1000);
            return await _sessionFactory.CreateSessionAsync(config, endpoint, "OtOpcUaClient", sessionTimeoutMs, identity,
-                ct);
+                settings.ValidateServerHostName, ct);
        }
        catch
        {
@@ -35,6 +35,7 @@ public sealed class OpcUaClientDriverBrowser : IDriverBrowser

    private readonly ILogger<OpcUaClientDriverBrowser> _logger;
    private readonly ISecretResolver? _secretResolver;
+    private readonly string? _pkiRootOverride;

    /// <summary>Creates a new browser. Logger defaults to NullLogger when not supplied.</summary>
    /// <param name="logger">Optional logger; defaults to <see cref="NullLogger{T}"/>.</param>
@@ -49,6 +50,18 @@ public sealed class OpcUaClientDriverBrowser : IDriverBrowser
        _secretResolver = secretResolver;
    }

+    /// <summary>
+    ///     Test seam (Gitea #818): a browser whose PKI lives under <paramref name="pkiRoot"/> instead of the shared
+    ///     <c>%LocalAppData%/OtOpcUa/adminui-browse-pki</c>, so an integration test can trust a fixture's public
+    ///     certificate in a throwaway store without touching the operator's browse trust list.
+    /// </summary>
+    /// <param name="pkiRoot">Directory holding the <c>own</c>/<c>trusted</c>/<c>issuers</c>/<c>rejected</c> stores.</param>
+    internal OpcUaClientDriverBrowser(string pkiRoot) : this()
+    {
+        ArgumentException.ThrowIfNullOrWhiteSpace(pkiRoot);
+        _pkiRootOverride = pkiRoot;
+    }
+
    /// <inheritdoc />
    //     Sourced from the shared DriverTypeNames set rather than the hand-written literal
    //     this line used to carry. The value is unchanged; the constant is what
@@ -82,7 +95,7 @@ public sealed class OpcUaClientDriverBrowser : IDriverBrowser
                    "driver's PKI store instead.",
                    endpoint);

-            var app = await BuildBrowseAppConfigurationAsync(cancellationToken).ConfigureAwait(false);
+            var app = await BuildBrowseAppConfigurationAsync(_pkiRootOverride, cancellationToken).ConfigureAwait(false);
            ownedApplication = app;
            var appConfig = app.ApplicationConfiguration!;

@@ -101,6 +114,9 @@ public sealed class OpcUaClientDriverBrowser : IDriverBrowser
                appConfig,
                configuredEndpoint,
                updateBeforeConnect: false,
+                // Gitea #816: the same host-name policy as the deployed driver (the SDK's overload without this
+                // argument hard-codes false), checked against the configured host the endpoint was rewritten to.
+                checkDomain: opts.CertificateValidation.EffectiveValidateServerHostName,
                sessionName: "OtOpcUa AdminUI Browse",
                (uint)opts.SessionTimeout.TotalMilliseconds,
                identity,
@@ -143,8 +159,10 @@ public sealed class OpcUaClientDriverBrowser : IDriverBrowser
    ///     the runtime driver (<c>%LocalAppData%/OtOpcUa/adminui-browse-pki/</c>). Browse
    ///     trust decisions don't leak into the deployed driver's cert store.
    /// </summary>
+    /// <param name="pkiRootOverride">Test-only PKI root; <c>null</c> = the shared browse PKI.</param>
    /// <param name="ct">Cancellation token for the configuration build.</param>
-    private static async Task<ApplicationInstance> BuildBrowseAppConfigurationAsync(CancellationToken ct)
+    private static async Task<ApplicationInstance> BuildBrowseAppConfigurationAsync(
+        string? pkiRootOverride, CancellationToken ct)
    {
        // The ITelemetryContext overload is the non-obsolete ctor; an empty builder keeps SDK
        // internals off any logging provider.
@@ -154,7 +172,7 @@ public sealed class OpcUaClientDriverBrowser : IDriverBrowser
            ApplicationType = ApplicationType.Client,
        };

-        var pkiRoot = Path.Combine(
+        var pkiRoot = pkiRootOverride ?? Path.Combine(
            Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData),
            "OtOpcUa", "adminui-browse-pki");

@@ -19,5 +19,6 @@
    </ItemGroup>
    <ItemGroup>
        <InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests"/>
+        <InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.IntegrationTests"/>
    </ItemGroup>
 </Project>
@@ -231,6 +231,35 @@ public sealed record OpcUaCertificateValidationOptions
    ///     operator-managed CRL set. <c>null</c>/empty (default) = <c>%LocalAppData%/OtOpcUa/pki/issuers</c>.
    /// </summary>
    public string? IssuerStorePath { get; init; }
+
+    /// <summary>
+    ///     The value an unset <see cref="ValidateServerHostName"/> resolves to. <c>false</c> keeps the behaviour every
+    ///     deployment had before Gitea #816 (the SDK never checked the host name for this driver family).
+    /// </summary>
+    public const bool DefaultValidateServerHostName = false;
+
+    /// <summary>
+    ///     Gitea #816 — whether the server certificate must list the host the operator CONFIGURED (the endpoint URL's
+    ///     host, which the shared <see cref="OpcUaEndpointUrlRewriter"/> keeps in place of the server-advertised one)
+    ///     among its DNS / IP subject-alternative names. A mismatch refuses the session with
+    ///     <c>BadCertificateHostNameInvalid</c>.
+    ///     <list type="bullet">
+    ///         <item><c>null</c> (default) = <see cref="DefaultValidateServerHostName"/> (off).</item>
+    ///         <item><c>true</c> = validate. Worth it when trust comes from a CA (a trusted issuer vouches for every
+    ///         certificate it signed, so only the host check ties the certificate to THIS server); adds little when the
+    ///         server's own self-signed certificate is pinned in the trusted-peer store.</item>
+    ///         <item><c>false</c> = explicit opt-out — the NAT / port-publish / IP-addressed case, where the server's
+    ///         certificate names its private host and the reachable address is something else.</item>
+    ///     </list>
+    ///     Only applies to secured sessions (a <c>None</c> channel validates no server certificate at all), and
+    ///     <see cref="OpcUaClientDriverOptions.AutoAcceptCertificates"/> accepts a host-name mismatch like any other
+    ///     certificate error.
+    /// </summary>
+    public bool? ValidateServerHostName { get; init; }
+
+    /// <summary><see cref="ValidateServerHostName"/> with the default applied.</summary>
+    [System.Text.Json.Serialization.JsonIgnore]
+    public bool EffectiveValidateServerHostName => ValidateServerHostName ?? DefaultValidateServerHostName;
 }

 /// <summary>OPC UA message security mode.</summary>
@@ -581,7 +581,7 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit
                catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { throw; }
                catch (Exception ex)
                {
-                    attemptErrors.Add($"{url} -> {ex.GetType().Name}: {ex.Message}");
+                    attemptErrors.Add($"{url} -> {ex.GetType().Name}: {ex.Message}{HostNameRefusalHint(ex)}");
                }
            }

@@ -726,6 +726,14 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit
    private void WriteLifecycleHealth(long generation, Func<DriverHealth, DriverHealth> next) =>
        UpdateHealth(generation, next);

+    /// <summary>
+    ///     Test seam (Gitea #816): when set, the PKI stores live under this directory instead of the shared
+    ///     <c>%LocalAppData%/OtOpcUa/pki</c>, so an integration test can trust a fixture's certificate in a throwaway
+    ///     store with <see cref="OpcUaClientDriverOptions.AutoAcceptCertificates"/> off (auto-accept also accepts a
+    ///     host-name mismatch, which would hide the check under test). Set before <c>InitializeAsync</c>.
+    /// </summary>
+    internal string? PkiRootOverride { get; set; }
+
    /// <summary>
    ///     Build a minimal in-memory <see cref="ApplicationConfiguration"/>. Certificates live
    ///     under the OS user profile — on Windows that's <c>%LocalAppData%\OtOpcUa\pki</c>
@@ -747,7 +755,7 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit
        };

        certValidation.Application = app;
-        var pkiRoot = Path.Combine(
+        var pkiRoot = PkiRootOverride ?? Path.Combine(
            Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData),
            "OtOpcUa", "pki");

@@ -1051,10 +1059,14 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit
        endpointConfig.OperationTimeout = (int)_options.Timeout.TotalMilliseconds;
        var endpoint = new ConfiguredEndpoint(null, selected, endpointConfig);

+        // Gitea #816: the SDK's 8-argument CreateAsync hard-codes checkDomain:false, so before #816 the server
+        // certificate's host name was never checked. The check (when enabled) runs against endpoint.EndpointUrl —
+        // the CONFIGURED host, because SelectMatching rewrote the advertised one.
        var session = await new DefaultSessionFactory(_sdkTelemetry).CreateAsync(
            appConfig,
            endpoint,
            false,                                              // updateBeforeConnect
+            _options.CertificateValidation.EffectiveValidateServerHostName, // checkDomain (#816)
            _options.SessionName,
            (uint)_options.SessionTimeout.TotalMilliseconds,
            identity,
@@ -1085,6 +1097,19 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit
        return OpcUaEndpointSelection.SelectMatching(all.ToList(), endpointUrl, policy, mode);
    }

+    /// <summary>
+    ///     Gitea #816 — the operator-facing remedy appended to a <c>BadCertificateHostNameInvalid</c> connect failure (only
+    ///     reachable with <c>certificateValidation.validateServerHostName</c> on). The SDK's own text names the host it
+    ///     checked but not the knob.
+    /// </summary>
+    /// <param name="ex">The per-endpoint connect failure.</param>
+    /// <returns>The hint, or an empty string for any other failure.</returns>
+    internal static string HostNameRefusalHint(Exception ex) =>
+        ex is ServiceResultException sre && sre.StatusCode == StatusCodes.BadCertificateHostNameInvalid
+            ? " (the server certificate does not name the configured endpoint host; add that host to the certificate, " +
+              "connect by a name it lists, or — behind NAT / a port publish — set certificateValidation.validateServerHostName=false)"
+            : string.Empty;
+
    /// <summary>Delegates to the shared <see cref="OpcUaEndpointUrlRewriter.Rewrite"/> (Gitea #676 / #806).</summary>
    /// <param name="advertisedUrl">The <c>EndpointUrl</c> the server returned from GetEndpoints.</param>
    /// <param name="configuredUrl">The URL the driver was configured with (the one GetEndpoints was sent to).</param>
@@ -28,6 +28,8 @@

    <ItemGroup>
        <InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests"/>
+        <!-- Gitea #816: the host-name tests trust the fixture certificate in a throwaway PKI (PkiRootOverride). -->
+        <InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.IntegrationTests"/>
        <!-- Gitea #656: the native-alarm re-statement end-to-end tests drive this driver into the real DriverHostActor. -->
        <InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.Runtime.Tests"/>
        <!-- Gitea #670 review M1: the wire test deletes the driver's subscription server-side through its own session. -->
@@ -192,6 +192,15 @@
                </div>
                <div class="form-text">Revoked certificates are always rejected.</div>
            </div>
+            <div class="col-md-3">
+                <label class="form-label">Server host-name check</label>
+                <InputSelect @bind-Value="_form.OpcUa.ValidateServerHostName" @bind-Value:after="EmitAsync" class="form-select form-select-sm">
+                    <option value="">Default (off)</option>
+                    <option value="true">Validate configured host</option>
+                    <option value="false">Off (NAT / port publish)</option>
+                </InputSelect>
+                <div class="form-text">Secured sessions only: the server certificate must name the endpoint host you configured.</div>
+            </div>
            <div class="col-md-6">
                <label class="form-label">Issuer store / CRL directory</label>
                <InputText @bind-Value="_form.OpcUa.IssuerStorePath" @bind-Value:after="EmitAsync" class="form-control form-control-sm mono" />
@@ -349,6 +358,8 @@
        public int SubscriptionPriority { get; set; }
        public bool RejectUnknownRevocationStatus { get; set; }
        public string? IssuerStorePath { get; set; }
+        // #816 tri-state: "" = unset (the driver default) so the form never re-emits a concrete default; "true"/"false".
+        public string ValidateServerHostName { get; set; } = "";
        public string? DiscoveryUrl { get; set; }
        public string? DiscoveryServerUri { get; set; }

@@ -381,6 +392,12 @@
            SubscriptionPriority = r.SubscriptionPriority,
            RejectUnknownRevocationStatus = r.CertificateValidation.RejectUnknownRevocationStatus,
            IssuerStorePath = r.CertificateValidation.IssuerStorePath,
+            ValidateServerHostName = r.CertificateValidation.ValidateServerHostName switch
+            {
+                true => "true",
+                false => "false",
+                null => "",
+            },
            DiscoveryUrl = r.DiscoveryUrl,
            DiscoveryServerUri = r.DiscoveryServerUri,
        };
@@ -417,6 +434,12 @@
            {
                RejectUnknownRevocationStatus = RejectUnknownRevocationStatus,
                IssuerStorePath = string.IsNullOrWhiteSpace(IssuerStorePath) ? null : IssuerStorePath,
+                ValidateServerHostName = ValidateServerHostName switch
+                {
+                    "true" => true,
+                    "false" => false,
+                    _ => null,
+                },
            },
            DiscoveryUrl = string.IsNullOrWhiteSpace(DiscoveryUrl) ? null : DiscoveryUrl,
            DiscoveryServerUri = string.IsNullOrWhiteSpace(DiscoveryServerUri) ? null : DiscoveryServerUri,