Skip to content

Fix JWT DateTimeOffset handling with DateTime.MinValue #33651

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 23, 2021
Merged

Fix JWT DateTimeOffset handling with DateTime.MinValue #33651

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions src/Security/Authentication/JwtBearer/src/JwtBearerHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,8 @@ protected override async Task<AuthenticateResult> HandleAuthenticateAsync()
SecurityToken = validatedToken
};

tokenValidatedContext.Properties.ExpiresUtc = validatedToken.ValidTo;
tokenValidatedContext.Properties.IssuedUtc = validatedToken.ValidFrom;
tokenValidatedContext.Properties.ExpiresUtc = GetSafeDateTime(validatedToken.ValidTo);
tokenValidatedContext.Properties.IssuedUtc = GetSafeDateTime(validatedToken.ValidFrom);

await Events.TokenValidated(tokenValidatedContext);
if (tokenValidatedContext.Result != null)
Expand Down Expand Up @@ -202,6 +202,17 @@ protected override async Task<AuthenticateResult> HandleAuthenticateAsync()
}
}

private static DateTime? GetSafeDateTime(DateTime dateTime)
{
// Assigning DateTime.MinValue or default(DateTime) to a DateTimeOffset when in a UTC+X timezone will throw
// Since we don't really care about DateTime.MinValue in this case let's just set the field to null
Copy link
Member

@Tratcher Tratcher Jun 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link to the runtime issue.

Copy link
Member Author

@BrennanConroy BrennanConroy Jun 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out it's by design

Copy link

@Clockwork-Muse Clockwork-Muse Jun 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frankly, this feels like the wrong approach. That you're getting an error here means the returned DateTimes aren't DateTimeKind.Utc, which seems... not correct for their intended use. There's no reason for them to be anything other than Utc, and the fact that the "safe" method doesn't otherwise correct the issue means it's going to cause problems further down the line. If you echo/pass on the claims, is this currently causing these timestamps to change due to local/utc confusion?

The JWT claims are effectively ignorant of timezone, but are still absolute timestamps, and so can be considered UTC - a far better approach would be ensuring that the parsing method ensures that it returns the DateTime with the proper kind, and ensure that anything that could modify has the output coerced to UTC (via SpecifyKind(DateTimeKind.Utc) - which is just going to "assign" the kind, and leave the timestamp otherwise unchanged, and won't throw).

Copy link
Member Author

@BrennanConroy BrennanConroy Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frankly, this feels like the wrong approach. That you're getting an error here means the returned DateTimes aren't DateTimeKind.Utc, which seems... not correct for their intended use.

I do agree that the times from the JWT library should be in UTC, and you can see that they tried to do that in most cases
https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/d6f2b66d788195b50f2b1f700beb497851194c73/src/Microsoft.IdentityModel.Tokens/EpochTime.cs#L70

However, whenever DateTime.MinValue is used, the library does not convert it to UTC:
https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/d6f2b66d788195b50f2b1f700beb497851194c73/src/System.IdentityModel.Tokens.Jwt/JwtSecurityToken.cs#L452
https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/d6f2b66d788195b50f2b1f700beb497851194c73/src/System.IdentityModel.Tokens.Jwt/JwtPayload.cs#L730
https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/d6f2b66d788195b50f2b1f700beb497851194c73/src/System.IdentityModel.Tokens.Jwt/JwtPayload.cs#L742

I recommend filing an issue at https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet and perhaps even contributing a change for it!

if (dateTime == DateTime.MinValue)
{
return null;
}
return dateTime;
}

/// <inheritdoc />
protected override async Task HandleChallengeAsync(AuthenticationProperties properties)
{
Expand Down
43 changes: 42 additions & 1 deletion src/Security/Authentication/test/JwtBearerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -846,6 +846,47 @@ public async Task ExpirationAndIssuedSetOnAuthenticateResult()
Assert.Equal(token.ValidFrom, dom.RootElement.GetProperty("issued").GetDateTimeOffset());
}

[Fact]
public async Task ExpirationAndIssuedNullWhenMinOrMaxValue()
{
var key = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(new string('a', 128)));
var creds = new SigningCredentials(key, SecurityAlgorithms.HmacSha256);

var claims = new[]
{
new Claim(ClaimTypes.NameIdentifier, "Bob")
};

var token = new JwtSecurityToken(
issuer: "issuer.contoso.com",
audience: "audience.contoso.com",
claims: claims,
expires: DateTime.MaxValue,
signingCredentials: creds);

var tokenText = new JwtSecurityTokenHandler().WriteToken(token);

using var host = await CreateHost(o =>
{
o.SaveToken = true;
o.TokenValidationParameters = new TokenValidationParameters()
{
ValidIssuer = "issuer.contoso.com",
ValidAudience = "audience.contoso.com",
IssuerSigningKey = key,
};
});

var newBearerToken = "Bearer " + tokenText;
using var server = host.GetTestServer();
var response = await SendAsync(server, "http://example.com/expiration", newBearerToken);
Assert.Equal(HttpStatusCode.OK, response.Response.StatusCode);
var responseBody = await response.Response.Content.ReadAsStringAsync();
using var dom = JsonDocument.Parse(responseBody);
Assert.Equal(JsonValueKind.Null, dom.RootElement.GetProperty("expires").ValueKind);
Assert.Equal(JsonValueKind.Null, dom.RootElement.GetProperty("issued").ValueKind);
}

class InvalidTokenValidator : ISecurityTokenValidator
{
public InvalidTokenValidator()
Expand Down Expand Up @@ -1065,7 +1106,7 @@ private static async Task<IHost> CreateHost(Action<JwtBearerOptions> options = n
{
var authenticationResult = await context.AuthenticateAsync(JwtBearerDefaults.AuthenticationScheme);
await context.Response.WriteAsJsonAsync(
new { Expires = authenticationResult.Properties.ExpiresUtc, Issued = authenticationResult.Properties.IssuedUtc });
new { Expires = authenticationResult.Properties?.ExpiresUtc, Issued = authenticationResult.Properties?.IssuedUtc });
}
else
{
Expand Down