RBAC/rpk: Introduce `--{allow,deny}-role` flags for ACL create/describe/delete #17416

+ROLES
+
+Alternatively, an ACL may be bound to a role. To create a role-bound ACL,
+follow the same procedure used to create a user ACL, but use the --allow-role


[question] IIUC, --allow-principal RedpandaRole:role" is no longer the preferred way? But it's still an option but not documented?

Yeah, that's the intent. Under the hood, these get parsed out in the same way, so I don't know whether there's any great advantage to restricting --allow-principal at the interface. Besides which it might be tricky.

If you have a strong opinion about this, I'm certainly open to suggestions.

Sounds good to me. As long as we document this as an alternative (it might be confusing) I'm fine 👍

as long as we document this as an alternative

Ah, so you're saying you prefer the old usage to be documented? I was thinking omission might be less confusing, but I could go either way.

Oh I'm so sorry! I meant as long as we _dont_ document this as an alternative

No worries! that's what I thought but didn't want to presume 🙂

oleiman · 2024-03-27T19:51:13Z

@r-vasquez - should I get eyes on this from the docs team? not sure exactly what the process is there.

r-vasquez · 2024-03-27T19:54:12Z

@oleiman Yes, I just added documentation group as a reviewer. Thanks!

Deflaimun · 2024-03-28T12:52:32Z

src/go/rpk/pkg/cli/acl/acl.go

+ROLES
+
+Alternatively, an ACL may be bound to a role. To create a role-bound ACL,
+follow the same procedure used to create a user ACL, but use the --allow-role


does this applies only to rpk acl user create ?

Why do we explicitly have the following? Can't we just explain the process here? (even if it contains duplication)

follow the same procedure used to create a user ACL

Can we have one example showing how to create the role?

does this applies only to rpk acl user create ?
No this is rpk acl create

even if it contains duplication

fine by me! my only concern would be keeping the two in sync, but it's unlikely to change. anything is better than trying to thread the role concept back through existing text IMHO.

src/go/rpk/pkg/cli/acl/acl.go

Deflaimun · 2024-03-28T12:57:50Z

src/go/rpk/pkg/cli/acl/acl.go

+You should be aware of a few key differences when creating a role ACL:
+
+  * Wildcard matching role names is not allowed. So --allow-role '*' is illegal.
+  * The type prefix is not necessary. The flag fully specifies that the


what happens if the user passes a type? is it ignored?

effectively, yes, unless it matches a specific prefix:

If the user passes, e.g., "User:my_role" we bind the resulting ACL to "RedpandaRole:User:my_role". This is analogous to the behavior of --allow-principal and friends.

However, if the user passes "RedpandaRole:my_role" we bind the ACL to "RedpandaRole:my_role". This is also analogous to --allow-principal but the prefix itself is deliberately left undocumented, since a role is always a role.

Deflaimun · 2024-03-28T12:58:57Z

src/go/rpk/pkg/cli/acl/acl.go

+follow the same procedure used to create a user ACL, but use the --allow-role
+and --deny-role flags to specify the role(s) to which the ACL should apply.
+
+You should be aware of a few key differences when creating a role ACL:


Suggested change

You should be aware of a few key differences when creating a role ACL:

The procedures and semantics for creating and managing ACLs

apply equally to role ACLs and user ACLs, except on the following:

if you use this suggestion, delete L98-100

reworked to make the section stand alone a bit better. lmk what you think

Deflaimun · 2024-03-28T13:01:43Z

Hi @oleiman. Left some questions and suggestions there. Feel free to reach us on slack if you have any questions

michael-redpanda

lgtm just one question

michael-redpanda · 2024-03-28T15:50:19Z

src/go/rpk/pkg/cli/acl/common.go

@@ -87,6 +89,12 @@ func unptr(str *string) string {
 	return *str
 }

+func PrefixRole(roles []string) {
+	for i, u := range roles {
+		roles[i] = rolePrefix + u


question: Do you care to, or have you already, check that the rolePrefix has already been applied before applying?

Yeah, I waffled on this a bit. Since it's undocumented by design, it feels sort of arbitrary, but I think I'll just add the check. No reason to leave a footgun there.

oleiman · 2024-03-28T20:52:27Z

force push:

docstring improvements
added prefix check for role names
slightly improved tests

Deflaimun · 2024-03-28T21:09:10Z

src/go/rpk/pkg/cli/acl/acl.go

-and the name. Within Redpanda, only one type is supported, "User". The reason
-for the prefix is that a potential future authorizer may add support for
-authorizing by Group or anything else.
+All ACLs require a principal (or a role; see ROLES, below). A principal is


Suggested change

All ACLs require a principal (or a role; see ROLES, below). A principal is

All ACLs require a principal or a role. A principal is

iMO no need to refer to the other section as is the next one.

Feediver1 · 2024-04-02T22:22:53Z

src/go/rpk/pkg/cli/acl/acl.go

-for the prefix is that a potential future authorizer may add support for
-authorizing by Group or anything else.
+All ACLs require a principal or a role. A principal comprises a user and type.
+Within Redpanda, only the "User" type is supported. The "User" prefix ensures


Why is "User" init cap here? It isn't above. Is the syntax for specifying a user case-sensitive? If not, no need for initcap here/throughout.

@oleiman In the UI/CLI, are types always initcap?

In the command rpk acl create --allow-principal "User:foo" ... the "User" prefix is indeed case sensitive (initcap). When we talk about users in the abstract, it's not usually capitalized as far as I can tell.

Feediver1 · 2024-04-02T22:24:14Z

src/go/rpk/pkg/cli/acl/acl.go

-and the name. Within Redpanda, only one type is supported, "User". The reason
-for the prefix is that a potential future authorizer may add support for
-authorizing by Group or anything else.
+All ACLs require a principal or a role. A principal comprises a user and type.


Suggested change

All ACLs require a principal or a role. A principal comprises a user and type.

All ACLs require a principal or a role. A principal is comprised of a user and type.

This suggestion seems clearer than the original.

Missed this. Fair point, just slightly non-standard usage for whatever that's worth. Would "composed of" have roughly the same effect?

Feediver1 · 2024-04-02T22:28:46Z

src/go/rpk/pkg/cli/acl/acl.go

+All ACLs require a principal or a role. A principal comprises a user and type.
+Within Redpanda, only the "User" type is supported. The "User" prefix ensures
+that potential future authorizers can add support for authorization by Group
+or some other type.


This is unclear to me. How does the user type ensure that future authorizers can add support for groups or other types? Are you suggesting:
Within Redpanda, only the "User" type is supported. Having prefixes for new types ensures that potential future authorizers can add authorization using other types, such as "Group".

This is sort of a carryover from the Kafka API. The general idea is to namespace user principals so that other types of principals can be added in the future. Incidentally, that's exactly how roles work under the hood! (a role named "my_role" becomes "RedpandaRole:my_role")

Maybe worth noting that this phrasing is not new to this diff and appears in publicly available rpk help text currently. Which is just to say that it probably shouldn't be a release blocker, though I agree that the language is confusing 😕

Missed the second half of your comment, yes, that's the thrust of it.

Feediver1 · 2024-04-03T01:59:43Z

src/go/rpk/pkg/cli/acl/acl.go

+For example '--allow-role "foo"' specifies a role "foo", whereas
+'--allow-role "User:foo"' specifies a role "User:foo". In either case, the
+result is assigned to an internal namespace for roles and will not collide
+with similarly named user principals.


Suggested change

with similarly named user principals.

with similarly-named user principals.

Feediver1 · 2024-04-03T02:00:59Z

src/go/rpk/pkg/cli/acl/acl.go

+present, it is considered part of the role name and processed accordingly.
+
+For example '--allow-role "foo"' specifies a role "foo", whereas
+'--allow-role "User:foo"' specifies a role "User:foo". In either case, the


Suggested change

'--allow-role "User:foo"' specifies a role "User:foo". In either case, the

'--allow-role "User:foo"' specifies a role "foo", which is assigned to a specific user. In either case, the

which is assigned to a specific user

This is not correct. The point is that the prefix is ignored here, but maybe the example just confuses things further?

ignored

More like "treated as text" rather than as type information

Feediver1 · 2024-04-03T02:02:16Z

src/go/rpk/pkg/cli/acl/acl.go

+result is assigned to an internal namespace for roles and will not collide
+with similarly named user principals.
+
+When you create a role, you must add ACLs for it before it can be used.


Suggested change

When you create a role, you must add ACLs for it before it can be used.

When you create a role, you must associate ACLs to it before it can be used.

^^ @oleiman--is this correct ?

Not incorrect, though "bind or associate" seems like a distinction w/o a difference. I would gently lean towards "associate" in this context.

Feediver1 · 2024-04-03T02:05:21Z

src/go/rpk/pkg/cli/acl/acl.go

+You can create / delete / list ACLs for that role with "<name>" in the
+--allow-role and --deny-role flags. Note that the wildcard name '*' is not
+permitted here. Attempting to create an ACL bound to role '*' results in an
+error.


Please explain. So you cannot allow a user/name to have access to all flags or deny a user access to all flags? In other words, you cannot user ACLs to give a user access equivalent to superuser? This paragraph could use an example to help clarify the point being made.

Only the wildcard role name is illegal. Wildcards are still permitted for resources and whatnot. Agree an example will help

oleiman · 2024-04-03T06:25:59Z

force push docs changes

michael-redpanda

lgtm

Deflaimun

LGTM @oleiman check cuz you have one unresolved suggestion from Joyce.

Feediver1

Only one open suggestion left, but not a PR blocker.

The goal here is to hide the "RedpandaRole:" prefix from the CLI frontend. Role names passed into rpk via these flags will be prepended with the appropriate type prefix and appended as a group to the appropriate principals slice before passing latter into the acls builder. Includes some documentation. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

oleiman · 2024-04-03T21:32:07Z

force push small change in phrasing and hopefully fix lint error 🤞

oleiman · 2024-04-03T22:46:15Z

/ci-repeat 1

oleiman · 2024-04-04T03:07:58Z

/ci-repeat 1

oleiman self-assigned this Mar 27, 2024

github-actions bot added the area/rpk label Mar 27, 2024

oleiman added the area/security label Mar 27, 2024

oleiman force-pushed the rbac/rpk/allow-role branch from 4ca31f0 to 4c3ad61 Compare March 27, 2024 15:11

oleiman marked this pull request as ready for review March 27, 2024 15:57

oleiman requested review from twmb, r-vasquez and gene-redpanda as code owners March 27, 2024 15:57

oleiman requested review from graphcareful and michael-redpanda March 27, 2024 18:54

r-vasquez previously approved these changes Mar 27, 2024

View reviewed changes

r-vasquez requested a review from a team March 27, 2024 19:51

Deflaimun reviewed Mar 28, 2024

View reviewed changes

src/go/rpk/pkg/cli/acl/acl.go Outdated Show resolved Hide resolved

Deflaimun reviewed Mar 28, 2024

View reviewed changes

Deflaimun requested a review from Feediver1 March 28, 2024 14:38

michael-redpanda reviewed Mar 28, 2024

View reviewed changes

oleiman dismissed r-vasquez’s stale review via 9a00a6a March 28, 2024 20:51

oleiman force-pushed the rbac/rpk/allow-role branch from 4c3ad61 to 9a00a6a Compare March 28, 2024 20:51

oleiman requested review from Deflaimun, r-vasquez and michael-redpanda March 28, 2024 20:52

Deflaimun reviewed Mar 28, 2024

View reviewed changes

oleiman requested a review from Feediver1 April 1, 2024 16:31

r-vasquez previously approved these changes Apr 1, 2024

View reviewed changes

oleiman requested a review from Deflaimun April 2, 2024 19:52

Feediver1 reviewed Apr 2, 2024

View reviewed changes

Feediver1 reviewed Apr 3, 2024

View reviewed changes

oleiman dismissed r-vasquez’s stale review via 0ca0423 April 3, 2024 06:25

oleiman force-pushed the rbac/rpk/allow-role branch from a202321 to 0ca0423 Compare April 3, 2024 06:25

oleiman requested a review from Feediver1 April 3, 2024 06:31

michael-redpanda previously approved these changes Apr 3, 2024

View reviewed changes

Deflaimun previously approved these changes Apr 3, 2024

View reviewed changes

Feediver1 previously approved these changes Apr 3, 2024

View reviewed changes

oleiman added 3 commits April 3, 2024 14:31

dt/rpk: Add support for --{allow,deny}-role

854bed0

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

dt/rpk_acl_test: Update test_role_acl to use role-specific functions

47dafef

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

oleiman dismissed stale reviews from Feediver1, Deflaimun, and michael-redpanda via 47dafef April 3, 2024 21:31

oleiman force-pushed the rbac/rpk/allow-role branch from 0ca0423 to 47dafef Compare April 3, 2024 21:31

Deflaimun approved these changes Apr 3, 2024

View reviewed changes

michael-redpanda approved these changes Apr 4, 2024

View reviewed changes

oleiman merged commit 8727992 into redpanda-data:dev Apr 4, 2024
23 checks passed

	You should be aware of a few key differences when creating a role ACL:
	The procedures and semantics for creating and managing ACLs
	apply equally to role ACLs and user ACLs, except on the following:

	All ACLs require a principal (or a role; see ROLES, below). A principal is
	All ACLs require a principal or a role. A principal is

	with similarly named user principals.
	with similarly-named user principals.

	'--allow-role "User:foo"' specifies a role "User:foo". In either case, the
	'--allow-role "User:foo"' specifies a role "foo", which is assigned to a specific user. In either case, the

	When you create a role, you must add ACLs for it before it can be used.
	When you create a role, you must associate ACLs to it before it can be used.

RBAC/rpk: Introduce --{allow,deny}-role flags for ACL create/describe/delete #17416

RBAC/rpk: Introduce --{allow,deny}-role flags for ACL create/describe/delete #17416

Conversation

oleiman commented Mar 27, 2024 • edited

Backports Required

Release Notes

Features

oleiman commented Mar 27, 2024

vbotbuildovich commented Mar 27, 2024 • edited

r-vasquez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleiman commented Mar 27, 2024

r-vasquez commented Mar 27, 2024

Deflaimun Mar 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Deflaimun Mar 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Deflaimun commented Mar 28, 2024

michael-redpanda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleiman commented Mar 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Feediver1 Apr 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Feediver1 Apr 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleiman commented Apr 3, 2024

michael-redpanda left a comment

Choose a reason for hiding this comment

Deflaimun left a comment

Choose a reason for hiding this comment

Feediver1 left a comment

Choose a reason for hiding this comment

oleiman commented Apr 3, 2024

oleiman commented Apr 3, 2024

oleiman commented Apr 4, 2024

RBAC/rpk: Introduce `--{allow,deny}-role` flags for ACL create/describe/delete #17416

RBAC/rpk: Introduce `--{allow,deny}-role` flags for ACL create/describe/delete #17416

oleiman commented Mar 27, 2024 •

edited

vbotbuildovich commented Mar 27, 2024 •

edited

Deflaimun Mar 28, 2024 •

edited

Deflaimun Mar 28, 2024 •

edited

Feediver1 Apr 2, 2024 •

edited

Feediver1 Apr 3, 2024 •

edited