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

Introduce principal_type::role #16768

Merged
merged 6 commits into from
Mar 5, 2024
Merged

Conversation

oleiman
Copy link
Member

@oleiman oleiman commented Feb 28, 2024

This PR introduces a new type of security principal for RBAC.
Updates security::to_acl_principal and security::to_kafka_principal to support "Role:" prefix.
Includes a small rpk update to respect the "Role:" prefix when creating/deleting ACLs

Closes https://github.com/redpanda-data/core-internal/issues/1100

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x
  • v23.1.x

Release Notes

  • none

@oleiman
Copy link
Member Author

oleiman commented Feb 28, 2024

/dt

@oleiman
Copy link
Member Author

oleiman commented Feb 29, 2024

/dt

@oleiman oleiman requested a review from BenPope February 29, 2024 06:56
Copy link
Member

@BenPope BenPope left a comment

Choose a reason for hiding this comment

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

LGTM

I did wonder if Role: prefix might be taken elsewhere, or at some point in the future, and wondered if "namespacing it", i.e., RedpandaRole: might be useful.

tests/rptest/tests/rpk_acl_test.py Outdated Show resolved Hide resolved
@oleiman oleiman marked this pull request as ready for review February 29, 2024 17:09
@oleiman
Copy link
Member Author

oleiman commented Feb 29, 2024

I did wonder if Role: prefix might be taken elsewhere, or at some point in the future, and wondered if "namespacing it", i.e., RedpandaRole: might be useful.

Ya, posted something in slack so folks can see, but I'm going to change this.

@oleiman
Copy link
Member Author

oleiman commented Feb 29, 2024

force push: "Role:" -> "RedpandaRole:"

@oleiman
Copy link
Member Author

oleiman commented Feb 29, 2024

force push: typo

@oleiman
Copy link
Member Author

oleiman commented Feb 29, 2024

force push rebase to fix merge conflict

r-vasquez
r-vasquez previously approved these changes Mar 1, 2024
Copy link
Contributor

@r-vasquez r-vasquez left a comment

Choose a reason for hiding this comment

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

rpk bits LGTM 👍

Copy link
Contributor

@pgellert pgellert left a comment

Choose a reason for hiding this comment

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

lgtm

src/v/kafka/server/handlers/details/security.h Outdated Show resolved Hide resolved
tests/rptest/tests/kafka_cli_client_compat_test.py Outdated Show resolved Hide resolved
Also adds "RedpandaRole:" prefix handling to conversion helpers:

- to_acl_principal(...)
- to_kafka_principal(...)
Normally rpk adds a "User:" prefix to any principal name that doesn't
include it. This commit adds an exception to the rule.
Verify that we can create them and that ACLs for "RedpandaRole:<name>" don't
bleed over to "User:<name>".

Verifies that franz-go based clients won't choke on "RedpandaRole:" type principals
@oleiman
Copy link
Member Author

oleiman commented Mar 1, 2024

force push fix test docstring and remove out-of-date linter suppression

@oleiman oleiman requested a review from BenPope March 1, 2024 21:25
Copy link
Contributor

@michael-redpanda michael-redpanda 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. I would try to add in a test or two that tests that error handling works (e.g. if a principal doesn't begin with either User: or RedpandaRole:

Verifying that Java-based clients won't choke on "RedpandaRole:"

Verifying that Redpanda rejects ACL bindings with a bogus principal type
and that Java-based clients properly handle the result. In this case that
means a non-zero exit status becuase shell scripts are fun!
Verifying that librdkafka-based clients won't choke on "RedpandaRole:".

Verifying that ACL bindings with a bogus principal are rejected and that
librdkafka-based clients won't choke on the result.
@oleiman
Copy link
Member Author

oleiman commented Mar 4, 2024

force push to add tests for invalid role prefix handling (librdkafka and java only b/c rpk prepends "User:" to create a valid principal).

Copy link
Member

@BenPope BenPope left a comment

Choose a reason for hiding this comment

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

LGTM

@vbotbuildovich
Copy link
Collaborator

@oleiman oleiman merged commit 10a9e64 into redpanda-data:dev Mar 5, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants