Fix JWT DateTimeOffset handling with DateTime.MinValue #33651
Conversation
|
/backport to release/6.0-preview6 |
|
Started backporting to release/6.0-preview6: https://github.com/dotnet/aspnetcore/actions/runs/951304071 |
| 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 |
There was a problem hiding this comment.
Turns out it's by design
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Frankly, this feels like the wrong approach. That you're getting an error here means the returned
DateTimes aren'tDateTimeKind.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!
ghost
added this to the
Fixes #33634