Role based authorization #17025

oleiman · 2024-03-13T01:31:53Z

This PR wires roles into the authorization flow in redpanda.

closes https://github.com/redpanda-data/core-internal/issues/1101

Preliminary benchmarks:

test	iterations	median	mad	min	max	allocs
role_store_bench.get_member_roles	12917749	49.132ns	0.044ns	48.993ns	49.264ns	0.000
role_store_bench.get_member_roles_bare_query	13760304	49.146ns	0.118ns	48.996ns	49.708ns	0.000
role_store_bench.user_range_query	9672507	78.019ns	0.068ns	77.803ns	78.220ns	0.000
role_store_bench.user_range_query_bare_query	20385250	25.463ns	0.026ns	25.398ns	25.521ns	0.000
role_store_bench.remove_role	47	18.703us	658.064ns	18.023us	29.874us	0.000
role_store_bench.update_role	47	60.956us	255.915ns	60.310us	61.285us	525.704
role_store_bench.put_role	47	24.174us	211.787ns	23.914us	24.997us	1.696
role_store_bench.role_authz_512_roles_1Ki_members	28388	5.719us	13.480ns	5.630us	5.734us	4.032
role_store_bench.role_authz_256_roles_1Ki_members	31263	2.995us	2.483ns	2.990us	3.002us	4.036
role_store_bench.role_authz_128_roles_1Ki_members	34120	1.619us	0.803ns	1.617us	1.621us	3.918
role_store_bench.role_authz_64_roles_1Ki_members	34654	921.467ns	2.078ns	912.048ns	924.389ns	3.993
role_store_bench.role_authz_64_roles_1Ki_members_4_extra_bindings	32975	1.034us	1.012ns	1.031us	1.035us	3.988
role_store_bench.role_authz_64_roles_1Ki_members_8_extra_bindings	31677	1.040us	0.607ns	1.038us	1.044us	3.991
role_store_bench.role_authz_64_roles_1Ki_members_16_extra_bindings	28099	1.165us	1.988ns	1.156us	1.167us	3.993
role_store_bench.role_authz_64_roles_512_members	70935	924.544ns	4.125ns	915.173ns	928.669ns	3.976
role_store_bench.role_authz_64_roles_512_members_deny	72286	691.401ns	0.590ns	690.265ns	693.075ns	3.972
role_store_bench.role_authz_8_roles_1Ki_members	36066	299.003ns	0.320ns	298.146ns	299.322ns	4.028
role_store_bench.role_authz_empty_store	1774757	95.098ns	0.028ns	94.953ns	95.189ns	2.000

Backports Required

Release Notes

none

oleiman · 2024-03-13T21:31:37Z

/dt

oleiman · 2024-03-13T22:46:14Z

/dt

oleiman · 2024-03-14T01:55:50Z

/dt

vbotbuildovich · 2024-03-14T04:34:29Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46153#018e3af3-01ab-4eb6-af27-906578b1c89d

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46204#018e3e14-4c9a-4216-ad72-a1c8917a9404

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46204#018e3e26-0fd5-48f3-942f-de7b167e67f5

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46238#018e3fca-13e1-4d9b-b73b-c69e71b85250

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46238#018e3fca-13e4-4bac-9d1a-33097f4b5080

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46238#018e3fda-8c28-46f3-a9bc-049b0957bd93

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46309#018e440a-4b31-4892-afa0-e62afaf71885

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46313#018e449c-087d-4bd5-a108-32ff71188e3e

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46313#018e45b9-f9c7-42fb-a352-36ed46d0c09c

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46339#018e4e61-10ee-44f9-a7ee-36a798550a7d

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46391#018e5314-0165-44ed-8ac2-887b75f7629a

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46532#018e5e63-ada4-4d6b-8ac0-83f7026cef71

src/v/security/role.cc

src/v/cluster/controller.cc

src/v/security/acl.cc

oleiman · 2024-03-15T00:48:07Z

force push review comments and minor commit history cleanups

oleiman · 2024-03-15T03:59:54Z

CI Failure:

storage_e2e_single_thread_rpunit

dotnwat

those changes lgtm

BenPope

Looking good.

I wonder if it's worth reducing allocs by changing the interface away from:

auto acls = _store.find(type, resource_name());

src/v/security/role.cc

src/v/security/acl.h

src/v/security/authorizer.h

src/v/security/acl.h

BenPope · 2024-03-15T13:42:50Z

src/v/security/acl.cc

@@ -185,19 +220,31 @@ std::vector<std::vector<acl_binding>> acl_store::remove_bindings(
            continue;
        }

+        // NOTE(oren): deleted bindings are held in the enclosing scope, so
+        // we can use a non-owning reference here to avoid copying.
+        ss::chunked_fifo<acl_principal_view> maybe_roles;


No preference here, more of a question of why not?

Suggested change

ss::chunked_fifo<acl_principal_view> maybe_roles;

chunked_vector<acl_principal_view> maybe_roles;

The ambient guidance I've heard is "use chunked_vector where you would use std::vector"; I don't need random access here, so I chose the fifo. That might be an overly legalistic interpretation of the heuristic though. Will peek inside and/or ask around.

I think either is fine. chunked_fifo has items_per_chunk = 128 regardless of sizeof(item). I don't think it will matter much here.

Actually I'm not sure why we're collecting maybe_role inside of erase_if instead of returning true, is it to deal with dry_run?

is it to deal with dry_run

Partly. Also the erase_if operates on the internal acl_entry container (not the acl_entry_set itself), so there's no obvious way to plug in the role_cache accounting. I agree it's yucky.

Maybe the right thing is to do a little refactoring here. There are plenty of tests to support that effort.

still looking at a refactor, but I did switch to chunked_vector

src/v/security/acl_store.h

src/v/security/authorizer.h

src/v/security/tests/role_store_bench.cc

oleiman · 2024-03-15T20:43:19Z

force push code review comments, notably:

some enum safety (?)
potentially clarifying cleanup in authorizer::authorized
switched to a chunked_vector for tracking deleted roles bindings in remove_bindings

oleiman · 2024-03-15T20:48:24Z

I wonder if it's worth reducing allocs by changing the interface away from:

auto acls = _store.find(type, resource_name());

Taking a look this afternoon 👍

oleiman · 2024-03-15T21:25:03Z

I wonder if it's worth reducing allocs by changing the interface away from:
auto acls = _store.find(type, resource_name());

Taking a look this afternoon 👍

Actually I'm confused - @BenPope are you referring to the std::vector inside acl_matches?

BenPope · 2024-03-15T21:32:10Z

I wonder if it's worth reducing allocs by changing the interface away from:
auto acls = _store.find(type, resource_name());

Taking a look this afternoon 👍

Actually I'm confused - @BenPope are you referring to the std::vector inside acl_matches?

The return of the vector of matches

oleiman · 2024-03-17T20:39:50Z

force push moved some new code to a follow-on PR: #17152

src/v/security/authorizer.h

BenPope · 2024-03-18T10:58:11Z

src/v/security/authorizer.h

+
+        auto member_roles =
+          [this, &principal]() -> security::role_store::roles_range {
+            if (principal.type() == security::principal_type::user) {


I think it makes sense to allow ephemeral users to have role, although other parts of the code don't allow this.

I'd like to talk through this a bit. Ticket to follow up: https://github.com/redpanda-data/core-internal/issues/1175

src/v/security/authorizer.h

oleiman · 2024-03-18T18:03:13Z

force push to address CR comments. Shifting code around and collapsing a few lambdas in authorizer::authorized

oleiman · 2024-03-18T18:23:08Z

force push correct move semantics to save a copy

rockwotj

I only got part way through, but wanted to leave my feedback. I am mostly asking for comments on new methods/classes that are added, I know this code is pretty lightly doc'd at the moment, but we can at least improve the new stuff.

src/v/security/role.cc

src/v/security/acl.h

src/v/security/authorizer.h

src/v/security/role.h

Helper functions for needed conversions between role_members and acl_principal-like things. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

Non-allocating string_view-based principal-like-thing. Also introduces acl_principal_base, which is needed for polymorphic usage at various internal interfaces, and corresponding changes to those interfaces. A side effect of this is that `acl_principal::name` now returns a string_view, so we also introduce `acl_principal::adl_name` to preserve the original behavior for ADL-based serialization. Note that both acl_principal_view and (now) acl_principal are declared final, which should allow devirtualization to take place in release mode. As a result, this change should (and based on subsequent benchmarking does) not carry any appreciable performance penalty. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

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

oleiman · 2024-03-20T22:42:15Z

force push modest header documentation improvements

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

Non-allocating role-member-like thing for transparent role_store lookups. Saves an allocation in the authorizer. Adjusts role_store::roles_for_member to accept either one of these or the usual role_member struct. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

Cache of role principal names on acl_entry_set to avoid linear-time lookups for role principals that don't have any bindings. We can do this because role bindings are strict (no name wildcards). Also adds a special case so acl_principal.wildcard() returns false for role type principals. This avoids matching wildcard users against wildcard roles (which shouldn't exist). Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

role_store::roles_for_member returns a boost::iterator_range into the members store. For convenience in security::authorizer, expose that type via an alias in role_store's public interface. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

This does come at a cost, since we have to search the role_store to confidently assert "no match". However, if the role_store is empty (or if the authN'ed principal is not found in the store), this cost should be on the order of a node_hash_map lookup (internal to role_store::roles_for_member). See subsequent benchmarks for detail. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

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

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

Single invocations of various role_store calls are more interesting than many in a loop. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

Notably, the cost of a single authorized operation increases super- logarithmically but sub-linearly with the number of roles in the store. Performance degradation in the presence of many member entries is marginal. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

oleiman · 2024-03-20T23:25:29Z

force push authorizer takes a pointer rather than a reference.

BenPope

LGTM

rockwotj

A few questions, LGTM

rockwotj · 2024-03-21T13:31:03Z

src/v/security/acl.cc

+        }
+        // ensure that elements won't outlive the corresponding bindings
+        // in enclosing scope.
+        maybe_roles.erase_to_end(maybe_roles.begin());


Is this clear?

Ah, my intention was to avoid releasing the memory, but it does look like this decays to fragmented_vector::clear

maybe a std::vector is actually a better choice here?

I don't have a good mental model of the limits here, but it feels safer to alloc a bit more other than crash (if this could grow to an oversized alloc).

Oh, good spot - I hadn't noticed that it took the path of clear.

don't have a good mental model of the limits here

Neither do I, but I'd expect the number of iterations to be quite small in the common case. So potential savings is marginal compared to risk of the unknown. Probably just leave it as is for now.

rockwotj · 2024-03-21T13:34:32Z

src/v/security/authorizer.h

+              "Role principal should be non-null and have 'role' type if "
+              "present");


Someday we should either upgrade absl to use the nullability helpers or just create a similar abstraction: https://github.com/abseil/abseil-cpp/blob/master/absl/base/nullability.h

Nothing to do here, just food for thought.

Yes, I'll have all the contracts and monads please and thanks. Anyway, good call out, I wasn't aware of that stuff at all. Very cool.

rockwotj · 2024-03-21T13:37:29Z

src/v/security/authorizer.h

+        // NOTE(oren): We know there isn't a match at this point, but I've left
+        // this as an opt_acl_match to preserve semantics, namely that this
+        // will return a non-authorized result irrespective of the
+        // allow_empty_matches flag. On the other hand, switching to an
+        // empty_match_result _should_ alter semantics but doesn't break any
+        // tests. Not clear whether this is a bug, intended behavior, a gap
+        // in test coverage, or a combination.


Is there a followup ticket to track this down?

Not yet, but I'll make one now. Left the comment there on the off-chance somebody would drop into the PR with ad hoc knowledge, but that didn't happen

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

rockwotj · 2024-03-21T13:39:10Z

src/v/security/authorizer.h

+                auto result = _role_store->roles_for_member(
+                                security::role_member_view::from_principal(
+                                  principal))
+                              | std::views::transform([](const auto& e) {
+                                    return role::to_principal_view(e);
+                                })
+                              | std::views::transform(
+                                [&user, &check_access, perm](const auto& e) {
+                                    return check_access(perm, user, &e);
+                                })
+                              | std::views::filter(
+                                [](const std::optional<auth_result>& r) {
+                                    return r.has_value();
+                                })
+                              | std::views::take(1);


dotnwat · 2024-03-21T18:34:49Z

src/v/security/acl.cc

@@ -17,17 +17,52 @@
 #include <seastar/coroutine/maybe_yield.hh>

 #include <absl/container/flat_hash_map.h>
+#include <container/fragmented_vector.h>


@oleiman use "..." for internal includes

Good spot, thanks. Not sure how that slipped in.

oleiman self-assigned this Mar 13, 2024

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

oleiman force-pushed the rbac/authZ branch 2 times, most recently from 73d6cc8 to 789bfab Compare March 13, 2024 21:22

oleiman force-pushed the rbac/authZ branch from 789bfab to 3fcc61c Compare March 13, 2024 22:45

This comment was marked as outdated.

Sign in to view

oleiman force-pushed the rbac/authZ branch from 3fcc61c to e46eb52 Compare March 14, 2024 01:55

oleiman force-pushed the rbac/authZ branch 3 times, most recently from 7df47a5 to 8f68758 Compare March 14, 2024 16:53

oleiman marked this pull request as ready for review March 14, 2024 16:53

oleiman requested review from BenPope and pgellert March 14, 2024 19:09

dotnwat reviewed Mar 14, 2024

View reviewed changes

src/v/security/role.cc Outdated Show resolved Hide resolved

src/v/cluster/controller.cc Outdated Show resolved Hide resolved

src/v/security/acl.cc Outdated Show resolved Hide resolved

oleiman force-pushed the rbac/authZ branch from 8f68758 to 2881d9e Compare March 15, 2024 00:45

dotnwat reviewed Mar 15, 2024

View reviewed changes

BenPope reviewed Mar 15, 2024

View reviewed changes

oleiman force-pushed the rbac/authZ branch from 2881d9e to 1761d34 Compare March 15, 2024 20:41

oleiman force-pushed the rbac/authZ branch from edda140 to 1761d34 Compare March 17, 2024 20:27

BenPope reviewed Mar 18, 2024

View reviewed changes

oleiman force-pushed the rbac/authZ branch from 1761d34 to 55c2a6c Compare March 18, 2024 17:58

oleiman requested a review from BenPope March 18, 2024 18:03

oleiman force-pushed the rbac/authZ branch from 55c2a6c to d2ad0dd Compare March 18, 2024 18:22

oleiman mentioned this pull request Mar 18, 2024

acl_store: Return prefix matches as a view #17152

Merged

6 tasks

oleiman requested a review from rockwotj March 20, 2024 16:04

rockwotj reviewed Mar 20, 2024

View reviewed changes

src/v/security/role.cc Outdated Show resolved Hide resolved

src/v/security/acl.h Show resolved Hide resolved

src/v/security/acl.h Show resolved Hide resolved

src/v/security/authorizer.h Outdated Show resolved Hide resolved

src/v/security/role.h Show resolved Hide resolved

oleiman added 3 commits March 20, 2024 15:39

security/role: member <-> acl_principal

96736ce

Helper functions for needed conversions between role_members and acl_principal-like things. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

security/authz: Introduce role_acl_match

d428d59

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

oleiman force-pushed the rbac/authZ branch from d2ad0dd to c1779e4 Compare March 20, 2024 22:41

oleiman added 9 commits March 20, 2024 16:24

cluster: Wire security::role_store into security::authorizer

558bc29

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

security/test: Add "authz_" prefix to authorizer tests

471b0db

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

security/test: Add rbac tests to authorizer_test.cc

c5cd3e5

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

security/test: role_store_bench cleanups

32412e8

Single invocations of various role_store calls are more interesting than many in a loop. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

oleiman force-pushed the rbac/authZ branch from c1779e4 to 0e64f45 Compare March 20, 2024 23:24

oleiman requested a review from rockwotj March 21, 2024 04:14

BenPope approved these changes Mar 21, 2024

View reviewed changes

rockwotj approved these changes Mar 21, 2024

View reviewed changes

oleiman merged commit ae4d3b9 into redpanda-data:dev Mar 21, 2024
16 checks passed

dotnwat reviewed Mar 21, 2024

View reviewed changes

	ss::chunked_fifo<acl_principal_view> maybe_roles;
	chunked_vector<acl_principal_view> maybe_roles;

		"Role principal should be non-null and have 'role' type if "
		"present");

Role based authorization #17025

Role based authorization #17025

Conversation

oleiman commented Mar 13, 2024 • edited

Backports Required

Release Notes

oleiman commented Mar 13, 2024

oleiman commented Mar 13, 2024

This comment was marked as outdated.

oleiman commented Mar 14, 2024

vbotbuildovich commented Mar 14, 2024 • edited

oleiman commented Mar 15, 2024

oleiman commented Mar 15, 2024

dotnwat 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

oleiman Mar 15, 2024 • edited

Choose a reason for hiding this comment

oleiman Mar 15, 2024 • edited

Choose a reason for hiding this comment

oleiman commented Mar 15, 2024

oleiman commented Mar 15, 2024

oleiman commented Mar 15, 2024

BenPope commented Mar 15, 2024

oleiman commented Mar 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleiman commented Mar 18, 2024

oleiman commented Mar 18, 2024

rockwotj left a comment

Choose a reason for hiding this comment

oleiman commented Mar 20, 2024

oleiman commented Mar 20, 2024

BenPope left a comment

Choose a reason for hiding this comment

rockwotj 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleiman commented Mar 13, 2024 •

edited

vbotbuildovich commented Mar 14, 2024 •

edited

BenPope Mar 15, 2024 •

edited

oleiman Mar 15, 2024 •

edited

oleiman Mar 15, 2024 •

edited