From 7d8d4e864bb24bf31741a43469fa5e918cf0cfce Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Mon, 30 Sep 2024 10:12:15 -0500 Subject: [PATCH 1/6] Add failing test showing dpop cnf issue --- .../LocalApiAuthenticationTests.cs | 48 +++++++++++++++++-- 1 file changed, 45 insertions(+), 3 deletions(-) diff --git a/test/IdentityServer.IntegrationTests/Hosting/LocalApiAuthentication/LocalApiAuthenticationTests.cs b/test/IdentityServer.IntegrationTests/Hosting/LocalApiAuthentication/LocalApiAuthenticationTests.cs index 2753d391c..e45d9d2e7 100644 --- a/test/IdentityServer.IntegrationTests/Hosting/LocalApiAuthentication/LocalApiAuthenticationTests.cs +++ b/test/IdentityServer.IntegrationTests/Hosting/LocalApiAuthentication/LocalApiAuthenticationTests.cs @@ -4,7 +4,9 @@ using System; using System.Collections.Generic; +using System.IdentityModel.Tokens.Jwt; using System.Linq; +using System.Net; using System.Net.Http; using System.Net.Http.Headers; using System.Security.Claims; @@ -41,11 +43,16 @@ public class LocalApiAuthenticationTests public ClaimsPrincipal ApiPrincipal { get; set; } static LocalApiAuthenticationTests() + { + _jwk = GenerateKey(); + } + + private static string GenerateKey() { var rsaKey = new RsaSecurityKey(RSA.Create(2048)); var jsonWebKey = JsonWebKeyConverter.ConvertFromRSASecurityKey(rsaKey); jsonWebKey.Alg = "PS256"; - _jwk = JsonSerializer.Serialize(jsonWebKey); + return JsonSerializer.Serialize(jsonWebKey); } public LocalApiAuthenticationTests() @@ -151,9 +158,9 @@ async Task GetAccessTokenAsync(bool dpop = false) return result.AccessToken; } - string CreateProofToken(string method, string url, string accessToken = null, string nonce = null) + string CreateProofToken(string method, string url, string accessToken = null, string nonce = null, string jwkString = null) { - var jsonWebKey = new Microsoft.IdentityModel.Tokens.JsonWebKey(_jwk); + var jsonWebKey = new Microsoft.IdentityModel.Tokens.JsonWebKey(jwkString ?? _jwk); // jwk: representing the public key chosen by the client, in JSON Web Key (JWK) [RFC7517] format, // as defined in Section 4.1.3 of [RFC7515]. MUST NOT contain a private key. @@ -267,6 +274,41 @@ public async Task dpop_token_should_validate() ApiPrincipal.Identity.IsAuthenticated.Should().BeTrue(); } + [Fact] + [Trait("Category", Category)] + public async Task dpop_token_should_not_validate_if_cnf_does_not_match_proof_token() + { + var req = new HttpRequestMessage(HttpMethod.Get, "https://server/api"); + var at = await GetAccessTokenAsync(true); + req.Headers.Authorization = new AuthenticationHeaderValue("DPoP", at); + + // Use a new key to make the proof token that we present when we make the API request. + // This doesn't prove that we have possession of the key that the access token is bound to, + // so it should fail. + var newKey = GenerateKey(); + var newJwk = new Microsoft.IdentityModel.Tokens.JsonWebKey(newKey); + var newJkt = Base64Url.Encode(newJwk.ComputeJwkThumbprint()); + var proofToken = CreateProofToken("GET", "https://server/api", at, jwkString: newKey); + req.Headers.Add("DPoP", proofToken); + + // Double check that the thumbprint in the access token's cnf claim doesn't match + // the thumbprint of the new key we just used. + var handler = new JwtSecurityTokenHandler(); + var parsedAt = handler.ReadJwtToken(at); + var parsedProof = handler.ReadJwtToken(proofToken); + var cnf = parsedAt.Claims.FirstOrDefault(c => c.Type == JwtClaimTypes.Confirmation); + var json = JsonSerializer.Deserialize>(cnf.Value); + if (json.TryGetValue(JwtClaimTypes.ConfirmationMethods.JwkThumbprint, out var jktJson)) + { + var accessTokenJkt = jktJson.ToString(); + accessTokenJkt.Should().NotBeEquivalentTo(newJkt); + } + + var response = await _pipeline.BackChannelClient.SendAsync(req); + + response.StatusCode.Should().Be(HttpStatusCode.Unauthorized); + } + [Fact] [Trait("Category", Category)] public async Task dpop_nonce_required_should_require_nonce() From 1d3c9aefd3cc7ec5610fa668b2b4300f099ec8d9 Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Mon, 30 Sep 2024 19:50:14 -0500 Subject: [PATCH 2/6] Validate access token cnf thumbprint matches proof key --- .../Default/DefaultDPoPProofValidator.cs | 39 +++++++++++++++++-- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/src/IdentityServer/Validation/Default/DefaultDPoPProofValidator.cs b/src/IdentityServer/Validation/Default/DefaultDPoPProofValidator.cs index 651930f41..d1c7c61fb 100644 --- a/src/IdentityServer/Validation/Default/DefaultDPoPProofValidator.cs +++ b/src/IdentityServer/Validation/Default/DefaultDPoPProofValidator.cs @@ -13,7 +13,6 @@ using IdentityModel; using System.Linq; using Duende.IdentityServer.Services; -using static Duende.IdentityServer.IdentityServerConstants; using Duende.IdentityServer.Models; using Microsoft.AspNetCore.DataProtection; using System.Security.Cryptography; @@ -126,10 +125,10 @@ public async Task ValidateAsync(DPoPProofValidatonCont protected virtual Task ValidateHeaderAsync(DPoPProofValidatonContext context, DPoPProofValidatonResult result) { JsonWebToken token; + var handler = new JsonWebTokenHandler(); try { - var handler = new JsonWebTokenHandler(); token = handler.ReadJsonWebToken(context.ProofToken); } catch (Exception ex) @@ -185,7 +184,41 @@ protected virtual Task ValidateHeaderAsync(DPoPProofValidatonContext context, DP result.JsonWebKey = jwkJson; result.JsonWebKeyThumbprint = jwk.CreateThumbprint(); - result.Confirmation = jwk.CreateThumbprintCnf(); + + if (context.ValidateAccessToken) + { + if (handler.CanReadToken(context.AccessToken)) + { + var accessToken = handler.ReadJsonWebToken(context.AccessToken); + var cnf = accessToken.Claims.FirstOrDefault(c => c.Type == JwtClaimTypes.Confirmation); + if (cnf == null) + { + result.IsError = true; + result.ErrorDescription = "Invalid 'cnf' value."; + return Task.CompletedTask; + } + var json = JsonSerializer.Deserialize>(cnf.Value); + if (json.TryGetValue(JwtClaimTypes.ConfirmationMethods.JwkThumbprint, out var jktJson)) + { + var accessTokenJkt = jktJson.ToString(); + if (accessTokenJkt != result.JsonWebKeyThumbprint) + { + result.IsError = true; + result.ErrorDescription = "Invalid 'cnf' value."; + return Task.CompletedTask; + } + result.Confirmation = cnf.Value; + } + } + else + { + // TODO + } + } + else + { + result.Confirmation = jwk.CreateThumbprintCnf(); + } return Task.CompletedTask; } From 181ac05f3567b442ce56096e55b7305ee9b85eff Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Mon, 30 Sep 2024 20:13:02 -0500 Subject: [PATCH 3/6] Add test for wrong proof key at local api auth with reference tokens --- .../LocalApiAuthenticationTests.cs | 55 ++++++++++++++++++- 1 file changed, 52 insertions(+), 3 deletions(-) diff --git a/test/IdentityServer.IntegrationTests/Hosting/LocalApiAuthentication/LocalApiAuthenticationTests.cs b/test/IdentityServer.IntegrationTests/Hosting/LocalApiAuthentication/LocalApiAuthenticationTests.cs index e45d9d2e7..f0fe5f752 100644 --- a/test/IdentityServer.IntegrationTests/Hosting/LocalApiAuthentication/LocalApiAuthenticationTests.cs +++ b/test/IdentityServer.IntegrationTests/Hosting/LocalApiAuthentication/LocalApiAuthenticationTests.cs @@ -64,6 +64,14 @@ public LocalApiAuthenticationTests() AllowedGrantTypes = GrantTypes.ClientCredentials, ClientSecrets = { new Secret("secret".Sha256()) }, AllowedScopes = new List { "api1", "api2" }, + }, + new Client + { + ClientId = "introspection", + AllowedGrantTypes = GrantTypes.ClientCredentials, + ClientSecrets = { new Secret("secret".Sha256()) }, + AllowedScopes = new List { "api1", "api2" }, + AccessTokenType = AccessTokenType.Reference } }); @@ -135,12 +143,12 @@ void Init(LocalApiTokenMode mode = LocalApiTokenMode.DPoPAndBearer) _pipeline.Initialize(); } - async Task GetAccessTokenAsync(bool dpop = false) + async Task GetAccessTokenAsync(bool dpop = false, bool reference = false) { var req = new ClientCredentialsTokenRequest { Address = "https://server/connect/token", - ClientId = "client", + ClientId = reference ? "introspection" : "client", ClientSecret = "secret", Scope = "api1", }; @@ -276,7 +284,7 @@ public async Task dpop_token_should_validate() [Fact] [Trait("Category", Category)] - public async Task dpop_token_should_not_validate_if_cnf_does_not_match_proof_token() + public async Task dpop_token_should_not_validate_if_cnf_from_jwt_access_token_does_not_match_proof_token() { var req = new HttpRequestMessage(HttpMethod.Get, "https://server/api"); var at = await GetAccessTokenAsync(true); @@ -309,6 +317,47 @@ public async Task dpop_token_should_not_validate_if_cnf_does_not_match_proof_tok response.StatusCode.Should().Be(HttpStatusCode.Unauthorized); } + [Fact] + [Trait("Category", Category)] + public async Task dpop_token_should_not_validate_if_cnf_from_introspection_does_not_match_proof_token() + { + var req = new HttpRequestMessage(HttpMethod.Get, "https://server/api"); + var at = await GetAccessTokenAsync(dpop: true, reference: true); + req.Headers.Authorization = new AuthenticationHeaderValue("DPoP", at); + + // Use a new key to make the proof token that we present when we make the API request. + // This doesn't prove that we have possession of the key that the access token is bound to, + // so it should fail. + var newKey = GenerateKey(); + var newJwk = new Microsoft.IdentityModel.Tokens.JsonWebKey(newKey); + var newJkt = Base64Url.Encode(newJwk.ComputeJwkThumbprint()); + var proofToken = CreateProofToken("GET", "https://server/api", at, jwkString: newKey); + req.Headers.Add("DPoP", proofToken); + + + var introspectionRequest = new TokenIntrospectionRequest + { + Address = "https://server/connect/introspect", + ClientId = "introspection", + ClientSecret = "secret", + Token = at + }; + var introspectionResponse = await _pipeline.BackChannelClient.IntrospectTokenAsync(introspectionRequest); + introspectionResponse.IsError.Should().BeFalse(); + + var cnf = introspectionResponse.Claims.FirstOrDefault(c => c.Type == JwtClaimTypes.Confirmation); + var json = JsonSerializer.Deserialize>(cnf.Value); + if (json.TryGetValue(JwtClaimTypes.ConfirmationMethods.JwkThumbprint, out var jktJson)) + { + var accessTokenJkt = jktJson.ToString(); + accessTokenJkt.Should().NotBeEquivalentTo(newJkt); + } + + var response = await _pipeline.BackChannelClient.SendAsync(req); + + response.StatusCode.Should().Be(HttpStatusCode.Unauthorized); + } + [Fact] [Trait("Category", Category)] public async Task dpop_nonce_required_should_require_nonce() From 5d32db47a34991d0eb97b3cd73f7128c50b6c55a Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Mon, 30 Sep 2024 20:52:24 -0500 Subject: [PATCH 4/6] Validate dpop cnf for reference tokens in local api authentication handler --- .../LocalApiAuthenticationHandler.cs | 1 + .../Contexts/DPoPProofValidatonContext.cs | 12 ++++++- .../Default/DefaultDPoPProofValidator.cs | 32 +++++++------------ .../LocalApiAuthenticationTests.cs | 1 - .../Validation/DPoPProofValidatorTests.cs | 8 ++++- 5 files changed, 31 insertions(+), 23 deletions(-) diff --git a/src/IdentityServer/Hosting/LocalApiAuthentication/LocalApiAuthenticationHandler.cs b/src/IdentityServer/Hosting/LocalApiAuthentication/LocalApiAuthenticationHandler.cs index fef61f6cf..82c33fce3 100644 --- a/src/IdentityServer/Hosting/LocalApiAuthentication/LocalApiAuthenticationHandler.cs +++ b/src/IdentityServer/Hosting/LocalApiAuthentication/LocalApiAuthenticationHandler.cs @@ -129,6 +129,7 @@ protected override async Task HandleAuthenticateAsync() Url = Context.Request.Scheme + "://" + Context.Request.Host + Context.Request.PathBase + Context.Request.Path, ValidateAccessToken = true, AccessToken = token, + AccessTokenClaims = tokenResult.Claims, ExpirationValidationMode = client.DPoPValidationMode, ClientClockSkew = client.DPoPClockSkew, }; diff --git a/src/IdentityServer/Validation/Contexts/DPoPProofValidatonContext.cs b/src/IdentityServer/Validation/Contexts/DPoPProofValidatonContext.cs index 017905ce2..431b9a873 100644 --- a/src/IdentityServer/Validation/Contexts/DPoPProofValidatonContext.cs +++ b/src/IdentityServer/Validation/Contexts/DPoPProofValidatonContext.cs @@ -6,6 +6,9 @@ using Duende.IdentityServer.Models; using System; +using System.Collections.Generic; +using System.Linq; +using System.Security.Claims; namespace Duende.IdentityServer.Validation; @@ -47,7 +50,14 @@ public class DPoPProofValidatonContext public bool ValidateAccessToken { get; set; } /// - /// The access token to validate if ValidateAccessToken is set + /// The access token to validate if is set /// public string? AccessToken { get; set; } + + /// + /// The claims associated with the access token, used if is set. + /// This is included separately from the because getting the claims + /// might be an expensive operation (especially if the token is a reference token). + /// + public IEnumerable AccessTokenClaims { get; set; } = Enumerable.Empty(); } diff --git a/src/IdentityServer/Validation/Default/DefaultDPoPProofValidator.cs b/src/IdentityServer/Validation/Default/DefaultDPoPProofValidator.cs index d1c7c61fb..487c7e646 100644 --- a/src/IdentityServer/Validation/Default/DefaultDPoPProofValidator.cs +++ b/src/IdentityServer/Validation/Default/DefaultDPoPProofValidator.cs @@ -187,32 +187,24 @@ protected virtual Task ValidateHeaderAsync(DPoPProofValidatonContext context, DP if (context.ValidateAccessToken) { - if (handler.CanReadToken(context.AccessToken)) + var cnf = context.AccessTokenClaims.FirstOrDefault(c => c.Type == JwtClaimTypes.Confirmation); + if (cnf == null) { - var accessToken = handler.ReadJsonWebToken(context.AccessToken); - var cnf = accessToken.Claims.FirstOrDefault(c => c.Type == JwtClaimTypes.Confirmation); - if (cnf == null) + result.IsError = true; + result.ErrorDescription = "Missing 'cnf' value."; + return Task.CompletedTask; + } + var json = JsonSerializer.Deserialize>(cnf.Value); + if (json.TryGetValue(JwtClaimTypes.ConfirmationMethods.JwkThumbprint, out var jktJson)) + { + var accessTokenJkt = jktJson.ToString(); + if (accessTokenJkt != result.JsonWebKeyThumbprint) { result.IsError = true; result.ErrorDescription = "Invalid 'cnf' value."; return Task.CompletedTask; } - var json = JsonSerializer.Deserialize>(cnf.Value); - if (json.TryGetValue(JwtClaimTypes.ConfirmationMethods.JwkThumbprint, out var jktJson)) - { - var accessTokenJkt = jktJson.ToString(); - if (accessTokenJkt != result.JsonWebKeyThumbprint) - { - result.IsError = true; - result.ErrorDescription = "Invalid 'cnf' value."; - return Task.CompletedTask; - } - result.Confirmation = cnf.Value; - } - } - else - { - // TODO + result.Confirmation = cnf.Value; } } else diff --git a/test/IdentityServer.IntegrationTests/Hosting/LocalApiAuthentication/LocalApiAuthenticationTests.cs b/test/IdentityServer.IntegrationTests/Hosting/LocalApiAuthentication/LocalApiAuthenticationTests.cs index f0fe5f752..3f1e605cb 100644 --- a/test/IdentityServer.IntegrationTests/Hosting/LocalApiAuthentication/LocalApiAuthenticationTests.cs +++ b/test/IdentityServer.IntegrationTests/Hosting/LocalApiAuthentication/LocalApiAuthenticationTests.cs @@ -334,7 +334,6 @@ public async Task dpop_token_should_not_validate_if_cnf_from_introspection_does_ var proofToken = CreateProofToken("GET", "https://server/api", at, jwkString: newKey); req.Headers.Add("DPoP", proofToken); - var introspectionRequest = new TokenIntrospectionRequest { Address = "https://server/connect/introspect", diff --git a/test/IdentityServer.UnitTests/Validation/DPoPProofValidatorTests.cs b/test/IdentityServer.UnitTests/Validation/DPoPProofValidatorTests.cs index 411ca5c35..048210b6c 100644 --- a/test/IdentityServer.UnitTests/Validation/DPoPProofValidatorTests.cs +++ b/test/IdentityServer.UnitTests/Validation/DPoPProofValidatorTests.cs @@ -4,11 +4,13 @@ using System; using System.Collections.Generic; +using System.Security.Claims; using System.Security.Cryptography; using System.Text; using System.Text.Json; using System.Threading.Tasks; using Duende.IdentityServer.Configuration; +using Duende.IdentityServer.Extensions; using Duende.IdentityServer.Models; using Duende.IdentityServer.Validation; using FluentAssertions; @@ -48,7 +50,7 @@ public DateTime UtcNow Method = "POST" }; -Dictionary _header; + Dictionary _header; Dictionary _payload; string _privateJWK = "{\"Crv\":null,\"D\":\"QeBWodq0hSYjfAxxo0VZleXLqwwZZeNWvvFfES4WyItao_-OJv1wKA7zfkZxbWkpK5iRbKrl2AMJ52AtUo5JJ6QZ7IjAQlgM0lBg3ltjb1aA0gBsK5XbiXcsV8DiAnRuy6-XgjAKPR8Lo-wZl_fdPbVoAmpSdmfn_6QXXPBai5i7FiyDbQa16pI6DL-5SCj7F78QDTRiJOqn5ElNvtoJEfJBm13giRdqeriFi3pCWo7H3QBgTEWtDNk509z4w4t64B2HTXnM0xj9zLnS42l7YplJC7MRibD4nVBMtzfwtGRKLj8beuDgtW9pDlQqf7RVWX5pHQgiHAZmUi85TEbYdQ\",\"DP\":\"h2F54OMaC9qq1yqR2b55QNNaChyGtvmTHSdqZJ8lJFqvUorlz-Uocj2BTowWQnaMd8zRKMdKlSeUuSv4Z6WmjSxSsNbonI6_II5XlZLWYqFdmqDS-xCmJY32voT5Wn7OwB9xj1msDqrFPg-PqSBOh5OppjCqXqDFcNvSkQSajXc\",\"DQ\":\"VABdS20Nxkmq6JWLQj7OjRxVJuYsHrfmWJmDA7_SYtlXaPUcg-GiHGQtzdDWEeEi0dlJjv9I3FdjKGC7CGwqtVygW38DzVYJsV2EmRNJc1-j-1dRs_pK9GWR4NYm0mVz_IhS8etIf9cfRJk90xU3AL3_J6p5WNF7I5ctkLpnt8M\",\"E\":\"AQAB\",\"K\":null,\"KeyOps\":[],\"Kty\":\"RSA\",\"N\":\"yWWAOSV3Z_BW9rJEFvbZyeU-q2mJWC0l8WiHNqwVVf7qXYgm9hJC0j1aPHku_Wpl38DpK3Xu3LjWOFG9OrCqga5Pzce3DDJKI903GNqz5wphJFqweoBFKOjj1wegymvySsLoPqqDNVYTKp4nVnECZS4axZJoNt2l1S1bC8JryaNze2stjW60QT-mIAGq9konKKN3URQ12dr478m0Oh-4WWOiY4HrXoSOklFmzK-aQx1JV_SZ04eIGfSw1pZZyqTaB1BwBotiy-QA03IRxwIXQ7BSx5EaxC5uMCMbzmbvJqjt-q8Y1wyl-UQjRucgp7hkfHSE1QT3zEex2Q3NFux7SQ\",\"Oth\":null,\"P\":\"_T7MTkeOh5QyqlYCtLQ2RWf2dAJ9i3wrCx4nEDm1c1biijhtVTL7uJTLxwQIM9O2PvOi5Dq-UiGy6rhHZqf5akWTeHtaNyI-2XslQfaS3ctRgmGtRQL_VihK-R9AQtDx4eWL4h-bDJxPaxby_cVo_j2MX5AeoC1kNmcCdDf_X0M\",\"Q\":\"y5ZSThaGLjaPj8Mk2nuD8TiC-sb4aAZVh9K-W4kwaWKfDNoPcNb_dephBNMnOp9M1br6rDbyG7P-Sy_LOOsKg3Q0wHqv4hnzGaOQFeMJH4HkXYdENC7B5JG9PefbC6zwcgZWiBnsxgKpScNWuzGF8x2CC-MdsQ1bkQeTPbJklIM\",\"QI\":\"i716Vt9II_Rt6qnjsEhfE4bej52QFG9a1hSnx5PDNvRrNqR_RpTA0lO9qeXSZYGHTW_b6ZXdh_0EUwRDEDHmaxjkIcTADq6JLuDltOhZuhLUSc5NCKLAVCZlPcaSzv8-bZm57mVcIpx0KyFHxvk50___Jgx1qyzwLX03mPGUbDQ\",\"Use\":null,\"X\":null,\"X5c\":[],\"X5t\":null,\"X5tS256\":null,\"X5u\":null,\"Y\":null,\"KeySize\":2048,\"HasPrivateKey\":true,\"CryptoProviderFactory\":{\"CryptoProviderCache\":{},\"CustomCryptoProvider\":null,\"CacheSignatureProviders\":true,\"SignatureProviderObjectPoolCacheSize\":80}}"; string _publicJWK = "{\"kty\":\"RSA\",\"use\":\"sig\",\"x5t\":null,\"e\":\"AQAB\",\"n\":\"yWWAOSV3Z_BW9rJEFvbZyeU-q2mJWC0l8WiHNqwVVf7qXYgm9hJC0j1aPHku_Wpl38DpK3Xu3LjWOFG9OrCqga5Pzce3DDJKI903GNqz5wphJFqweoBFKOjj1wegymvySsLoPqqDNVYTKp4nVnECZS4axZJoNt2l1S1bC8JryaNze2stjW60QT-mIAGq9konKKN3URQ12dr478m0Oh-4WWOiY4HrXoSOklFmzK-aQx1JV_SZ04eIGfSw1pZZyqTaB1BwBotiy-QA03IRxwIXQ7BSx5EaxC5uMCMbzmbvJqjt-q8Y1wyl-UQjRucgp7hkfHSE1QT3zEex2Q3NFux7SQ\",\"x5c\":null,\"x\":null,\"y\":null,\"crv\":null}"; @@ -144,6 +146,10 @@ public async Task ath_with_valid_access_token_should_pass_validation() _context.ValidateAccessToken = true; _context.AccessToken = "access_token"; + var jwk = new Microsoft.IdentityModel.Tokens.JsonWebKey(_publicJWK); + var cnf = jwk.CreateThumbprintCnf(); + _context.AccessTokenClaims = [new Claim(JwtClaimTypes.Confirmation, cnf)]; + using var sha = SHA256.Create(); var bytes = Encoding.UTF8.GetBytes(_context.AccessToken); var hash = sha.ComputeHash(bytes); From 400ff7771becc4b01246956dc3dca802b9917a1e Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Wed, 2 Oct 2024 16:45:12 -0500 Subject: [PATCH 5/6] Refactor DPoP validation logic and add new validation tests Refactored DPoP proof validator to simplify access token confirmation claim handling. Added tests for various error scenarios including missing, malformed, null, and mismatched 'cnf' claims. --- .../Default/DefaultDPoPProofValidator.cs | 40 ++++- .../LocalApiAuthenticationTests.cs | 8 +- .../Validation/DPoPProofValidatorTests.cs | 137 ++++++++++++++++-- 3 files changed, 164 insertions(+), 21 deletions(-) diff --git a/src/IdentityServer/Validation/Default/DefaultDPoPProofValidator.cs b/src/IdentityServer/Validation/Default/DefaultDPoPProofValidator.cs index 487c7e646..6d7df3d7b 100644 --- a/src/IdentityServer/Validation/Default/DefaultDPoPProofValidator.cs +++ b/src/IdentityServer/Validation/Default/DefaultDPoPProofValidator.cs @@ -194,24 +194,50 @@ protected virtual Task ValidateHeaderAsync(DPoPProofValidatonContext context, DP result.ErrorDescription = "Missing 'cnf' value."; return Task.CompletedTask; } - var json = JsonSerializer.Deserialize>(cnf.Value); - if (json.TryGetValue(JwtClaimTypes.ConfirmationMethods.JwkThumbprint, out var jktJson)) + try { - var accessTokenJkt = jktJson.ToString(); - if (accessTokenJkt != result.JsonWebKeyThumbprint) + var cnfJson = JsonSerializer.Deserialize>(cnf.Value); + if (cnfJson == null) { + Logger.LogDebug("Null cnf value in DPoP access token."); result.IsError = true; - result.ErrorDescription = "Invalid 'cnf' value."; + result.ErrorDescription = "Missing 'cnf' value."; return Task.CompletedTask; + } + else if (cnfJson.TryGetValue(JwtClaimTypes.ConfirmationMethods.JwkThumbprint, out var jktJson)) + { + var accessTokenJkt = jktJson.ToString(); + if (accessTokenJkt == result.JsonWebKeyThumbprint) + { + result.Confirmation = cnf.Value; + } + else + { + Logger.LogDebug("jkt in DPoP access token does not match proof token key thumbprint."); + } + } + else + { + Logger.LogDebug("jkt member missing from cnf claim in DPoP access token."); } - result.Confirmation = cnf.Value; + } + catch (JsonException e) + { + Logger.LogDebug("Failed to parse DPoP cnf claim: {JsonExceptionMessage}", e.Message); + } + + if (result.Confirmation == null) + { + result.IsError = true; + result.ErrorDescription = "Invalid 'cnf' value."; } } else { + // The ValidateAccessToken == false case occurs when we are generating tokens. The confirmation value here + // ultimately is put into the generated token's cnf claim. result.Confirmation = jwk.CreateThumbprintCnf(); } - return Task.CompletedTask; } diff --git a/test/IdentityServer.IntegrationTests/Hosting/LocalApiAuthentication/LocalApiAuthenticationTests.cs b/test/IdentityServer.IntegrationTests/Hosting/LocalApiAuthentication/LocalApiAuthenticationTests.cs index 3f1e605cb..73220bddd 100644 --- a/test/IdentityServer.IntegrationTests/Hosting/LocalApiAuthentication/LocalApiAuthenticationTests.cs +++ b/test/IdentityServer.IntegrationTests/Hosting/LocalApiAuthentication/LocalApiAuthenticationTests.cs @@ -44,10 +44,10 @@ public class LocalApiAuthenticationTests static LocalApiAuthenticationTests() { - _jwk = GenerateKey(); + _jwk = GenerateJwk(); } - private static string GenerateKey() + private static string GenerateJwk() { var rsaKey = new RsaSecurityKey(RSA.Create(2048)); var jsonWebKey = JsonWebKeyConverter.ConvertFromRSASecurityKey(rsaKey); @@ -293,7 +293,7 @@ public async Task dpop_token_should_not_validate_if_cnf_from_jwt_access_token_do // Use a new key to make the proof token that we present when we make the API request. // This doesn't prove that we have possession of the key that the access token is bound to, // so it should fail. - var newKey = GenerateKey(); + var newKey = GenerateJwk(); var newJwk = new Microsoft.IdentityModel.Tokens.JsonWebKey(newKey); var newJkt = Base64Url.Encode(newJwk.ComputeJwkThumbprint()); var proofToken = CreateProofToken("GET", "https://server/api", at, jwkString: newKey); @@ -328,7 +328,7 @@ public async Task dpop_token_should_not_validate_if_cnf_from_introspection_does_ // Use a new key to make the proof token that we present when we make the API request. // This doesn't prove that we have possession of the key that the access token is bound to, // so it should fail. - var newKey = GenerateKey(); + var newKey = GenerateJwk(); var newJwk = new Microsoft.IdentityModel.Tokens.JsonWebKey(newKey); var newJkt = Base64Url.Encode(newJwk.ComputeJwkThumbprint()); var proofToken = CreateProofToken("GET", "https://server/api", at, jwkString: newKey); diff --git a/test/IdentityServer.UnitTests/Validation/DPoPProofValidatorTests.cs b/test/IdentityServer.UnitTests/Validation/DPoPProofValidatorTests.cs index 048210b6c..154e341a8 100644 --- a/test/IdentityServer.UnitTests/Validation/DPoPProofValidatorTests.cs +++ b/test/IdentityServer.UnitTests/Validation/DPoPProofValidatorTests.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Security.Claims; using System.Security.Cryptography; using System.Text; @@ -145,54 +146,170 @@ public async Task ath_with_valid_access_token_should_pass_validation() { _context.ValidateAccessToken = true; _context.AccessToken = "access_token"; + _context.AccessTokenClaims = [CnfClaim()]; + var accessTokenHash = HashAccessToken(); + _payload["ath"] = accessTokenHash; + _context.ProofToken = CreateDPoPProofToken(); + + var result = await _subject.ValidateAsync(_context); - var jwk = new Microsoft.IdentityModel.Tokens.JsonWebKey(_publicJWK); + result.IsError.Should().BeFalse(); + result.JsonWebKeyThumbprint.Should().Be(_JKT); + result.AccessTokenHash.Should().Be(accessTokenHash); + } + + private Claim CnfClaim(string jwkString = null ) + { + jwkString ??= _publicJWK; + var jwk = new Microsoft.IdentityModel.Tokens.JsonWebKey(jwkString); var cnf = jwk.CreateThumbprintCnf(); - _context.AccessTokenClaims = [new Claim(JwtClaimTypes.Confirmation, cnf)]; + return new Claim(JwtClaimTypes.Confirmation, cnf); + } + private string HashAccessToken() + { using var sha = SHA256.Create(); var bytes = Encoding.UTF8.GetBytes(_context.AccessToken); var hash = sha.ComputeHash(bytes); - var accessTokenHash = Base64Url.Encode(hash); - _payload["ath"] = accessTokenHash; + return Base64Url.Encode(hash); + } + + [Fact] + [Trait("Category", Category)] + public async Task missing_ath_should_fail() + { + _context.ValidateAccessToken = true; + _context.AccessToken = "access_token"; + _context.AccessTokenClaims = [CnfClaim()]; _context.ProofToken = CreateDPoPProofToken(); var result = await _subject.ValidateAsync(_context); - result.IsError.Should().BeFalse(); - result.JsonWebKeyThumbprint.Should().Be(_JKT); - result.AccessTokenHash.Should().Be(accessTokenHash); + result.IsError.Should().BeTrue(); + result.Error.Should().Be(OidcConstants.TokenErrors.InvalidDPoPProof); + result.ErrorDescription.Should().Be("Invalid 'ath' value."); } [Fact] [Trait("Category", Category)] - public async Task missing_ath_should_fail() + public async Task invalid_ath_should_fail() { _context.ValidateAccessToken = true; _context.AccessToken = "access_token"; + _context.AccessTokenClaims = [CnfClaim()]; + _payload["ath"] = "invalid"; + _context.ProofToken = CreateDPoPProofToken(); + + var result = await _subject.ValidateAsync(_context); + result.IsError.Should().BeTrue(); + result.ErrorDescription.Should().Be("Invalid 'ath' value."); + result.Error.Should().Be(OidcConstants.TokenErrors.InvalidDPoPProof); + } + + [Fact] + [Trait("Category", Category)] + public async Task missing_cnf_should_fail() + { + _context.ValidateAccessToken = true; + _context.AccessToken = "access_token"; + _context.AccessTokenClaims.Should() + .NotContain(c => c.Type == JwtClaimTypes.Confirmation); + var accessTokenHash = HashAccessToken(); + _payload["ath"] = accessTokenHash; _context.ProofToken = CreateDPoPProofToken(); var result = await _subject.ValidateAsync(_context); result.IsError.Should().BeTrue(); + result.Error.Should().Be(OidcConstants.TokenErrors.InvalidDPoPProof); + result.ErrorDescription.Should().Be("Missing 'cnf' value."); } + + [Fact] + [Trait("Category", Category)] + public async Task malformed_cnf_should_fail() + { + _context.ValidateAccessToken = true; + _context.AccessToken = "access_token"; + _context.AccessTokenClaims = [new Claim(JwtClaimTypes.Confirmation, "not-a-json-object")]; + var accessTokenHash = HashAccessToken(); + _payload["ath"] = accessTokenHash; + _context.ProofToken = CreateDPoPProofToken(); + var result = await _subject.ValidateAsync(_context); + + result.IsError.Should().BeTrue(); + result.Error.Should().Be(OidcConstants.TokenErrors.InvalidDPoPProof); + result.ErrorDescription.Should().Be("Invalid 'cnf' value."); + } + [Fact] [Trait("Category", Category)] - public async Task invalid_ath_should_fail() + public async Task null_cnf_values_should_fail() { _context.ValidateAccessToken = true; _context.AccessToken = "access_token"; + _context.AccessTokenClaims = [new Claim(JwtClaimTypes.Confirmation, "null")]; + var accessTokenHash = HashAccessToken(); + _payload["ath"] = accessTokenHash; + _context.ProofToken = CreateDPoPProofToken(); - _payload["ath"] = "invalid"; + var result = await _subject.ValidateAsync(_context); + + result.IsError.Should().BeTrue(); + result.Error.Should().Be(OidcConstants.TokenErrors.InvalidDPoPProof); + result.ErrorDescription.Should().Be("Missing 'cnf' value."); + } + + [Fact] + [Trait("Category", Category)] + public async Task cnf_missing_jkt_should_fail() + { + _context.ValidateAccessToken = true; + _context.AccessToken = "access_token"; + var cnfObject = new Dictionary + { + { "no-jkt-member-in-this-object", "causes-failure" } + }; + _context.AccessTokenClaims = [new Claim(JwtClaimTypes.Confirmation, JsonSerializer.Serialize(cnfObject))]; + var accessTokenHash = HashAccessToken(); + _payload["ath"] = accessTokenHash; _context.ProofToken = CreateDPoPProofToken(); var result = await _subject.ValidateAsync(_context); result.IsError.Should().BeTrue(); + result.Error.Should().Be(OidcConstants.TokenErrors.InvalidDPoPProof); + result.ErrorDescription.Should().Be("Invalid 'cnf' value."); } + [Fact] + [Trait("Category", Category)] + public async Task mismatched_jkt_should_fail() + { + _context.ValidateAccessToken = true; + _context.AccessToken = "access_token"; + _context.AccessTokenClaims = [CnfClaim(GenerateJwk())]; + var accessTokenHash = HashAccessToken(); + _payload["ath"] = accessTokenHash; + _context.ProofToken = CreateDPoPProofToken(); + + var result = await _subject.ValidateAsync(_context); + + result.IsError.Should().BeTrue(); + result.Error.Should().Be(OidcConstants.TokenErrors.InvalidDPoPProof); + result.ErrorDescription.Should().Be("Invalid 'cnf' value."); + } + + private static string GenerateJwk() + { + var rsaKey = new RsaSecurityKey(RSA.Create(2048)); + var jsonWebKey = JsonWebKeyConverter.ConvertFromRSASecurityKey(rsaKey); + jsonWebKey.Alg = "PS256"; + return JsonSerializer.Serialize(jsonWebKey); + } + [Fact] [Trait("Category", Category)] public async Task server_clock_skew_should_allow_tokens_outside_normal_duration() From 56602c7a78f7f6b4760a374ac87524d224bbc8c4 Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Tue, 22 Oct 2024 21:57:29 -0500 Subject: [PATCH 6/6] Add error check for empty cnf value --- .../Default/DefaultDPoPProofValidator.cs | 2 +- .../Validation/DPoPProofValidatorTests.cs | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/IdentityServer/Validation/Default/DefaultDPoPProofValidator.cs b/src/IdentityServer/Validation/Default/DefaultDPoPProofValidator.cs index 6d7df3d7b..45601b66f 100644 --- a/src/IdentityServer/Validation/Default/DefaultDPoPProofValidator.cs +++ b/src/IdentityServer/Validation/Default/DefaultDPoPProofValidator.cs @@ -188,7 +188,7 @@ protected virtual Task ValidateHeaderAsync(DPoPProofValidatonContext context, DP if (context.ValidateAccessToken) { var cnf = context.AccessTokenClaims.FirstOrDefault(c => c.Type == JwtClaimTypes.Confirmation); - if (cnf == null) + if (cnf is not { Value.Length: > 0 }) { result.IsError = true; result.ErrorDescription = "Missing 'cnf' value."; diff --git a/test/IdentityServer.UnitTests/Validation/DPoPProofValidatorTests.cs b/test/IdentityServer.UnitTests/Validation/DPoPProofValidatorTests.cs index 154e341a8..a291d8cc3 100644 --- a/test/IdentityServer.UnitTests/Validation/DPoPProofValidatorTests.cs +++ b/test/IdentityServer.UnitTests/Validation/DPoPProofValidatorTests.cs @@ -226,6 +226,25 @@ public async Task missing_cnf_should_fail() result.ErrorDescription.Should().Be("Missing 'cnf' value."); } + [Fact] + [Trait("Category", Category)] + public async Task empty_cnf_should_fail() + { + _context.ValidateAccessToken = true; + _context.AccessToken = "access_token"; + _context.AccessTokenClaims = [new Claim(JwtClaimTypes.Confirmation, "")]; + + var accessTokenHash = HashAccessToken(); + _payload["ath"] = accessTokenHash; + _context.ProofToken = CreateDPoPProofToken(); + + var result = await _subject.ValidateAsync(_context); + + result.IsError.Should().BeTrue(); + result.Error.Should().Be(OidcConstants.TokenErrors.InvalidDPoPProof); + result.ErrorDescription.Should().Be("Missing 'cnf' value."); + } + [Fact] [Trait("Category", Category)] public async Task malformed_cnf_should_fail()