-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Allow Operator Generated bootstrap token #14437
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
```release-note:improvement | ||
acl: Added option to allow for an operator-generated bootstrap token to be passed to the `acl bootstrap` command. | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -162,7 +162,7 @@ func (a *ACL) aclPreCheck() error { | |
|
||
// BootstrapTokens is used to perform a one-time ACL bootstrap operation on | ||
// a cluster to get the first management token. | ||
func (a *ACL) BootstrapTokens(args *structs.DCSpecificRequest, reply *structs.ACLToken) error { | ||
func (a *ACL) BootstrapTokens(args *structs.ACLInitialTokenBootstrapRequest, reply *structs.ACLToken) error { | ||
if err := a.aclPreCheck(); err != nil { | ||
return err | ||
} | ||
|
@@ -207,9 +207,24 @@ func (a *ACL) BootstrapTokens(args *structs.DCSpecificRequest, reply *structs.AC | |
if err != nil { | ||
return err | ||
} | ||
secret, err := lib.GenerateUUID(a.srv.checkTokenUUID) | ||
if err != nil { | ||
return err | ||
secret := args.BootstrapSecret | ||
if secret == "" { | ||
secret, err = lib.GenerateUUID(a.srv.checkTokenUUID) | ||
if err != nil { | ||
return err | ||
} | ||
} else { | ||
_, err = uuid.ParseUUID(secret) | ||
if err != nil { | ||
return err | ||
} | ||
ok, err := a.srv.checkTokenUUID(secret) | ||
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. This deserves a comment. On FIRST bootstrap this doesn't matter because there are no tokens. On rebootstrap however there may already be tokens and you don't want to collide with them. If we didn't allow re-bootstrap this function call would be unnecessary. 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.
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. It's fine to leave it as is because it is important to check against the existing database during the reset procedure: https://developer.hashicorp.com/consul/tutorials/security/access-control-troubleshoot#reset-the-acl-system |
||
if err != nil { | ||
return err | ||
} | ||
if !ok { | ||
return fmt.Errorf("Provided token cannot be used because a token with that secret already exists.") | ||
} | ||
} | ||
|
||
req := structs.ACLTokenBootstrapRequest{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1351,10 +1351,20 @@ type ACLTokenBatchDeleteRequest struct { | |
TokenIDs []string // Tokens to delete | ||
} | ||
|
||
type ACLInitialTokenBootstrapRequest struct { | ||
BootstrapSecret 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. How do you feel about calling it (whatever happens here it should also be mirrored to the matching 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. I don't have strong feelings; I mirrored nomad: https://github.com/hashicorp/nomad/blob/fbe9f590489e291b3abcfb0fb1548a9f435a5e2d/nomad/structs/structs.go#L12169 -- I'd be fine with changing it if you think another name is better. |
||
Datacenter string | ||
QueryOptions | ||
} | ||
|
||
func (r *ACLInitialTokenBootstrapRequest) RequestDatacenter() string { | ||
return r.Datacenter | ||
} | ||
|
||
// ACLTokenBootstrapRequest is used only at the Raft layer | ||
// for ACL bootstrapping | ||
// | ||
// The RPC layer will use a generic DCSpecificRequest to indicate | ||
// The RPC layer will use ACLInitialTokenBootstrapRequest to indicate | ||
// that bootstrapping must be performed but the actual token | ||
// and the resetIndex will be generated by that RPC endpoint | ||
type ACLTokenBootstrapRequest struct { | ||
|
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.
For the sake of anyone else reading this PR, note that here we are switching the request type completely from one struct to another, and that is being encoded using
msgpack
and sent over RPC.The new struct type
ACLInitialTokenBootstrapRequest
only contains a subset of the fields that exist onDCSpecificRequest
so a surface read would say that this is backwards incompatible by msgpack encoding rules and would be guaranteed to lose data if you were using a different version of consul on both sides of this RPC (as you would during windows of time during an upgrade).A deeper read shows that actually this generic request struct is used in many places, but is largely unused within the acl bootstrap codepath and all of the relevant fields were already copied to your new type with the same name+types, so it would be effectively backwards compatible.
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.
Is this a line of code that a future dev might look at and go "wait, this looks like it might be wrong..."? If so, we might want an in-source comment. I defer to @rboyer
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 think in-source might not be correct because it only matters for version-straddling. If it would go anywhere I'd say it would make sense in the body of the git commit so it's historically annotating the change as "yep, this was fine from a msgpack perspective at the time we did it".