-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
ACL Token Persistence and Reloading #5328
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
6c08fed
Refactor to refer to a generic “replication” token
mkeeler 3214d76
Implement ACL token persistence and reloading.
mkeeler 42d4277
A few mods from PR feedback
mkeeler d4347d4
Update agent/config/runtime_test.go
rboyer aae3d71
Update docs
mkeeler 70b6dd2
Decode into structure instead of map[string]string
mkeeler fe0c7ce
Use persistedTokens structure in the agent/tokens endpoint
mkeeler 208d822
All API functions to fallback to old token names
mkeeler File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not hard-fail if the file is unreadable or corrupt? It seems like an unlikely event that should warrant extraordinary operational attention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it could be a hard fail. I modeled this off of other load* functions which did similarly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should a corrupt file prevent the agent from starting at all? Thats what would happen with a hard fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should corrupt persisted services prevent the agent from starting up?. I think the answer is no and we should instead just log the warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we go for worst case scenario, the agent isn't going to be terribly useful on restart if the sole store of tokens is that file, it's corrupt, and the operator is has a full belt-and-suspenders ACL setup configured.
Sure the agent will start, but it won't have any acl tokens with which to operate. If you had a fleet of hundreds of agents how can you tell the difference between an operational one and one that starts up but has no valid tokens?
But then I guess this is nearly indistinguishable from an agent having all of its acl tokens be rotated into the bit bucket already today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but with the agent up you can remedy the situation by PUTing new tokens to the API. Which is better than needing to, remote in, blow away/fix the corrupt file and restart (maybe still need to repost up tokens depending on the corruption).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well if you were being secure about it, the API would only be bound to loopback anyway, so remoting in is still required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree that the only secure way to enable the API is bound to loopback. If using HTTPs + ACLs I would allow it on the LAN.
Additionally, even if not able to directly connect to the API I would say that doing a
consul acl set-agent-token ...
is much easier/reliable then having to hand edit files (which is still always a viable alternative).