fix(security): resolve Security-001/002/003 — reachable StartTLS path, Secure cookie, JWT signing key validation

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

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(

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

src/ScadaLink.Security/LdapTransport.cs

@@ -0,0 +1,26 @@
+namespace ScadaLink.Security;
+
+/// <summary>
+/// Transport security mode for the LDAP connection. The design requires either
+/// LDAPS or StartTLS in production; <see cref="None"/> is for dev/test only and
+/// must be paired with <see cref="SecurityOptions.AllowInsecureLdap"/>.
+/// </summary>
+public enum LdapTransport
+{
+    /// <summary>
+    /// LDAPS — TLS negotiated at connection time (typically port 636).
+    /// </summary>
+    Ldaps,
+
+    /// <summary>
+    /// StartTLS — connect in plaintext (typically port 389), then upgrade the
+    /// session to TLS before binding.
+    /// </summary>
+    StartTls,
+
+    /// <summary>
+    /// No transport security. Dev/test only — requires
+    /// <see cref="SecurityOptions.AllowInsecureLdap"/> to be true.
+    /// </summary>
+    None
+}

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;
+
+    /// <summary>
+    /// Transport security mode for the LDAP connection. Defaults to LDAPS.
+    /// Use <see cref="LdapTransport.StartTls"/> to connect on the plaintext port
+    /// and upgrade the session before binding.
+    /// </summary>
+    public LdapTransport LdapTransport { get; set; } = LdapTransport.Ldaps;
+
+    /// <summary>
+    /// True when the configured transport provides encryption (LDAPS or StartTLS).
+    /// Retained for backward compatibility: assigning a value maps onto
+    /// <see cref="LdapTransport"/> (true =&gt; LDAPS, false =&gt; None).
+    /// </summary>
+    public bool LdapUseTls
+    {
+        get => LdapTransport != LdapTransport.None;
+        set => LdapTransport = value ? LdapTransport.Ldaps : LdapTransport.None;
+    }
 
     /// <summary>
     /// Allow insecure (non-TLS) LDAP connections. ONLY for dev/test with GLAuth.
@@ -39,7 +56,16 @@ public class SecurityOptions
     /// </summary>
     public string LdapGroupAttribute { get; set; } = "memberOf";
 
+    /// <summary>
+    /// Symmetric HMAC-SHA256 signing key for cookie-embedded JWTs. Must be at least
+    /// 32 bytes (256 bits) — validated at <see cref="JwtTokenService"/> construction.
+    /// </summary>
     public string JwtSigningKey { get; set; } = string.Empty;
+
+    /// <summary>
+    /// Minimum signing-key length in bytes required for HMAC-SHA256 (256 bits).
+    /// </summary>
+    public const int MinJwtSigningKeyBytes = 32;
     public int JwtExpiryMinutes { get; set; } = 15;
     public int IdleTimeoutMinutes { get; set; } = 30;
 

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();

tests/ScadaLink.Security.Tests/UnitTest1.cs

@@ -357,6 +357,124 @@ public class RoleMapperTests : IDisposable
 
 #endregion
 
+#region Code Review Regression Tests
+
+/// <summary>
+/// Regression tests for Security-001 (StartTLS dead code), Security-002 (cookie not
+/// marked Secure), and Security-003 (JWT signing key length never validated).
+/// </summary>
+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<InvalidOperationException>(() =>
+            new JwtTokenService(Options.Create(JwtOptions("")), NullLogger<JwtTokenService>.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<InvalidOperationException>(() =>
+            new JwtTokenService(Options.Create(JwtOptions(shortKey)), NullLogger<JwtTokenService>.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<JwtTokenService>.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<IOptionsMonitor<Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationOptions>>()
+            .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<LdapAuthService>.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<LdapAuthService>.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