dohertj2/scadalink-design
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(security): resolve Security-012..015 — fail login on partial LDAP outage, escape-aware DN parsing, idle check on refresh, username normalization

Browse Source
This commit is contained in:
Joseph Doherty
2026-05-17 03:18:33 -04:00
parent f5199e9da9
commit a58cec5776
4 changed files with 411 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files

58
code-reviews/Security/findings.md
View File

@@ -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`.

16
src/ScadaLink.Security/JwtTokenService.cs
View File

@@ -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 <see cref="RecordActivity"/> to advance the
/// anchor when handling a genuine user request.
/// <para>
/// A principal that is already past the idle timeout cannot be refreshed: this
/// method returns <c>null</c> (the same "cannot refresh" signal it uses for missing
/// claims). Enforcing the idle check here — rather than relying on the caller to
/// invoke <see cref="IsIdleTimedOut"/> 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).
/// </para>
/// </summary>
public string? RefreshToken(ClaimsPrincipal currentPrincipal, IReadOnlyList<string> currentRoles, IReadOnlyList<string>? 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));
}

142
src/ScadaLink.Security/LdapAuthService.cs
View File

@@ -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<string>();
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)
/// <summary>
/// Normalises a username by trimming leading and trailing whitespace. Applied once
/// at the top of <see cref="AuthenticateAsync"/> so the same canonical value flows
/// into the LDAP filter, the fallback bind DN, and the JWT <c>Username</c> claim —
/// avoiding two distinct identities for the same person (Security-015).
/// </summary>
public static string NormalizeUsername(string username)
=> username?.Trim() ?? string.Empty;
/// <summary>
/// Builds the final <see cref="LdapAuthResult"/> for a login attempt once the user
/// bind has succeeded. When the group/attribute lookup failed
/// (<paramref name="groupLookupSucceeded"/> 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 <paramref name="groups"/> list is a genuine
/// "no mapped groups" outcome and the login succeeds.
/// </summary>
public static LdapAuthResult BuildAuthResultFromGroupLookup(
string username,
string displayName,
IReadOnlyList<string> 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);
}
/// <summary>
/// Extracts the value of the first RDN from a DN, e.g.
/// <c>ou=SCADA-Admins,ou=groups,dc=...</c> → <c>SCADA-Admins</c>. The scan is
/// RFC 4514 escape-aware: a backslash-escaped <c>,</c> 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).
/// </summary>
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');
}

230
tests/ScadaLink.Security.Tests/SecurityTests.cs
View File

@@ -804,6 +804,236 @@ public class SecurityReviewRegressionTests3
#endregion
#region Code Review Regression Tests — Security-012/013/014/015
/// <summary>
/// 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 (<c>ExtractFirstRdnValue</c> mis-parses group DNs containing an escaped
/// comma), Security-014 (<c>RefreshToken</c> 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).
/// </summary>
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<JwtTokenService>.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<LdapAuthService>.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)
/// <summary>
/// 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.
/// </summary>
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<string>(),
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<string>(),
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<string> { "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