-
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
Allow Operator Generated bootstrap token #14437
Conversation
26de4d3
to
6925845
Compare
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 reviewed 14437.txt and bootstrap.mdx on behalf of the Docs team.
Great work - I made suggestions for style and additional clarity. Please let me know if you have any questions or need any additional assistance.
Approving on behalf of consul-docs.
4217013
to
022f47f
Compare
Hello @apollo13, Thank you for submitting this PR for our consideration! To help with that consideration, can you provide more context on:
|
Sure, thank you for answering.
I want to be able to programmatically bootstrap a Consul cluster. If nothing else, the hashicorp products should preferably behave the same way and this makes it nicely match nomad.
I am aware of the initial management token but I rather not write (even if it is just an initial token) secrets like that onto the harddisk of a server if not needed. I understand that the raft/storage-layer will write the token anyways, but that storage layer is ecxpected to hold sensitive data (connect key material among others and in the worst case kv data is sensitive as well). To make matters worse I am trying to precreate usable containers/vm images. The less key material I have to roll out the better. Initial server bootstrap works nicely via mdns cloud-join (got that working for nomad & consul). I understand that I will probably have to put other tokens into the config as well, but the fallout of those other tokens would be far less than the root key… I hope this helps; I realize I am probably not making the best arguments for this feature -- it is not an absolute must-have (I understand that), but nevertheless it would make things easier for a few people out here. |
Thank you for the context! I'll share this with the team as a next step before engineering review. |
This pull request has been automatically flagged for inactivity because it has not been acted upon in the last 60 days. It will be closed if no new activity occurs in the next 30 days. Please feel free to re-open to resurrect the change if you feel this has happened by mistake. Thank you for your contributions. |
//Unstable
…On Tue, Nov 1, 2022, at 02:37, github-actions[bot] wrote:
This pull request has been automatically flagged for inactivity because
it has not been acted upon in the last 60 days. It will be closed if no
new activity occurs in the next 30 days. Please feel free to re-open to
resurrect the change if you feel this has happened by mistake. Thank
you for your contributions.
—
Reply to this email directly, view it on GitHub
<#14437 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAT5C5ODTPHX2XSUO63VCDWGBX35ANCNFSM6AAAAAAQCHZ7ZM>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
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.
Great job on this! I have a variety of small comments and suggestions I'm adding below but none of them should be surprising or tricky to clean up before merging.
One of the longer comments is not actionable and is pointing out a msgpack
non-issue to a future PR archaeologist.
if err != nil { | ||
return err | ||
} | ||
ok, err := a.srv.checkTokenUUID(secret) |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
checkTokenUUID
also checks that the token does not start with ACLReservedPrefix
, I'd like to at least keep that check. Should I switch to using ACLIDReserved(secret)
?
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.
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
@@ -34,9 +34,20 @@ func (s *HTTPHandlers) ACLBootstrap(resp http.ResponseWriter, req *http.Request) | |||
return nil, aclDisabled | |||
} | |||
|
|||
args := structs.DCSpecificRequest{ | |||
args := structs.ACLInitialTokenBootstrapRequest{ |
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 on DCSpecificRequest
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".
@@ -1350,10 +1350,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 comment
The reason will be displayed to describe this comment to others. Learn more.
How do you feel about calling it InitialToken
or InitialManagementToken
instead? The config file approach for bootstrap refers to the same concept as initial_management so this would align them.
(whatever happens here it should also be mirrored to the matching api
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.
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.
Thank you for the reviews. I will need a few days to get this updated. |
3366b91
to
00bfc9d
Compare
@kisunji Any chance of another review? |
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.
LGTM; I think @rboyer's remaining comments were non-blocking but will ping for final review
00bfc9d
to
9ec20e5
Compare
Add support to provide an initial token via the bootstrap HTTP API, similar to hashicorp/nomad#12520
Description
Added support to provide an initial token via the bootstrap HTTP API, similar to hashicorp/nomad#12520
PR Checklist