search

dazinator / AspNetCore.LegacyAuthCookieCompat

Provides classes to encrypt / decrypt asp.net 2 / 3.5 / 4 and 4.5 FormsAuthenticationTickets (cookies) without relying on system.web
MIT License
74 stars 18 forks source link

Fix #20: UTC vs Local DateTime #22

Closed lemoinem closed 5 years ago

lemoinem commented 5 years ago

Here is the fix for #20 I tweaked the test a little bit so we don't run into this kind of issue in the future.

This is really a major issue for us. Once merged, would it be possible to push a new version to NuGet ASAP?

dazinator commented 5 years ago

Thanks for the PR. Yep I'll get this pushed tomorrow morning.

dazinator commented 5 years ago

Just so I understand this correctly, a quick question:

Does this mean in scenario where there is legacy site A, and a .net core site B:

  1. Server A sets expiry date and issue date using its local time zone. When it receives a request with it's own cookie, the data is parsed using its local time zone (DateTimeKind.Local) and comparison behaves as expected (comparison converts from local to utc, I guess just in case the clocks just went back in local time by an hour or something)

  2. Server A redirects to Server B. Suppose server B has different local time zone to server A. Server B parses expiry and issue date as "Local" and then converts to UTC for comparison. If the time zone of server B is different from server A, the issue date may be in the future, and IsValid check will fail?

Therefore for SSO to work accross both sites, they must both use same local time zone?

lemoinem commented 5 years ago

The datetime is save as UTC Ticks in the Cookies, but are stored as local DateTime in the FormsAuthenticationTicket. The conversion is done at deserialization time (this is the step that was missing).

Even if both servers have different timezones, this wouldn't be an issue, as the time is still serialized as UTC Ticks in the Cookie. Any timezone conversion is local to each server.

Why it was implemented in this way instead of using UTC all the time is, quite frankly, beyond me. But that's the way legacy ASP.Net works...

With the code before this PR:

Whether site A or B creates the cookie doesn't matter, as it's serialized properly. For this reason as well, whatever A and B's timezones, A (legacy ASP.Net) will be able to deserialize and check for the issue and expiration date properly.

However, when B deserializes the cookie, the FormsAuthenticationTicket's fields remained with the UTC DateTime.

For example, we are in EDT (UTC-4) and use 1h lifetime for tickets. So in our case, for example, when we receive a cookie back from the client: Instead of having a ticket that is Valid (Issue date is 10minutes ago and Expiration 50minutes in the future), we have a ticket from the future (Issue date is 3h50 in the future and expiration is 4h50 in the future as well).

This occurs even when the ticket was created by AspNetCore.LegacyAuthCookieCompat because only the deserialization is problematic.

There were two solution possible:

  1. Either we always use UTC DateTimes in AspNetCore.LegacyAuthCookieCompat's FormsAuthenticationTicket and change the Serialize and Deserialize method appropriately, but that sounds like a major breaking change and is not in line with legacy ASP.Net code
  2. Or we always use local DateTimes in AspNetCore.LegacyAuthCookieCompat's FormsAuthenticationTicket and fix the FromUtc() method and Expired property. This is a breaking change as well, but somewhat smaller one and makes it compatible with code using the legacy ASP.Net FormsAuthenticationTicket.

Since the goal of the package is to provide compatibility with legacy ASP.Net, I went with option 2.

dazinator commented 5 years ago

Thanks for the explanation.

I've merged, after verifying the tests all run locally for me. I've tagged master so the next release will be v2.0.0 to take into account this could be a breaking change in behaviour I thought the major version bump was warranted.

One thing though - even though the tests all pass locally for me, for some reason the test fails on the appveyor server: https://ci.appveyor.com/project/dazinator/aspnetcore-legacyauthcookiecompat

I can only think it must be something to do with local time and regional settings? If you have a moment would you be able to look into the failing test - as that's the only thing that is stopping the build from successfully completing and a new nuget package being pushed out!

lemoinem commented 5 years ago

@dazinator Thanks!

I had a look at the AppVeyor failure. I've provided a new PR using a non-strict comparison in IsValid. I hope this solves the issue. There are more details in the new PR.