Secret Expiration behaves badly when DateTimeKind=Local

[SSMKittel]

Using: IdentityServer 4.1.0

When an IClientStore or an IResourceStore returns a Client/ApiResource with a secret that has an expiration, it only works correctly if the expiration is in DateTimeKind.Utc For DateTimeKind.Local, the secret is effectively offset by the local timezone. i.e. With positive timezone offsets the secret is invalidated later than it should be. With negative timezone offsets the secret is invalidated earlier than it should be.

Here's an example which illustrates the problem for ApiResource by using the InMemory store (though it occurs for client secret validation as well)

    class Program
    {
        static async Task Main(string[] args)
        {
            await Run("UTC expired", DateTime.UtcNow.AddMinutes(-10), true);
            await Run("UTC not expired", DateTime.UtcNow.AddMinutes(10), false);

            await Run("Local expired", DateTime.Now.AddMinutes(-10), true); // This fails for positive timezone offsets
            await Run("Local not expired", DateTime.Now.AddMinutes(10), false); // This fails for negative timezone offsets
        }

        static async Task Run(
            string description,
            DateTime secretExpiration,
            bool expectedError)
        {
            var sc = new ServiceCollection();
            sc.AddIdentityServer()
            .AddDefaultSecretParsers()
            .AddDefaultSecretValidators()
            .AddInMemoryApiResources(new[] {
                new ApiResource("test") {
                    Enabled = true,
                    ApiSecrets = new []
                    {
                        new Secret("secret".Sha256(), secretExpiration)
                    }
                }
            });
            var sp = sc.BuildServiceProvider();
            var sv = sp.GetRequiredService<IApiSecretValidator>();
            
            var context = new DefaultHttpContext();
            context.Request.Headers.Add(
                "Authorization",
                "Basic " + Convert.ToBase64String(Encoding.UTF8.GetBytes("test:secret")));

            var result = await sv.ValidateAsync(context);
            if (result.IsError != expectedError)
            {
                throw new Exception($"{description}: Expected IsError={expectedError} but was IsError={result.IsError}");
            }
        }
    }

[leastprivilege]

Thanks. We'll look into it.

[brockallen]

From just inspecting the code, DateTimeExtensions.HasExpired is where the check is happening, and it's using DateTime > operator which seems to take the DateTimeKind into account. So I'm confused why it's not working.

[brockallen]

For example, this code emits these values:

            var d1 = new DateTime(2020, 01, 01, 9, 0, 0, DateTimeKind.Utc);
            var d2 = new DateTime(2020, 01, 01, 9, 0, 0, DateTimeKind.Local);

            Console.WriteLine(d1 == d2); // emits true
            Console.WriteLine(d1 < d2); // emits false
            Console.WriteLine(d1 > d2); // emits false

[SSMKittel]

According to the documentation on DateTime, the operators don't take Kind into account and instead just compares the Ticks value (https://docs.microsoft.com/en-us/dotnet/api/system.datetime.op_greaterthan?view=netcore-3.1). Ticks appears to be independent of Kind, so both those dates have Ticks = 637134660000000000 and are considered equal. However if you have d2 = d1.ToLocalTime(), you get the erratic behaviour when comparing them:

            // Both represent the same instant in time
            var d1 = new DateTime(2020, 01, 01, 9, 0, 0, DateTimeKind.Utc);
            var d2 = d1.ToLocalTime();

            Console.WriteLine(d1.Ticks); // 637134660000000000
            Console.WriteLine(d2.Ticks); // 637135038000000000 (timezone-dependent)

            // DateTime operators don't take kind into account and
            // gets inconsistent results when comparing different kinds
            Console.WriteLine(d1 == d2); // emits false
            Console.WriteLine(d1 < d2); // emits true (timezone-dependent)
            Console.WriteLine(d1 > d2); // emits false (timezone-dependent)

Even though it's documented that comparing dates should always be the same kind, that seems like terrible behaviour on dotnet's part that's kept for backwards compatibility.

[brockallen]

So then presumably calling .ToUniversalTime() on each then comparing would be the acceptable fix?

[SSMKittel]

I would assume so

[brockallen]

Submitted a draft PR. Please have a look. Turns out we are doing DateTime comparisons in a few places, so I wanted to get all of them.

[brockallen]

While looking into this, it seems that ToUniversalTime is quite expensive if the DateTime is not UTC already. I'm tempted to simply state that we expect all DateTime to be UTC (which is of course how the code is all written). Can you explain why your store is returning non-UTC DateTime values?

[SSMKittel]

Sorry for the late reply.

I believe the datetime was in local time because that's how the database driver (npgsql) handled fields of type "timestamp with time zone" (despite the name, it's a date/time in UTC in postgres).

[brockallen]

Yea, since this thread I looked into related topics and IMO the DB layer should be returning DateTimes with the correct Kind set.