diff --git a/code-reviews/Security/findings.md b/code-reviews/Security/findings.md index 997025f..4bc5441 100644 --- a/code-reviews/Security/findings.md +++ b/code-reviews/Security/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 4 | +| Open findings | 0 (1 deferred — Security-008) | ## Summary @@ -315,7 +315,7 @@ Security-side defect — the reset-on-refresh bug — is fully fixed here. Regre |--|--| | Severity | Low | | Category | Performance & resource management | -| Status | Open | +| Status | Deferred | | Location | `src/ScadaLink.Security/RoleMapper.cs:25-48` | **Description** @@ -333,7 +333,18 @@ Add a repository method that loads scope rules for a set of mapping IDs in one q **Resolution** -_Unresolved._ +Deferred 2026-05-16 (commit `pending commit`). Finding confirmed accurate: `RoleMapper.MapGroupsToRolesAsync` +issues one `GetScopeRulesForMappingAsync` round-trip per matched Deployment mapping — a +genuine N+1 on the login / 15-minute-refresh path. However, the only correct fix +(a batch `GetScopeRulesForMappingsAsync(IEnumerable)` repository method, or an +eager-load navigation property) requires changes to `ISecurityRepository` +(`src/ScadaLink.Commons`) and `SecurityRepository` (`src/ScadaLink.ConfigurationDatabase`). +Both are outside the Security module's permitted edit scope for this review pass, and the +existing `ISecurityRepository` surface offers no per-set scope-rule query, so the N+1 +cannot be removed from within `RoleMapper.cs` alone. Severity is Low (bounded by the +number of site-scoped Deployment groups, typically small). Deferred to a cross-module +change that adds the batch repository method; `RoleMapper` should then resolve all scope +rules in a single call. ### Security-009 — CancellationToken not honored inside `Task.Run` LDAP calls @@ -341,7 +352,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Security/LdapAuthService.cs:42`, `:46`, `:51`, `:56-57`, `:67-73`, `:135`, `:139-145` | **Description** @@ -361,7 +372,17 @@ work-item scheduling, or implement a timeout-with-disconnect fallback. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit `pending commit`). Confirmed: the synchronous Novell LDAP +calls were wrapped in `Task.Run(..., ct)` where `ct` only prevents the work item from +starting and cannot interrupt an in-progress blocking `Connect`/`Bind`/`Search`, and no +network/operation timeout was configured on the `LdapConnection`. Added a configurable +`SecurityOptions.LdapConnectionTimeoutMs` (default 10s) and a new `ApplyConnectionTimeout` +helper that sets both `LdapConnection.ConnectionTimeout` (socket connect) and +`LdapConstraints.TimeLimit` (per-operation limit) before connecting, so a hung LDAP +server can no longer pin a thread-pool thread indefinitely. The `ct`-only-guards-scheduling +limitation is now documented in the option's XML doc and an inline comment. Regression +tests `SecurityOptions_LdapConnectionTimeout_HasSaneDefault` and +`AuthenticateAsync_UnreachableHost_FailsWithinConfiguredTimeout`. ### Security-010 — Design doc contradicts itself on Windows Integrated Authentication @@ -369,7 +390,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Design-document adherence | -| Status | Open | +| Status | Resolved | | Location | `docs/requirements/Component-Security.md:13` (vs. `:23`) | **Description** @@ -388,7 +409,14 @@ to match the implemented behavior and the rest of the document. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit `pending commit`). Confirmed: the Responsibilities bullet at +line 13 said "using Windows Integrated Authentication", directly contradicting the +Authentication section's "No Windows Integrated Authentication ... authenticates directly +against LDAP/AD, not via Kerberos/NTLM", CLAUDE.md, and the implementation (`LdapAuthService` +performs a direct username/password bind). Documentation-only finding — no regression test +is meaningful. Reworded the Responsibilities bullet to "Authenticate users against +LDAP/Active Directory using a direct LDAP/AD bind (username/password)", matching the rest +of the document and the code. ### Security-011 — Missing tests for security-critical paths @@ -396,7 +424,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Testing coverage | -| Status | Open | +| Status | Resolved | | Location | `tests/ScadaLink.Security.Tests/UnitTest1.cs` | **Description** @@ -417,4 +445,14 @@ DN-escaping of hostile usernames, and idle-timeout behavior across a refresh. Re **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit `pending commit`). Re-triage: most of the listed gaps were +already closed by the Security-001..007 fixes — the regression classes +`SecurityReviewRegressionTests` (StartTLS path, JWT empty/short-key rejection) and +`SecurityReviewRegressionTests2` (DN-injection / hostile-username escaping, `uid`/`cn` +fallback consistency, idle-timeout preserved across refresh) cover them. The finding's +reference to a `SecurityHardeningTests` class is stale — no such class exists in the +suite. Remaining gaps closed here: renamed the test file `UnitTest1.cs` → +`SecurityTests.cs`, and added a new `SecurityReviewRegressionTests3` class with the +LDAP-timeout coverage (Security-009) plus extra no-service-account / DN-path edge cases +(`BuildFallbackUserDn_NoSearchBase_ReturnsBareRdn`, `EscapeLdapDn_LeadingHash_IsEscaped`, +`EscapeLdapDn_NullOrEmpty_ReturnedUnchanged`). Full module suite: 54 tests, all green. diff --git a/docs/requirements/Component-Security.md b/docs/requirements/Component-Security.md index 0e137ef..1d040a7 100644 --- a/docs/requirements/Component-Security.md +++ b/docs/requirements/Component-Security.md @@ -10,7 +10,7 @@ Central cluster. Sites do not have user-facing interfaces and do not perform ind ## Responsibilities -- Authenticate users against LDAP/Active Directory using Windows Integrated Authentication. +- Authenticate users against LDAP/Active Directory using a direct LDAP/AD bind (username/password). - Map LDAP group memberships to system roles. - Enforce role-based access control on all API and UI operations. - Support site-scoped permissions for the Deployment role. diff --git a/src/ScadaLink.Security/LdapAuthService.cs b/src/ScadaLink.Security/LdapAuthService.cs index 62082fc..9ad6ecc 100644 --- a/src/ScadaLink.Security/LdapAuthService.cs +++ b/src/ScadaLink.Security/LdapAuthService.cs @@ -34,6 +34,12 @@ public class LdapAuthService { using var connection = new LdapConnection(); + // Bound how long a hung LDAP server can pin a thread-pool thread. The + // `ct` passed to Task.Run below only prevents the work item from starting; + // it cannot interrupt an in-progress blocking Connect/Bind/Search. This + // timeout is the real safeguard (Security-009). + ApplyConnectionTimeout(connection); + // LDAPS: TLS negotiated at connection time. StartTLS: connect plaintext, // then upgrade the session before any credentials are sent. if (_options.LdapTransport == LdapTransport.Ldaps) @@ -127,6 +133,27 @@ public class LdapAuthService } } + /// + /// Applies to both the socket + /// connect timeout and the per-operation (bind/search) time limit, so a hung or + /// unresponsive LDAP server cannot pin a thread-pool thread indefinitely. The + /// CancellationToken handed to the Task.Run wrappers only guards + /// work-item scheduling and cannot interrupt an in-progress blocking call. + /// + private void ApplyConnectionTimeout(LdapConnection connection) + { + var timeoutMs = _options.LdapConnectionTimeoutMs; + if (timeoutMs <= 0) + return; + + connection.ConnectionTimeout = timeoutMs; + + // LdapConstraints.TimeLimit is the server-side operation time limit in ms. + var constraints = connection.Constraints; + constraints.TimeLimit = timeoutMs; + connection.Constraints = constraints; + } + /// /// Resolves the user's full DN. When a service account is configured, performs a /// search-then-bind lookup. Otherwise falls back to constructing the DN directly. diff --git a/src/ScadaLink.Security/SecurityOptions.cs b/src/ScadaLink.Security/SecurityOptions.cs index 909d23d..2b01579 100644 --- a/src/ScadaLink.Security/SecurityOptions.cs +++ b/src/ScadaLink.Security/SecurityOptions.cs @@ -65,6 +65,16 @@ public class SecurityOptions /// public string LdapGroupAttribute { get; set; } = "memberOf"; + /// + /// Network timeout, in milliseconds, applied to the LDAP socket connect and to + /// LDAP operations (bind/search). The synchronous Novell LDAP calls are wrapped + /// in Task.Run, where the CancellationToken only guards work-item + /// scheduling — it cannot interrupt an in-progress blocking call. This timeout is + /// the real safeguard: it bounds how long a hung LDAP server can pin a thread-pool + /// thread (Security-009). Default 10 seconds. + /// + public int LdapConnectionTimeoutMs { get; set; } = 10_000; + /// /// Symmetric HMAC-SHA256 signing key for cookie-embedded JWTs. Must be at least /// 32 bytes (256 bits) — validated at construction. diff --git a/tests/ScadaLink.Security.Tests/SecurityTests.cs b/tests/ScadaLink.Security.Tests/SecurityTests.cs index 6f5c58c..a044913 100644 --- a/tests/ScadaLink.Security.Tests/SecurityTests.cs +++ b/tests/ScadaLink.Security.Tests/SecurityTests.cs @@ -672,6 +672,81 @@ public class SecurityReviewRegressionTests2 #endregion +#region Code Review Regression Tests — Security-009/011 + +/// +/// Regression tests for Security-009 (no LDAP connection timeout — a hung server can +/// pin a thread-pool thread indefinitely because ct only guards work-item +/// scheduling) and the remaining Security-011 coverage gaps. +/// +public class SecurityReviewRegressionTests3 +{ + // --- Security-009: LDAP connection timeout must be configurable and bounded --- + + [Fact] + public void SecurityOptions_LdapConnectionTimeout_HasSaneDefault() + { + var options = new SecurityOptions(); + // A positive, finite default so a hung LDAP server cannot pin a thread forever. + Assert.True(options.LdapConnectionTimeoutMs > 0); + Assert.True(options.LdapConnectionTimeoutMs <= 60_000, + "Default LDAP connection timeout should be bounded (<= 60s)."); + } + + [Fact] + public async Task AuthenticateAsync_UnreachableHost_FailsWithinConfiguredTimeout() + { + // A routable-but-non-responsive address would otherwise hang for the OS default + // (often minutes). With LdapConnectionTimeoutMs applied to the LdapConnection the + // call must give up promptly. 198.51.100.0/24 (TEST-NET-2, RFC 5737) is reserved + // and not routed, so the connect attempt stalls until the timeout fires. + var options = new SecurityOptions + { + LdapServer = "198.51.100.1", + LdapPort = 389, + LdapTransport = LdapTransport.None, + AllowInsecureLdap = true, + LdapSearchBase = "dc=example,dc=com", + LdapConnectionTimeoutMs = 2_000 + }; + var service = new LdapAuthService(Options.Create(options), NullLogger.Instance); + + var sw = System.Diagnostics.Stopwatch.StartNew(); + var result = await service.AuthenticateAsync("user", "password"); + sw.Stop(); + + Assert.False(result.Success); + // Generous ceiling: the 2s timeout plus scheduling/CI overhead, far below the + // multi-minute OS default that an unconfigured connection would incur. + Assert.True(sw.Elapsed < TimeSpan.FromSeconds(30), + $"Authentication did not honour the LDAP connection timeout: took {sw.Elapsed}."); + } + + // --- Security-011: additional coverage for the no-service-account / DN paths --- + + [Fact] + public void BuildFallbackUserDn_NoSearchBase_ReturnsBareRdn() + { + var dn = LdapAuthService.BuildFallbackUserDn("alice", "", "uid"); + Assert.Equal("uid=alice", dn); + } + + [Fact] + public void EscapeLdapDn_LeadingHash_IsEscaped() + { + Assert.Equal(@"\#admin", LdapAuthService.EscapeLdapDn("#admin")); + } + + [Fact] + public void EscapeLdapDn_NullOrEmpty_ReturnedUnchanged() + { + Assert.Equal("", LdapAuthService.EscapeLdapDn("")); + Assert.Null(LdapAuthService.EscapeLdapDn(null!)); + } +} + +#endregion + #region WP-9: Authorization Policy Tests public class AuthorizationPolicyTests