ueberauth / guardian

Elixir Authentication
MIT License
3.4k stars 381 forks source link

system_time vs os_time and time drift #721

Closed pejrich closed 10 months ago

pejrich commented 1 year ago

Steps to Reproduce

This one might be a little hard to reproduce exactly as some of the issues might be OS/machine dependent. But the steps seem to be:

  1. Start a server, leave it running for an extended period of time(typically >12 hours)
  2. Try to generate a token(I was doing is via exchange, but from what I can tell the issue is with the Guardian.timestamp function)
  3. Token is generated, but already expired

I've written a bit more about it in this ElixirForum post: https://elixirforum.com/t/guardian-generating-an-expired-token-because-system-system-time-datetime-is-this-time-warp/56110/13

My system has some major drift between System.system_time and System.os_time, by about 97 minutes. I realize that this may not be a everyday common occurance for most people, and I'm not 100% sure of why it's happening locally, but it does seem that System.os_time will always return a more reliable timestamp, as will DateTime.utc_now() which seems to use os_time internally. I'm wondering if there's a reason for the use of System.system_time rather than os_time in this library, as system_time seems to at least sometimes have consistency issues that the other options don't seem to have.

Expected Result

A non expired JWT token

Actual Result

An expired JWT token.

iex(138)> {:ok, %{access_token: token}} = MyApp.Accounts.Guardian.sign_in(user)
iex(137)> MyApp.Accounts.Guardian.peek(token) |> Map.get(:claims) |> Map.get("exp") |> Kernel.<(DateTime.utc_now() |> DateTime.to_unix)
true

The code underlying the sign_in function:

def sign_in(%User{} = user) do
    with {:ok, refresh, _} <- refresh_token(user),
         {:ok, access} <- exchange_refresh(refresh) do
      {:ok, %{access_token: access, refresh_token: refresh, user: user}}
    end
  end

def refresh_token(resource) do
  encode_and_sign(resource, %{}, token_type: "refresh", ttl: MyApp.Config.token_refresh_ttl())
end

def exchange_refresh(refresh_token) do
  case exchange(refresh_token, "refresh", "access", ttl: MyApp.Config.token_access_ttl()) do
    {:ok, {^refresh_token, _}, {access_token, _}} -> {:ok, access_token}
    {:error, _err} = err -> err
  end
end

Call to system_time showing the time drift:

iex(138)> DateTime.to_unix(DateTime.utc_now()) - System.system_time(:second)
5782

~96 minutes, 1.6 hours. My access TTL is 60 minutes, hence the expiration immediately on creation.

It's harder to show here via code, so you'll have to trust me, but the DateTime call is in line with the true time, and the system_time call is behind the true time, rather than DateTime being ahead, and that's confirmed with my laptop's displayed time, my cell phones displayed time, and https://www.unixtimestamp.com/ time.

Happy to submit a PR for this, but wanted to first check if there was conscious decision to use system_time over os_time, or a reason not to use os_time/DateTime.

yordis commented 10 months ago

closed by #725