fix(opcua): credential lockout review fixes (#833)

- Reserve an in-flight slot per client before authenticating (TryBegin / Complete / Abandon under one lock); refuse once recorded failures + in-flight attempts reach MaxFailedAttempts, so a parallel burst cannot pass the check before its first failure is recorded. - An expired lockout resets the count (LockoutSeconds < FailureWindowSeconds relocked on every single failure after the first lockout). - Validate OpcUa:CredentialLockout at host start: disabled, or both durations > 0. - Log the first refusal of a lockout episode at Information, the rest at Debug. - Document that on None endpoints the key is effectively IP-only (NAT neighbours share a UserName count) and a null peer shares one key.
2026-11-11 02:32:40 -05:00
parent 0856261ef8
commit 988927c8ac
8 changed files with 386 additions and 70 deletions
@@ -56,4 +56,27 @@ public sealed class OpcUaApplicationHostOptionsValidatorTests
        result.Failed.ShouldBeTrue();
        result.Failures.ShouldContain("OpcUa:EnabledSecurityProfiles must contain at least 1 item(s) (had 0)");
    }
+
+    /// <summary>#833 review: an enabled lockout with a non-positive duration fails; a disabled one is not checked.</summary>
+    [Theory]
+    [InlineData(5, 0, 60, false)]
+    [InlineData(5, 300, 0, false)]
+    [InlineData(5, -1, -1, false)]
+    [InlineData(5, 30, 600, true)]
+    [InlineData(0, 0, 0, true)]
+    [InlineData(-1, 0, 0, true)]
+    public void Credential_lockout_durations_must_be_positive_while_enabled(
+        int maxFailedAttempts, int lockoutSeconds, int windowSeconds, bool valid)
+    {
+        var result = Sut.Validate(null, new OpcUaApplicationHostOptions
+        {
+            CredentialLockout = new ZB.MOM.WW.OtOpcUa.OpcUaServer.Security.OpcUaCredentialLockoutOptions
+            {
+                MaxFailedAttempts = maxFailedAttempts, LockoutSeconds = lockoutSeconds, FailureWindowSeconds = windowSeconds,
+            },
+        });
+
+        result.Succeeded.ShouldBe(valid);
+        if (!valid) result.Failures!.ShouldContain(f => f.Contains("OpcUa:CredentialLockout:LockoutSeconds"));
+    }
 }
@@ -102,6 +102,94 @@ public sealed class FailedCredentialLockoutTests
        lockout.TrackedCount.ShouldBe(1, "expired entries are pruned to make room");
    }

+    /// <summary>
+    ///     #833 review — the authenticate call is awaited between the check and the outcome. A burst of parallel
+    ///     activations that all check before any failure is recorded must still be held to the threshold: the check
+    ///     reserves an in-flight slot, so only MaxFailedAttempts may be in progress at once.
+    /// </summary>
+    [Fact]
+    public async Task A_parallel_burst_is_held_to_the_threshold_before_any_failure_is_recorded()
+    {
+        var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 3 }, new ManualClock());
+        const int burst = 20;
+        using var allChecked = new CountdownEvent(burst);
+        var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+        var admitted = 0;
+
+        var attempts = Enumerable.Range(0, burst).Select(attempt => Task.Run(async () =>
+        {
+            var ok = lockout.TryBegin(Client, out _, out _, out _);
+            if (ok) Interlocked.Increment(ref admitted);
+            allChecked.Signal();
+            await release.Task; // every attempt checks before any outcome lands — the race window
+            if (ok) lockout.Complete(Client, succeeded: false);
+        }, TestContext.Current.CancellationToken)).ToArray();
+
+        allChecked.Wait(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken).ShouldBeTrue();
+        release.SetResult();
+        await Task.WhenAll(attempts);
+
+        admitted.ShouldBe(3, "only MaxFailedAttempts activations may be checked at once");
+        lockout.IsLockedOut(Client, out _).ShouldBeTrue("the three failures lock the client");
+    }
+
+    [Fact]
+    public void In_flight_attempts_are_refused_as_TooManyInFlight_and_released_slots_free_up()
+    {
+        var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 2 }, new ManualClock());
+        lockout.TryBegin(Client, out _, out _, out _).ShouldBeTrue();
+        lockout.TryBegin(Client, out _, out _, out _).ShouldBeTrue();
+        lockout.TryBegin(Client, out var refusal, out _, out var first).ShouldBeFalse();
+        refusal.ShouldBe(CredentialLockoutRefusal.TooManyInFlight);
+        first.ShouldBeTrue();
+
+        lockout.Abandon(Client);                       // a cancelled activation frees its slot, uncounted
+        lockout.Complete(Client, succeeded: true);     // a success clears the count
+        lockout.TryBegin(Client, out _, out _, out _).ShouldBeTrue();
+        lockout.Complete(Client, succeeded: true);
+        lockout.TrackedCount.ShouldBe(0);
+    }
+
+    /// <summary>
+    ///     #833 review — with LockoutSeconds shorter than FailureWindowSeconds, the count survived the expired lockout
+    ///     (the last failure was still inside the window), so the very next failure relocked the client.
+    /// </summary>
+    [Fact]
+    public void An_expired_lockout_starts_a_fresh_count_even_inside_the_failure_window()
+    {
+        var clock = new ManualClock();
+        var lockout = new FailedCredentialLockout(
+            new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 2, FailureWindowSeconds = 600, LockoutSeconds = 10 }, clock);
+        lockout.RecordFailure(Client).ShouldBeFalse();
+        lockout.RecordFailure(Client).ShouldBeTrue();
+
+        clock.Advance(TimeSpan.FromSeconds(11));
+        lockout.IsLockedOut(Client, out _).ShouldBeFalse();
+        lockout.RecordFailure(Client).ShouldBeFalse("one failure after the lockout is the first of a new run");
+        lockout.IsLockedOut(Client, out _).ShouldBeFalse();
+        lockout.RecordFailure(Client).ShouldBeTrue();
+    }
+
+    [Fact]
+    public void Only_the_first_refusal_of_a_lockout_episode_is_flagged_for_Information_logging()
+    {
+        var clock = new ManualClock();
+        var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 1, LockoutSeconds = 10 }, clock);
+        lockout.RecordFailure(Client).ShouldBeTrue();
+
+        lockout.TryBegin(Client, out var r1, out _, out var first1).ShouldBeFalse();
+        r1.ShouldBe(CredentialLockoutRefusal.LockedOut);
+        first1.ShouldBeTrue();
+        lockout.TryBegin(Client, out _, out _, out var first2).ShouldBeFalse();
+        first2.ShouldBeFalse();
+
+        clock.Advance(TimeSpan.FromSeconds(11));
+        lockout.TryBegin(Client, out _, out _, out _).ShouldBeTrue();
+        lockout.Complete(Client, succeeded: false).ShouldBeTrue("a new episode");
+        lockout.TryBegin(Client, out _, out _, out var first3).ShouldBeFalse();
+        first3.ShouldBeTrue("each lockout episode logs its first refusal once");
+    }
+
    [Fact]
    public void The_host_switches_the_sdk_lockout_off()
    {