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

CLI should not handle OIDC token refresh #349

Open
jakedoublev opened this issue Aug 29, 2024 · 0 comments
Open

CLI should not handle OIDC token refresh #349

jakedoublev opened this issue Aug 29, 2024 · 0 comments

Comments

@jakedoublev
Copy link
Contributor

jakedoublev commented Aug 29, 2024

Background

Support was recently added for user authentication to the otdfctl CLI utilizing the auth code PKCE flow:

  1. CLI spawns a server at http://localhost:9000 that listens with a callback handler
  2. CLI opens a browser tab on the native OS to the identity provider with appropriate OIDC url params for the PKCE code auth flow
  3. CLI receives the tokens back and stores them in the keyring

When the user is found to have an expired access token, the CLI returns a warning asking the user to reauthenticate. An expiration of a token is the perfect use case of a refresh_token and an OIDC refresh request.

However, without the refresh token and refresh flow, the user runs otdfctl auth login once more and re-authenticates with the idP. If they are in the same browser, many idPs will have stored cookies that will result in almost instant authentication with a new access token provided to the CLI for storage and use within the profile. This is outside our control and is in the scope of idP configuration.

At this time, we are making a decision not to handle refreshes within the CLI.

🟩 : Good, because we require a valid redirectUri allowlisted within an idP of http://localhost:9000 which has some risk, but this can be somewhat mitigated by not storing a refresh token on the OS for potential sniffing
🟩 : Good, because this minimizes idP public client requirements, which is good as it will likely be shared among PEPs
🟩 : Good, because it limits the lifetime a machine can be authenticated to the lifetime of the AT (rather than the lifetime of the AT + RT), which reduces the blast radius for the "one machine, multiple users" scenario
🟨 / 🟥 : Neutral/Bad, because the user must invoke another command auth login to re-authenticate, but it is minimal effort if the idP employs cookies and is more secure to require fully re-authenticating over refreshing

Acceptance Criteria

  1. Ensure refresh token provided back from authentication flow is not stored within the profile
  2. Ensure login/revoke still work as desired
  3. Update any e2e tests covering refresh token logic
  4. The decisioning above is documented in a .adr/<name>.md ADR in the PR that closes this issue
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

No branches or pull requests

1 participant