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

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

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

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

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

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

...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

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

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

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

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

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

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

...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.

nvharikrishna

Few more comments.

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

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

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

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

bbotella

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

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

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

yifan-c

submit what I have so far.

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

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

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

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?

Should we have READ and STREAM. We can use read for listing snapshots, reading topology, listing restore job in place of VIEW to align with http methods. STREAM feels like a separate action, hence distinction?

One for use case for separating READ and STREAM . We have to allow permission for listing cdc segments and streaming them, currently we have READ:CDC and STREAM:CDC separately.

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

yifan-c

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

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

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

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

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

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

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

sarankk · 2025-01-07T22:06:05Z

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

Currently operators will have to directly insert into sidecar role_permissions_v1 table for granting sidecar related permissions and use GRANT cql grammer for granting Cassandra related permissions

nvharikrishna

Few minor comments, otherwise, I'm good from my side.

nvharikrishna · 2025-01-12T09:43:31Z

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

+
+/**
+ * sidecar_internal.role_permissions_v1 table holds custom sidecar permissions that are not stored in Cassandra.
+ * Permissions are stored against resource.


Permissions are stored against both role and resource right (both role and resource are part of primary key)?

Yes, I ported over the same schema we have in Cassandra

nvharikrishna · 2025-01-12T09:48:28Z

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

+        return getAllRolesAndPermissions;
+    }
+
+    private static class CqlLiterals


This private static class is holding only one static method. Any reason to have this class? Can the static method of this class can be part of its parent?

We have followed this approach in other TableSchema classes, I followed pattern for consistency

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

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

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

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

nvharikrishna · 2025-01-12T17:33:40Z

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

+    public Set<Authorization> requiredAuthorizations()
+    {
+        List<String> eligibleResources = VariableAwareResource.DATA_WITH_KEYSPACE.expandedResources();
+        return Collections.singleton(SidecarPermissions.READ_RING.toAuthorization(eligibleResources));


Doesn't it need authorization/permission for keyspace?

Yes it does, in Cassandra if user has permission for data resource then they get access to all keyspaces under data. Hence we are expanding resources to allow access when user has access to a wider resource.

nvharikrishna · 2025-01-12T17:51:16Z

server/src/test/java/org/apache/cassandra/sidecar/routes/AccessProtectedRouteBuilderTest.java

+/**
+ * Test for {@link AccessProtectedRouteBuilder}
+ */
+public class AccessProtectedRouteBuilderTest


Can you add 1) a positive test and 2) build AcessProtectedRouteBuilder without any AccessProtected handler?

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

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

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

frankgh

Added a couple more comments that we need to address.

frankgh · 2025-01-03T17:31:55Z

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

@@ -16,21 +16,21 @@
 * limitations under the License.
 */

-package org.apache.cassandra.sidecar.exceptions;
+package org.apache.cassandra.sidecar.common.server.exceptions;


let's avoid moving classes to another package.

I don't see the exception there I see it under org.apache.cassandra.sidecar.exceptions

oh looks like this is a old comment

frankgh · 2025-01-11T07:10:18Z

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

@@ -1,101 +1,38 @@
-/*


we should preserve the license header.

Not understanding

again looks like an old comment.

frankgh · 2025-01-14T04:28:12Z

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

+        Preconditions.checkArgument(router != null, "Router must be set");
+        Preconditions.checkArgument(method != null, "Http method must be set");
+        Preconditions.checkArgument(endpoint != null && !endpoint.isEmpty(), "Endpoint must be set");


NIT, use Objects.requireNonNull instead

I check for empty string too endpoint != null && !endpoint.isEmpty()

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

...r/src/main/java/org/apache/cassandra/sidecar/routes/sstableuploads/SSTableImportHandler.java

frankgh · 2025-01-18T00:50:23Z

server/src/test/integration/org/apache/cassandra/sidecar/testing/IntegrationTestModule.java

+        CassandraInputValidationConfiguration inputValidationConfiguration
+        = new CassandraInputValidationConfigurationImpl(DEFAULT_FORBIDDEN_KEYSPACES,
+                                                        // some tests generate table folders with -
+                                                        "[a-zA-Z][a-zA-Z0-9_-]{0,47}",


this is not correct. For the stream sstable from snapshot endpoint we will get a table from the path that has the tableId. We should remove tableId from the path to be able to correctly validate. Otherwise my suspicion is that you will always get unauthorized for the table, because it won't match the table name

Good catch, will update this

…ssion changed

frankgh

+1 thanks for this work!

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 2 times, most recently from bd34d9e to d88b2db Compare December 20, 2024 22:30

nvharikrishna reviewed Dec 25, 2024

View reviewed changes

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

bbotella approved these changes Jan 3, 2025

View reviewed changes

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

...test/integration/org/apache/cassandra/sidecar/acl/RoleBasedAuthorizationIntegrationTest.java Show resolved Hide resolved

yifan-c reviewed Jan 4, 2025

View reviewed changes

This was referenced Jan 8, 2025

CASSANDRASC-141: Mutual TLS Authentication and Authorization to Sidecar #131

Closed

CASSSIDECAR-170: Reprepare statements when sidecar reconnects to Cass… #156

Closed

sarankk requested a review from yifan-c January 10, 2025 05:46

nvharikrishna reviewed Jan 12, 2025

View reviewed changes

yifan-c reviewed Jan 15, 2025

View reviewed changes

server/src/main/java/org/apache/cassandra/sidecar/routes/AccessProtectedRouteBuilder.java Outdated Show resolved Hide resolved

frankgh force-pushed the authorization branch from 58fa80f to f4af6be Compare January 16, 2025 00:56

sarankk and others added 11 commits January 15, 2025 17:38

Add authorization

a89edad

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

Address Hari's comments

5b26f13

Add deleted file

7478904

Rename to CassandraPermissions

2306b34

Start cluster with auth settings to avoid repreparing statements

59c9f18

Remove reprepare changes

fe6cb5c

Pull over changes for avoiding repreparing statements

dc15233

Address Yifan's comments

e2d0641

Address Yifan's comments

4d48900

Fix tests

bd50ca5

Review

aa935e7

sarankk added 2 commits January 16, 2025 18:35

Fix tests

2ff5760

Fix test

d754c6b

sarankk requested review from yifan-c and nvharikrishna January 17, 2025 04:50

sarankk added 4 commits January 17, 2025 10:40

Rename OPERATIONAL_JOB to TASKS

2052e9d

Address review

526d87f

Rename wildcard permission to domain aware permission

63e64e7

Change to asserting latch complete in test

38307a8

yifan-c approved these changes Jan 17, 2025

View reviewed changes

frankgh reviewed Jan 18, 2025

View reviewed changes

sarankk and others added 10 commits January 17, 2025 18:55

Update

0de49c2

Fix imports

a60f579

Do not always remove the tableId

53fd530

Restore setting qualified table name in the context

e12dc4e

Add a extra check and change test keyspace since schema handler permi…

907c8dd

…ssion changed

Remove check

3c7a840

Increase cache refresh time

b705dc7

Test fails on circle ci

9566607

Heavy tag not required

081bbeb

Fix style

813ff1c

frankgh approved these changes Jan 19, 2025

View reviewed changes

Increase test timeout

8fe8a6e

nvharikrishna approved these changes Jan 19, 2025

View reviewed changes

sarankk added 5 commits January 19, 2025 00:02

Stream snapshot after listing them, instead of reading from a fixed file

e7bd825

Increase timeout

514c259

Do not block thread

5cd058d

Update thread to not sleepp instead use vertx timer

99fe33f

Fix checkstyle

07ca61f

frankgh merged commit 5a19e34 into apache:trunk Jan 20, 2025

frankgh deleted the authorization branch January 20, 2025 00:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sarankk commented Dec 16, 2024

nvharikrishna left a comment

sarankk commented Dec 20, 2024

nvharikrishna left a comment

bbotella left a comment

yifan-c left a comment

yifan-c Jan 4, 2025

sarankk Jan 7, 2025

sarankk Jan 7, 2025

yifan-c left a comment

sarankk commented Jan 7, 2025

nvharikrishna left a comment

nvharikrishna Jan 12, 2025

sarankk Jan 13, 2025

nvharikrishna Jan 12, 2025

sarankk Jan 13, 2025

nvharikrishna Jan 12, 2025

sarankk Jan 13, 2025

nvharikrishna Jan 12, 2025

frankgh left a comment

frankgh Jan 3, 2025

sarankk Jan 18, 2025

sarankk Jan 18, 2025

frankgh Jan 11, 2025

sarankk Jan 18, 2025

sarankk Jan 18, 2025

frankgh Jan 14, 2025

sarankk Jan 18, 2025

frankgh Jan 18, 2025

sarankk Jan 18, 2025

frankgh left a comment

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

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

sarankk commented Dec 20, 2024

nvharikrishna left a comment

Choose a reason for hiding this comment

bbotella left a 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

yifan-c left a comment

Choose a reason for hiding this comment

sarankk commented Jan 7, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankgh 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

frankgh left a comment

Choose a reason for hiding this comment