Skip to content
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.

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

feat: follow new recommendation for idle/authentication timeouts #967

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

paulswartz
Copy link
Member

Summary of changes

  • Idle timeout: 30 minutes
  • Maximum session length: 12 hours

This aligns Arrow with the NIST SP 800-63 guidelines.

Reviewer Checklist

  • Meets ticket's acceptance criteria
  • Any new or changed functions have typespecs
  • Tests were added for any new functionality (don't just rely on Codecov)
  • This branch was deployed to the staging environment and is currently running with no unexpected increase in warnings, and no errors or crashes.

Copy link

Coverage of commit 6500abe

Summary coverage rate:
  lines......: 88.0% (683 of 776 lines)
  functions..: 59.5% (521 of 875 functions)
  branches...: no data found

Files changed coverage rate:
                                                                     |Lines       |Functions  |Branches    
  Filename                                                           |Rate     Num|Rate    Num|Rate     Num
  =========================================================================================================
  lib/arrow/ueberauth/strategy/fake.ex                               | 100%      7| 100%     7|    -      0
  lib/arrow_web/auth_manager.ex                                      |40.0%     10|18.5%    65|    -      0
  lib/arrow_web/auth_manager/error_handler.ex                        |83.3%      6| 100%     2|    -      0
  lib/arrow_web/auth_manager/pipeline.ex                             |83.3%      6|77.8%     9|    -      0
  lib/arrow_web/controllers/auth_controller.ex                       |75.0%     28|85.7%     7|    -      0

Download coverage report

@@ -14,4 +22,19 @@ defmodule ArrowWeb.AuthManager do
end

def resource_from_claims(_), do: {:error, :invalid_claims}

@impl true
def verify_claims(%{"iat" => iat, "auth_time" => auth_time} = claims, _opts) do
Copy link
Member

Choose a reason for hiding this comment

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

Where do the iat and auth_time claims come from? I couldn't find any reference to them in ueberauth_oidcc

Copy link
Member Author

@paulswartz paulswartz Apr 19, 2024

Choose a reason for hiding this comment

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

auth_time is coming from EntraID (via AuthController), and iat is from Guardian (all tokens have that claim).

roles: roles
},
ttl: {expiration - current_time, :seconds}
ttl: {1, :minute}
Copy link
Member

Choose a reason for hiding this comment

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

A TTL of one minute? Could you set this token to expire when the iat expires or the auth_time expires, whichever comes first? Or would that prevent revoking access to accounts that are deactivated in EntraID?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, is this so that you reset the 15 minute idle timeout (by having to refresh the token and get a new iat claim) on the next request after the 1m is up?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly; the 1 minute makes sure this token is valid long enough for the next request to be made, at which point the token will be refreshed. It would also work to use the idle timeout here, but then this module also needs to reference the idle timeout value so it seemed cleaner to avoid that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants