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

Unify token updating code #3442

Merged
merged 4 commits into from
Apr 15, 2024
Merged

Unify token updating code #3442

merged 4 commits into from
Apr 15, 2024

Conversation

btoews
Copy link
Member

@btoews btoews commented Apr 10, 2024

There's a lot of stuff we need to do to make macaroon tokens work well in flyctl:

  • Prune expired/invalid tokens
  • Fetch and refresh the user's discharge tokens
  • Fetch new macaroons when the user is added to an org (we weren't doing this)
  • Prune macaroons when the user is removed from an org (we were just letting them expire after 10 minutes)

This was complicated further by wanting to keep tokens in sync between the flyctl agent and any foreground processes. Long running commands were also winding up with expired tokens because we were only doing a lot of this work when the command started.

This PR centralizes all of this logic and runs it both in foreground flyctl processes as well as in the background agent. When flyctl makes changes to its set of tokens, it writes those back to the config file (so long as flyctl wasn't started with FLY_AUTH_TOKEN). This should allow everything to stay nicely up to date and synced between processes.

There was also some annoyance (/cc @jipperinbham) when running commands with FLY_AUTH_TOKEN. The background agent and foreground commands would often end up running with a different set of tokens and one or the other would run into authorization errors. I'm adding a new set-token RPC to the fly agent in this PR. Clients send this command before sending other commands, letting the agent know what tokens to use for the session.

This PR depends on superfly/fly-go#34

TODO:

  • Make sure send-token command fails nicely when sent to old agent Checked old agent + new flyctl and vice versa.
  • Store agent tunnels by "raw slug" instead of "slug" to avoid confusion between multiple personal orgs when using different tokens.
  • Decide if we need to relax the authz checks on the ValidateWireGuardPeers mutation in web. That might make the agent work better when using FLY_AUTH_TOKEN.

PS: There's been a lot of churn in the tokens code in flyctl. Sorry about that. I'd been trying to avoid writing this big PR, but half measure weren't cutting it.

Copy link
Contributor

@timflyio timflyio left a comment

Choose a reason for hiding this comment

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

I looked over the code, had some questions and have some minor things to point out. I'm still a little short on the big picture and context around this.

agent/server/session.go Outdated Show resolved Hide resolved
agent/server/session.go Show resolved Hide resolved
agent/server/session.go Outdated Show resolved Hide resolved
internal/config/tokens.go Outdated Show resolved Hide resolved
internal/config/tokens.go Outdated Show resolved Hide resolved
@btoews btoews force-pushed the agent-tokens branch 2 times, most recently from 9026005 to ca472d9 Compare April 15, 2024 17:23
@btoews btoews merged commit 22fb63e into master Apr 15, 2024
39 checks passed
@btoews btoews deleted the agent-tokens branch April 15, 2024 17:41
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