From f0c3fc690a5b0f1aa3f603007d6fc64ae0610017 Mon Sep 17 00:00:00 2001 From: vvdb-architecture Date: Wed, 9 Oct 2024 10:30:15 +0200 Subject: [PATCH] According to https://www.rfc-editor.org/rfc/rfc7519 section 10.1, claim names (a.k.a. "types") are case-sensitive. Claims manipulations have been factored out in a separate utility class. --- .../AppPrincipalTransform.cs | 43 +++---- .../Security/AppServicePrincipalFactory.cs | 12 +- .../Security/Principal/AppPrincipalFactory.cs | 40 +++--- .../Security/ClaimsUtility.cs | 117 ++++++++++++++++++ 4 files changed, 150 insertions(+), 62 deletions(-) create mode 100644 src/Arc4u.Standard.OAuth2/Security/ClaimsUtility.cs diff --git a/src/Arc4u.Standard.OAuth2.AspNetCore/AppPrincipalTransform.cs b/src/Arc4u.Standard.OAuth2.AspNetCore/AppPrincipalTransform.cs index c726ba58..03d2a4d3 100644 --- a/src/Arc4u.Standard.OAuth2.AspNetCore/AppPrincipalTransform.cs +++ b/src/Arc4u.Standard.OAuth2.AspNetCore/AppPrincipalTransform.cs @@ -1,6 +1,5 @@ using System.Diagnostics; using System.Diagnostics.CodeAnalysis; -using System.Globalization; using System.Security.Claims; using Arc4u.Configuration; using Arc4u.Dependency.Attribute; @@ -9,6 +8,7 @@ using Arc4u.OAuth2.Options; using Arc4u.OAuth2.Token; using Arc4u.Security.Principal; +using Arc4u.Standard.OAuth2.Security; using Microsoft.AspNetCore.Authentication; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; @@ -98,8 +98,6 @@ public async Task TransformAsync(ClaimsPrincipal principal) } #region Handling extra claims - private const string tokenExpirationClaimType = "exp"; - private static readonly HashSet ClaimsToExclude = new HashSet(StringComparer.OrdinalIgnoreCase) { "aud", "iss", "iat", "nbf", "acr", "aio", "appidacr", "ipaddr", "scp", "sub", "tid", "uti", "unique_name", "apptype", "appid", "ver", "http://schemas.microsoft.com/ws/2008/06/identity/claims/authenticationinstant", "http://schemas.microsoft.com/identity/claims/scope" }; /// /// This code is similar to the code in AppPrincipalFactory where the claims are stored in a secureCache. @@ -127,9 +125,8 @@ private async Task LoadExtraClaimsAsync(ClaimsIdentity? identity) // if cachedClaims is not null, it means that the information was in the cache and didn't expire yet. if (cachedClaims is not null) { - // Add the new claims to the identity - identity.AddClaims(cachedClaims.Where(c => !identity.Claims.Any(c1 => StringComparer.OrdinalIgnoreCase.Equals(c1.Type, c.ClaimType))) - .Select(c => new Claim(c.ClaimType, c.Value))); + // Add the new claims to the identity. Note that we need to merge the same way as in the case they are retrieved (see below) + identity.MergeClaims(cachedClaims); return; } @@ -142,37 +139,28 @@ private async Task LoadExtraClaimsAsync(ClaimsIdentity? identity) // Should receive specific extra claims. This is the responsibility of the caller to provide the right claims. // We expect the exp claim to be present. // if not the persistence time will be the default one. - var claims = (await _claimsFiller.GetAsync(identity, settings, null).ConfigureAwait(false)).Where(c => !ClaimsToExclude.Contains(c.ClaimType)).ToList(); + var claims = (await _claimsFiller.GetAsync(identity, settings, null).ConfigureAwait(false)).Sanitize().ToList(); // Load the claims into the identity but exclude the exp claim and the one already present. - identity.AddClaims(claims.Where(c => !StringComparer.OrdinalIgnoreCase.Equals(c.ClaimType, tokenExpirationClaimType)) - .Where(c => !identity.Claims.Any(c1 => StringComparer.OrdinalIgnoreCase.Equals(c1.Type, c.ClaimType))) - .Select(c => new Claim(c.ClaimType, c.Value))); + identity.MergeClaims(claims); + + TimeSpan timeout; // Check expiry claim explicitly returned in the call to _claimsFiller.GetAsync // If no expiry claim found in the call to _claimsFiller.GetAsync, try to locate one in the existing identity claims (which are normally obtained from the bearer token) - DateTimeOffset? expDate; - - if (!TryParseExpirationDate(claims.FirstOrDefault(c => StringComparer.OrdinalIgnoreCase.Equals(c.ClaimType, tokenExpirationClaimType))?.Value, out expDate) && !TryParseExpirationDate(identity.Claims.FirstOrDefault(c => StringComparer.OrdinalIgnoreCase.Equals(c.Type, tokenExpirationClaimType))?.Value, out expDate)) + if (claims.TryGetExpiration(out var ticks) || identity.Claims.TryGetExpiration(out ticks)) + { + // there's an expiration claim in the claims returned by the claims filler or in the existing identity claims, so we can compute an expiration date + timeout = DateTimeOffset.FromUnixTimeSeconds(ticks) - DateTimeOffset.UtcNow; + } + else { // fall back to the configured max time. - expDate = DateTimeOffset.UtcNow.Add(_options.MaxTime); + timeout = _options.MaxTime; } - SaveClaimsToCache(claims, cacheKey, expDate!.Value - DateTimeOffset.UtcNow); - } - } - - private static bool TryParseExpirationDate(string? value, out DateTimeOffset? expirationDate) - { - if (value is not null && long.TryParse(value, NumberStyles.Integer, CultureInfo.InvariantCulture, out var ticks)) - { - expirationDate = DateTimeOffset.FromUnixTimeSeconds(ticks); - return true; + SaveClaimsToCache(claims, cacheKey, timeout); } - - expirationDate = null; - return false; } private List? GetClaimsFromCache(string? cacheKey) @@ -198,6 +186,7 @@ private void SaveClaimsToCache([DisallowNull] IEnumerable claims, stri return; } + // Only if the cache key is valid does it make sense to test the claims validity ArgumentNullException.ThrowIfNull(claims); if (timeout > TimeSpan.Zero) diff --git a/src/Arc4u.Standard.OAuth2.AspNetCore/Security/AppServicePrincipalFactory.cs b/src/Arc4u.Standard.OAuth2.AspNetCore/Security/AppServicePrincipalFactory.cs index 604b98db..b0d55442 100644 --- a/src/Arc4u.Standard.OAuth2.AspNetCore/Security/AppServicePrincipalFactory.cs +++ b/src/Arc4u.Standard.OAuth2.AspNetCore/Security/AppServicePrincipalFactory.cs @@ -1,10 +1,6 @@ -using System; using System.Diagnostics; using System.IdentityModel.Tokens.Jwt; -using System.Linq; using System.Security.Claims; -using System.Threading; -using System.Threading.Tasks; using Arc4u.Configuration; using Arc4u.Dependency; using Arc4u.Dependency.Attribute; @@ -12,6 +8,7 @@ using Arc4u.OAuth2.Token; using Arc4u.Security.Principal; using Arc4u.ServiceModel; +using Arc4u.Standard.OAuth2.Security; using Microsoft.AspNetCore.Authentication; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -23,8 +20,6 @@ public class AppServicePrincipalFactory : IAppPrincipalFactory { public const string ProviderKey = "ProviderId"; - public static readonly string tokenExpirationClaimType = "exp"; - public static readonly string[] ClaimsToExclude = { "exp", "aud", "iss", "iat", "nbf", "acr", "aio", "appidacr", "ipaddr", "scp", "sub", "tid", "uti", "unique_name", "apptype", "appid", "ver", "http://schemas.microsoft.com/ws/2008/06/identity/claims/authenticationinstant", "http://schemas.microsoft.com/identity/claims/scope" }; private readonly IContainerResolve _container; private readonly ILogger _logger; @@ -102,13 +97,12 @@ private async Task BuildTheIdentity(ClaimsIdentity identity, IKeyValueSettings s } // Check the settings contains the service url. - TokenInfo? token = null; try { - token = await provider.GetTokenAsync(settings, parameter).ConfigureAwait(true); + var token = await provider.GetTokenAsync(settings, parameter).ConfigureAwait(true); identity.BootstrapContext = token.Token; var jwtToken = new JwtSecurityToken(token.Token); - identity.AddClaims(jwtToken.Claims.Where(c => !ClaimsToExclude.Any(arg => arg.Equals(c.Type))).Select(c => new Claim(c.Type, c.Value))); + identity.MergeClaims(jwtToken.Claims.Sanitize()); } catch (Exception ex) { diff --git a/src/Arc4u.Standard.OAuth2.Client/Security/Principal/AppPrincipalFactory.cs b/src/Arc4u.Standard.OAuth2.Client/Security/Principal/AppPrincipalFactory.cs index 4017ea14..819ce4ec 100644 --- a/src/Arc4u.Standard.OAuth2.Client/Security/Principal/AppPrincipalFactory.cs +++ b/src/Arc4u.Standard.OAuth2.Client/Security/Principal/AppPrincipalFactory.cs @@ -1,11 +1,5 @@ -using System; -using System.Collections.Generic; -using System.Globalization; using System.IdentityModel.Tokens.Jwt; -using System.Linq; using System.Security.Claims; -using System.Threading; -using System.Threading.Tasks; using Arc4u.Caching; using Arc4u.Configuration; using Arc4u.Dependency; @@ -16,6 +10,7 @@ using Arc4u.OAuth2.Token; using Arc4u.Security.Principal; using Arc4u.ServiceModel; +using Arc4u.Standard.OAuth2.Security; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -28,8 +23,6 @@ public class AppPrincipalFactory : IAppPrincipalFactory public const string DefaultSettingsResolveName = "OAuth2"; public const string PlatformParameters = "platformParameters"; - public static readonly string tokenExpirationClaimType = "exp"; - public static readonly string[] ClaimsToExclude = { "exp", "aud", "iss", "iat", "nbf", "acr", "aio", "appidacr", "ipaddr", "scp", "sub", "tid", "uti", "unique_name", "apptype", "appid", "ver", "http://schemas.microsoft.com/ws/2008/06/identity/claims/authenticationinstant", "http://schemas.microsoft.com/identity/claims/scope" }; private readonly IContainerResolve _container; private readonly INetworkInformation _networkInformation; @@ -118,10 +111,6 @@ private async Task BuildTheIdentity(ClaimsIdentity identity, IKeyValueSettings s // The token has claims filled by the STS. // We can fill the new federated identity with the claims from the token. var jwtToken = new JwtSecurityToken(token.Token); - var expTokenClaim = jwtToken.Claims.FirstOrDefault(c => c.Type.Equals(tokenExpirationClaimType, StringComparison.InvariantCultureIgnoreCase)); - long expTokenTicks = 0; - if (null != expTokenClaim) - long.TryParse(expTokenClaim.Value, NumberStyles.Integer, CultureInfo.InvariantCulture, out expTokenTicks); // The key for the cache is based on the claims from a ClaimsIdentity => build a dummy identity with the claim from the token. var dummyIdentity = new ClaimsIdentity(jwtToken.Claims); @@ -132,16 +121,12 @@ private async Task BuildTheIdentity(ClaimsIdentity identity, IKeyValueSettings s // if we have a token "cached" from the system, we can take the authorization claims from the cache (if exists)... // so we avoid too many backend calls for nothing. // But every time we have a token that has been refreshed, we will call the backend (if available and reload the claims). - var cachedExpiredClaim = cachedClaims.FirstOrDefault(c => c.ClaimType.Equals(tokenExpirationClaimType, StringComparison.InvariantCultureIgnoreCase)); - long cachedExpiredTicks = 0; - - if (null != cachedExpiredClaim) - long.TryParse(cachedExpiredClaim.Value, NumberStyles.Integer, CultureInfo.InvariantCulture, out cachedExpiredTicks); // we only call the backend if the ticks are not the same. - bool copyClaimsFromCache = cachedExpiredTicks > 0 && expTokenTicks > 0 && cachedClaims.Count > 0 && cachedExpiredTicks == expTokenTicks; - - if (copyClaimsFromCache) + var copyClaimsFromCache = jwtToken.Claims.TryGetExpiration(out Claim? expTokenClaim) && expTokenClaim.Value.TryParseTicks(out var expTokenTicks) + && cachedClaims.TryGetExpiration(out var cachedExpiredTicks) + && cachedExpiredTicks == expTokenTicks; + if (copyClaimsFromCache && cachedClaims.Count > 0) { identity.AddClaims(cachedClaims.Select(p => new Claim(p.ClaimType, p.Value))); messages.Add(new Message(ServiceModel.MessageCategory.Technical, MessageType.Information, "Create the principal from the cache, token has not been refreshed.")); @@ -149,21 +134,24 @@ private async Task BuildTheIdentity(ClaimsIdentity identity, IKeyValueSettings s else { // Fill the claims based on the token and the backend call - identity.AddClaims(jwtToken.Claims.Where(c => !ClaimsToExclude.Any(arg => arg.Equals(c.Type))).Select(c => new Claim(c.Type, c.Value))); - identity.AddClaim(expTokenClaim); + identity.AddClaims(jwtToken.Claims.Sanitize().Select(c => new Claim(c.Type, c.Value))); + // there is no guarantee that expTokenClaim is not null, so we need to check it. + if (expTokenClaim is not null) + { + identity.AddClaim(expTokenClaim); + } if (_container.TryResolve(out IClaimsFiller claimFiller)) // Fill the claims with more information. { try { // Get the claims and clean any technical claims in case of. - var claims = (await claimFiller.GetAsync(identity, new List { settings }, parameter)) - .Where(c => !ClaimsToExclude.Any(arg => arg.Equals(c.ClaimType))).ToList(); + var claims = (await claimFiller.GetAsync(identity, new List { settings }, parameter)).Sanitize().ToList(); // We copy the claims from the backend but the exp claim will be the value of the token (front end definition) and not the backend one. Otherwhise there will be always a difference. - identity.AddClaims(claims.Where(c => !identity.Claims.Any(c1 => c1.Type == c.ClaimType)).Select(c => new Claim(c.ClaimType, c.Value))); + identity.MergeClaims(claims); - messages.Add(new Message(ServiceModel.MessageCategory.Technical, MessageType.Information, $"Add {claims.Count()} claims to the principal.")); + messages.Add(new Message(ServiceModel.MessageCategory.Technical, MessageType.Information, $"Add {claims.Count} claims to the principal.")); messages.Add(new Message(ServiceModel.MessageCategory.Technical, MessageType.Information, $"Save claims to the cache.")); } catch (Exception e) diff --git a/src/Arc4u.Standard.OAuth2/Security/ClaimsUtility.cs b/src/Arc4u.Standard.OAuth2/Security/ClaimsUtility.cs new file mode 100644 index 00000000..0dbaeb43 --- /dev/null +++ b/src/Arc4u.Standard.OAuth2/Security/ClaimsUtility.cs @@ -0,0 +1,117 @@ +#if NET6_0_OR_GREATER +using System.Diagnostics.CodeAnalysis; +#endif +using System.Globalization; +using System.Security.Claims; +using Arc4u.IdentityModel.Claims; + +namespace Arc4u.Standard.OAuth2.Security; + +/// +/// Claims utilities and extension methods, used in AppPrincipalFactory and AppPrincipalTransform. +/// +/// +/// Claim type comparison is always case-sensitive. See https://www.rfc-editor.org/rfc/rfc7519 section 10.1. +/// +public static class ClaimsUtility +{ + private const string ExpirationClaimType = "exp"; + private static readonly HashSet ClaimsToExclude = ["aud", "iss", "iat", "nbf", "acr", "aio", "appidacr", "ipaddr", "scp", "sub", "tid", "uti", "unique_name", "apptype", "appid", "ver", "http://schemas.microsoft.com/ws/2008/06/identity/claims/authenticationinstant", "http://schemas.microsoft.com/identity/claims/scope"]; + + /// + /// Sanitize the list of external claims by removing the claims we don't need. + /// The may come from an external system. If there is an "exp" claim in the list, it will be included. + /// + /// + /// + public static IEnumerable Sanitize(this IEnumerable claims) + { + foreach (var claim in claims) + { + if (ClaimsToExclude.Contains(claim.ClaimType)) + { + continue; + } + yield return claim; + } + } + + /// + /// Sanitize the list of internal claims by removing the claims we don't need. + /// The come from the token claims . If there is an "exp" claim in the list, it will be *excluded* as well + /// since it may be replaced by the equivalent external claim. + /// + /// + /// + public static IEnumerable Sanitize(this IEnumerable claims) + { + foreach (var claim in claims) + { + if (ClaimsToExclude.Contains(claim.Type) || claim.Type == ExpirationClaimType) + { + continue; + } + yield return claim; + } + } + + /// + /// Merge the into the , but only the claims that are not already present. + /// The "exp" claim is always excluded from the merge. + /// + /// + /// + public static void MergeClaims(this ClaimsIdentity claimsIdentity, IEnumerable claimsToMerge) + { + claimsIdentity.AddClaims(claimsToMerge.Where(c => c.ClaimType != ExpirationClaimType && !claimsIdentity.HasClaim(c1 => c1.Type == c.ClaimType)).Select(c => new Claim(c.ClaimType, c.Value))); + } + + /// + /// Merge the into the , but only the claims that are not already present. + /// The "exp" claim is always excluded from the merge. + /// + /// + /// + public static void MergeClaims(this ClaimsIdentity claimsIdentity, IEnumerable claimsToMerge) + { + claimsIdentity.AddClaims(claimsToMerge.Where(c => c.Type != ExpirationClaimType && !claimsIdentity.HasClaim(c1 => c1.Type == c.Type)).Select(c => new Claim(c.Type, c.Value))); + } + +#if NET6_0_OR_GREATER + public static bool TryGetExpiration(this IEnumerable claims, [NotNullWhen(true)] out Claim? expClaim) +#else + public static bool TryGetExpiration(this IEnumerable claims, out Claim expClaim) +#endif + { + expClaim = claims.FirstOrDefault(c => c.Type == ExpirationClaimType); + return expClaim is not null; + } + + public static bool TryGetExpiration(this IEnumerable claims, out long ticks) + { + var claim = claims.FirstOrDefault(c => c.Type == ExpirationClaimType); + + if (claim is null) + { + ticks = 0; + return false; + } + + return claim.Value.TryParseTicks(out ticks); + } + + public static bool TryGetExpiration(this IEnumerable claims, out long ticks) + { + var claim = claims.FirstOrDefault(c => c.ClaimType == ExpirationClaimType); + + if (claim is null) + { + ticks = 0; + return false; + } + + return claim.Value.TryParseTicks(out ticks); + } + + public static bool TryParseTicks(this string s, out long ticks) => long.TryParse(s, NumberStyles.Integer, CultureInfo.InvariantCulture, out ticks); +}