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

[#5336] feat(auth-ranger): Remove MANAGED_BY_GRAVITINO limit and compatible for existing ranger policy #5629

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

theoryxu
Copy link
Contributor

@theoryxu theoryxu commented Nov 20, 2024

What changes were proposed in this pull request?

Many clients and users have used Ranger for a while. Gravitino should be compatible with these cases.

There are some principles Gravitino needs to follow when it pushes down policies:

  1. Gravitino can't modify existing policy names because users may have their own name rules.
  2. Gravitino and users could share the same policy and not disturb each other for the same resource.

For the target, this PR includes the following changes:

  1. wildcardSearchPolies removes the MANAGED_BY_GRAVITINO filter.
  2. Gravitino managed role name add the prefix GRAVITINO_.
  3. Using Gravitino Managed role to identify and operate policy items.

Despite doing these, users should be cautious about directly managing the ranger policy. There are some restricts:

  1. Don't directly rename Gravitino-managed policies.
  2. Don't directly modify policy resources in the policy that have Gravitino Managed roles.
  3. Don't directly modify policy items that have Gravitino Managed roles.

Why are the changes needed?

Fix: #5336

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

Added ITs

@xunliu
Copy link
Member

xunliu commented Nov 20, 2024

hi @theoryxu Thank you for your attention to this problem

The problem now is that Gravitino will only maintain a Ranger Policy with the MANAGED_BY_GRAVITINO label, but if a user already has a Ranger service, that can lead to conflicts.

  1. Gravitino's Policy has its own set of management rules. May conflict with the user randomly set;
  2. Therefore, only a Ranger Policy with the MANAGED_BY_GRAVITINO label is maintained.

But that's a pretty big limitation.

  1. There is only one Ranger policy for each resource (db1.tab1).
  2. If a user's old ranger service already has the db1.tab1 policy, but this policy may not conform to Gravitino's authority specification, and there may be problems if Gravitino is asked to directly update this Policy.
    3, so now the Gravitino through RangerHelper.WildcardSearchPolies() function will only find the policy with MANAGED_BY_GRAVITINO label.
  3. If the old ranger already has this policy, but Gravitino cannot operate it, there will be problems.

@xunliu xunliu changed the title [#5336]feat(auth-ranger): Remove MANAGED_BY_GRAVITINO limit and compatible for existing ranger policy [#5336] feat(auth-ranger): Remove MANAGED_BY_GRAVITINO limit and compatible for existing ranger policy Nov 20, 2024
@xunliu xunliu self-requested a review November 20, 2024 10:42
Comment on lines 59 to +66
public static final String GRAVITINO_METALAKE_OWNER_ROLE = "GRAVITINO_METALAKE_OWNER_ROLE";
public static final String GRAVITINO_CATALOG_OWNER_ROLE = "GRAVITINO_CATALOG_OWNER_ROLE";

// marking owner policy items
public static final String GRAVITINO_PLACEHOLDER_OWNER_ROLE = "GRAVITINO_PLACEHOLDER_OWNER_ROLE";

public static final String GRAVITINO_ROLE_PREFIX = "GRAVITINO_";

Copy link
Member

@xunliu xunliu Nov 26, 2024

Choose a reason for hiding this comment

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

I think it's better to change these code

  public static final String GRAVITINO_ROLE_PREFIX = "GRAVITINO_";
  public static final String GRAVITINO_METALAKE_OWNER_ROLE = GRAVITINO_ROLE_PREFIX + "METALAKE_OWNER_ROLE";
  public static final String GRAVITINO_CATALOG_OWNER_ROLE = GRAVITINO_ROLE_PREFIX + "CATALOG_OWNER_ROLE";
  // marking owner policy items
  public static final String GRAVITINO_OWNER_ROLE = GRAVITINO_ROLE_PREFIX + "OWNER_ROLE";

The GRAVITINO_PLACEHOLDER_OWNER_ROLE is too long.

I think we need to add a description(GRAVITINO_ROLE_PREFIX, GRAVITINO_METALAKE_OWNER_ROLE, GRAVITINO_CATALOG_OWNER_ROLE and GRAVITINO_OWNER_ROLE) in the https://github.com/apache/gravitino/blob/main/docs/security/authorization-pushdown.md

try {
policy.setPolicyItems(
policy.getPolicyItems().stream()
.filter(i -> !isGravitinoManagedPolicyItemAccess(i))
Copy link
Member

Choose a reason for hiding this comment

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

I think better to change i to item.

"The role name should be GRAVITINO_METALAKE_OWNER_ROLE or GRAVITINO_CATALOG_OWNER_ROLE");
|| roleName.equalsIgnoreCase(GRAVITINO_CATALOG_OWNER_ROLE)
|| roleName.equalsIgnoreCase(GRAVITINO_PLACEHOLDER_OWNER_ROLE),
"The role name should be GRAVITINO_METALAKE_OWNER_ROLE or GRAVITINO_CATALOG_OWNER_ROLE or GRAVITINO_PLACEHOLDER_OWNER_ROLE");
Copy link
Member

Choose a reason for hiding this comment

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

I think better change

"The role name should be GRAVITINO_METALAKE_OWNER_ROLE or GRAVITINO_CATALOG_OWNER_ROLE or GRAVITINO_PLACEHOLDER_OWNER_ROLE"

to

String.format("The role name should be %s or %s or %s", GRAVITINO_METALAKE_OWNER_ROLE, GRAVITINO_CATALOG_OWNER_ROLE, GRAVITINO_PLACEHOLDER_OWNER_ROLE)

@@ -376,6 +425,8 @@ protected void updatePolicyOwner(RangerPolicy policy, Owner preOwner, Owner newO
} else {
policyItem.getGroups().add(newOwner.name());
}
// mark the policy item is created by Gravitino
policyItem.getRoles().add(GRAVITINO_PLACEHOLDER_OWNER_ROLE);
Copy link
Member

@xunliu xunliu Nov 26, 2024

Choose a reason for hiding this comment

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

Maybe we need to add judgment?

if (!policyItem. getRoles().contains(GRAVITINO_PLACEHOLDER_OWNER_ROLE)) {
  policyItem.getRoles().add(GRAVITINO_PLACEHOLDER_OWNER_ROLE);
}

Comment on lines +509 to 511
if (!policyItem.getRoles().contains(generateGravitinoRoleName(ownerRoleName))) {
policyItem.getRoles().add(generateGravitinoRoleName(ownerRoleName));
}
Copy link
Member

Choose a reason for hiding this comment

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

I saw many place need add role into policy item.
I think maybe we need to add a function to add a rule in the policyItem.

AddRoleToPolicyItemIfNoExists(PolicyItem policyItem, String roleName)
{
         String gravitinoRoleName = generateGravitinoRoleName(roleName);
         if (!policyItem.getRoles().contains(gravitinoRoleName) {
            policyItem.getRoles().add(gravitinoRoleName);
          }
}

Assertions.assertTrue(
policyItems.stream()
.anyMatch(
i ->
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to change i to item.

Assertions.assertTrue(
policyItems.stream()
.anyMatch(
i ->
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to change i to item.

Assertions.assertFalse(
policyItems.stream()
.anyMatch(
i ->
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to change i to item.

Assertions.assertTrue(
policyItems.stream()
.anyMatch(
i ->
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to change i to item.

List<RangerPolicy.RangerPolicyItem> items, String roleName) {
return items.stream()
.anyMatch(
i ->
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to change i to item.

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.

[Subtask] Remove MANAGED_BY_GRAVITINO limit
3 participants