From 0d9363766d32fccaed9560f7dca5c77dc40bda2b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 16 May 2026 19:47:17 -0400 Subject: [PATCH] =?UTF-8?q?fix(security):=20resolve=20Security-001/002/003?= =?UTF-8?q?=20=E2=80=94=20reachable=20StartTLS=20path,=20Secure=20cookie,?= =?UTF-8?q?=20JWT=20signing=20key=20validation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- code-reviews/Security/findings.md | 31 +++-- src/ScadaLink.Security/JwtTokenService.cs | 13 ++ src/ScadaLink.Security/LdapAuthService.cs | 14 ++- src/ScadaLink.Security/LdapTransport.cs | 26 ++++ src/ScadaLink.Security/SecurityOptions.cs | 28 ++++- .../ServiceCollectionExtensions.cs | 3 + tests/ScadaLink.Security.Tests/UnitTest1.cs | 118 ++++++++++++++++++ 7 files changed, 222 insertions(+), 11 deletions(-) create mode 100644 src/ScadaLink.Security/LdapTransport.cs diff --git a/code-reviews/Security/findings.md b/code-reviews/Security/findings.md index 5ef117c..4b0cf76 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 | 11 | +| Open findings | 8 | ## Summary @@ -51,7 +51,7 @@ defects that should be fixed before any production deployment. |--|--| | Severity | High | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Security/LdapAuthService.cs:37-47` | **Description** @@ -74,7 +74,12 @@ session is encrypted before binding. Remove the unreachable conditional. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit ``). 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` @@ -82,7 +87,7 @@ _Unresolved._ |--|--| | Severity | High | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Security/ServiceCollectionExtensions.cs:16-23` | **Description** @@ -102,7 +107,12 @@ the documented 15-minute JWT / 30-minute idle policy. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit ``). 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 @@ -110,7 +120,7 @@ _Unresolved._ |--|--| | Severity | High | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Security/JwtTokenService.cs:33`, `src/ScadaLink.Security/SecurityOptions.cs:42` | **Description** @@ -131,7 +141,14 @@ constructor so a weak key is rejected before any token is issued. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit ``). 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=` diff --git a/src/ScadaLink.Security/JwtTokenService.cs b/src/ScadaLink.Security/JwtTokenService.cs index f4f892b..14747b3 100644 --- a/src/ScadaLink.Security/JwtTokenService.cs +++ b/src/ScadaLink.Security/JwtTokenService.cs @@ -22,6 +22,19 @@ public class JwtTokenService { _options = options?.Value ?? throw new ArgumentNullException(nameof(options)); _logger = logger ?? throw new ArgumentNullException(nameof(logger)); + + // Fail fast: a missing or short signing key produces trivially forgeable tokens. + // HMAC-SHA256 requires a key of at least 256 bits (32 bytes). + var keyByteLength = string.IsNullOrEmpty(_options.JwtSigningKey) + ? 0 + : Encoding.UTF8.GetByteCount(_options.JwtSigningKey); + if (keyByteLength < SecurityOptions.MinJwtSigningKeyBytes) + { + throw new InvalidOperationException( + $"SecurityOptions.JwtSigningKey must be at least {SecurityOptions.MinJwtSigningKeyBytes} bytes " + + $"(256 bits) for HMAC-SHA256; the configured key is {keyByteLength} byte(s). " + + "Configure a strong signing key before starting the service."); + } } public string GenerateToken( diff --git a/src/ScadaLink.Security/LdapAuthService.cs b/src/ScadaLink.Security/LdapAuthService.cs index 6656adb..2ddffb7 100644 --- a/src/ScadaLink.Security/LdapAuthService.cs +++ b/src/ScadaLink.Security/LdapAuthService.cs @@ -24,7 +24,7 @@ public class LdapAuthService return new LdapAuthResult(false, null, null, null, "Password is required."); // Enforce TLS unless explicitly allowed for dev/test - if (!_options.LdapUseTls && !_options.AllowInsecureLdap) + if (_options.LdapTransport == LdapTransport.None && !_options.AllowInsecureLdap) { return new LdapAuthResult(false, null, null, null, "Insecure LDAP connections are not allowed. Enable TLS or set AllowInsecureLdap for dev/test."); @@ -34,16 +34,24 @@ public class LdapAuthService { using var connection = new LdapConnection(); - if (_options.LdapUseTls) + // LDAPS: TLS negotiated at connection time. StartTLS: connect plaintext, + // then upgrade the session before any credentials are sent. + if (_options.LdapTransport == LdapTransport.Ldaps) { connection.SecureSocketLayer = true; } await Task.Run(() => connection.Connect(_options.LdapServer, _options.LdapPort), ct); - if (_options.LdapUseTls && !connection.SecureSocketLayer) + if (_options.LdapTransport == LdapTransport.StartTls) { await Task.Run(() => connection.StartTls(), ct); + + if (!connection.Tls) + { + return new LdapAuthResult(false, null, null, null, + "StartTLS upgrade did not produce an encrypted session."); + } } // Resolve the user's actual DN, then bind with their credentials diff --git a/src/ScadaLink.Security/LdapTransport.cs b/src/ScadaLink.Security/LdapTransport.cs new file mode 100644 index 0000000..42508fd --- /dev/null +++ b/src/ScadaLink.Security/LdapTransport.cs @@ -0,0 +1,26 @@ +namespace ScadaLink.Security; + +/// +/// Transport security mode for the LDAP connection. The design requires either +/// LDAPS or StartTLS in production; is for dev/test only and +/// must be paired with . +/// +public enum LdapTransport +{ + /// + /// LDAPS — TLS negotiated at connection time (typically port 636). + /// + Ldaps, + + /// + /// StartTLS — connect in plaintext (typically port 389), then upgrade the + /// session to TLS before binding. + /// + StartTls, + + /// + /// No transport security. Dev/test only — requires + /// to be true. + /// + None +} diff --git a/src/ScadaLink.Security/SecurityOptions.cs b/src/ScadaLink.Security/SecurityOptions.cs index 71b784d..e6aac5f 100644 --- a/src/ScadaLink.Security/SecurityOptions.cs +++ b/src/ScadaLink.Security/SecurityOptions.cs @@ -4,7 +4,24 @@ public class SecurityOptions { public string LdapServer { get; set; } = string.Empty; public int LdapPort { get; set; } = 389; - public bool LdapUseTls { get; set; } = true; + + /// + /// Transport security mode for the LDAP connection. Defaults to LDAPS. + /// Use to connect on the plaintext port + /// and upgrade the session before binding. + /// + public LdapTransport LdapTransport { get; set; } = LdapTransport.Ldaps; + + /// + /// True when the configured transport provides encryption (LDAPS or StartTLS). + /// Retained for backward compatibility: assigning a value maps onto + /// (true => LDAPS, false => None). + /// + public bool LdapUseTls + { + get => LdapTransport != LdapTransport.None; + set => LdapTransport = value ? LdapTransport.Ldaps : LdapTransport.None; + } /// /// Allow insecure (non-TLS) LDAP connections. ONLY for dev/test with GLAuth. @@ -39,7 +56,16 @@ public class SecurityOptions /// public string LdapGroupAttribute { get; set; } = "memberOf"; + /// + /// Symmetric HMAC-SHA256 signing key for cookie-embedded JWTs. Must be at least + /// 32 bytes (256 bits) — validated at construction. + /// public string JwtSigningKey { get; set; } = string.Empty; + + /// + /// Minimum signing-key length in bytes required for HMAC-SHA256 (256 bits). + /// + public const int MinJwtSigningKeyBytes = 32; public int JwtExpiryMinutes { get; set; } = 15; public int IdleTimeoutMinutes { get; set; } = 30; diff --git a/src/ScadaLink.Security/ServiceCollectionExtensions.cs b/src/ScadaLink.Security/ServiceCollectionExtensions.cs index e7c6eff..4e0d25b 100644 --- a/src/ScadaLink.Security/ServiceCollectionExtensions.cs +++ b/src/ScadaLink.Security/ServiceCollectionExtensions.cs @@ -20,6 +20,9 @@ public static class ServiceCollectionExtensions options.Cookie.Name = "ScadaLink.Auth"; options.Cookie.HttpOnly = true; options.Cookie.SameSite = Microsoft.AspNetCore.Http.SameSiteMode.Strict; + // The cookie carries the embedded JWT (a bearer credential); never + // transmit it over plain HTTP. Design: "HttpOnly and Secure (requires HTTPS)". + options.Cookie.SecurePolicy = Microsoft.AspNetCore.Http.CookieSecurePolicy.Always; }); services.AddScadaLinkAuthorization(); diff --git a/tests/ScadaLink.Security.Tests/UnitTest1.cs b/tests/ScadaLink.Security.Tests/UnitTest1.cs index 1ccdd1a..23c53a3 100644 --- a/tests/ScadaLink.Security.Tests/UnitTest1.cs +++ b/tests/ScadaLink.Security.Tests/UnitTest1.cs @@ -357,6 +357,124 @@ public class RoleMapperTests : IDisposable #endregion +#region Code Review Regression Tests + +/// +/// Regression tests for Security-001 (StartTLS dead code), Security-002 (cookie not +/// marked Secure), and Security-003 (JWT signing key length never validated). +/// +public class SecurityReviewRegressionTests +{ + // --- Security-003: JWT signing key length validation --- + + private static SecurityOptions JwtOptions(string signingKey) => new() + { + JwtSigningKey = signingKey, + JwtExpiryMinutes = 15, + IdleTimeoutMinutes = 30, + JwtRefreshThresholdMinutes = 5 + }; + + [Fact] + public void JwtTokenService_EmptySigningKey_ThrowsAtConstruction() + { + var ex = Assert.Throws(() => + new JwtTokenService(Options.Create(JwtOptions("")), NullLogger.Instance)); + Assert.Contains("JwtSigningKey", ex.Message); + } + + [Fact] + public void JwtTokenService_ShortSigningKey_ThrowsAtConstruction() + { + // 31 bytes — one short of the 256-bit HMAC-SHA256 minimum. + var shortKey = new string('k', 31); + var ex = Assert.Throws(() => + new JwtTokenService(Options.Create(JwtOptions(shortKey)), NullLogger.Instance)); + Assert.Contains("32", ex.Message); + } + + [Fact] + public void JwtTokenService_AdequateSigningKey_ConstructsSuccessfully() + { + var key = new string('k', 32); + var service = new JwtTokenService(Options.Create(JwtOptions(key)), NullLogger.Instance); + var token = service.GenerateToken("User", "user", new[] { "Admin" }, null); + Assert.False(string.IsNullOrEmpty(token)); + } + + // --- Security-002: authentication cookie must be marked Secure --- + + [Fact] + public void AddSecurity_AuthCookie_IsMarkedSecureAlways() + { + var services = new ServiceCollection(); + services.AddLogging(); + services.AddDataProtection(); + services.AddSecurity(); + + using var provider = services.BuildServiceProvider(); + var cookieOptions = provider + .GetRequiredService>() + .Get(Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationDefaults.AuthenticationScheme); + + Assert.Equal( + Microsoft.AspNetCore.Http.CookieSecurePolicy.Always, + cookieOptions.Cookie.SecurePolicy); + Assert.True(cookieOptions.Cookie.HttpOnly); + } + + // --- Security-001: StartTLS transport must be reachable --- + + [Fact] + public void SecurityOptions_LdapTransport_DefaultsToLdaps() + { + var options = new SecurityOptions(); + Assert.Equal(LdapTransport.Ldaps, options.LdapTransport); + } + + [Fact] + public async Task AuthenticateAsync_StartTlsTransport_AttemptsConnection() + { + // With StartTLS selected the service must not be blocked by the insecure-LDAP + // guard and must reach the connection stage (which fails against a dead host). + // This proves the StartTLS path is reachable rather than dead code. + var options = new SecurityOptions + { + LdapServer = "nonexistent.invalid", + LdapPort = 389, + LdapTransport = LdapTransport.StartTls, + LdapSearchBase = "dc=example,dc=com" + }; + var service = new LdapAuthService(Options.Create(options), NullLogger.Instance); + + var result = await service.AuthenticateAsync("user", "password"); + + // Connection fails (host invalid) — but crucially NOT with the insecure-LDAP message. + Assert.False(result.Success); + Assert.DoesNotContain("Insecure LDAP", result.ErrorMessage ?? string.Empty); + } + + [Fact] + public async Task AuthenticateAsync_NoTlsTransport_RejectedWithoutAllowInsecure() + { + 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("user", "password"); + + Assert.False(result.Success); + Assert.Contains("Insecure LDAP", result.ErrorMessage); + } +} + +#endregion + #region WP-9: Authorization Policy Tests public class AuthorizationPolicyTests