-
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 6 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,13 @@ | ||
package agent | ||
|
||
import ( | ||
"encoding/json" | ||
"errors" | ||
"fmt" | ||
"log" | ||
"net" | ||
"net/http" | ||
"path/filepath" | ||
"strconv" | ||
"strings" | ||
"time" | ||
|
@@ -22,9 +24,11 @@ import ( | |
"github.com/hashicorp/consul/agent/debug" | ||
"github.com/hashicorp/consul/agent/local" | ||
"github.com/hashicorp/consul/agent/structs" | ||
token_store "github.com/hashicorp/consul/agent/token" | ||
"github.com/hashicorp/consul/api" | ||
"github.com/hashicorp/consul/ipaddr" | ||
"github.com/hashicorp/consul/lib" | ||
"github.com/hashicorp/consul/lib/file" | ||
"github.com/hashicorp/consul/logger" | ||
"github.com/hashicorp/consul/types" | ||
"github.com/hashicorp/logutils" | ||
|
@@ -1262,30 +1266,70 @@ func (s *HTTPServer) AgentToken(resp http.ResponseWriter, req *http.Request) (in | |
return nil, nil | ||
} | ||
|
||
if s.agent.config.ACLEnableTokenPersistence { | ||
// we hold the lock around updating the internal token store | ||
// as well as persisting the tokens because we don't want to write | ||
// into the store to have something else wipe it out before we can | ||
// persist everything (like an agent config reload). The token store | ||
// lock is only held for those operations so other go routines that | ||
// just need to read some token out of the store will not be impacted | ||
// any more than they would be without token persistence. | ||
s.agent.persistedTokensLock.Lock() | ||
defer s.agent.persistedTokensLock.Unlock() | ||
} | ||
|
||
// Figure out the target token. | ||
target := strings.TrimPrefix(req.URL.Path, "/v1/agent/token/") | ||
switch target { | ||
case "acl_token": | ||
s.agent.tokens.UpdateUserToken(args.Token) | ||
|
||
case "acl_agent_token": | ||
s.agent.tokens.UpdateAgentToken(args.Token) | ||
case "acl_token", "default": | ||
s.agent.tokens.UpdateUserToken(args.Token, token_store.TokenSourceAPI) | ||
|
||
case "acl_agent_master_token": | ||
s.agent.tokens.UpdateAgentMasterToken(args.Token) | ||
case "acl_agent_token", "agent": | ||
s.agent.tokens.UpdateAgentToken(args.Token, token_store.TokenSourceAPI) | ||
|
||
case "acl_replication_token": | ||
s.agent.tokens.UpdateACLReplicationToken(args.Token) | ||
case "acl_agent_master_token", "agent_master": | ||
s.agent.tokens.UpdateAgentMasterToken(args.Token, token_store.TokenSourceAPI) | ||
|
||
case "connect_replication_token": | ||
s.agent.tokens.UpdateConnectReplicationToken(args.Token) | ||
case "acl_replication_token", "replication": | ||
s.agent.tokens.UpdateReplicationToken(args.Token, token_store.TokenSourceAPI) | ||
|
||
default: | ||
resp.WriteHeader(http.StatusNotFound) | ||
fmt.Fprintf(resp, "Token %q is unknown", target) | ||
return nil, nil | ||
} | ||
|
||
if s.agent.config.ACLEnableTokenPersistence { | ||
tokens := make(map[string]string) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The persistedTokens struct could be used here as well |
||
|
||
if tok, source := s.agent.tokens.UserTokenAndSource(); tok != "" && source == token_store.TokenSourceAPI { | ||
tokens["default"] = tok | ||
} | ||
|
||
if tok, source := s.agent.tokens.AgentTokenAndSource(); tok != "" && source == token_store.TokenSourceAPI { | ||
tokens["agent"] = tok | ||
} | ||
|
||
if tok, source := s.agent.tokens.AgentMasterTokenAndSource(); tok != "" && source == token_store.TokenSourceAPI { | ||
tokens["agent_master"] = tok | ||
} | ||
|
||
if tok, source := s.agent.tokens.ReplicationTokenAndSource(); tok != "" && source == token_store.TokenSourceAPI { | ||
tokens["replication"] = tok | ||
} | ||
|
||
data, err := json.Marshal(tokens) | ||
if err != nil { | ||
s.agent.logger.Printf("[WARN] agent: failed to persist tokens - %v", err) | ||
return nil, fmt.Errorf("Failed to marshal tokens for persistence: %v", err) | ||
} | ||
|
||
if err := file.WriteAtomicWithPerms(filepath.Join(s.agent.config.DataDir, tokensPath), data, 0600); err != nil { | ||
s.agent.logger.Printf("[WARN] agent: failed to persist tokens - %v", err) | ||
mkeeler marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return nil, fmt.Errorf("Failed to persist tokens - %v", err) | ||
} | ||
} | ||
|
||
s.agent.logger.Printf("[INFO] agent: Updated agent's ACL token %q", target) | ||
return nil, nil | ||
} | ||
|
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).