Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update privilege model to support granting CatalogRole access to PrincipalRoles #361

Merged
merged 2 commits into from
Oct 9, 2024

Conversation

collado-mike
Copy link
Contributor

@collado-mike collado-mike commented Oct 9, 2024

Description

This updates the authorization model to allow catalog_admin roles to grant other CatalogRoles within their catalog to a PrincipalRole. This addresses #359 , in which it is stated that neither the catalog_admin or the service_admin roles alone are sufficient to grant catalog access to principal roles.

The fix is implemented by removing the requirement for PRINCIPAL_ROLE_MANAGE_GRANTS_FOR_GRANTEE privilege from the ASSIGN_CATALOG_ROLE_TO_PRINCIPAL_ROLE operation. Implicitly, this states that a user only needs privileges to manage the grants on the catalog as a securable, but does not need privilege to manage the grants on a PrincipalRole as a grantee.

A caller does require PRINCIPAL_ROLE_MANAGE_GRANTS_FOR_GRANTEE to revoke a privilege, which, admittedly, does make this operation a little bit lopsided. However, having the ability to create grants does not implicitly mean the user has the privilege to revoke grants for the same securable. For example, a service_admin may delegate catalog_admin to another user/role, but may wish to retain catalog_admin privileges. Without requiring PRINCIPAL_ROLE_MANAGE_GRANTS_FOR_GRANTEE, a catalog_admin could revoke grants from any PrincipalRole, even if the grants weren't originally created by the catalog_admin itself.

For consistency, I also removed the corresponding *_MANAGE_GRANTS_FOR_GRANTEE requirements for other ASSIGN* operations. So a user, having privilege to manage grants on a securable, can create a grant for that securable for any grantee without needing special privilege on that grantee. As an example, a user having CATALOG_MANAGE_ACCESS at a namespace level can create grants for any entity at that namespace or below and grant it to any CatalogRole in the catalog. The revoke authorization is also consistent with the ASSIGN_CATALOG_ROLE_TO_PRINCIPAL_ROLE operation.

I confirmed with tests that the catalog_admin and other roles cannot create grant records that cross catalogs (i.e., can't grant CREATE_TABLE privilege to a catalog role in another catalog).

Fixes #359

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • Documentation update
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Added tests to confirm that a service_admin can grant catalog_admin to another principal role and a principal with that role can create CatalogRoles and grant them to other PrincipalRoles. Also confirmed that callers that have the CATALOG_MANAGE_ACCESSS privilege can create grants on CatalogRoles within their own catalog, but across catalogs.

  • Test A
  • Test B

Test Configuration:

  • Hardware:
  • Toolchain:
  • SDK:

Checklist:

Please delete options that are not relevant.

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • If adding new functionality, I have discussed my implementation with the community using the linked GitHub issue

Copy link
Contributor

@dennishuo dennishuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM; I agree the ability to grant a privilege to a grantee in other systems usually only requires the grant-management-equivalent privilege on the securable being granted rather than requiring both ends, whereas revoke is a more privileged action with regards to the grantee so it makes sense to keep both ends for revoke.

Some suggestions on test comments - all the RBAC stuff is hard for newcomers to learn, so if they read the test cases to better understand, we could be more explicit about "why" a given grant is able to happen, especially since the privileges of catalog_admins are kind of hidden in the original setup of catalog_admins outside of the test case itself.

PrincipalRole principalRole2 = new PrincipalRole(principalRoleName2);
createPrincipalRole(principalRole2);

// create a catalog role and grant it manage_content privilege
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For people reading the test, we might want to expand this comment to make it clear that the new catalog role itself doesn't get any grant-management abilities from CATALOG_MANAGE_CONTENT, just so the reader knows that this grant here isn't related to whether or not the catalogAdmin can successfully grant this new catalogRole to mypr2.

Otherwise at first glance it's easy to read the test as line 1847 saying "the cataalog admin can grant the new catalog role... (because of the privilege grant we see right before the line?)"

catalogAdminToken,
Response.Status.CREATED);

// The catalog admin can grant the new catalog role to the mypr2 principal role
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would help to make this comment more explicit by adding ... because the catalog admin has CATALOG_MANAGE_ACCESS on the whole catalog, which transitively provides the required CATALOG_ROLE_MANAGE_GRANTS_ON_SECURABLE on all the CatalogRoles in the catalog.

public void testCatalogAdminGrantAndRevokeCatalogRoles() {
// Create a PrincipalRole and a new catalog. Grant the catalog_admin role to the new principal
// role
String principalRoleName = "mypr33";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just to avoid conflicts across tests? If that's true, we might want to harden this against multiple implementations of PolarisServiceImplIntegrationTest running. Can we just randomize it until we have better isolation across tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually did fix this more correctly by adding the logic to the teardown method. Previously things weren't getting deleted because the catalogs weren't empty, but now they're emptied before everything else gets deleted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds really good for protecting us across sequential runs, but I think there are still potentially issues with concurrent tests. Anyway, all sorts of tests have this issue so it's not urgent to address here.

Comment on lines +1808 to +1810
new AwsStorageConfigInfo(
"arn:aws:iam::012345678901:role/jdoe", StorageConfigInfo.StorageTypeEnum.S3))
.setProperties(new CatalogProperties("s3://bucket1/"))
Copy link
Contributor

@eric-maynard eric-maynard Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: If we're not actually using S3, I think we may as well use FILE

Copy link
Contributor

@eric-maynard eric-maynard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left some non-blocking comments on the tests

@collado-mike collado-mike enabled auto-merge (squash) October 9, 2024 20:58
@collado-mike collado-mike merged commit 5c1f9bb into apache:main Oct 9, 2024
4 checks passed
dennishuo added a commit to dennishuo/polaris that referenced this pull request Oct 10, 2024
Fix consistency of ASSIGN_PRINCIPAL_ROLE in line with apache#361

Test cases for:
assignPrincipalRoleToPrincipal
revokePrincipalRoleFromPrincipal
assignCatalogRoleToPrincipalRole
revokeCatalogRoleFromPrincipalRole
grantPrivilegeOnRootContainerToPrincipalRole
revokePrivilegeOnRootContainerFromPrincipalRole
grantPrivilegeOnCatalogToRole
revokePrivilegeOnCatalogFromRole
grantPrivilegeOnNamespaceToRole
revokePrivilegeOnNamespaceFromRole
grantPrivilegeOnTableToRole
revokePrivilegeOnTableFromRole
grantPrivilegeOnViewToRole
revokePrivilegeOnViewFromRole
eric-maynard pushed a commit that referenced this pull request Oct 14, 2024
…ns (#364)

Fix consistency of ASSIGN_PRINCIPAL_ROLE in line with #361

Test cases for:
assignPrincipalRoleToPrincipal
revokePrincipalRoleFromPrincipal
assignCatalogRoleToPrincipalRole
revokeCatalogRoleFromPrincipalRole
grantPrivilegeOnRootContainerToPrincipalRole
revokePrivilegeOnRootContainerFromPrincipalRole
grantPrivilegeOnCatalogToRole
revokePrivilegeOnCatalogFromRole
grantPrivilegeOnNamespaceToRole
revokePrivilegeOnNamespaceFromRole
grantPrivilegeOnTableToRole
revokePrivilegeOnTableFromRole
grantPrivilegeOnViewToRole
revokePrivilegeOnViewFromRole
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] CatalogAdmin cannot grant catalog roles to principal roles
4 participants