Skip to content

[CC-5719] Add support for builtin global-read-only policy#18319

Merged
jjacobson93 merged 13 commits intomainfrom
jer/read-only-policy
Aug 1, 2023
Merged

[CC-5719] Add support for builtin global-read-only policy#18319
jjacobson93 merged 13 commits intomainfrom
jer/read-only-policy

Conversation

@jjacobson93
Copy link
Copy Markdown
Contributor

@jjacobson93 jjacobson93 commented Jul 28, 2023 •
edited
Loading

Description

This adds a new builtin policy that provides global read-only access, in contrast to the global read-write access that the builtin global-management policy provides. Other changes were made to process builtin policies more generically, since there are several places where checks or validations are done before processing or altering a policy.

Links

Ticket
RFC

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

@jjacobson93 jjacobson93 added backport/1.14 backport/1.15 This release series is no longer active on CE. Use backport/ent/1.15. backport/1.16 This release series is no longer active on CE. Use backport/ent/1.16. labels Jul 28, 2023
@jjacobson93 jjacobson93 requested review from lornasong and pglass July 28, 2023 16:24
@github-actions github-actions bot added the theme/acls ACL and token generation label Jul 28, 2023
@jjacobson93 jjacobson93 requested a review from a team as a code owner July 28, 2023 17:40
@jjacobson93 jjacobson93 added the type/docs Documentation needs to be created/updated/clarified label Jul 28, 2023
lornasong
lornasong reviewed Jul 28, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@lornasong lornasong left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This makes sense to me as far as I understand policy implementation in Consul! I like how clean the changes are to handle global-management and global-read-only policy ✨

Left mostly nits and a couple suggestions. We also chatted about adding consul docs for the builtin policy. Will take a look again then!
EDIT: I missed the new commit to add the docs! Taking a look now 😄

trujillo-adam
trujillo-adam approved these changes Jul 28, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@trujillo-adam trujillo-adam left a comment

Choose a reason for hiding this comment

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

Looks good from the docs perspective; just a minor nit

website/content/docs/security/acl/acl-policies.mdx Outdated

### Global Read-Only

The `builtin/global-read-only` policy grants unrestricted _read-only_ privileges to any token linked to it. The policy is assigned the reserved ID of `00000000-0000-0000-0000-000000000002`. You can rename the global read-only policy, but Consul will prevent you from modifying any other attributes, including the rule set and datacenter scope.
Copy link
Copy Markdown
Contributor

@trujillo-adam trujillo-adam Jul 28, 2023

Choose a reason for hiding this comment

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

Suggested change
The `builtin/global-read-only` policy grants unrestricted _read-only_ privileges to any token linked to it. The policy is assigned the reserved ID of `00000000-0000-0000-0000-000000000002`. You can rename the global read-only policy, but Consul will prevent you from modifying any other attributes, including the rule set and datacenter scope.
The `builtin/global-read-only` policy grants unrestricted _read-only_ privileges to any token linked to it. The policy is assigned the reserved ID of `00000000-0000-0000-0000-000000000002`. You can rename the global read-only policy, but Consul prevents you from modifying any other attributes, including the rule set and datacenter scope.

Per the style guide, we should use present tense.

Copy link
Copy Markdown
Contributor Author

@jjacobson93 jjacobson93 Jul 28, 2023

Choose a reason for hiding this comment

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

Good catch, I didn't know that. I actually copy-pasted from the above description 😬 Should I change that one as well?

@jjacobson93
Copy link
Copy Markdown
Contributor Author

jjacobson93 commented Jul 31, 2023

@pglass just bumping this up, if you get a chance to review, please. Thanks!

lornasong
lornasong approved these changes Jul 31, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@lornasong lornasong left a comment •
edited
Loading

Choose a reason for hiding this comment

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

looks good to me! left 2 non-blocking questions (edit - one left in an earlier thread), thank you!

@pglass pglass requested a review from cthain July 31, 2023 20:17
pglass
pglass reviewed Jul 31, 2023
View reviewed changes
Copy link
Copy Markdown

@pglass pglass left a comment

Choose a reason for hiding this comment

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

👍 I added another randomly-selected ZTS team member for review (@cthain fyi)

lornasong
lornasong reviewed Jul 31, 2023
View reviewed changes
cthain
cthain reviewed Jul 31, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@cthain cthain left a comment

Choose a reason for hiding this comment

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

Looking good. I had some comments/questions around ID allocation and naming.

@jjacobson93 @cthain
Update acl/validation.go
d63fa54 
Co-authored-by: Chris Thain <[email protected]>