admin: implement create role API #17089

pgellert · 2024-03-14T09:44:25Z

This implements the create_role_handler which will allow the creation of
an empty role to which users can later be assigned to.

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

Backports Required

Release Notes

none

vbotbuildovich · 2024-03-14T12:39:26Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46176#018e3cb2-a8a2-4046-a540-6f7a49252b1f

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46361#018e51da-2f39-4a8f-8f2a-c77dd7f4aff8

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46361#018e51ec-7d88-42b8-bdc0-9ae96a2f1695

src/v/redpanda/admin/security.cc

oleiman

Looks good. Several minor comments, mostly carry-over from POC I think.

FWIW - if you see something on the POC branch that looks very janky, it probably is. I won't be offended if you want to blow some of it up 🙂

src/v/redpanda/admin/security.cc

tests/rptest/tests/rbac_test.py

src/v/redpanda/admin/security.cc

pgellert · 2024-03-14T19:33:33Z

Force-pushed to address the comments above

oleiman

Couple things on the ducktape side and an error path I missed; otherwise looking good.

Probably a good idea to get one more set of eyes before merging :)

oleiman · 2024-03-14T20:02:24Z

tests/rptest/tests/rbac_test.py

-        with expect_role_error(RoleErrorCode.MALFORMED_DEF):
-            self.superuser_admin.create_role(role=role_name0)
+        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()}"


Redundant with test_create_user, right?

oleiman · 2024-03-14T20:07:46Z

tests/rptest/tests/rbac_test.py

+    def test_create_role(self):
+        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()}"


Meant to comment before - I think we'd be well served to test misuse of the API here, specifically the malformed body cases. Also idempotency.

Good call. Because I used parse_json_body, we return a 400 with a non-role-specific error if the body is missing / not well-formed json. I think that's fine and not worth the trouble changing (RPK/console should just send us well-formed requests anyway). But maybe we should call this out in the RFC. What do you think?

src/v/redpanda/admin/security.cc

oleiman · 2024-03-15T02:44:50Z

src/v/redpanda/admin/security.cc

+    }
+
+    co_await throw_on_error(*req, err, model::controller_ntp);
+    co_return ss::json::json_return_type(j_res);


Something else I've been meaning to address - we intended to return 201 (Created) on this endpoint because REST... I don't know how easy that is. It's possible that the seastar machinery is clever enough to glean it from the swagger, but I'm not sure.

I don't think it's smart enough, unfortunately. It's returning 200 (OK) at the moment.

I spent a bit of time trying to refactor this to allow setting a result code, but it's not straightforward as far as I can tell. We'd have to at least refactor register_route and replicate what seastar is doing for creating a json handler for this to work.

I'd be tempted to keep this returning 200 to be consistent with the other admin API endpoints (and to keep things simple), but let me know what you think.

Bummer. I'll add it my short list of minor API changes. Nothing for you to do, but I'll post an update on the channel today. I have a couple others, also arising from seastar::httpd implementation details

It can be done, but you have to be a bit more manual about it: https://github.com/redpanda-data/redpanda/blob/dev/src/v/pandaproxy/reply.h#L75-L82

Nice. We'd need access to the http_reply object, so we'd want to use register_route_raw_async to register the route IIUC

My mistake, I think this is the correct overload:

redpanda/src/v/redpanda/admin/server.h

Lines 392 to 398 in dc7b8d8

template<auth_level required_auth>

void register_route(

ss::httpd::path_description const& path, request_handler_fn handler) {

path.set(

_server._routes,

new handler_impl<required_auth>{*this, std::move(handler)});

}

Created an issue to make this change as a follow up: https://github.com/redpanda-data/core-internal/issues/1173

oleiman

This is looking good to me. As noted elsewhere, let's wait for a spot check from someone not me and

tests/rptest/tests/rbac_test.py

BenPope

Looks pretty good

src/v/redpanda/admin/security.cc

BenPope · 2024-03-15T16:06:10Z

src/v/redpanda/admin/security.cc

+    if (err == cluster::errc::role_exists) {
+        // Idempotency: if the empty role already exists,
+        // suppress the role_exists error and return success.
+        auto role = _controller->get_role_store().local().get(role_name);


Suggested change

auto role = _controller->get_role_store().local().get(role_name);

This is now just duplicating the auto role = ... above.

Right, I removed this now. I was thinking that it might be worth shortcutting a controller log entry replication by checking the existence of the role before calling create_role. But it does simplify the code a bit if we don't double-check.

BenPope · 2024-03-15T16:09:48Z

src/v/redpanda/admin/security.cc

+        // Idempotency: if the empty role already exists,
+        // suppress the role_exists error and return success.


Isn't this checked above? Or are you gearing up for something more than an empty role?

I think this is just double-checking to guard against a race condition with a _controller->get_security_frontend().local().create_role(...) on another node. It's mimicking the create_user_handler:

redpanda/src/v/redpanda/admin/security.cc

Lines 361 to 370 in 066794c

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

// Idempotency: if user is same as one that already exists,

// suppress the user_exists error and return success.

const auto& credentials_store

= _controller->get_credential_store().local();

std::optional<security::scram_credential> creds

= credentials_store.get<security::scram_credential>(username);

if (creds.has_value() && match_scram_credential(doc, creds.value())) {

co_return ss::json::json_return_type(ss::json::json_void());

}

BenPope · 2024-03-15T16:21:00Z

src/v/redpanda/admin/security.cc

+    }
+
+    co_await throw_on_error(*req, err, model::controller_ntp);
+    co_return ss::json::json_return_type(j_res);


It can be done, but you have to be a bit more manual about it: https://github.com/redpanda-data/redpanda/blob/dev/src/v/pandaproxy/reply.h#L75-L82

pgellert · 2024-03-18T10:28:38Z

Force-pushed to simplify checking for existing roles based on feedback from @BenPope

BenPope

LGTM

BenPope · 2024-03-18T12:48:36Z

src/v/redpanda/admin/security.cc

+    auto err
+      = co_await _controller->get_security_frontend().local().create_role(
+        role_name, security::role{}, model::timeout_clock::now() + 5s);
+
+    if (err == cluster::errc::role_exists) {
+        // Idempotency: if the empty role already exists,
+        // suppress the role_exists error and return success.
+        auto role = _controller->get_role_store().local().get(role_name);
+        if (role.has_value() && role.value().members().empty()) {


This is a nitpick, which seems a little clearer to me, and will help with the role is not empty.

Suggested change

auto err

= co_await _controller->get_security_frontend().local().create_role(

role_name, security::role{}, model::timeout_clock::now() + 5s);

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

// Idempotency: if the empty role already exists,

// suppress the role_exists error and return success.

auto role = _controller->get_role_store().local().get(role_name);

if (role.has_value() && role.value().members().empty()) {

security::role role{};

auto err

= co_await _controller->get_security_frontend().local().create_role(

role_name, role, model::timeout_clock::now() + 5s);

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

// Idempotency: if the empty role already exists,

// suppress the role_exists error and return success.

if (_controller->get_role_store().local().get(role_name) == role) {

Nice, I like this. Addressed

This implements the create_role_handler which will allow the creation of an empty role to which users can later be assigned to.

This tests the create_role_handler of the admin API, testing that an empty role can successfully be created. Additionally, it verifies that the validation on incorrect role names works.

michael-redpanda

LGTM!

pgellert requested a review from oleiman March 14, 2024 09:44

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

oleiman reviewed Mar 14, 2024

View reviewed changes

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

oleiman assigned pgellert Mar 14, 2024

oleiman reviewed Mar 14, 2024

View reviewed changes

pgellert force-pushed the rbac/admin-api-create branch from 7c3c251 to 9037f4b Compare March 14, 2024 19:32

pgellert requested a review from oleiman March 14, 2024 19:33

oleiman reviewed Mar 14, 2024

View reviewed changes

oleiman reviewed Mar 15, 2024

View reviewed changes

pgellert force-pushed the rbac/admin-api-create branch from 9037f4b to b3255c3 Compare March 15, 2024 11:50

pgellert requested review from oleiman, BenPope, graphcareful and michael-redpanda March 15, 2024 11:50

oleiman reviewed Mar 15, 2024

View reviewed changes

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

BenPope reviewed Mar 15, 2024

View reviewed changes

pgellert force-pushed the rbac/admin-api-create branch from b3255c3 to 55b8670 Compare March 15, 2024 16:54

pgellert requested review from BenPope and oleiman March 15, 2024 16:54

pgellert force-pushed the rbac/admin-api-create branch from 55b8670 to e8182ea Compare March 18, 2024 10:24

BenPope previously approved these changes Mar 18, 2024

View reviewed changes

pgellert added 2 commits March 18, 2024 13:01

admin: implement create_role_handler

3652226

This implements the create_role_handler which will allow the creation of an empty role to which users can later be assigned to.

admin/test: create empty role

02a391a

This tests the create_role_handler of the admin API, testing that an empty role can successfully be created. Additionally, it verifies that the validation on incorrect role names works.

pgellert dismissed BenPope’s stale review via 02a391a March 18, 2024 13:01

pgellert force-pushed the rbac/admin-api-create branch from e8182ea to 02a391a Compare March 18, 2024 13:01

pgellert requested a review from BenPope March 18, 2024 13:02

BenPope approved these changes Mar 18, 2024

View reviewed changes

michael-redpanda approved these changes Mar 18, 2024

View reviewed changes

BenPope mentioned this pull request Mar 18, 2024

rbac: Add Telemetry and Licence nag #17163

Merged

6 tasks

pgellert mentioned this pull request Mar 18, 2024

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

Merged

6 tasks

pgellert merged commit 036ce1f into redpanda-data:dev Mar 18, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

admin: implement create role API #17089

admin: implement create role API #17089

pgellert commented Mar 14, 2024

vbotbuildovich commented Mar 14, 2024 •

edited

oleiman left a comment

pgellert commented Mar 14, 2024

oleiman left a comment

oleiman Mar 14, 2024

oleiman Mar 14, 2024

pgellert Mar 15, 2024

oleiman Mar 15, 2024

oleiman Mar 15, 2024

pgellert Mar 15, 2024

oleiman Mar 15, 2024

BenPope Mar 15, 2024

oleiman Mar 15, 2024

oleiman Mar 18, 2024

pgellert Mar 18, 2024

oleiman left a comment

BenPope left a comment

BenPope Mar 15, 2024

BenPope Mar 15, 2024

pgellert Mar 18, 2024

BenPope Mar 15, 2024 •

edited

pgellert Mar 15, 2024

BenPope Mar 15, 2024

pgellert commented Mar 18, 2024

BenPope left a comment

BenPope Mar 18, 2024

pgellert Mar 18, 2024

michael-redpanda left a comment

	template<auth_level required_auth>
	void register_route(
	ss::httpd::path_description const& path, request_handler_fn handler) {
	path.set(
	_server._routes,
	new handler_impl<required_auth>{*this, std::move(handler)});
	}

		// Idempotency: if the empty role already exists,
		// suppress the role_exists error and return success.

	if (err == cluster::errc::user_exists) {
	// Idempotency: if user is same as one that already exists,
	// suppress the user_exists error and return success.
	const auto& credentials_store
	= _controller->get_credential_store().local();
	std::optional<security::scram_credential> creds
	= credentials_store.get<security::scram_credential>(username);
	if (creds.has_value() && match_scram_credential(doc, creds.value())) {
	co_return ss::json::json_return_type(ss::json::json_void());
	}

admin: implement create role API #17089

admin: implement create role API #17089

Conversation

pgellert commented Mar 14, 2024

Backports Required

Release Notes

vbotbuildovich commented Mar 14, 2024 • edited

oleiman left a comment

Choose a reason for hiding this comment

pgellert commented Mar 14, 2024

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

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

BenPope 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

BenPope Mar 15, 2024 • edited

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

BenPope left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-redpanda left a comment

Choose a reason for hiding this comment

vbotbuildovich commented Mar 14, 2024 •

edited

BenPope Mar 15, 2024 •

edited