fix(security): bearer token hardening follow-ups — nbf skew, TTL-0 path, cache docs, extra rejection tests (review 2026-10-21 05/U1, 05/P1 L1/L5/L6)

- JwtTokenService back-dates nbf by the 30 s validation skew so a peer central whose clock lags the issuer accepts a fresh token after failover. - RoleMappingCacheSeconds = 0 routes RoleMapper to the matched-rows loader (old cost) instead of loading every Deployer mapping's scope rules per call. - RoleMappingCache / SecurityOptions / Component-Security.md describe Invalidate() by its contract plus the TTL, not by a call site. - Tests: HS512 with the same key, issuer/audience mismatch, lagging-peer nbf, TTL-0 matched-only reads, refresh-vs-Invalidate generation race.
2026-10-22 08:25:20 -04:00
parent 1b3928a0a2
commit ea172edebc
7 changed files with 158 additions and 16 deletions
@@ -30,14 +30,14 @@ public class ManagementAuthenticatorBearerTests
        new(Options.Create(new SecurityOptions { JwtSigningKey = key, JwtExpiryMinutes = 15 }),
            NullLogger<JwtTokenService>.Instance, clock);

-    private static (HttpContext Context, ILdapAuthService Ldap) ContextWithAuthorization(string authorization)
+    private static (HttpContext Context, ILdapAuthService Ldap) ContextWithAuthorization(string authorization, string key = Key)
    {
        var ldap = Substitute.For<ILdapAuthService>();
        var services = new ServiceCollection();
        services.AddSingleton<IOptions<AuthDisableLoginOptions>>(
            Options.Create(new AuthDisableLoginOptions { DisableLogin = false }));
        services.AddSingleton<IOptions<SecurityOptions>>(
-            Options.Create(new SecurityOptions { JwtSigningKey = Key, JwtExpiryMinutes = 15 }));
+            Options.Create(new SecurityOptions { JwtSigningKey = key, JwtExpiryMinutes = 15 }));
        services.AddLogging();
        services.AddSingleton(ldap);
        services.AddScoped<JwtTokenService>();
@@ -162,6 +162,90 @@ public class ManagementAuthenticatorBearerTests
        Assert.NotNull(outcome.Failure);
    }

+    private static string HandMinted(string key, string algorithm, string issuer, string audience)
+    {
+        var credentials = new Microsoft.IdentityModel.Tokens.SigningCredentials(
+            new Microsoft.IdentityModel.Tokens.SymmetricSecurityKey(Encoding.UTF8.GetBytes(key)), algorithm);
+        var jwt = new JwtSecurityToken(
+            issuer: issuer,
+            audience: audience,
+            claims: [new Claim(JwtTokenService.UsernameClaimType, "admin"),
+                     new Claim(JwtTokenService.RoleClaimType, Roles.Administrator)],
+            expires: DateTime.UtcNow.AddMinutes(10),
+            signingCredentials: credentials);
+        var handler = new JwtSecurityTokenHandler();
+        handler.OutboundClaimTypeMap.Clear();
+        return handler.WriteToken(jwt);
+    }
+
+    /// <summary>A token signed with the right key but HS512 is rejected: only HS256 is accepted.</summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task Hs512WithSameKey_Is401()
+    {
+        // 64+ bytes so HS512 itself is satisfied; only the algorithm allow-list can reject it.
+        var longKey = new string('k', 80);
+        var token = HandMinted(longKey, Microsoft.IdentityModel.Tokens.SecurityAlgorithms.HmacSha512,
+            JwtTokenService.TokenIssuer, JwtTokenService.TokenAudience);
+        var (context, _) = ContextWithAuthorization($"Bearer {token}", longKey);
+
+        var outcome = await ManagementAuthenticator.AuthenticateAsync(context);
+
+        Assert.Null(outcome.User);
+        Assert.NotNull(outcome.Failure);
+    }
+
+    /// <summary>The same key and algorithm with a hand-minted HS256 token is accepted (control for the cases below).</summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task HandMintedHs256_OwnIssuerAndAudience_IsAccepted()
+    {
+        var token = HandMinted(Key, Microsoft.IdentityModel.Tokens.SecurityAlgorithms.HmacSha256,
+            JwtTokenService.TokenIssuer, JwtTokenService.TokenAudience);
+        var (context, _) = ContextWithAuthorization($"Bearer {token}");
+
+        var outcome = await ManagementAuthenticator.AuthenticateAsync(context);
+
+        Assert.NotNull(outcome.User);
+    }
+
+    /// <summary>A correctly signed token for another issuer or audience is rejected.</summary>
+    /// <param name="issuer">The token's issuer.</param>
+    /// <param name="audience">The token's audience.</param>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Theory]
+    [InlineData("some-other-system", JwtTokenService.TokenAudience)]
+    [InlineData(JwtTokenService.TokenIssuer, "some-other-audience")]
+    public async Task IssuerOrAudienceMismatch_Is401(string issuer, string audience)
+    {
+        var token = HandMinted(Key, Microsoft.IdentityModel.Tokens.SecurityAlgorithms.HmacSha256, issuer, audience);
+        var (context, _) = ContextWithAuthorization($"Bearer {token}");
+
+        var outcome = await ManagementAuthenticator.AuthenticateAsync(context);
+
+        Assert.Null(outcome.User);
+        Assert.NotNull(outcome.Failure);
+    }
+
+    /// <summary>
+    /// nbf is back-dated by the validation skew, so a peer node whose clock lags the issuer by
+    /// up to twice the skew accepts a fresh token (review L1).
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task FreshToken_FromIssuerAhead_IsAcceptedByLaggingPeer()
+    {
+        // Issuer clock 45 s ahead of this (validating) node — beyond the 30 s skew alone, within
+        // skew + the nbf back-dating.
+        var token = Jwt(clock: new FixedClock(DateTimeOffset.UtcNow.AddSeconds(45)))
+            .GenerateToken("A", "admin", [Roles.Administrator], null);
+        var (context, _) = ContextWithAuthorization($"Bearer {token}");
+
+        var outcome = await ManagementAuthenticator.AuthenticateAsync(context);
+
+        Assert.NotNull(outcome.User);
+    }
+
    private static byte[] Base64UrlDecode(string s)
    {
        var padded = s.Replace('-', '+').Replace('_', '/');
@@ -29,13 +29,17 @@ public class RoleMappingCacheTests
        public List<LdapGroupMapping> Mappings { get; } = [];
        public Dictionary<int, IReadOnlyList<SiteScopeRule>> ScopeRules { get; } = [];
        public int MappingReads { get; private set; }
+        public Func<Task>? BeforeMappingRead { get; set; }
        public int ScopeReads { get; private set; }

        /// <inheritdoc />
-        public Task<IReadOnlyList<LdapGroupMapping>> GetAllMappingsAsync(CancellationToken cancellationToken = default)
+        public async Task<IReadOnlyList<LdapGroupMapping>> GetAllMappingsAsync(CancellationToken cancellationToken = default)
        {
            MappingReads++;
-            return Task.FromResult<IReadOnlyList<LdapGroupMapping>>(Mappings.ToList());
+            var snapshot = Mappings.ToList();
+            if (BeforeMappingRead is not null)
+                await BeforeMappingRead();
+            return snapshot;
        }

        /// <inheritdoc />
@@ -140,10 +144,13 @@ public class RoleMappingCacheTests
        Assert.Empty(after.Roles);
    }

-    /// <summary>A zero TTL disables caching entirely.</summary>
+    /// <summary>
+    /// A zero TTL disables caching entirely and reads only the MATCHED Deployer mappings' scope
+    /// rules, not every Deployer mapping's (review L6).
+    /// </summary>
    /// <returns>A task that represents the asynchronous operation.</returns>
    [Fact]
-    public async Task ZeroTtl_DisablesCache()
+    public async Task ZeroTtl_DisablesCache_AndReadsOnlyMatchedScopeRules()
    {
        var (repo, _, _, mapper) = Create(ttlSeconds: 0);

@@ -151,6 +158,41 @@ public class RoleMappingCacheTests
        await mapper.MapGroupsToRolesAsync(["Admins"]);

        Assert.Equal(2, repo.MappingReads);
+        Assert.Equal(0, repo.ScopeReads); // "Admins" matches no Deployer mapping
+    }
+
+    /// <summary>
+    /// A refresh that read the table before an <see cref="RoleMappingCache.Invalidate"/> is not
+    /// published over it: the next mapping re-reads (review L5).
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task RefreshRacingInvalidate_IsNotPublished()
+    {
+        var (repo, _, cache, mapper) = Create();
+        var readStarted = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+        var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+        repo.BeforeMappingRead = async () =>
+        {
+            readStarted.TrySetResult();
+            await release.Task;
+        };
+
+        // Refresh #1 snapshots the table WITH the Admins mapping, then stalls.
+        var inFlight = mapper.MapGroupsToRolesAsync(["Admins"]);
+        await readStarted.Task;
+        repo.BeforeMappingRead = null;
+
+        // The mapping is revoked and the cache invalidated while #1 is still in flight.
+        repo.Mappings.RemoveAll(m => m.Id == 1);
+        cache.Invalidate();
+        release.SetResult();
+        Assert.Contains(Roles.Administrator, (await inFlight).Roles); // #1 answers its own caller
+
+        var after = await mapper.MapGroupsToRolesAsync(["Admins"]);
+
+        Assert.Empty(after.Roles); // the stale snapshot was never published
+        Assert.Equal(2, repo.MappingReads);
    }

    /// <summary>The cache is shared by mappers built over different (scoped) repositories.</summary>