jerryshao commented on code in PR #13606:
URL: https://github.com/apache/gravitino/pull/13606#discussion_r4215594650
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -1554,16 +1656,140 @@ private void clearRolePoliciesOnCacheRemoval(long
roleId) {
roleId);
return;
}
- clearRolePoliciesWithoutLock(roleId);
+ if (Objects.equals(roleBeingReplaced, roleId)) {
+ clearRolePoliciesWithoutLock(roleId);
+ } else {
+ clearRolePoliciesAndRecordGeneration(roleId);
+ }
} finally {
rolePolicyLock.writeLock().unlock();
}
}
- private void clearRolePoliciesWithoutLock(long roleId) {
+ private boolean clearRolePoliciesWithoutLock(long roleId) {
String roleIdStr = String.valueOf(roleId);
- allowEnforcer.removeFilteredPolicy(0, roleIdStr);
- denyEnforcer.removeFilteredPolicy(0, roleIdStr);
+ boolean allowRemoved = allowEnforcer.removeFilteredPolicy(0, roleIdStr);
+ boolean denyRemoved = denyEnforcer.removeFilteredPolicy(0, roleIdStr);
+ return allowRemoved || denyRemoved;
+ }
+
+ /**
+ * Clears a role's policies that are not replaced in the same critical
section, and records the
+ * clear so that in-flight requests bound to the role reload it before their
next check. Only a
+ * clear that removed policies is recorded: clearing an already empty role
changes nothing a
+ * request could have observed, and recording it would make a deleted role
look unstable forever.
+ * Must be called under the write lock of {@link #rolePolicyLock}.
+ */
+ private void clearRolePoliciesAndRecordGeneration(long roleId) {
+ if (clearRolePoliciesWithoutLock(roleId)) {
+ recordRoleClearGeneration(roleId);
+ }
+ }
+
+ /**
+ * Records a new clear generation for a role, pruning the older half of the
generations when the
+ * map outgrows {@link #maxRoleClearGenerations}. Must be called under the
write lock of {@link
+ * #rolePolicyLock}, so that readers see the pruned entries and the raised
{@link
+ * #prunedRoleClearGeneration} together.
+ */
+ private void recordRoleClearGeneration(long roleId) {
+ roleClearGenerations.put(roleId,
rolePolicyGenerationCounter.incrementAndGet());
+ if (roleClearGenerations.size() <= maxRoleClearGenerations) {
+ return;
+ }
+ long[] generations =
+
roleClearGenerations.values().stream().mapToLong(Long::longValue).sorted().toArray();
+ long cutoff = generations[generations.length / 2];
+ roleClearGenerations.values().removeIf(generation -> generation <= cutoff);
+ prunedRoleClearGeneration = cutoff;
+ }
+
+ /**
+ * Evaluates one authorization check against role policies that are at least
as complete as the
+ * ones the request loaded.
+ *
+ * <p>A request loads its roles once, but each check releases {@link
#rolePolicyLock} before the
+ * next one starts. In between, a {@link #loadedRoles} TTL or size eviction,
or a privilege change
+ * on one of the roles, may clear that role's policies while the user's
grouping row stays. A
+ * later check would then evaluate without the role: it can deny what the
role allows, or allow
+ * what the role denies. So before each evaluation this method checks, under
the read lock,
+ * whether a bound role was cleared after the request's recorded generation.
If so, it reloads the
+ * cleared roles outside the lock, the same way a request's role load does,
and checks again.
+ *
+ * <p>Earlier checks of the request stay valid because they ran against
complete policies. This
+ * keeps each check's evaluation inside one read-locked section without
holding the lock across
+ * metadata or DB lookups.
+ *
+ * @return the evaluation result, or empty when the request's roles kept
being cleared or could
+ * not be reloaded; the caller must then fail closed
+ */
+ private Optional<Boolean> evaluateWithLoadedRolePolicies(
+ String metalake, AuthorizationRequestContext requestContext,
BooleanSupplier evaluation) {
+ for (int reloads = 0; ; reloads++) {
+ rolePolicyLock.readLock().lock();
+ try {
+ if (!hasClearedBoundRole(requestContext)) {
+ return Optional.of(evaluation.getAsBoolean());
+ }
+ } finally {
+ rolePolicyLock.readLock().unlock();
+ }
+
+ if (reloads == MAX_ROLE_POLICY_RELOADS) {
+ LOG.warn(
+ "Policies of roles {} were cleared again after {} reloads within
one authorization "
+ + "check; failing closed. Check for frequent privilege changes
on these roles or a "
+ + "loaded role cache that is too small on this node.",
+ requestContext.getBoundRoleIds(),
+ MAX_ROLE_POLICY_RELOADS);
+ return Optional.empty();
+ }
+
+ long rolePolicyGeneration = rolePolicyGenerationCounter.get();
+ Set<Long> unreadableRoleIds;
+ try {
+ unreadableRoleIds =
+ versionCheckAndLoadRoles(
+ metalake, new ArrayList<>(requestContext.getBoundRoleIds()),
requestContext);
+ } catch (RuntimeException e) {
+ LOG.warn(
+ "Failed to reload cleared role policies for roles {}; failing
closed",
+ requestContext.getBoundRoleIds(),
+ e);
+ return Optional.empty();
+ }
+ // A role whose entity could not be read has lost its policies without a
replacement. Its
+ // missing rows prove neither the absence of a deny nor of an allow, so
the check cannot be
+ // evaluated. Partially loaded and dropped roles are evaluated as usual:
their policy state is
+ // the same one any request would see.
+ if (!unreadableRoleIds.isEmpty()) {
Review Comment:
[Question] Should a role that reloads only *partially* also fail
`hasDenyPolicy` closed?
The comment here says partially loaded roles are evaluated as usual, and
`testPartiallyLoadedRoleDoesNotFailClosedAfterEviction` pins that. For the
per-object `enforce` paths I agree it is harmless: `resolveRolePolicies` only
drops rows whose securable object did not resolve (lines 1856-1862), and a
check on such an object already returns `false` at `authorizeObject` when
`resolveMetadataId` is empty.
`hasDenyPolicy` is different, because its scan is deliberately
scope-agnostic (line 501-513): it answers "does any deny on these privileges
exist for the user's roles, at any scope?". A partial reload that drops a DENY
row for a currently unresolvable parent (e.g. a transient
`metadataIdCache`/resolution miss, not only a dropped object) turns that answer
from `true` to `false` and re-enables the parent-scope short-circuit at
`MetadataAuthzHelper.java:342-349`, which skips per-object checks for the whole
listing.
This is pre-existing rather than introduced here - partial loads could
already shrink the deny enforcer - so I am raising it as a question, not a
blocker: is it in scope to also fail `hasDenyPolicy` closed when
`replaceRolePolicies` installed an incomplete resolution for a bound role, or
is that better handled with #13612?
Verified by: read `resolveRolePolicies` (1846-1888), `replaceRolePolicies`
(1571-1608), the `hasDenyPolicy` scan (489-519) and its single caller
`MetadataAuthzHelper.java:342-349`.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -1244,6 +1317,8 @@ private void loadRolePrivilege(
if (!allRoleIds.isEmpty()) {
versionCheckAndLoadRoles(metalake, allRoleIds, requestContext);
}
+ requestContext.setBoundRoleIds(allRoleIds);
Review Comment:
[Important] The request's own role load drops the fail-closed signal, so the
lost-deny this PR fixes stays open for the first load.
`versionCheckAndLoadRoles` now returns the roles whose entity could not be
read, and `evaluateWithLoadedRolePolicies` fails the check closed on a
non-empty set (lines 1423-1429, 1765-1769). But the call at line 1316 inside
this `loadRole` lambda discards that return value, and then lines 1320-1321
record `boundRoleIds` and the generation as if the load had succeeded.
Concretely, for a role that is stale or not loaded on this node and whose
entity read throws something other than `NoSuchEntityException` (line
1510-1513): `bindUserRoles` has already added the user's g-row for it (lines
1336/1347/1388/1399, which run before line 1316), no policies are installed,
and no clear generation is recorded - so `hasClearedBoundRole` stays `false`
for the whole request and every check evaluates as if the role were empty. If
that role carried a DENY, `deny()` returns `false` and `hasDenyPolicy()`
returns `false`, which re-enables the parent-scope list short-circuit at
`MetadataAuthzHelper.java:342-349`. That is the same outcome
`testReloadReadFailureMustNotLoseDeny` is written to prevent, just one step
earlier in the request.
Suggested fix: keep the returned set (e.g. on the context) and have
`evaluateWithLoadedRolePolicies` treat a role in it as unresolved on the first
check too - the existing `Optional.empty()` plumbing already handles the rest -
or at minimum record a clear generation for those roles so each check retries
and then fails closed.
Verified by: read `loadRolePrivilege` (1255-1322),
`versionCheckAndLoadRoles` (1429-1555) and the `bindUserRoles` call sites in
`loadUserRoles`/`loadGroupRoles` on this head; confirmed
`MetadataAuthzHelper.java:342-349` is the only caller of `hasDenyPolicy`.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -1554,16 +1656,140 @@ private void clearRolePoliciesOnCacheRemoval(long
roleId) {
roleId);
return;
}
- clearRolePoliciesWithoutLock(roleId);
+ if (Objects.equals(roleBeingReplaced, roleId)) {
+ clearRolePoliciesWithoutLock(roleId);
+ } else {
+ clearRolePoliciesAndRecordGeneration(roleId);
+ }
} finally {
rolePolicyLock.writeLock().unlock();
}
}
- private void clearRolePoliciesWithoutLock(long roleId) {
+ private boolean clearRolePoliciesWithoutLock(long roleId) {
String roleIdStr = String.valueOf(roleId);
- allowEnforcer.removeFilteredPolicy(0, roleIdStr);
- denyEnforcer.removeFilteredPolicy(0, roleIdStr);
+ boolean allowRemoved = allowEnforcer.removeFilteredPolicy(0, roleIdStr);
+ boolean denyRemoved = denyEnforcer.removeFilteredPolicy(0, roleIdStr);
+ return allowRemoved || denyRemoved;
+ }
+
+ /**
+ * Clears a role's policies that are not replaced in the same critical
section, and records the
+ * clear so that in-flight requests bound to the role reload it before their
next check. Only a
+ * clear that removed policies is recorded: clearing an already empty role
changes nothing a
+ * request could have observed, and recording it would make a deleted role
look unstable forever.
+ * Must be called under the write lock of {@link #rolePolicyLock}.
+ */
+ private void clearRolePoliciesAndRecordGeneration(long roleId) {
+ if (clearRolePoliciesWithoutLock(roleId)) {
+ recordRoleClearGeneration(roleId);
+ }
+ }
+
+ /**
+ * Records a new clear generation for a role, pruning the older half of the
generations when the
+ * map outgrows {@link #maxRoleClearGenerations}. Must be called under the
write lock of {@link
+ * #rolePolicyLock}, so that readers see the pruned entries and the raised
{@link
+ * #prunedRoleClearGeneration} together.
+ */
+ private void recordRoleClearGeneration(long roleId) {
+ roleClearGenerations.put(roleId,
rolePolicyGenerationCounter.incrementAndGet());
+ if (roleClearGenerations.size() <= maxRoleClearGenerations) {
+ return;
+ }
+ long[] generations =
+
roleClearGenerations.values().stream().mapToLong(Long::longValue).sorted().toArray();
+ long cutoff = generations[generations.length / 2];
+ roleClearGenerations.values().removeIf(generation -> generation <= cutoff);
+ prunedRoleClearGeneration = cutoff;
+ }
+
+ /**
+ * Evaluates one authorization check against role policies that are at least
as complete as the
+ * ones the request loaded.
+ *
+ * <p>A request loads its roles once, but each check releases {@link
#rolePolicyLock} before the
+ * next one starts. In between, a {@link #loadedRoles} TTL or size eviction,
or a privilege change
+ * on one of the roles, may clear that role's policies while the user's
grouping row stays. A
+ * later check would then evaluate without the role: it can deny what the
role allows, or allow
+ * what the role denies. So before each evaluation this method checks, under
the read lock,
+ * whether a bound role was cleared after the request's recorded generation.
If so, it reloads the
+ * cleared roles outside the lock, the same way a request's role load does,
and checks again.
+ *
+ * <p>Earlier checks of the request stay valid because they ran against
complete policies. This
+ * keeps each check's evaluation inside one read-locked section without
holding the lock across
+ * metadata or DB lookups.
+ *
+ * @return the evaluation result, or empty when the request's roles kept
being cleared or could
+ * not be reloaded; the caller must then fail closed
+ */
+ private Optional<Boolean> evaluateWithLoadedRolePolicies(
+ String metalake, AuthorizationRequestContext requestContext,
BooleanSupplier evaluation) {
+ for (int reloads = 0; ; reloads++) {
+ rolePolicyLock.readLock().lock();
+ try {
+ if (!hasClearedBoundRole(requestContext)) {
+ return Optional.of(evaluation.getAsBoolean());
+ }
+ } finally {
+ rolePolicyLock.readLock().unlock();
+ }
+
+ if (reloads == MAX_ROLE_POLICY_RELOADS) {
+ LOG.warn(
+ "Policies of roles {} were cleared again after {} reloads within
one authorization "
+ + "check; failing closed. Check for frequent privilege changes
on these roles or a "
+ + "loaded role cache that is too small on this node.",
+ requestContext.getBoundRoleIds(),
+ MAX_ROLE_POLICY_RELOADS);
+ return Optional.empty();
+ }
+
+ long rolePolicyGeneration = rolePolicyGenerationCounter.get();
+ Set<Long> unreadableRoleIds;
+ try {
+ unreadableRoleIds =
+ versionCheckAndLoadRoles(
+ metalake, new ArrayList<>(requestContext.getBoundRoleIds()),
requestContext);
+ } catch (RuntimeException e) {
+ LOG.warn(
+ "Failed to reload cleared role policies for roles {}; failing
closed",
+ requestContext.getBoundRoleIds(),
+ e);
+ return Optional.empty();
+ }
+ // A role whose entity could not be read has lost its policies without a
replacement. Its
+ // missing rows prove neither the absence of a deny nor of an allow, so
the check cannot be
+ // evaluated. Partially loaded and dropped roles are evaluated as usual:
their policy state is
+ // the same one any request would see.
+ if (!unreadableRoleIds.isEmpty()) {
+ LOG.warn(
+ "Failed to read roles {} to reload their cleared policies; failing
closed",
+ unreadableRoleIds);
+ return Optional.empty();
+ }
+ requestContext.setRolePolicyGeneration(rolePolicyGeneration);
Review Comment:
[Nit] This write is not monotonic, and one `AuthorizationRequestContext` is
explicitly shared by concurrent list workers.
`AuthorizationRequestContext`'s class javadoc states that entry
authorization and list filtering of one request may share an instance and that
list workers receive it explicitly - which is why its maps are
`ConcurrentHashMap`. Two workers can therefore both be in
`evaluateWithLoadedRolePolicies`: worker A reads generation 100 at line 1748,
worker B reads 105, B finishes and writes 105, then A writes 100 and the
recorded generation moves backwards.
Nothing unsafe follows - a lower generation is the conservative direction -
but it costs an extra detect/reload round on later checks, and those rounds
count against `MAX_ROLE_POLICY_RELOADS`, so in the worst case a rollback
contributes to a spurious hard deny. A max-based update
(`setRolePolicyGeneration` keeping the larger value, or an `AtomicLong` with
`accumulateAndGet(g, Math::max)`) removes the rollback entirely.
Verified by: read `AuthorizationRequestContext` lines 55-61 (shared-instance
contract) and 92-105/248-288 (plain `volatile` setters), against the read/write
pair at lines 1748 and 1771.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -153,6 +167,43 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
*/
private final ReentrantReadWriteLock rolePolicyLock = new
ReentrantReadWriteLock();
+ /**
+ * Source of role policy generations. Advanced under the write lock of
{@link #rolePolicyLock}
+ * whenever a role's policies are cleared without being replaced in the same
critical section.
+ */
+ private final AtomicLong rolePolicyGenerationCounter = new AtomicLong();
+
+ /**
+ * roleId -> generation of the most recent clear of that role's policies. A
request records the
+ * generation before it loads its roles; a bound role whose clear generation
is newer has lost
+ * policies the request relies on and must be reloaded before the request
evaluates it again. The
+ * map is bounded by {@link #maxRoleClearGenerations}; see {@link
#prunedRoleClearGeneration} for
+ * how pruned entries stay safe.
+ */
+ private final Map<Long, Long> roleClearGenerations = new
ConcurrentHashMap<>();
+
+ /**
+ * Upper bound on {@link #roleClearGenerations} entries, set to the role
cache size. Guarded by
+ * the write lock of {@link #rolePolicyLock}.
+ */
+ private long maxRoleClearGenerations;
Review Comment:
[Nit] The javadoc's guard claim does not match how this field is written.
The javadoc says "Guarded by the write lock of `rolePolicyLock`", but the
only production write is `maxRoleClearGenerations = roleCacheSize;` at line 280
in `initialize()`, with no lock held; the reads at line 1698 are the ones under
the write lock. It is benign in practice because `initialize()` runs before the
authorizer is used, but the comment points a future reader at a guarantee the
code does not provide. Either say "set once in `initialize()`, read under the
write lock", or make it `volatile` (the pruning test already mutates it by
reflection, so `final` is not an option).
Verified by: grepped every reference to `maxRoleClearGenerations` on this
head - declaration at 189, write at 280, read at 1698, plus the reflective
write in `testRoleClearGenerationsArePrunedWithoutMissingClears`.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -1554,16 +1656,140 @@ private void clearRolePoliciesOnCacheRemoval(long
roleId) {
roleId);
return;
}
- clearRolePoliciesWithoutLock(roleId);
+ if (Objects.equals(roleBeingReplaced, roleId)) {
+ clearRolePoliciesWithoutLock(roleId);
+ } else {
+ clearRolePoliciesAndRecordGeneration(roleId);
+ }
} finally {
rolePolicyLock.writeLock().unlock();
}
}
- private void clearRolePoliciesWithoutLock(long roleId) {
+ private boolean clearRolePoliciesWithoutLock(long roleId) {
String roleIdStr = String.valueOf(roleId);
- allowEnforcer.removeFilteredPolicy(0, roleIdStr);
- denyEnforcer.removeFilteredPolicy(0, roleIdStr);
+ boolean allowRemoved = allowEnforcer.removeFilteredPolicy(0, roleIdStr);
+ boolean denyRemoved = denyEnforcer.removeFilteredPolicy(0, roleIdStr);
+ return allowRemoved || denyRemoved;
+ }
+
+ /**
+ * Clears a role's policies that are not replaced in the same critical
section, and records the
+ * clear so that in-flight requests bound to the role reload it before their
next check. Only a
+ * clear that removed policies is recorded: clearing an already empty role
changes nothing a
+ * request could have observed, and recording it would make a deleted role
look unstable forever.
+ * Must be called under the write lock of {@link #rolePolicyLock}.
+ */
+ private void clearRolePoliciesAndRecordGeneration(long roleId) {
+ if (clearRolePoliciesWithoutLock(roleId)) {
+ recordRoleClearGeneration(roleId);
+ }
+ }
+
+ /**
+ * Records a new clear generation for a role, pruning the older half of the
generations when the
+ * map outgrows {@link #maxRoleClearGenerations}. Must be called under the
write lock of {@link
+ * #rolePolicyLock}, so that readers see the pruned entries and the raised
{@link
+ * #prunedRoleClearGeneration} together.
+ */
+ private void recordRoleClearGeneration(long roleId) {
+ roleClearGenerations.put(roleId,
rolePolicyGenerationCounter.incrementAndGet());
+ if (roleClearGenerations.size() <= maxRoleClearGenerations) {
+ return;
+ }
+ long[] generations =
+
roleClearGenerations.values().stream().mapToLong(Long::longValue).sorted().toArray();
+ long cutoff = generations[generations.length / 2];
+ roleClearGenerations.values().removeIf(generation -> generation <= cutoff);
+ prunedRoleClearGeneration = cutoff;
+ }
+
+ /**
+ * Evaluates one authorization check against role policies that are at least
as complete as the
+ * ones the request loaded.
+ *
+ * <p>A request loads its roles once, but each check releases {@link
#rolePolicyLock} before the
+ * next one starts. In between, a {@link #loadedRoles} TTL or size eviction,
or a privilege change
+ * on one of the roles, may clear that role's policies while the user's
grouping row stays. A
+ * later check would then evaluate without the role: it can deny what the
role allows, or allow
+ * what the role denies. So before each evaluation this method checks, under
the read lock,
+ * whether a bound role was cleared after the request's recorded generation.
If so, it reloads the
+ * cleared roles outside the lock, the same way a request's role load does,
and checks again.
+ *
+ * <p>Earlier checks of the request stay valid because they ran against
complete policies. This
+ * keeps each check's evaluation inside one read-locked section without
holding the lock across
+ * metadata or DB lookups.
+ *
+ * @return the evaluation result, or empty when the request's roles kept
being cleared or could
+ * not be reloaded; the caller must then fail closed
+ */
+ private Optional<Boolean> evaluateWithLoadedRolePolicies(
+ String metalake, AuthorizationRequestContext requestContext,
BooleanSupplier evaluation) {
+ for (int reloads = 0; ; reloads++) {
+ rolePolicyLock.readLock().lock();
+ try {
+ if (!hasClearedBoundRole(requestContext)) {
+ return Optional.of(evaluation.getAsBoolean());
+ }
+ } finally {
+ rolePolicyLock.readLock().unlock();
+ }
+
+ if (reloads == MAX_ROLE_POLICY_RELOADS) {
+ LOG.warn(
+ "Policies of roles {} were cleared again after {} reloads within
one authorization "
+ + "check; failing closed. Check for frequent privilege changes
on these roles or a "
+ + "loaded role cache that is too small on this node.",
+ requestContext.getBoundRoleIds(),
+ MAX_ROLE_POLICY_RELOADS);
+ return Optional.empty();
+ }
+
+ long rolePolicyGeneration = rolePolicyGenerationCounter.get();
+ Set<Long> unreadableRoleIds;
+ try {
+ unreadableRoleIds =
+ versionCheckAndLoadRoles(
+ metalake, new ArrayList<>(requestContext.getBoundRoleIds()),
requestContext);
Review Comment:
[Nit] The defensive copy here is redundant.
`setBoundRoleIds` already stores `Collections.unmodifiableList(new
ArrayList<>(...))` (`AuthorizationRequestContext.java:264-269`), and
`versionCheckAndLoadRoles` only reads its `roleIds` argument
(`roleIds.stream().distinct()` at line 1430). Passing
`requestContext.getBoundRoleIds()` directly drops an allocation that happens on
every reload attempt of every check.
Verified by: read `setBoundRoleIds`/`getBoundRoleIds` in
`AuthorizationRequestContext.java` and every use of `roleIds` inside
`versionCheckAndLoadRoles` (1429-1555).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
