admin: implement RBAC List/Get/Rename/Delete APIs #17166

pgellert · 2024-03-18T15:16:31Z

This builds on #17089 to implement more of the RBAC admin API endpoints. It implements listing roles, getting a role (describing the role), updating the role (renaming it), and deleting the role.

Closes https://github.com/redpanda-data/core-internal/issues/1104
Closes https://github.com/redpanda-data/core-internal/issues/1105
Closes https://github.com/redpanda-data/core-internal/issues/1106
Closes https://github.com/redpanda-data/core-internal/issues/1107
Closes https://github.com/redpanda-data/core-internal/issues/1173

Backports Required

Release Notes

none

pgellert · 2024-03-18T15:18:42Z

The first two commits are from #17089

vbotbuildovich · 2024-03-18T17:45:16Z

new failures in https://buildkite.com/redpanda/redpanda/builds/46368#018e5264-d54c-4705-9a88-a55a4fa32a93:

"rptest.tests.rbac_test.RBACTest.test_get_role"

new failures in https://buildkite.com/redpanda/redpanda/builds/46507#018e5d69-c189-4636-b8f9-46746d3589f2:

"rptest.tests.offset_for_leader_epoch_archival_test.OffsetForLeaderEpochArchivalTest.test_querying_remote_partitions.remote_reads=.False.True"

dotnwat

🔥

oleiman

looking good. few questions, mostly focused around ducktape

tests/rptest/tests/rbac_test.py

src/v/redpanda/admin/security.cc

pgellert · 2024-03-19T09:36:07Z

Force-pushed 3 times to:

Address code review feedback
Fix flaky ducktape tests
Rebase to dev to resolve conflicts

tests/rptest/tests/rbac_test.py

oleiman

I've been working on similar endpoints, so, as before, it'll be good to get a second set of eyes. But LGTM!

tests/rptest/tests/rbac_test.py

pgellert · 2024-03-20T16:34:31Z

Force-pushed to also include returning the correct REST status codes (201 Created, 204 No Content) to also close https://github.com/redpanda-data/core-internal/issues/1173.

oleiman

nice, thanks for doing that!

src/v/redpanda/admin/security.cc

pgellert · 2024-03-20T18:09:29Z

Force-pushed to refactor with Oren's recommendation: #17166 (comment)

oleiman

lgtm

vbotbuildovich · 2024-03-20T21:15:14Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46507#018e5d69-c189-4636-b8f9-46746d3589f2

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46554#018e6131-24dc-4d16-9f69-6ff00ef3d713

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46699#018e74e3-55c7-4992-b73f-2fa16b791169

pgellert · 2024-03-21T12:14:34Z

Rebased to dev to resolve conflicts

BenPope · 2024-03-21T13:53:01Z

src/v/redpanda/admin/security.cc

+ss::future<ss::json::json_return_type>
+admin_server::list_roles_handler(std::unique_ptr<ss::http::request> req) {
+    auto filter = req->get_query_param("filter");
+    auto user = req->get_query_param("user");


This query parameter is registered in the swagger as principal and set as principal in the admin.list_roles?. Why aren't there failing tests?

why aren't there failing tests

Member update endpoints just went in. @pgellert we can resolve those todos now 🙂

src/v/redpanda/admin/security.cc

BenPope · 2024-03-21T14:30:52Z

src/v/redpanda/admin/security.cc

+    ss::httpd::security_json::roles_list j_res{};
+    for (const auto& role_name : roles) {
+        ss::httpd::security_json::role_description j_desc;
+        j_desc.name = ss::sstring{role_name};
+        j_res.roles.push(j_desc);
+    }
+
+    return ss::make_ready_future<ss::json::json_return_type>(j_res);


This generated code isn't very nice to work with; it could do with some constructors, and move semantics. and a push_back, so that you can use it with std::transform and std::back_inserter (although we could easily write a json::pusher that does calls push on the list), and then not copy the whole thing into the response.

And if this is expected to get large, hopefully the array is streamed?

Probably not worth fixing for this PR, though.

This generated code isn't very nice to work with; it could do with some constructors, and move semantics. and a push_back, so that you can use it with std::transform and std::back_inserter (although we could easily write a json::pusher that does calls push on the list), and then not copy the whole thing into the response.

Do you think it's worth a backlog ticket to make this change to seastar?

And if this is expected to get large, hopefully the array is streamed?

It does look like it is streamed: https://github.com/scylladb/seastar/blob/cd8a9133d2c02f63dbd578d882cf7333a427e194/include/seastar/json/formatter.hh#L103-L118

generated code isn't very nice to work with

fwiw, I've been working on changing this in spare time over the last few days. lots of room for improvement.

BenPope · 2024-03-21T14:50:06Z

src/v/redpanda/admin/security.cc

+ss::future<ss::json::json_return_type>
+admin_server::get_role_handler(std::unique_ptr<ss::http::request> req) {
+    ss::sstring role_v;
+    if (!admin::path_decode(req->param["role"], role_v)) {


I thought you added a get_path_param, or did that not get merged?

The upstreaming of that to seastar got stuck. Let me circle back to that and fix these in bulk.

src/v/redpanda/admin/security.cc

BenPope · 2024-03-21T14:57:12Z

src/v/redpanda/admin/security.cc

+        // In order that we can do a reliably ordered validation of
+        // the request (and drop no-op requests), run on controller leader;


I'm not sure why we can't forward this to the controller?

You mean why we can't forward it internally to security_frontend? Like e.g. describe_acls?

We'd need an internal RPC to forward to the controller leader node, right? How do we decide when to forward internally with an RPC vs relying on client forwarding?

We'd need an internal RPC

Yeah, I believe so.

How do we decide

No clue, honestly. w/in the admin API, the redirect seems more general to me, but if there's a hidden cost to doing things this way, we should make a determination.

Probably not for this PR though (since this usage is fairly prevalent across v1/security/*

We'd need an internal RPC to forward to the controller leader node, right?

Yes.

How do we decide when to forward internally with an RPC vs relying on client forwarding?

Good question. I wasn't actually aware of how this 307 worked. Should be ok.

Probably not for this PR though (since this usage is fairly prevalent across

Fine by me.

src/v/redpanda/admin/security.cc

BenPope · 2024-03-21T15:04:09Z

tests/rptest/tests/rbac_test.py

+            assert self._set_of_user_roles() in [{self.role_name0},
+                                                 {self.role_name1}]


This check seems redundant give the next check for set equivalence.

This is to guard against seeing the intermediary state of both roles being present. I've updated the comment to make this even clearer.

src/v/redpanda/admin/security.cc

BenPope · 2024-03-21T15:09:09Z

tests/rptest/tests/rbac_test.py

+                   backoff_sec=0.5)
+
+        # Assert idempotency
+        self.superuser_admin.delete_role(role=self.role_name0)


Should the status_code be checked?

This implements the list_roles_handler which will allow the listing of roles known to the redpanda cluster.

Validate that the list roles API is wokring.

This changes the create and delete role admin API handlers to return 201 and 204 as prescribed by the REST standard.

pgellert · 2024-03-25T08:18:54Z

Force-pushed twice, first to address code review comments and then to rebase to dev to resolve conflicts.

BenPope · 2024-03-25T18:06:40Z

src/v/redpanda/admin/security.cc

+                               role_entry,
+                               security::role_member{
+                                 security::role_member_type::user, user});
+        return name_prefix_matches && role_has_user;


question (non-blocking): Is it worth making role_has_user lazy in case it can be short-circuited at name_prefix_matches?

You can make it a named lambda and invoke it after the &&

Suggested change

return name_prefix_matches && role_has_user;

return name_prefix_matches && role_has_user();

I'm not sure, I think I prefer the clearer code for now and we can optimize this if it becomes a bottleneck. But happy to take this up if you disagree.

BenPope · 2024-03-25T18:15:35Z

tests/rptest/tests/rbac_test.py

        res = self.superuser_admin.create_role(role=self.role_name0)
        created_role = res.json()['role']
        assert created_role == self.role_name0, f"Incorrect create role response: {res.json()}"

-        # Also verify idempotency
+        wait_until(lambda: self._set_of_user_roles() == {self.role_name0},


nitpick: We only need to check that the role exists, bot that no other role exists.

Suggested change

wait_until(lambda: self._set_of_user_roles() == {self.role_name0},

wait_until(lambda: self.role_name0 in self._set_of_user_roles()

Fair, although here both should be true

BenPope · 2024-03-25T18:17:24Z

tests/rptest/tests/rbac_test.py

+                   backoff_sec=2,
+                   err_msg="Role was not created")
+
+        self.logger.debug("Also test idempotency of create_role")


Should the status_code be checked? (as well as above)

It's checked above (I added that as the last commit, so I think that's why it's not showing up on the commit you commented on.) I think it's fine to check it only once.

tests/rptest/tests/rbac_test.py

oleiman

lgtm

oleiman · 2024-03-25T21:17:31Z

src/v/redpanda/admin/security.cc

+    auto principal_type = req->get_query_param("principal_type");
+
+    if (!principal_type.empty() && principal_type != "User") {
+        throw_role_exception(
+          role_errc::malformed_def,
+          fmt::format(
+            "Role membership reserved for user principals, got {{{}}}",
+            principal_type));
+    }


nitpick: It would be nice if we could express more directly that principal_type takes a default value ("User") and feed that value (explicitly set or defaulted) into the predicate. More a limitation of the roles interface than anything I think.

oleiman · 2024-03-25T21:19:21Z

tests/rptest/tests/rbac_test.py

+                    principal=bob.name,
+                    principal_type="User",
+                    expected_roles=[role_two])
+


suggestion (if-minor): Could add a sanity check that default principal_type works as expected, e.g

test_filter(filter="aaaa", principal=bob.name, expected_roles=[role_two])

Should be a well-formed request.

I have exactly this on line 234 :)

oleiman · 2024-03-25T22:37:21Z

src/v/redpanda/admin/security.cc

+    if (err == cluster::errc::role_exists) {
+        throw_role_exception(role_errc::role_name_conflict);
+    } else if (err == cluster::errc::role_does_not_exist) {
+        throw_role_exception(role_errc::role_not_found);
+    }


suggestion (non-blocking): here and maybe elsewhere

Suggested change

if (err == cluster::errc::role_exists) {

throw_role_exception(role_errc::role_name_conflict);

} else if (err == cluster::errc::role_does_not_exist) {

throw_role_exception(role_errc::role_not_found);

}

throw_on_role_command_err(err);

Maybe we can do one last round of polish if it's worth it as a follow-up PR for this.

github-actions bot added the area/redpanda label Mar 18, 2024

pgellert self-assigned this Mar 18, 2024

pgellert requested review from oleiman and BenPope March 18, 2024 15:17

dotnwat reviewed Mar 18, 2024

View reviewed changes

oleiman reviewed Mar 18, 2024

View reviewed changes

pgellert force-pushed the rbac/admin-api-crud branch 3 times, most recently from 78a1928 to 7fdf042 Compare March 19, 2024 09:33

pgellert commented Mar 19, 2024

View reviewed changes

tests/rptest/tests/rbac_test.py Outdated Show resolved Hide resolved

pgellert force-pushed the rbac/admin-api-crud branch from 7fdf042 to 669ac7b Compare March 19, 2024 13:59

pgellert requested review from oleiman and dotnwat March 19, 2024 14:01

oleiman previously approved these changes Mar 20, 2024

View reviewed changes

tests/rptest/tests/rbac_test.py Outdated Show resolved Hide resolved

pgellert dismissed oleiman’s stale review via 5b68ea8 March 20, 2024 16:31

pgellert force-pushed the rbac/admin-api-crud branch from 669ac7b to 5b68ea8 Compare March 20, 2024 16:31

pgellert requested a review from oleiman March 20, 2024 16:34

oleiman previously approved these changes Mar 20, 2024

View reviewed changes

src/v/redpanda/admin/security.cc Show resolved Hide resolved

src/v/redpanda/admin/security.cc Outdated Show resolved Hide resolved

pgellert dismissed oleiman’s stale review via c3b258b March 20, 2024 18:08

pgellert force-pushed the rbac/admin-api-crud branch from 5b68ea8 to c3b258b Compare March 20, 2024 18:08

oleiman previously approved these changes Mar 20, 2024

View reviewed changes

oleiman requested a review from michael-redpanda March 20, 2024 20:30

pgellert dismissed oleiman’s stale review via f1ed886 March 21, 2024 12:13

pgellert force-pushed the rbac/admin-api-crud branch from c3b258b to f1ed886 Compare March 21, 2024 12:13

BenPope reviewed Mar 21, 2024

View reviewed changes

pgellert force-pushed the rbac/admin-api-crud branch from f1ed886 to dfe63cf Compare March 25, 2024 08:05

pgellert added 9 commits March 25, 2024 08:05

admin: implement list roles API

373796f

This implements the list_roles_handler which will allow the listing of roles known to the redpanda cluster.

admin/test: list roles API

2d3c8a0

Validate that the list roles API is wokring.

admin: implement get role API

d0c723d

admin/test: get role API

3fa3580

admin: implement update (rename) role

af44c1c

admin/test: update (rename) role API

bb25e0d

admin: implement delete role API

0ad9714

admin/test: delete role API

9535f43

admin: return REST status codes for rbac

263346d

This changes the create and delete role admin API handlers to return 201 and 204 as prescribed by the REST standard.

pgellert force-pushed the rbac/admin-api-crud branch from dfe63cf to 263346d Compare March 25, 2024 08:17

pgellert requested review from BenPope and oleiman March 25, 2024 08:19

BenPope approved these changes Mar 25, 2024

View reviewed changes

oleiman approved these changes Mar 25, 2024

View reviewed changes

pgellert merged commit 9dae9fe into redpanda-data:dev Mar 26, 2024
18 checks passed

		// In order that we can do a reliably ordered validation of
		// the request (and drop no-op requests), run on controller leader;

		assert self._set_of_user_roles() in [{self.role_name0},
		{self.role_name1}]

	return name_prefix_matches && role_has_user;
	return name_prefix_matches && role_has_user();

	wait_until(lambda: self._set_of_user_roles() == {self.role_name0},
	wait_until(lambda: self.role_name0 in self._set_of_user_roles()

admin: implement RBAC List/Get/Rename/Delete APIs #17166

admin: implement RBAC List/Get/Rename/Delete APIs #17166

Conversation

pgellert commented Mar 18, 2024 • edited

Backports Required

Release Notes

pgellert commented Mar 18, 2024

vbotbuildovich commented Mar 18, 2024 • edited

dotnwat left a comment

Choose a reason for hiding this comment

oleiman left a comment

Choose a reason for hiding this comment

pgellert commented Mar 19, 2024

oleiman left a comment

Choose a reason for hiding this comment

pgellert commented Mar 20, 2024

oleiman left a comment

Choose a reason for hiding this comment

pgellert commented Mar 20, 2024

oleiman left a comment

Choose a reason for hiding this comment

vbotbuildovich commented Mar 20, 2024 • edited

pgellert commented Mar 21, 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

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

BenPope Mar 25, 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

pgellert commented Mar 25, 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

oleiman 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

pgellert commented Mar 18, 2024 •

edited

vbotbuildovich commented Mar 18, 2024 •

edited

vbotbuildovich commented Mar 20, 2024 •

edited

BenPope Mar 25, 2024 •

edited