20 KiB
Code Review — Security
| Field | Value |
|---|---|
| Module | src/ScadaLink.Security |
| Design doc | docs/requirements/Component-Security.md |
| Status | Reviewed |
| Last reviewed | 2026-05-16 |
| Reviewer | claude-agent |
| Commit reviewed | 9c60592 |
| Open findings | 4 |
Summary
The Security module is small and reasonably structured: a stateless LdapAuthService
for search-then-bind authentication, a JwtTokenService for HMAC-signed cookie tokens,
a RoleMapper that resolves LDAP groups to roles, and ASP.NET Core authorization
policies plus a site-scope handler. Unit-test coverage of the happy paths is decent.
However, the review surfaced several real security weaknesses, the most serious being
that StartTLS is dead code (the design's "LDAPS or StartTLS" requirement is only
half met), that the authentication cookie is not marked Secure despite the design
mandating it, and that the JWT signing key is never length-validated so a weak or
empty key is silently accepted. There is also a genuine DN-injection gap in the
no-service-account fallback path, a filter/DN attribute mismatch (uid= vs cn=) that
makes that fallback path internally inconsistent, and an N+1 query in RoleMapper.
JWT validation also disables issuer/audience checks and the idle-timeout claim is reset
on every refresh, weakening the documented 30-minute idle policy. None of these are
crash/data-loss bugs, but the TLS, cookie, and key-validation items are security
defects that should be fixed before any production deployment.
Checklist coverage
| # | Category | Examined | Notes |
|---|---|---|---|
| 1 | Correctness & logic bugs | ☑ | uid=/cn= attribute mismatch between search filter and fallback DN construction (Security-004); StartTLS branch is unreachable (Security-001). |
| 2 | Akka.NET conventions | ☑ | No actors in this module — AddSecurityActors is an empty placeholder. Nothing to assess. |
| 3 | Concurrency & thread safety | ☑ | Services are stateless and DI-scoped; LDAP sync calls wrapped in Task.Run. No shared mutable state. No issues found. |
| 4 | Error handling & resilience | ☑ | LDAP failure paths return structured LdapAuthResult; group-lookup failure is tolerated per design. ct not honored inside Task.Run bodies (Security-009). |
| 5 | Security | ☑ | StartTLS dead code (Security-001), cookie not Secure (Security-002), JWT key unvalidated (Security-003), DN injection (Security-005), no issuer/audience validation (Security-006), idle-timeout reset on refresh (Security-007). |
| 6 | Performance & resource management | ☑ | N+1 scope-rule query in RoleMapper (Security-008). LdapConnection correctly disposed via using. |
| 7 | Design-document adherence | ☑ | StartTLS unsupported and Secure cookie missing both contradict the design doc; design also says "Windows Integrated Authentication" in Responsibilities, contradicting its own Authentication section (Security-010). |
| 8 | Code organization & conventions | ☑ | SecurityOptions correctly owned by the component; repository interface in Commons. No issues found. |
| 9 | Testing coverage | ☑ | No tests for RoleMapper N+1 behavior, DN-injection inputs, StartTLS path, or idle-timeout-after-refresh. Insecure-config combinations under-tested (Security-011). |
| 10 | Documentation & comments | ☑ | SecurityOptions XML docs say direct bind uses cn={username} while the search filter uses uid= — comment is misleading (covered under Security-004). |
Findings
Security-001 — StartTLS upgrade path is unreachable dead code
| Severity | High |
| Category | Security |
| Status | Resolved |
| Location | src/ScadaLink.Security/LdapAuthService.cs:37-47 |
Description
When LdapUseTls is true the code sets connection.SecureSocketLayer = true (LDAPS).
The subsequent StartTLS block is guarded by if (_options.LdapUseTls && !connection.SecureSocketLayer).
Because SecureSocketLayer was just set to true, the second condition !connection.SecureSocketLayer
is always false, so connection.StartTls() is never called. The design doc explicitly
states LDAP connections must use "LDAPS (port 636) or StartTLS" — StartTLS is in
practice unsupported. A deployment that intends to use StartTLS on port 389 would get a
plaintext LDAPS-mode connection attempt that fails, or worse, an operator may disable
TLS entirely to make it work, sending credentials in cleartext.
Recommendation
Introduce an explicit transport mode (e.g. LdapTransport { Ldaps, StartTls, None })
or a separate LdapUseStartTls flag. For StartTLS, leave SecureSocketLayer false,
call connection.Connect, then call connection.StartTls() and verify the negotiated
session is encrypted before binding. Remove the unreachable conditional.
Resolution
Resolved 2026-05-16 (commit <pending>). Added an explicit LdapTransport enum
(Ldaps/StartTls/None); SecureSocketLayer is set only for LDAPS, and the
StartTLS branch now connects in plaintext, calls StartTls(), and verifies
connection.Tls before binding. LdapUseTls is retained as a compatibility shim
mapping onto the enum. Regression tests AuthenticateAsync_StartTlsTransport_AttemptsConnection
and AuthenticateAsync_NoTlsTransport_RejectedWithoutAllowInsecure.
Security-002 — Authentication cookie is not marked Secure
| Severity | High |
| Category | Security |
| Status | Resolved |
| Location | src/ScadaLink.Security/ServiceCollectionExtensions.cs:16-23 |
Description
AddCookie sets HttpOnly = true and SameSite = Strict but never sets
options.Cookie.SecurePolicy. The ASP.NET Core default is CookieSecurePolicy.SameAsRequest,
which permits the cookie (carrying the embedded JWT — a bearer credential) to be sent
over plain HTTP. The design doc states the cookie is "HttpOnly and Secure (requires
HTTPS)". As written, the module does not enforce that requirement; a misconfigured or
HTTP-fronted deployment would transmit the session token in cleartext.
Recommendation
Set options.Cookie.SecurePolicy = CookieSecurePolicy.Always in AddCookie. Consider
also setting ExpireTimeSpan and SlidingExpiration to align the cookie lifetime with
the documented 15-minute JWT / 30-minute idle policy.
Resolution
Resolved 2026-05-16 (commit <pending>). Confirmed the cookie is configured in this
module (ServiceCollectionExtensions.AddSecurity), not CentralUI — no misattribution.
Added options.Cookie.SecurePolicy = CookieSecurePolicy.Always so the JWT-bearing
cookie is never sent over plain HTTP. Regression test
AddSecurity_AuthCookie_IsMarkedSecureAlways. (ExpireTimeSpan/SlidingExpiration
tuning left as a separate, lower-priority improvement.)
Security-003 — JWT signing key length is never validated
| Severity | High |
| Category | Security |
| Status | Resolved |
| Location | src/ScadaLink.Security/JwtTokenService.cs:33, src/ScadaLink.Security/SecurityOptions.cs:42 |
Description
SecurityOptions.JwtSigningKey defaults to string.Empty and is fed directly into
new SymmetricSecurityKey(Encoding.UTF8.GetBytes(_options.JwtSigningKey)) with no
validation. HMAC-SHA256 requires a key of at least 256 bits (32 bytes); a short or empty
key produces a trivially forgeable token. The SecurityHardeningTests comment claims a
minimum length is "enforced", but no code in this module enforces it — the test only
asserts that a 32+ char key works. A deployment with a missing or short JwtSigningKey
would start successfully and issue weakly-signed tokens.
Recommendation
Validate JwtSigningKey at startup — fail fast if it is empty or shorter than 32 bytes.
Use an IValidateOptions<SecurityOptions> validator or guard in the JwtTokenService
constructor so a weak key is rejected before any token is issued.
Resolution
Resolved 2026-05-16 (commit <pending>). The JwtTokenService constructor now
fails fast with an InvalidOperationException when JwtSigningKey is empty or
shorter than 32 bytes (SecurityOptions.MinJwtSigningKeyBytes), so a weak key is
rejected before any token is issued. The misleading SecurityOptions XML doc was
corrected to state the requirement. Regression tests
JwtTokenService_EmptySigningKey_ThrowsAtConstruction,
JwtTokenService_ShortSigningKey_ThrowsAtConstruction, and
JwtTokenService_AdequateSigningKey_ConstructsSuccessfully.
Security-004 — Search filter uses uid= while fallback DN construction uses cn=
| Severity | Medium |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | src/ScadaLink.Security/LdapAuthService.cs:66, :138, :157-159 |
Description
AuthenticateAsync and ResolveUserDnAsync build the search filter as
(uid={username}), but the no-service-account fallback in ResolveUserDnAsync
constructs the bind DN as cn={username},{LdapSearchBase}. The SecurityOptions.LdapServiceAccountDn
XML comment also documents the fallback as cn={username},{LdapSearchBase}. A directory
keyed on uid will succeed via search-then-bind but fail via the direct-bind fallback
(and vice versa). The attribute used for lookup is hard-coded and inconsistent across
the two code paths, so the two configuration modes are not interchangeable.
Recommendation
Introduce a single configurable LdapUserIdAttribute (default uid) and use it
consistently in both the search filter and the fallback DN. Update the XML doc to match.
Resolution
Resolved 2026-05-16 (commit pending). Confirmed: the search filter was hard-coded
(uid={username}) (both in AuthenticateAsync and ResolveUserDnAsync) while the
fallback DN used cn={username} — the two auth modes were not interchangeable. Added
a configurable SecurityOptions.LdapUserIdAttribute (default uid) used for both the
search filter and the fallback DN via the new BuildFallbackUserDn helper, and
corrected the LdapServiceAccountDn XML doc to reference {LdapUserIdAttribute}.
Regression tests BuildFallbackUserDn_UsesConfiguredUserIdAttribute,
BuildFallbackUserDn_HonoursNonDefaultUserIdAttribute,
SecurityOptions_LdapUserIdAttribute_DefaultsToUid.
Security-005 — DN injection in the no-service-account bind fallback
| Severity | Medium |
| Category | Security |
| Status | Resolved |
| Location | src/ScadaLink.Security/LdapAuthService.cs:157-159 |
Description
When no service account is configured, the user-supplied username is interpolated
directly into a distinguished name: $"cn={username},{LdapSearchBase}". EscapeLdapFilter
escapes search-filter metacharacters, but DN construction requires a different
escaping scheme (RFC 4514 — ,, +, ", \, <, >, ;, leading/trailing spaces).
No DN escaping is applied here. A username such as victim,ou=admins alters the DN
structure, allowing a caller to attempt a bind as a different DN than intended. Combined
with the username.Contains('=') shortcut at line 129 — which lets a caller supply a
full arbitrary DN — the fallback path gives the client undue control over the bind
identity.
Recommendation
Apply RFC 4514 DN-component escaping to username before interpolation, or use the
LDAP library's DN-builder API. Reconsider the Contains('=') shortcut — accepting a
raw DN from untrusted input is risky; restrict it or remove it.
Resolution
Resolved 2026-05-16 (commit pending). Confirmed: the fallback path interpolated
the raw username straight into cn={username},{LdapSearchBase} with no DN escaping,
and the username.Contains('=') shortcut let a caller supply an arbitrary bind DN.
Added an RFC 4514 EscapeLdapDn helper (escapes , + " \ < > ;, leading/trailing
space, leading #, NUL) applied in BuildFallbackUserDn, so a username such as
victim,ou=admins can no longer alter the DN structure. The Contains('=') raw-DN
shortcut was removed entirely — untrusted input no longer controls the bind identity.
Regression tests BuildFallbackUserDn_EscapesDnMetacharacters,
EscapeLdapDn_EscapesAllRfc4514Specials, EscapeLdapDn_EscapesLeadingAndTrailingSpaces.
Security-006 — JWT validation disables issuer and audience checks
| Severity | Medium |
| Category | Security |
| Status | Resolved |
| Location | src/ScadaLink.Security/JwtTokenService.cs:67-75, :56-59 |
Description
ValidateToken sets ValidateIssuer = false and ValidateAudience = false, and
GenerateToken never sets an iss or aud. With a shared symmetric HMAC key, any
other system or component that signs JWTs with the same key would produce tokens this
service accepts. While the design states the key is shared only between the two central
nodes, omitting issuer/audience binding removes a cheap defense-in-depth control and
makes accidental key reuse (e.g. the same secret used for another internal token)
silently exploitable.
Recommendation
Set a fixed Issuer and Audience (e.g. "scadalink-central") when generating tokens
and enable ValidateIssuer/ValidateAudience with the matching expected values during
validation.
Resolution
Resolved 2026-05-16 (commit pending). Confirmed: GenerateToken set neither iss
nor aud and ValidateToken had ValidateIssuer = false/ValidateAudience = false.
GenerateToken now binds JwtTokenService.TokenIssuer/TokenAudience
(both "scadalink-central") into every token, and ValidateToken enables
ValidateIssuer/ValidateAudience against those fixed values — a token signed with
the shared key but a foreign issuer is now rejected. Regression tests
GenerateToken_SetsIssuerAndAudience, ValidateToken_RejectsTokenWithWrongIssuer,
ValidateToken_AcceptsOwnIssuerAndAudience.
Security-007 — Idle-timeout claim is reset on every token refresh
| Severity | Medium |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | src/ScadaLink.Security/JwtTokenService.cs:40, :111-123 |
Description
The design states the 30-minute idle timeout is tracked via a "last-activity timestamp
in the token", and IsIdleTimedOut reads the LastActivity claim. But RefreshToken
calls GenerateToken, which unconditionally writes LastActivity = DateTimeOffset.UtcNow.
Token refresh fires whenever a request arrives within ~5 minutes of expiry. The result
is that LastActivity reflects token issuance time, not genuine user activity — and
since refresh itself is a request, the timestamp keeps moving forward. A more subtle
consequence: the idle window is effectively measured from the last refresh, not the
last real interaction, so the documented "no requests within the idle window" semantics
are not faithfully implemented. The claim name LastActivity is also misleading.
Recommendation
Decide explicitly how activity is tracked. Either (a) carry the original LastActivity
forward across refreshes and update it only on real request handling in the middleware,
or (b) rename the claim to IssuedAt/TokenCreated and document that the idle window
is measured from issuance. Whichever is chosen, ensure IsIdleTimedOut and the refresh
path agree on the semantics.
Resolution
Resolved 2026-05-16 (commit pending). Confirmed: RefreshToken → GenerateToken
unconditionally wrote LastActivity = UtcNow, so the idle clock reset on every
refresh and the documented 30-minute idle timeout could never fire for a client that
polls in the background. Implemented option (a) — the Security-side half of the
documented "15-min sliding + 30-min idle" policy (the cross-module partner of
CentralUI-005): GenerateToken now takes an optional lastActivity anchor;
RefreshToken carries the existing LastActivity claim forward unchanged, and a
new explicit RecordActivity method advances the anchor to now — to be called by the
CentralUI request pipeline on genuine user interaction (not on a background refresh).
IsIdleTimedOut is unchanged and now agrees with the refresh path. The remaining
CentralUI-side wiring (calling RecordActivity from the middleware, setting
SlidingExpiration/ExpireTimeSpan) stays tracked under CentralUI-005; this finding's
Security-side defect — the reset-on-refresh bug — is fully fixed here. Regression tests
RefreshToken_PreservesOriginalLastActivityClaim,
RefreshToken_DoesNotResetIdleTimeoutWhenUserIsActuallyIdle,
RecordActivity_UpdatesLastActivityToNow.
Security-008 — N+1 query loading site-scope rules in RoleMapper
| Severity | Low |
| Category | Performance & resource management |
| Status | Open |
| Location | src/ScadaLink.Security/RoleMapper.cs:25-48 |
Description
MapGroupsToRolesAsync first calls GetAllMappingsAsync, then inside the per-mapping
loop calls GetScopeRulesForMappingAsync(mapping.Id, ct) once for every matched
Deployment mapping. This is an N+1 query pattern executed on the login hot path and on
every 15-minute token refresh. With multiple site-scoped Deployment groups it issues a
round-trip per group.
Recommendation
Add a repository method that loads scope rules for a set of mapping IDs in one query (or eager-loads them with the mappings), and resolve all scope rules with a single call.
Resolution
Unresolved.
Security-009 — CancellationToken not honored inside Task.Run LDAP calls
| Severity | Low |
| Category | Error handling & resilience |
| Status | Open |
| Location | src/ScadaLink.Security/LdapAuthService.cs:42, :46, :51, :56-57, :67-73, :135, :139-145 |
Description
The synchronous Novell LDAP calls are wrapped in Task.Run(() => ..., ct). The ct
argument only prevents the work item from starting if cancellation is already
signaled; once a connection.Connect/Bind/Search call is in progress it cannot be
cancelled. A cancelled or timed-out login request will continue to occupy a thread-pool
thread and an LDAP connection until the blocking call returns on its own. There is also
no explicit network/operation timeout configured on the LdapConnection.
Recommendation
Configure LdapConnection.ConnectionTimeout and search/operation time limits so a
hung LDAP server cannot pin a thread indefinitely. Document that ct only guards
work-item scheduling, or implement a timeout-with-disconnect fallback.
Resolution
Unresolved.
Security-010 — Design doc contradicts itself on Windows Integrated Authentication
| Severity | Low |
| Category | Design-document adherence |
| Status | Open |
| Location | docs/requirements/Component-Security.md:13 (vs. :23) |
Description
The Responsibilities section states the component authenticates "using Windows Integrated Authentication", but the Authentication section (line 23) and CLAUDE.md explicitly state "No Windows Integrated Authentication ... authenticates directly against LDAP/AD, not via Kerberos/NTLM" — which is what the code actually does (direct LDAP bind). The Responsibilities line is stale and contradicts both the rest of the doc and the implementation.
Recommendation
Fix Component-Security.md:13 to say "using a direct LDAP/Active Directory bind"
to match the implemented behavior and the rest of the document.
Resolution
Unresolved.
Security-011 — Missing tests for security-critical paths
| Severity | Low |
| Category | Testing coverage |
| Status | Open |
| Location | tests/ScadaLink.Security.Tests/UnitTest1.cs |
Description
The test suite covers happy paths well but omits several security-relevant cases:
no test exercises the StartTLS path (Security-001), the DN-injection / Contains('=')
fallback inputs (Security-005), JWT validation with a too-short or empty signing key
(Security-003), IsIdleTimedOut returning true after a token has been refreshed
(Security-007), or the uid/cn mismatch in the no-service-account path (Security-004).
The integration SecurityHardeningTests only asserts default option values, not
enforcement. The test file is still named UnitTest1.cs.
Recommendation
Add negative/edge-case tests for the items above, particularly key-length rejection,
DN-escaping of hostile usernames, and idle-timeout behavior across a refresh. Rename
UnitTest1.cs to a descriptive name.
Resolution
Unresolved.