CASSSIDECAR-161: Add RBAC Authorization support in Sidecar #165

sarankk · 2024-12-16T22:11:09Z

No description provided.

nvharikrishna

Reviewed it partially. Will continue to review.

server-common/src/main/java/org/apache/cassandra/sidecar/db/schema/AbstractSchema.java

server/src/main/java/org/apache/cassandra/sidecar/acl/AuthCache.java

nvharikrishna · 2024-12-19T18:27:13Z

server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/Action.java

+     */
+    default Authorization toAuthorization()
+    {
+        return toAuthorization(null);


What does null as a resource represent? All resources or no resources?

When resource is not set, resource matching is not done. So all resources. But in sidecar, we require resource set for all endpoints. This method is to get the Authorization and set resource at endpoint level like here https://github.com/sarankk/cassandra-sidecar/blob/1d808bc2f2b5c4569099f13954f38019713d09be/server/src/main/java/org/apache/cassandra/sidecar/routes/snapshots/CreateSnapshotHandler.java#L73

The code in the specified link is using the Authorization toAuthorization(String resource); method. It it used only in test cases. Is it required? If yes, then it is good to have expected behavior in the documentation comment.

server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/Action.java

server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/AllowAllAuthorization.java

nvharikrishna · 2024-12-19T19:00:08Z

server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/AdminIdentityResolver.java

+        String role = identityToRoleCache.get(identity);
+        Boolean isSuperuser = superUserCache.isSuperUser(role);
+        // Cassandra superusers have admin privileges. For fallback, sidecar configured admin identities are honored
+        return isSuperuser || adminIdentities.contains(identity);


If the superUserCache doesn't have the given role, then it returns null, right? Unboxing null Boolean result NPE.

How does SuperUserCache behaves if the role is null?

Good catch, will fix this. When role is null we should throw, for RBAC we need role to retrieve authorizations.

.../main/java/org/apache/cassandra/sidecar/acl/authorization/AllowAllAuthorizationProvider.java

bbotella · 2024-12-19T21:29:57Z

client-common/src/main/java/org/apache/cassandra/sidecar/common/ApiEndpointsV1.java

-    public static final String TABLE_PATH_PARAM = ":table";
+    public static final String KEYSPACE = "keyspace";
+    public static final String TABLE = "table";
+    public static final String KEYSPACE_PATH_PARAM = ":" + KEYSPACE;


If this is needed, what about extracting the : to its own static variable as well? And maybe refactoring the rest of params?

If we do that we might have to touch all the other path params. I didn't want to do that.

...n/java/org/apache/cassandra/sidecar/common/server/exceptions/SchemaUnavailableException.java

server-common/src/main/java/org/apache/cassandra/sidecar/db/schema/AbstractSchema.java

server/src/main/java/org/apache/cassandra/sidecar/acl/AuthCache.java

bbotella · 2024-12-19T21:42:57Z

server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/Action.java

+     */
+    static Action fromName(@NotNull String name)
+    {
+        boolean isWildCard = name.equals(WILDCARD_TOKEN) || name.contains(WILDCARD_PART_DIVIDER_TOKEN);


Don't we want to support partial wildcards? Things like: foo*?

Also, don't quite love that the static variables are part of WildcardAction. If we are going to share them between an interface and one of its implementations, we should extract them to somewhere else.

foo* is not supported instead we support foo:*. Sure I can add this method to a helper class instead

server/src/main/java/org/apache/cassandra/sidecar/db/SystemAuthDatabaseAccessor.java

server/src/main/java/org/apache/cassandra/sidecar/routes/AccessProtectedRouteBuilder.java

server/src/main/java/org/apache/cassandra/sidecar/routes/RingHandler.java

bbotella · 2024-12-19T22:56:13Z

...test/integration/org/apache/cassandra/sidecar/acl/RoleBasedAuthorizationIntegrationTest.java

Do we have tests for "deny access" scenarios?

we do cover them inside RoleBasedAuthorizationIntegrationTest

server/src/test/java/org/apache/cassandra/sidecar/acl/authorization/ActionTest.java

nvharikrishna · 2024-12-20T18:35:01Z

server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/CassandraActions.java

+/**
+ * Cassandra actions allowed.
+ */
+public class CassandraActions


Cassandra calls these things as permissions. https://cassandra.apache.org/doc/5.0/cassandra/developing/cql/security.html#cql-permissions. Can it be renamed as CassandraPermissions?

Sure, since we call SidecarActions named it such for consistency

nvharikrishna · 2024-12-20T18:43:12Z

server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/Action.java

+import static org.apache.cassandra.sidecar.acl.authorization.WildcardAction.WILDCARD_TOKEN;
+
+/**
+ * Represents an action that can be granted to a user on a resource or across resources.


Generally it is the permissions granted on a resource but not action. Actions that can be performed on a resource are defined by the resource itself. Granting permission is the authorization to do something. I think you mean it to be a Permission rather than an Action. If yes, can this class be renamed as Permission?

Correct, here I am just breaking down the permission string into 2 parts <action_allowed>:<action_target> and this entire permission string is allowed for a resource. For e.g. UPLOAD:SSTABLE permission allows UPLOAD action on SSTABLE target for resource data/sample_keyspace/sample_table

If you mean targets of a resource should be combined with the resource itself, it makes sense to me. But then we would be storing resource string differently in Sidecar's role_permissions_v1 table, currently in Cassandra's table we only have data/<ks>/<table>, they do not have permissions listed for snapshot, SSTables

Based on the observation from the StandardAction and WildcardAction which are constructing PermissionBasedAuthorization & WildcardPermissionBasedAuthorization which are accepting permission as the constructor parameter. For consistency I feel it is apt to call it as Permission.

nvharikrishna · 2024-12-20T18:53:55Z

...er/src/main/java/org/apache/cassandra/sidecar/acl/authorization/RoleAuthorizationsCache.java

+
+    public Set<Authorization> getAuthorizations(String role)
+    {
+        return get(UNIQUE_CACHE_ENTRY).get(role);


get(UNIQUE_CACHE_ENTRY) - trying to fetch entry from cache. I think we need a null check here (in case the cache fails to load entries).

Good catch thanks, updated here

...er/src/main/java/org/apache/cassandra/sidecar/acl/authorization/RoleAuthorizationsCache.java

server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/StandardAction.java

sarankk · 2024-12-20T20:05:05Z

Thanks for the review @nvharikrishna and @bbotella appreciate it, addressed your comments.

Co-authored-by: Raymond Welgosh <[email protected]> Co-authored-by: Saranya Krishnakumar <[email protected]>

nvharikrishna

Few more comments.

nvharikrishna · 2024-12-24T13:44:43Z

server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/Action.java

+     */
+    default Authorization toAuthorization()
+    {
+        return toAuthorization(null);


The code in the specified link is using the Authorization toAuthorization(String resource); method. It it used only in test cases. Is it required? If yes, then it is good to have expected behavior in the documentation comment.

server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/Action.java

nvharikrishna · 2024-12-24T13:55:56Z

server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/Action.java

+import static org.apache.cassandra.sidecar.acl.authorization.WildcardAction.WILDCARD_TOKEN;
+
+/**
+ * Represents an action that can be granted to a user on a resource or across resources.


Based on the observation from the StandardAction and WildcardAction which are constructing PermissionBasedAuthorization & WildcardPermissionBasedAuthorization which are accepting permission as the constructor parameter. For consistency I feel it is apt to call it as Permission.

server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/AdminIdentityResolver.java

server/src/main/java/org/apache/cassandra/sidecar/db/schema/SystemAuthSchema.java

nvharikrishna · 2024-12-25T18:36:54Z

server/src/main/java/org/apache/cassandra/sidecar/routes/AccessProtectedRouteBuilder.java

+
+            route.handler(authorizationHandler);
+        }
+        handlers.forEach(route::handler);


It doesn't allow to add a blocking handler. Can it allow adding a blocking handler?

what blocking handler are we talking about? Router in vertx allows taking in only these Handler<RoutingContext>

We generally do not use blockingHandler to register handler. Dispatching blocking actions to executor pool provides better flexibility.

server/src/main/java/org/apache/cassandra/sidecar/routes/AccessProtectedRouteBuilder.java

nvharikrishna · 2024-12-25T18:45:51Z

server/src/main/java/org/apache/cassandra/sidecar/routes/AccessProtectedRouteBuilder.java

+    private BiConsumer<RoutingContext, AuthorizationContext> routeGenericVariableConsumer()
+    {
+        return (routingCtx, authZContext) -> {
+            if (routingCtx.pathParams().containsKey(KEYSPACE))


Any reason to select/pass only KEYSPACE & TABLE? What if we copy all path params?

At the moment we are only checking authorizations for data resource, we can add them later when they are required I feel.

bbotella

Thanks a lot for addressing so many comments! Just a couple of nits, but it's a +1 from my end (nb).

bbotella · 2025-01-03T00:46:04Z

...er/src/main/java/org/apache/cassandra/sidecar/acl/authorization/RoleAuthorizationsCache.java

+        super(NAME,
+              vertx,
+              executorPools,
+              k -> loadAuthorizations(systemAuthDatabaseAccessor,


I was thinking on some kind of lazy load wrapper, but on second thoughts, that's probably overkill as this is not going to be executed that often.

bbotella · 2025-01-03T15:34:33Z

...test/integration/org/apache/cassandra/sidecar/acl/RoleBasedAuthorizationIntegrationTest.java

+    }
+
+    private void verifyAccess(VertxTestContext context, Checkpoint checkpoint, HttpMethod method,
+                              String testRoute, Path clientKeystorePath, boolean expectForbidden)


Nit: Please consider changing the flag meaning to a expectAllowed. I know this may be personal preference, but in my head, passing a true "should" mean that the access should be allowed.

We use this method to check both access allowed cases and access denied cases. Hence named it verifyAccess

yifan-c

submit what I have so far.

yifan-c · 2025-01-03T22:29:52Z

...er/src/main/java/org/apache/cassandra/sidecar/acl/authorization/RoleAuthorizationsCache.java

+    {
+        // when entries in cache are not found, null is returned. We can not add null in Map
+        Map<String, Set<Authorization>>  roleAuthorizations
+        = Optional.ofNullable(systemAuthDatabaseAccessor.getAllRolesAndPermissions()).orElse(Collections.emptyMap());


systemAuthDatabaseAccessor.getAllRolesAndPermissions() does not return null. Optional.ofNullable is unnecessary.

yifan-c · 2025-01-03T22:32:35Z

...er/src/main/java/org/apache/cassandra/sidecar/acl/authorization/RoleAuthorizationsCache.java

+        if (isSidecarSchemaEnabled)
+        {
+            Map<String, Set<Authorization>> sidecarAuthorizations
+            = Optional.ofNullable(sidecarPermissionsDatabaseAccessor.getAllRolesAndPermissions()).orElse(Collections.emptyMap());


Similarly, Optional.ofNullable is unnecessary.

yifan-c · 2025-01-03T22:35:06Z

...er/src/main/java/org/apache/cassandra/sidecar/acl/authorization/RoleAuthorizationsCache.java

+        Map<String, Set<Authorization>>  roleAuthorizations
+        = Optional.ofNullable(systemAuthDatabaseAccessor.getAllRolesAndPermissions()).orElse(Collections.emptyMap());
+
+        if (isSidecarSchemaEnabled)


I think this condition is incomplete. You want to check org.apache.cassandra.sidecar.db.schema.SidecarSchema#isInitialized instead, before querying from the sidecar table.

yifan-c · 2025-01-03T22:41:07Z

...er/src/main/java/org/apache/cassandra/sidecar/acl/authorization/RoleAuthorizationsCache.java

+        super(NAME,
+              vertx,
+              executorPools,
+              k -> loadAuthorizations(systemAuthDatabaseAccessor,


Why the input k from the loadFunction is unused? It looks like that both loadFunction and bulkLoadFunction do the bulk load of all authorization. The same Set<Authorization> is duplicated in the cache.

yifan-c · 2025-01-03T22:45:01Z

...main/java/org/apache/cassandra/sidecar/acl/authorization/RoleBasedAuthorizationProvider.java

+
+        if (identities.isEmpty())
+        {
+            return Future.failedFuture(new HttpException(HttpResponseStatus.FORBIDDEN.code(),


I agree with Hari. HttpException is not the proper exception for this domain. Ideally, HttpException is only thrown from request handlers.
A domain specific exception is preferable here.

yifan-c · 2025-01-04T01:06:45Z

server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/SidecarPermissions.java

+public class SidecarPermissions
+{
+    // cassandra cluster related permissions
+    public static final Permission VIEW_CLUSTER = new WildcardPermission("VIEW:CLUSTER");


VIEW:CLUSTER reads very general and vague. Can you make it more precise?
What does "CLUSTER" mean here?

yifan-c · 2025-01-04T01:09:20Z

server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/SidecarPermissions.java

+    // SSTable related permissions
+    public static final Permission UPLOAD_SSTABLE = new WildcardPermission("UPLOAD:SSTABLE");
+    public static final Permission IMPORT_SSTABLE = new WildcardPermission("IMPORT:SSTABLE");
+    public static final Permission STREAM_SSTABLE = new WildcardPermission("STREAM:SSTABLE");


What about using READ to replace both VIEW and STREAM? Do we need to distinguish between those 2 read operations?

yifan-c · 2025-01-04T01:10:49Z

server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/SidecarPermissions.java

+    public static final Permission UPDATE_RESTORE_JOB = new WildcardPermission("UPDATE:RESTORE_JOB");
+    public static final Permission ABORT_RESTORE_JOB = new WildcardPermission("ABORT:RESTORE_JOB");


How about EDIT instead of UPDATE and DELETE instead of ABORT?

will change to DELETE, I wanted to reuse action names if possible e.g. UPDATE:SSTABLE any reason for preferring EDIT?

The action names are reused. The suggestion is applied to all actions.

I was thinking from the perspective of HTTP methods. Given that it is web service, all interactions are performed via the REST APIs (i.e. http method + resource). I think we should model the permissions from it.

DELETE is directly from the delete method. EDIT is for patch method (as it partially update the entity). Then, UPDATE is for put method.

Makes sense, will update it.

yifan-c · 2025-01-04T01:11:53Z

server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/SidecarPermissions.java

+    // cassandra data related actions
+    public static final Permission VIEW_SCHEMA = new WildcardPermission("VIEW:SCHEMA");
+    public static final Permission VIEW_TOPOLOGY = new WildcardPermission("VIEW:TOPOLOGY");


SCHEMA and TOPOLOGY is different from CLUSTER? (I do not like CLUSTER, but still curious about the difference here)

yifan-c · 2025-01-04T01:12:29Z

server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/SidecarPermissions.java

+    public static final Permission VIEW_CDC = new WildcardPermission("VIEW:CDC");
+
+    // sidecar operation related permissions
+    public static final Permission VIEW_TASKS = new WildcardPermission("VIEW:TASKS");


TASKS is too general. It is OPERATIONAL_JOBS

yifan-c

Good stuff.
Besides the inline comments, I do have one more question. How do operators configures the permissions in the sidecar table?

yifan-c · 2025-01-04T01:54:28Z

server/src/main/java/org/apache/cassandra/sidecar/routes/KeyspaceRingHandler.java

+ * A handler that provides ring information for a specific keyspace for the Cassandra cluster
+ */
+@Singleton
+public class KeyspaceRingHandler extends AbstractHandler<Name> implements AccessProtected


Can you either drop this class and only have RingHandler? or extend from RingHandler to remove code duplication?

yifan-c · 2025-01-04T01:55:05Z

server/src/main/java/org/apache/cassandra/sidecar/routes/KeyspaceSchemaHandler.java

+ * The {@link KeyspaceSchemaHandler} class handles schema request for a keyspace
+ */
+@Singleton
+public class KeyspaceSchemaHandler extends AbstractHandler<Name> implements AccessProtected


Same, please drop this class or extend from SchemaHandler.

yifan-c · 2025-01-04T17:03:23Z

server/src/main/java/org/apache/cassandra/sidecar/routes/KeyspaceRingHandler.java

+    public Set<Authorization> requiredAuthorizations()
+    {
+        String resource = VariableAwareResource.DATA_WITH_KEYSPACE.resource();
+        return ImmutableSet.of(SidecarPermissions.VIEW_CLUSTER.toAuthorization(resource));
+    }


The method seems to be the only reason that this class exists. Since the requiredAuthorizations is configured statically, maybe extending from RingHandler and only overriding requiredAuthorizations is a more viable option.

I have already tried it, it is not possible. since input we take is different between 2 classes Name vs Void

yifan-c · 2025-01-04T17:09:32Z

server/src/main/java/org/apache/cassandra/sidecar/routes/restore/RestoreJobProgressHandler.java

+        String resource = VariableAwareResource.DATA_WITH_KEYSPACE_TABLE.resource();
+        Authorization createRestore = SidecarActions.CREATE_RESTORE.toAuthorization(resource);
+        Authorization viewRestore = SidecarActions.VIEW_RESTORE.toAuthorization(resource);
+        OrAuthorization createOrView


Is there any overlap between CREATE and VIEW? For this endpoint, I think only view is required. What is the reasoning of allowing a user with CREATE only permission to view the progress?

yifan-c · 2025-01-04T17:10:01Z

server/src/main/java/org/apache/cassandra/sidecar/routes/restore/RestoreJobSummaryHandler.java

+        Authorization viewRestore = SidecarPermissions.VIEW_RESTORE_JOB.toAuthorization(resource);
+        OrAuthorization createOrView
+        = new OrAuthorizationImpl().addAuthorization(createRestore).addAuthorization(viewRestore);
+        return ImmutableSet.of(createOrView);


Same, why allow create?

I thought if someone has create permission for restore job, then they should be able to view summary. I can remove create from update handler but for view it makes sense?

yifan-c · 2025-01-04T17:15:44Z

server/src/test/java/org/apache/cassandra/sidecar/server/ServerTest.java

+    void unrecognizedAuthorizationProviderSet()
+    {
+        assertThatThrownBy(() -> configureServer("config/sidecar_unrecognized_authorizer.yaml"))
+        .hasCauseInstanceOf(RuntimeException.class)


can it throw a dedicated exception instead of RuntimeException?

nvharikrishna reviewed Dec 19, 2024

View reviewed changes

bbotella reviewed Dec 19, 2024

View reviewed changes

nvharikrishna reviewed Dec 20, 2024

View reviewed changes

sarankk force-pushed the authorization branch from 3d0bf1c to bd34d9e Compare December 20, 2024 22:16

Add authorization

d88b2db

Co-authored-by: Raymond Welgosh <[email protected]> Co-authored-by: Saranya Krishnakumar <[email protected]>

sarankk force-pushed the authorization branch from bd34d9e to d88b2db Compare December 20, 2024 22:30

nvharikrishna reviewed Dec 25, 2024

View reviewed changes

sarankk added 2 commits January 2, 2025 14:07

Address Hari's comments

c69184b

Add deleted file

09b4ea1

sarankk requested review from nvharikrishna and bbotella January 2, 2025 23:27

bbotella approved these changes Jan 3, 2025

View reviewed changes

Rename to CassandraPermissions

0680b04

yifan-c reviewed Jan 4, 2025

View reviewed changes

		public static final Permission UPDATE_RESTORE_JOB = new WildcardPermission("UPDATE:RESTORE_JOB");
		public static final Permission ABORT_RESTORE_JOB = new WildcardPermission("ABORT:RESTORE_JOB");

CASSSIDECAR-161: Add RBAC Authorization support in Sidecar #165

Are you sure you want to change the base?

CASSSIDECAR-161: Add RBAC Authorization support in Sidecar #165

Conversation

sarankk commented Dec 16, 2024

nvharikrishna 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

sarankk Dec 19, 2024 • edited Loading

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

sarankk Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarankk Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarankk Dec 20, 2024 • edited Loading

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

sarankk commented Dec 20, 2024

nvharikrishna 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

nvharikrishna Dec 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbotella 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

yifan-c 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

yifan-c 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

sarankk Dec 19, 2024 •

edited

Loading

sarankk Dec 19, 2024 •

edited

Loading

sarankk Dec 20, 2024 •

edited

Loading

sarankk Dec 20, 2024 •

edited

Loading

nvharikrishna Dec 25, 2024 •

edited

Loading