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

ACL Token ID Initialization #5307

Merged
merged 14 commits into from
Apr 30, 2019
Merged

ACL Token ID Initialization #5307

merged 14 commits into from
Apr 30, 2019

Conversation

mkeeler
Copy link
Member

@mkeeler mkeeler commented Feb 1, 2019

Fixes #4977

This PR allows for the ACL token accessor and secret ids, and the ACL policy id to be set to a user-supplied UUID during their first creation. These IDs are still immutable once set but this allows for external configuration management systems to generate the UUIDs and populate consul agent configurations without having to have yet spun up the consul servers to create the tokens.

@mkeeler mkeeler added this to the 1.4.3 milestone Feb 1, 2019
@mkeeler mkeeler requested a review from a team February 1, 2019 21:34
agent/acl_endpoint_test.go Outdated Show resolved Hide resolved
@mkeeler mkeeler force-pushed the feature/acl-id-initialization branch from ea7e2b8 to 1e9bc58 Compare February 1, 2019 21:59
@mkeeler mkeeler requested a review from a team February 1, 2019 22:04
agent/acl_endpoint_test.go Show resolved Hide resolved
agent/acl_endpoint_test.go Outdated Show resolved Hide resolved
api/acl.go Outdated Show resolved Hide resolved
website/source/api/acl/policies.html.md Outdated Show resolved Hide resolved
@rboyer
Copy link
Member

rboyer commented Feb 1, 2019

Is there anything about replication or snapshot restore that would be affected by this change as implemented?

@mkeeler mkeeler force-pushed the feature/acl-id-initialization branch from 1e9bc58 to d7cb2cc Compare February 3, 2019 20:20
@mkeeler
Copy link
Member Author

mkeeler commented Feb 3, 2019

This will not affect replication or snapshotting at all. The raft requests being applied do not have any new data. The only difference is whether the ids originated from the user or were generated in the RPC code.

@mkeeler mkeeler requested a review from a team February 4, 2019 14:55
@mkeeler
Copy link
Member Author

mkeeler commented Feb 11, 2019

This PR should probably wait until we figure out how to remove policy links for deleted policies.

Otherwise we could create a policy, link some tokens to it, delete the original policy, and then recreate with the same id but different rules. The original assumption was that policy IDs were guaranteed to be unique and thus it isn't a problem if we left the links around. Allowing pre-generated IDs breaks that assumption.

rboyer added a commit that referenced this pull request Feb 21, 2019
Lifted from PR #5307 as it was an unrelated drive-by fix on that PR anyway.

s/token/policy/
rboyer added a commit that referenced this pull request Feb 22, 2019
Lifted from PR #5307 as it was an unrelated drive-by fix on that PR anyway.

s/token/policy/
@mkeeler mkeeler modified the milestones: 1.4.3, 1.4.4 Feb 28, 2019
@hanshasselberg hanshasselberg modified the milestones: 1.4.4, 1.5 Mar 19, 2019
@mkeeler mkeeler force-pushed the feature/acl-id-initialization branch from d7cb2cc to e95394b Compare April 12, 2019 14:19
@mkeeler mkeeler modified the milestones: 1.5.0, Upcoming Apr 16, 2019
@pearkes
Copy link
Contributor

pearkes commented Apr 17, 2019

Just wanted to note that this will not be part of 1.5.0 per #5307 (comment). More work required unfortunately.

@mkeeler mkeeler force-pushed the feature/acl-id-initialization branch from 76c849f to 86b2b24 Compare April 25, 2019 18:03
@mkeeler mkeeler modified the milestones: Upcoming, 1.5.0 Apr 25, 2019
@mkeeler
Copy link
Member Author

mkeeler commented Apr 26, 2019

It wasn't going to make it in then I had an idea about how to make it okay for now simply by not allowing to set policy IDs.

Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

🎉 this is great.

I had a couple of comments about adding warnings about picking good tokens that you can take or leave.

Should we also consider updating the ACL guide/reference docs somewhere to make a note this is now possible and can simplify bootstrapping?

api/acl.go Outdated Show resolved Hide resolved
Co-Authored-By: mkeeler <mkeeler@users.noreply.github.com>
mkeeler added 3 commits April 26, 2019 18:49
…ialization

# Conflicts:
#	agent/consul/acl_endpoint.go
#	agent/consul/acl_endpoint_test.go
#	agent/structs/acl.go
#	command/acl/token/create/token_create.go
#	command/acl/token/update/token_update.go
@mkeeler mkeeler changed the title ACL Token/Policy ID Initialization ACL Token ID Initialization Apr 30, 2019
@mkeeler mkeeler merged commit 4daa158 into master Apr 30, 2019
@mkeeler mkeeler deleted the feature/acl-id-initialization branch April 30, 2019 15:45
@scalp42
Copy link
Contributor

scalp42 commented May 4, 2019

Can't wait for this PR as it'll allow us to encrypt the SecretID with KMS in advance in Chef, decrypt at runtime and make the call against Consul /acl/token endpoint.

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.

Allow setting of SecretID and AccessorID on ACL tokens on Consul 1.4+
6 participants