Improving custom JWT authentication - #3681
Improving custom JWT authentication#3681Michael Wagner (MichaelWagner-blue-zone) wants to merge 8 commits into
Conversation
Merge missing improvements from origin
* optional JWKS URL support for JWT validation (jwksUrl) * JWT role normalization during token validation * related authorization updates to respect configured role claim types * PostgreSQL session-context support for propagated claims * a few smaller robustness fixes in the affected components
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
This PR updates Data API Builder’s authentication and authorization pipeline to improve JWT handling (configurable role-claim extraction/normalization and JWKS-based signing key resolution) and adds PostgreSQL session-context propagation for processed claims.
Changes:
- Adds configurable JWT role claim parsing/normalization (rolesPath, rolesSeparator) and updates authorization to respect configured role claim types.
- Introduces JWKS URL support for JWT validation and wires in signing-key retrieval during startup/hot-reload configuration.
- Adds PostgreSQL session-context propagation of processed claims via
set_config(...).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Service/Startup.cs | Configures JWT bearer auth, role-claim behavior, and JWKS fetching during startup. |
| src/Service.Tests/Authentication/Helpers/WebHostBuilderHelper.cs | Updates test host JWT configuration to use configurable role claim type. |
| src/Core/Resolvers/PostgresQueryBuilder.cs | Fixes JSON aggregation typing by casting empty array literal to jsonb. |
| src/Core/Resolvers/PostgreSqlExecutor.cs | Adds PostgreSQL session-context propagation and refactors managed identity token handling. |
| src/Core/Resolvers/OboSqlTokenProvider.cs | Updates authorization-context hashing to respect configured role claim types. |
| src/Core/Authorization/AuthorizationResolver.cs | Updates claim resolution to use configured role claim type while preserving original roles. |
| src/Core/AuthenticationHelpers/JwtRoleClaimsTransformer.cs | Adds role-claim normalization (JSON array + separated string support). |
| src/Core/AuthenticationHelpers/JwtHttpClientFactory.cs | Adds a shared HTTP client factory (incl. optional self-signed cert handling) for JWKS retrieval. |
| src/Core/AuthenticationHelpers/ConfigureJwtBearerOptions.cs | Updates hot-reload JWT bearer configuration to use JWKS and role-claim normalization. |
| src/Core/AuthenticationHelpers/ClientRoleHeaderAuthenticationMiddleware.cs | Hardens principal assignment and maps Generic OAuth provider to JWT bearer scheme. |
| src/Config/ObjectModel/JwtOptions.cs | Expands JWT config model with rolesPath/rolesSeparator/jwksUrl and derived helpers. |
| src/Config/ObjectModel/DataSource.cs | Adds PostgreSqlOptions typed options support. |
| schemas/dab.draft.schema.json | Extends JSON schema to document the new JWT configuration fields. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
src/Core/Resolvers/PostgreSqlExecutor.cs:393
SetSessionContextexecutes a reader and then fully drains resultsets even though the results are ignored. This adds overhead on every query;ExecuteNonQuery()is sufficient here.
cmd.CommandText = sql.ToString();
using DbDataReader reader = cmd.ExecuteReader();
do
src/Service.Tests/Authentication/Helpers/WebHostBuilderHelper.cs:149
- This test helper re-implements the role-claim resolution logic (
RolesPathvs default). Since JwtOptions now exposesResolvedRoleClaimType, using it avoids duplication and keeps tests aligned with production behavior.
string resolvedRoleClaimType = string.IsNullOrWhiteSpace(authOptions.Jwt.RolesPath)
? AuthenticationOptions.ROLE_CLAIM_TYPE
: authOptions.Jwt.RolesPath;
src/Core/AuthenticationHelpers/JwtRoleClaimsTransformer.cs:15
- New role normalization behavior (rolesPath/rolesSeparator handling, JSON-array parsing, and OnTokenValidated normalization) is introduced without any direct tests. Consider adding Service.Tests coverage for: (1) roles emitted as a JSON array, (2) roles emitted as a separator-delimited string, and (3) non-default role claim types via rolesPath to ensure authorization and OBO hashing behave as expected.
public static void NormalizeRoleClaims(
ClaimsPrincipal principal,
string sourceRoleClaimType,
string? separator)
{
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Suppressed comments (6)
src/Core/Resolvers/PostgreSqlExecutor.cs:278
- Typo in warning message: "DefaultAzureCrendential" should be "DefaultAzureCredential" (affects log/searchability).
string messagePrefix = "{correlationId} No password detected in the connection string. Attempt to retrieve a managed identity access token using DefaultAzureCredential failed due to:\n{errorMessage}";
string messageSuffix = firstAttemptAtDefaultAccessToken
? "If authentication with DefaultAzureCrendential is not intended, this warning can be safely ignored."
: string.Empty;
src/Service/Startup.cs:1181
- JWKS is fetched synchronously during service startup via GetStringAsync(...).GetResult() and the keys are captured into TokenValidationParameters.IssuerSigningKeys once. This blocks startup on network latency/outages and prevents automatic signing key rotation/refresh, which can cause valid tokens to start failing until the service is restarted. Consider using the built-in configuration/metadata manager (e.g., JwtBearerOptions.ConfigurationManager / Microsoft.IdentityModel.Protocols.ConfigurationManager) to cache and refresh keys.
JsonWebKeySet jwks;
using (HttpClient client = JwtHttpClientFactory.Create())
{
string jwksJson = client.GetStringAsync(jwksUrl).GetAwaiter().GetResult();
jwks = new JsonWebKeySet(jwksJson);
}
options.TokenValidationParameters = new Microsoft.IdentityModel.Tokens.TokenValidationParameters()
{
ValidAudience = authOptions.Jwt.Audience,
ValidIssuer = authOptions.Jwt.Issuer,
ValidateIssuer = true,
ValidateAudience = true,
ValidateLifetime = true,
ValidateIssuerSigningKey = true,
IssuerSigningKeys = jwks.Keys,
// Instructs the asp.net core middleware which JWT claim to use for User.IsInRole()
// Defaults to "roles" when not explicitly configured.
RoleClaimType = authOptions.Jwt.ResolvedRoleClaimType
};
src/Core/AuthenticationHelpers/ConfigureJwtBearerOptions.cs:82
- Hot-reload JwtBearerOptions configuration performs a synchronous JWKS HTTP fetch (GetStringAsync(...).GetResult()) and replaces TokenValidationParameters.IssuerSigningKeys without any caching/refresh strategy. This can block threads during reload and still doesn't support key rotation over time. Consider using a refreshable ConfigurationManager or cached key provider instead of doing a blocking fetch in Configure().
string? jwksUri = newAuthOptions.Jwt.ResolvedJwksUrl;
if (string.IsNullOrWhiteSpace(jwksUri))
{
return;
}
JsonWebKeySet jwks;
using (HttpClient client = JwtHttpClientFactory.Create())
{
string jwksJson = client.GetStringAsync(jwksUri).GetAwaiter().GetResult();
jwks = new JsonWebKeySet(jwksJson);
}
options.TokenValidationParameters = new Microsoft.IdentityModel.Tokens.TokenValidationParameters()
{
ValidAudience = newAuthOptions.Jwt.Audience,
ValidIssuer = newAuthOptions.Jwt.Issuer,
ValidateIssuer = true,
ValidateAudience = true,
ValidateLifetime = true,
ValidateIssuerSigningKey = true,
IssuerSigningKeys = jwks.Keys,
// Instructs the asp.net core middleware which JWT claim to use for User.IsInRole()
// Defaults to "roles" when not explicitly configured.
RoleClaimType = newAuthOptions.Jwt.ResolvedRoleClaimType
};
src/Service/Startup.cs:1159
- The exception message says "requires either issuer or jwksUrl", but the current config validator and schema require both Audience and Issuer for JWT identity providers. Consider aligning this message with actual validation expectations (or update validation/schema if issuer-only is no longer required).
if (string.IsNullOrWhiteSpace(jwksUrl))
{
throw new DataApiBuilderException(
message: "JWT configuration requires either issuer or jwksUrl.",
statusCode: System.Net.HttpStatusCode.ServiceUnavailable,
subStatusCode: DataApiBuilderException.SubStatusCodes.ConfigValidationError);
src/Core/AuthenticationHelpers/JwtRoleClaimsTransformer.cs:56
- Role normalization is new behavior (JSON array parsing + configurable separator expansion), but there are no unit tests covering NormalizeRoleClaims / ExpandClaimValues for common cases (single role, delimited string, JSON array, malformed JSON, case-insensitive de-dupe). Adding focused tests would help prevent regressions in authorization behavior.
public static void NormalizeRoleClaims(
ClaimsPrincipal principal,
string sourceRoleClaimType,
string? separator)
{
foreach (ClaimsIdentity identity in principal.Identities)
{
if (!identity.IsAuthenticated)
{
continue;
}
List<Claim> sourceClaims = identity.Claims
.Where(c => c.Type.Equals(sourceRoleClaimType, StringComparison.Ordinal))
.ToList();
if (sourceClaims.Count == 0)
{
continue;
}
HashSet<string> normalizedValues = new(StringComparer.OrdinalIgnoreCase);
foreach (Claim claim in sourceClaims)
{
foreach (string expandedValue in ExpandClaimValues(claim.Value, separator))
{
if (!string.IsNullOrWhiteSpace(expandedValue))
{
normalizedValues.Add(expandedValue.Trim());
}
}
}
foreach (string normalizedValue in normalizedValues)
{
bool exactClaimAlreadyExists = identity.Claims.Any(c =>
c.Type.Equals(sourceRoleClaimType, StringComparison.Ordinal) &&
c.Value.Equals(normalizedValue, StringComparison.OrdinalIgnoreCase));
if (!exactClaimAlreadyExists)
{
identity.AddClaim(new Claim(sourceRoleClaimType, normalizedValue, ClaimValueTypes.String));
}
}
}
src/Core/Resolvers/PostgreSqlExecutor.cs:366
- PostgreSQL session-context propagation is new behavior (set_config calls based on processed user claims), but there are no tests validating (a) which claims are written, (b) correct key normalization, and (c) that settings don’t leak across pooled connections. Adding unit/integration coverage for this feature would reduce the risk of security regressions.
/// <summary>
/// PostgreSQL override that first sets session settings on the already-open connection
/// using a dedicated command, then returns the actual data command.
/// </summary>
public override DbCommand PrepareDbCommand(
NpgsqlConnection conn,
string sqltext,
IDictionary<string, DbConnectionParam> parameters,
HttpContext? httpContext,
string dataSourceName)
{
SetSessionContext(conn, httpContext, dataSourceName);
NpgsqlCommand cmd = conn.CreateCommand();
cmd.CommandType = System.Data.CommandType.Text;
cmd.CommandText = sqltext;
if (parameters is not null)
{
foreach (KeyValuePair<string, DbConnectionParam> parameterEntry in parameters)
{
DbParameter parameter = cmd.CreateParameter();
parameter.ParameterName = parameterEntry.Key;
parameter.Value = parameterEntry.Value.Value ?? DBNull.Value;
PopulateDbTypeForParameter(parameterEntry, parameter);
cmd.Parameters.Add(parameter);
}
}
return cmd;
}
/// <summary>
/// Sets processed user claims into PostgreSQL custom settings on the same open connection.
/// This command's resultsets are consumed and ignored before the actual query command is created.
/// </summary>
private void SetSessionContext(
NpgsqlConnection conn,
HttpContext? httpContext,
string dataSourceName)
{
if (string.IsNullOrEmpty(dataSourceName))
{
dataSourceName = ConfigProvider.GetConfig().DefaultDataSourceName;
}
if (httpContext is null ||
!_dataSourceToSessionContextUsage.TryGetValue(dataSourceName, out bool enabled) ||
!enabled)
{
return;
}
Dictionary<string, string> sessionParams = AuthorizationResolver.GetProcessedUserClaims(httpContext);
if (sessionParams.Count == 0)
{
return;
}
| bool allowSelfSigned = Environment.GetEnvironmentVariable("USE_SELF_SIGNED_CERT") | ||
| ?.Equals("true", StringComparison.OrdinalIgnoreCase) == true; | ||
|
|
||
| HttpClientHandler handler = new(); | ||
|
|
||
| if (allowSelfSigned) | ||
| { | ||
| handler.ServerCertificateCustomValidationCallback = | ||
| HttpClientHandler.DangerousAcceptAnyServerCertificateValidator; | ||
| } |
There was a problem hiding this comment.
You are right, but using USE_SELF_SIGNED_CERT has to be configured explicitly by the user, so he is responsible for that. He should know about it, when to using it. I would not accept your suggestion.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Suppressed comments (7)
src/Core/AuthenticationHelpers/JwtRoleClaimsTransformer.cs:49
- This check for an existing claim compares the claim type using
StringComparison.Ordinal, which is inconsistent with the intended case-insensitive handling of role claim types. Align it with the normalization logic by usingOrdinalIgnoreCasefor the type comparison as well.
bool exactClaimAlreadyExists = identity.Claims.Any(c =>
c.Type.Equals(sourceRoleClaimType, StringComparison.Ordinal) &&
c.Value.Equals(normalizedValue, StringComparison.OrdinalIgnoreCase));
src/Service/Startup.cs:1159
- This validation error message is misleading: the current schema requires
issuer, and token validation below also always setsValidIssuerandValidateIssuer = true, so an issuer is still required even ifjwksUrlis configured. Update the message to reflect the actual requirement to avoid confusing users.
throw new DataApiBuilderException(
message: "JWT configuration requires either issuer or jwksUrl.",
statusCode: System.Net.HttpStatusCode.ServiceUnavailable,
subStatusCode: DataApiBuilderException.SubStatusCodes.ConfigValidationError);
src/Core/Resolvers/QueryExecutor.cs:15
using HotChocolate.Types.Pagination;is unused in this file, and will trigger build warnings (and may fail the build if warnings are treated as errors).
using HotChocolate.Types.Pagination;
src/Core/AuthenticationHelpers/ConfigureJwtBearerOptions.cs:66
- JWKS is fetched synchronously inside
IConfigureNamedOptions.Configure(...). This can block threads and re-fetch signing keys whenever options are rebuilt (e.g., hot reload), and there is no caching/refresh strategy or timeout/error handling here. Consider using the built-in OIDC/JWKS retrieval mechanisms (ConfigurationManager/Authority metadata) or adding an in-memory cache with refresh/backoff and a bounded timeout.
using (HttpClient client = JwtHttpClientFactory.Create())
{
string jwksJson = client.GetStringAsync(jwksUri).GetAwaiter().GetResult();
jwks = new JsonWebKeySet(jwksJson);
src/Core/AuthenticationHelpers/JwtRoleClaimsTransformer.cs:25
- Role-claim type matching is currently case-sensitive (
StringComparison.Ordinal). Other parts of the auth pipeline treat role claim types case-insensitively; this can cause configuredrolesPathvalues (or tokens) that differ only by case to not be normalized. UseOrdinalIgnoreCasefor claim type comparisons.
This issue also appears on line 47 of the same file.
List<Claim> sourceClaims = identity.Claims
.Where(c => c.Type.Equals(sourceRoleClaimType, StringComparison.Ordinal))
.ToList();
src/Core/AuthenticationHelpers/JwtHttpClientFactory.cs:6
- This file lives under
src/Core/AuthenticationHelpers/but declaresnamespace Azure.DataApiBuilder.Service;, while other types in this folder useAzure.DataApiBuilder.Core.AuthenticationHelpers(e.g., ClientRoleHeaderAuthenticationMiddleware.cs, SupportedAuthNProviders.cs). This mismatch makes the code harder to discover and maintain. Consider moving the file undersrc/Service/or changing the namespace to the Core.AuthenticationHelpers namespace and updating call sites accordingly.
namespace Azure.DataApiBuilder.Service;
public static class JwtHttpClientFactory
src/Service.Tests/Authentication/Helpers/WebHostBuilderHelper.cs:149
- Now that
JwtOptionsexposesResolvedRoleClaimType, this helper duplicates that resolution logic manually. Using the shared property keeps behavior consistent with production code and avoids drift if the resolution logic changes.
string resolvedRoleClaimType = string.IsNullOrWhiteSpace(authOptions.Jwt.RolesPath)
? AuthenticationOptions.ROLE_CLAIM_TYPE
: authOptions.Jwt.RolesPath;
| finally | ||
| { | ||
| // Explicitly RESET the custom settings before returning the connection to the pool | ||
| // to not leak one request's claims into the next request that reuses the connection | ||
| using (var resetCmd = conn.CreateCommand()) | ||
| { | ||
| resetCmd.CommandText = "RESET ALL;"; // or granular RESET <setting> calls | ||
| await resetCmd.ExecuteNonQueryAsync(); | ||
| } | ||
| } |
Why make this change?
Improving custom JWT authentication.
What is this change?
rolesPath,rolesSeparator)jwksUrl)How was this tested?
Sample Request(s)