diff --git a/code-reviews/Security/findings.md b/code-reviews/Security/findings.md index 69ce213..f91b950 100644 --- a/code-reviews/Security/findings.md +++ b/code-reviews/Security/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-17 | | Reviewer | claude-agent | | Commit reviewed | `39d737e` | -| Open findings | 4 (1 deferred — Security-008) | +| Open findings | 0 (1 deferred — Security-008) | ## Summary @@ -483,7 +483,7 @@ LDAP-timeout coverage (Security-009) plus extra no-service-account / DN-path edg |--|--| | Severity | Medium | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Security/LdapAuthService.cs:78-118` | **Description** @@ -515,7 +515,19 @@ or be surfaced. **Resolution** -_Unresolved._ +Resolved 2026-05-17 (commit `pending`). Confirmed: the group/attribute `Search` was +wrapped in a `catch (LdapException)` that logged a warning and returned +`new LdapAuthResult(true, …, groups: [])` — a partial LDAP outage produced an +authenticated, zero-role session — and the inner `while`-loop `catch { break; }` masked +real mid-enumeration errors as end-of-results. `AuthenticateAsync` now tracks a +`groupLookupSucceeded` flag (false when any `LdapException` is thrown by the search or +`Next()`); the inner swallowing catch was removed so such errors propagate; the new +`BuildAuthResultFromGroupLookup` helper returns a FAILED `LdapAuthResult` +("directory temporarily unavailable, please try again") on lookup failure while still +treating a genuine empty-groups result as a successful login. Regression tests +`BuildAuthResultFromGroupLookup_LookupFailed_FailsTheLogin`, +`BuildAuthResultFromGroupLookup_LookupSucceededNoGroups_IsAuthenticated`, +`BuildAuthResultFromGroupLookup_LookupSucceededWithGroups_CarriesGroups`. ### Security-013 — `ExtractFirstRdnValue` mis-parses group DNs containing escaped commas @@ -523,7 +535,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Security/LdapAuthService.cs:258-269` | **Description** @@ -545,7 +557,17 @@ library's DN-parsing API (`LdapDN` / RDN accessors) rather than hand-rolled `Ind **Resolution** -_Unresolved._ +Resolved 2026-05-17 (commit `pending`). Confirmed: `ExtractFirstRdnValue` took the +substring between the first `=` and the first `,`, splitting on an RFC 4514 +backslash-escaped comma so a CN like `Acme\, Inc Operators` became `Acme\`. Replaced the +naive `IndexOf` scan with an escape-aware character scan that ignores a backslash-escaped +`,`, unescapes single-character and `\XX` hex escape sequences, and only terminates the +value on an unescaped comma — so a comma-containing group CN is returned intact and +matches its configured `LdapGroupName`. Method was also made `public` for direct +testing. Regression tests `ExtractFirstRdnValue_EscapedComma_KeepsWholeGroupName`, +`ExtractFirstRdnValue_PlainDn_ReturnsFirstRdnValue`, +`ExtractFirstRdnValue_SingleRdn_ReturnsValue`, +`ExtractFirstRdnValue_EscapedSpecials_AreUnescaped`. ### Security-014 — `RefreshToken` re-issues a token without checking the idle timeout @@ -553,7 +575,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Security/JwtTokenService.cs:156-169` | **Description** @@ -580,7 +602,16 @@ regression test covering refresh of a 31-minute-idle token. **Resolution** -_Unresolved._ +Resolved 2026-05-17 (commit `pending`). Confirmed: `RefreshToken` carried `LastActivity` +forward and minted a fresh 15-minute token without ever calling `IsIdleTimedOut`, so the +idle policy depended entirely on caller discipline and a background refresh could keep an +idle-expired session alive indefinitely. `RefreshToken` now calls +`IsIdleTimedOut(currentPrincipal)` after the missing-claims check and returns `null` +(its existing "cannot refresh" signal) when the session is past the idle window, so the +30-minute idle policy holds regardless of caller order; XML doc updated to state the +invariant. Regression tests `RefreshToken_IdleExpiredPrincipal_ReturnsNull`, +`RefreshToken_ActiveSessionWithinIdleWindow_StillRefreshes`, +`RefreshToken_MissingLastActivityClaim_ReturnsNull`. ### Security-015 — Username is not trimmed before use in the LDAP filter, fallback DN, and JWT claims @@ -588,7 +619,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Security/LdapAuthService.cs:20-21`, `:80`, `:122`, `:169`, `:193` | **Description** @@ -613,4 +644,13 @@ trail. **Resolution** -_Unresolved._ +Resolved 2026-05-17 (commit `pending`). Confirmed: an otherwise-valid username with +leading/trailing whitespace passed the `IsNullOrWhiteSpace` guard and flowed verbatim +into the LDAP filter, the fallback DN, and the returned `LdapAuthResult` (and thus the +JWT `Username` claim). Added a `NormalizeUsername` helper (trims whitespace) called once +at the top of `AuthenticateAsync` immediately after the guard; the local `username` +variable is reassigned to the trimmed value so the filter, fallback DN, and result all +use the single canonical identity. Regression tests +`NormalizeUsername_TrimsLeadingAndTrailingWhitespace`, +`BuildFallbackUserDn_TrimmedUsername_NoLeadingTrailingSpace`, +`AuthenticateAsync_UsernameWithSurroundingWhitespace_StillRejectedForInsecure`. diff --git a/src/ScadaLink.Security/JwtTokenService.cs b/src/ScadaLink.Security/JwtTokenService.cs index 17ea864..b0cd4aa 100644 --- a/src/ScadaLink.Security/JwtTokenService.cs +++ b/src/ScadaLink.Security/JwtTokenService.cs @@ -152,6 +152,14 @@ public class JwtTokenService /// otherwise the documented 30-minute idle timeout could never fire for a client /// that polls in the background. Call to advance the /// anchor when handling a genuine user request. + /// + /// A principal that is already past the idle timeout cannot be refreshed: this + /// method returns null (the same "cannot refresh" signal it uses for missing + /// claims). Enforcing the idle check here — rather than relying on the caller to + /// invoke first — guarantees the documented 30-minute + /// idle policy holds regardless of caller discipline; otherwise an idle-expired + /// session could be kept alive indefinitely by background refreshes (Security-014). + /// /// public string? RefreshToken(ClaimsPrincipal currentPrincipal, IReadOnlyList currentRoles, IReadOnlyList? permittedSiteIds) { @@ -164,6 +172,14 @@ public class JwtTokenService return null; } + // An idle-expired session must not be renewed — the user must re-login. + if (IsIdleTimedOut(currentPrincipal)) + { + _logger.LogInformation( + "Cannot refresh token for {Username}: session is past the idle timeout", username); + return null; + } + return GenerateToken(displayName, username, currentRoles, permittedSiteIds, ReadLastActivity(currentPrincipal)); } diff --git a/src/ScadaLink.Security/LdapAuthService.cs b/src/ScadaLink.Security/LdapAuthService.cs index 9ad6ecc..1b8b161 100644 --- a/src/ScadaLink.Security/LdapAuthService.cs +++ b/src/ScadaLink.Security/LdapAuthService.cs @@ -23,6 +23,14 @@ public class LdapAuthService if (string.IsNullOrWhiteSpace(password)) return new LdapAuthResult(false, null, null, null, "Password is required."); + // Trim once, up front: a username with leading/trailing whitespace (copy-paste + // artefacts, mobile keyboards) is otherwise passed verbatim into the LDAP filter, + // the fallback bind DN, and — most consequentially — the JWT Username claim and + // audit trail, producing two distinct identities for the same person + // (Security-015). The IsNullOrWhiteSpace guard above already rejects an + // all-whitespace value, so the trimmed result here is always non-empty. + username = NormalizeUsername(username); + // Enforce TLS unless explicitly allowed for dev/test if (_options.LdapTransport == LdapTransport.None && !_options.AllowInsecureLdap) { @@ -74,6 +82,7 @@ public class LdapAuthService // Query for user attributes and group memberships var displayName = username; var groups = new List(); + var groupLookupSucceeded = true; try { @@ -86,40 +95,45 @@ public class LdapAuthService new[] { _options.LdapDisplayNameAttribute, _options.LdapGroupAttribute }, false), ct); + // `HasMore()` is the loop guard for end-of-results; it returns false + // when the enumeration is exhausted. An LdapException thrown by + // `Next()` inside a HasMore()-guarded loop is therefore NOT a benign + // "no more results" sentinel — it is a genuine error (referral failure, + // server-side limit, transport drop mid-enumeration). The previous + // `catch (LdapException) { break; }` silently truncated the group list + // and masked a partial outage (Security-012); such an exception now + // propagates to the outer catch and fails the login. while (searchResults.HasMore()) { - try - { - var entry = searchResults.Next(); - var dnAttr = entry.GetAttribute(_options.LdapDisplayNameAttribute); - if (dnAttr != null) - displayName = dnAttr.StringValue; + var entry = searchResults.Next(); - var groupAttr = entry.GetAttribute(_options.LdapGroupAttribute); - if (groupAttr != null) - { - foreach (var groupDn in groupAttr.StringValueArray) - { - groups.Add(ExtractFirstRdnValue(groupDn)); - } - } - } - catch (LdapException) + var dnAttr = entry.GetAttribute(_options.LdapDisplayNameAttribute); + if (dnAttr != null) + displayName = dnAttr.StringValue; + + var groupAttr = entry.GetAttribute(_options.LdapGroupAttribute); + if (groupAttr != null) { - // No more results - break; + foreach (var groupDn in groupAttr.StringValueArray) + { + groups.Add(ExtractFirstRdnValue(groupDn)); + } } } } catch (LdapException ex) { - _logger.LogWarning(ex, "Failed to query LDAP attributes for user {Username}; authentication succeeded but group lookup failed", username); - // Auth succeeded even if attribute lookup failed + // A failed group/attribute lookup on initial login means the directory + // is partially unavailable. The design's LDAP-failure rule requires new + // logins to FAIL when LDAP is unavailable — admitting the user here + // would yield an authenticated session with zero roles (Security-012). + _logger.LogWarning(ex, "LDAP group/attribute lookup failed for user {Username}; failing the login per the LDAP-failure rule", username); + groupLookupSucceeded = false; } connection.Disconnect(); - return new LdapAuthResult(true, displayName, username, groups, null); + return BuildAuthResultFromGroupLookup(username, displayName, groups, groupLookupSucceeded); } catch (LdapException ex) { @@ -255,16 +269,92 @@ public class LdapAuthService .Replace("\0", "\\00"); } - private static string ExtractFirstRdnValue(string dn) + /// + /// Normalises a username by trimming leading and trailing whitespace. Applied once + /// at the top of so the same canonical value flows + /// into the LDAP filter, the fallback bind DN, and the JWT Username claim — + /// avoiding two distinct identities for the same person (Security-015). + /// + public static string NormalizeUsername(string username) + => username?.Trim() ?? string.Empty; + + /// + /// Builds the final for a login attempt once the user + /// bind has succeeded. When the group/attribute lookup failed + /// ( is false) the directory is partially + /// unavailable, so the login is FAILED per the design's LDAP-failure rule rather + /// than returning an authenticated session with zero roles (Security-012). When the + /// lookup succeeded, an empty list is a genuine + /// "no mapped groups" outcome and the login succeeds. + /// + public static LdapAuthResult BuildAuthResultFromGroupLookup( + string username, + string displayName, + IReadOnlyList groups, + bool groupLookupSucceeded) { - // Extract the value of the first RDN from a DN. - // Handles cn=, ou=, or any attribute: "ou=SCADA-Admins,ou=groups,dc=..." → "SCADA-Admins" + if (!groupLookupSucceeded) + { + return new LdapAuthResult(false, null, username, null, + "The directory is temporarily unavailable. Please try again."); + } + + return new LdapAuthResult(true, displayName, username, groups, null); + } + + /// + /// Extracts the value of the first RDN from a DN, e.g. + /// ou=SCADA-Admins,ou=groups,dc=...SCADA-Admins. The scan is + /// RFC 4514 escape-aware: a backslash-escaped , inside the RDN value does + /// not terminate it, and recognised escape sequences are unescaped, so a group CN + /// that legitimately contains a comma is returned intact (Security-013). + /// + public static string ExtractFirstRdnValue(string dn) + { + if (string.IsNullOrEmpty(dn)) + return dn; + var equalsIndex = dn.IndexOf('='); if (equalsIndex < 0) return dn; var valueStart = equalsIndex + 1; - var commaIndex = dn.IndexOf(',', valueStart); - return commaIndex > valueStart ? dn[valueStart..commaIndex] : dn[valueStart..]; + var sb = new System.Text.StringBuilder(dn.Length - valueStart); + + for (var i = valueStart; i < dn.Length; i++) + { + var c = dn[i]; + if (c == '\\' && i + 1 < dn.Length) + { + var next = dn[i + 1]; + // RFC 4514 hex escape: \XX (two hex digits). + if (i + 2 < dn.Length && IsHexDigit(next) && IsHexDigit(dn[i + 2])) + { + sb.Append((char)Convert.ToInt32(dn.Substring(i + 1, 2), 16)); + i += 2; + } + else + { + // Single-character escape (e.g. \, \+ \\ \" \; etc.) — emit the + // escaped character literally and skip the backslash. + sb.Append(next); + i += 1; + } + continue; + } + + if (c == ',') + { + // Unescaped comma terminates the first RDN. + break; + } + + sb.Append(c); + } + + return sb.ToString(); } + + private static bool IsHexDigit(char c) + => (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F'); } diff --git a/tests/ScadaLink.Security.Tests/SecurityTests.cs b/tests/ScadaLink.Security.Tests/SecurityTests.cs index 9632719..4abc7af 100644 --- a/tests/ScadaLink.Security.Tests/SecurityTests.cs +++ b/tests/ScadaLink.Security.Tests/SecurityTests.cs @@ -804,6 +804,236 @@ public class SecurityReviewRegressionTests3 #endregion +#region Code Review Regression Tests — Security-012/013/014/015 + +/// +/// Regression tests for Security-012 (a partial LDAP outage during login — bind OK but +/// group search failing — silently yields a roleless authenticated session), +/// Security-013 (ExtractFirstRdnValue mis-parses group DNs containing an escaped +/// comma), Security-014 (RefreshToken re-issues a token without checking the idle +/// timeout, so an idle-expired session can be renewed indefinitely), and Security-015 +/// (a username with leading/trailing whitespace is not trimmed before use). +/// +public class SecurityReviewRegressionTests4 +{ + private static SecurityOptions JwtOptions() => new() + { + JwtSigningKey = "this-is-a-test-signing-key-for-hmac-sha256-must-be-long-enough", + JwtExpiryMinutes = 15, + IdleTimeoutMinutes = 30, + JwtRefreshThresholdMinutes = 5 + }; + + private static JwtTokenService CreateJwtService(SecurityOptions? options = null) => + new(Options.Create(options ?? JwtOptions()), NullLogger.Instance); + + // --- Security-014: RefreshToken must reject an idle-expired principal --- + + [Fact] + public void RefreshToken_IdleExpiredPrincipal_ReturnsNull() + { + // A user idle for 31 minutes (past the 30-minute idle window). Even though the + // DisplayName/Username claims are present, the refresh must be refused so an + // idle-expired session cannot be renewed by a background request. + var service = CreateJwtService(); + var idleActivity = DateTimeOffset.UtcNow.AddMinutes(-31); + var principal = new ClaimsPrincipal(new ClaimsIdentity(new[] + { + new Claim(JwtTokenService.DisplayNameClaimType, "User"), + new Claim(JwtTokenService.UsernameClaimType, "user"), + new Claim(JwtTokenService.LastActivityClaimType, idleActivity.ToString("o")) + }, "test")); + + var refreshed = service.RefreshToken(principal, new[] { "Admin" }, null); + + Assert.Null(refreshed); + } + + [Fact] + public void RefreshToken_ActiveSessionWithinIdleWindow_StillRefreshes() + { + // A user idle for 10 minutes (well inside the 30-minute window): refresh must + // still succeed — the idle-timeout guard must not break the normal sliding path. + var service = CreateJwtService(); + var recentActivity = DateTimeOffset.UtcNow.AddMinutes(-10); + var principal = new ClaimsPrincipal(new ClaimsIdentity(new[] + { + new Claim(JwtTokenService.DisplayNameClaimType, "User"), + new Claim(JwtTokenService.UsernameClaimType, "user"), + new Claim(JwtTokenService.LastActivityClaimType, recentActivity.ToString("o")) + }, "test")); + + var refreshed = service.RefreshToken(principal, new[] { "Admin" }, null); + + Assert.NotNull(refreshed); + } + + [Fact] + public void RefreshToken_MissingLastActivityClaim_ReturnsNull() + { + // No LastActivity claim — IsIdleTimedOut treats this as timed out, so the + // refresh must be refused rather than minting a fresh session out of nothing. + var service = CreateJwtService(); + var principal = new ClaimsPrincipal(new ClaimsIdentity(new[] + { + new Claim(JwtTokenService.DisplayNameClaimType, "User"), + new Claim(JwtTokenService.UsernameClaimType, "user") + }, "test")); + + var refreshed = service.RefreshToken(principal, new[] { "Admin" }, null); + + Assert.Null(refreshed); + } + + // --- Security-013: ExtractFirstRdnValue must honour RFC 4514 escaped commas --- + + [Fact] + public void ExtractFirstRdnValue_EscapedComma_KeepsWholeGroupName() + { + // A CN that legitimately contains a comma is RFC 4514 backslash-escaped in the + // memberOf DN. The extracted group name must be the full unescaped CN value, + // not the fragment before the escaped comma. + var name = LdapAuthService.ExtractFirstRdnValue( + @"cn=Acme\, Inc Operators,ou=groups,dc=example,dc=com"); + Assert.Equal("Acme, Inc Operators", name); + } + + [Fact] + public void ExtractFirstRdnValue_PlainDn_ReturnsFirstRdnValue() + { + var name = LdapAuthService.ExtractFirstRdnValue("ou=SCADA-Admins,ou=groups,dc=example,dc=com"); + Assert.Equal("SCADA-Admins", name); + } + + [Fact] + public void ExtractFirstRdnValue_SingleRdn_ReturnsValue() + { + var name = LdapAuthService.ExtractFirstRdnValue("cn=Operators"); + Assert.Equal("Operators", name); + } + + [Fact] + public void ExtractFirstRdnValue_EscapedSpecials_AreUnescaped() + { + // RFC 4514 escape sequences (escaped '+', '"', '\\') must be unescaped in the + // returned value so it matches the configured LdapGroupName verbatim. + var name = LdapAuthService.ExtractFirstRdnValue( + @"cn=A\+B\\C,ou=groups,dc=example,dc=com"); + Assert.Equal(@"A+B\C", name); + } + + // --- Security-015: username must be trimmed before use --- + + [Fact] + public void BuildFallbackUserDn_TrimmedUsername_NoLeadingTrailingSpace() + { + // The whitespace-edge escaping in EscapeLdapDn only fires when whitespace is NOT + // trimmed. AuthenticateAsync trims first; this asserts the trimmed value yields + // a clean DN with no escaped edge spaces. + var dn = LdapAuthService.BuildFallbackUserDn("alice".Trim(), "dc=example,dc=com", "uid"); + Assert.Equal("uid=alice,dc=example,dc=com", dn); + } + + [Fact] + public async Task AuthenticateAsync_UsernameWithSurroundingWhitespace_StillRejectedForInsecure() + { + // Sanity guard: a padded but otherwise-valid username is not rejected by the + // IsNullOrWhiteSpace guard — it passes through to the (here, insecure-LDAP) path. + var options = new SecurityOptions + { + LdapServer = "ldap.example.com", + LdapPort = 389, + LdapTransport = LdapTransport.None, + AllowInsecureLdap = false + }; + var service = new LdapAuthService(Options.Create(options), NullLogger.Instance); + + var result = await service.AuthenticateAsync(" alice ", "password"); + + // Reaches the insecure-LDAP guard (not the empty-username guard) — proves the + // padded username is treated as a real, non-empty username. + Assert.False(result.Success); + Assert.Contains("Insecure LDAP", result.ErrorMessage); + } + + [Fact] + public void NormalizeUsername_TrimsLeadingAndTrailingWhitespace() + { + Assert.Equal("alice", LdapAuthService.NormalizeUsername(" alice ")); + Assert.Equal("alice", LdapAuthService.NormalizeUsername("alice")); + Assert.Equal("alice", LdapAuthService.NormalizeUsername("\talice\n")); + } +} + +#endregion + +#region Code Review Regression Tests — Security-012 (partial LDAP outage) + +/// +/// Regression tests for Security-012: a partial LDAP outage during login (the user bind +/// succeeds but the subsequent group/attribute search fails) must fail the login per the +/// design's LDAP-failure rule, rather than returning an authenticated session with zero +/// roles. These exercise the seam through a stubbed group-lookup so the bind itself can +/// be treated as successful. +/// +public class Security012GroupLookupFailureTests +{ + private static SecurityOptions Options() => new() + { + LdapServer = "ldap.example.com", + LdapPort = 636, + LdapTransport = LdapTransport.Ldaps, + LdapSearchBase = "dc=example,dc=com" + }; + + [Fact] + public void BuildAuthResultFromGroupLookup_LookupFailed_FailsTheLogin() + { + // When the group lookup failed (directory partially unavailable mid-login) the + // result must be a FAILED login — not a success with an empty group list. + var result = LdapAuthService.BuildAuthResultFromGroupLookup( + username: "alice", + displayName: "Alice", + groups: new List(), + groupLookupSucceeded: false); + + Assert.False(result.Success); + Assert.Null(result.Groups); + Assert.False(string.IsNullOrEmpty(result.ErrorMessage)); + } + + [Fact] + public void BuildAuthResultFromGroupLookup_LookupSucceededNoGroups_IsAuthenticated() + { + // A genuine "user belongs to no mapped groups" outcome must remain a successful + // login — it must be distinguishable from a failed lookup. + var result = LdapAuthService.BuildAuthResultFromGroupLookup( + username: "alice", + displayName: "Alice", + groups: new List(), + groupLookupSucceeded: true); + + Assert.True(result.Success); + Assert.NotNull(result.Groups); + Assert.Empty(result.Groups!); + } + + [Fact] + public void BuildAuthResultFromGroupLookup_LookupSucceededWithGroups_CarriesGroups() + { + var result = LdapAuthService.BuildAuthResultFromGroupLookup( + username: "alice", + displayName: "Alice", + groups: new List { "SCADA-Admins" }, + groupLookupSucceeded: true); + + Assert.True(result.Success); + Assert.Equal(new[] { "SCADA-Admins" }, result.Groups); + } +} + +#endregion + #region WP-9: Authorization Policy Tests public class AuthorizationPolicyTests