jerryshao commented on code in PR #13778:
URL: https://github.com/apache/gravitino/pull/13778#discussion_r4384238948
##########
docs/policies.md:
##########
@@ -103,6 +103,34 @@ mask := action("show-last-4") if is_group_member("support")
else := action("replace-with-null")
```
+When the experimental Iceberg REST read-restriction loader resolves several
row-filter policies
+for one table, a filter that resolves to `true` adds no restriction and is
ignored. Identical
+effective filters are deduplicated. Distinct restrictive filters conflict and
cause the load-table
+request to fail; the loader does not combine them.
+
+If the selected column-mask action is not applicable to a column type, the
experimental loader
+fails the load-table request rather than omitting the mask and exposing the
original values.
+Unselected actions do not need to be applicable to that column. For example,
`replace-with-null`
+cannot apply to a required column when that action is selected.
+
+Experimental Iceberg REST read restrictions require authorization to be
enabled with
+`gravitino.authorization.enable=true` and a policy dispatcher to be available.
When a policy dispatcher
+is available but authorization is disabled, the Iceberg REST service fails to
start with an error
Review Comment:
[Nit] In auxiliary mode it is not the Iceberg REST service that fails to
start, it is the whole Gravitino server.
`RESTService.serviceInit` is called from
`AuxiliaryServiceManager.serviceInit`, which rethrows through
`doWithClassLoader`
(`core/src/main/java/org/apache/gravitino/auxiliary/AuxiliaryServiceManager.java:199-216`,
`218-229`), and that runs inside `GravitinoEnv.initGravitinoServerComponents`
(`core/src/main/java/org/apache/gravitino/GravitinoEnv.java:1059-1064`) during
`initializeFullComponents`. So the `IllegalStateException` aborts server
startup, not just this auxiliary service - which is the right posture, but an
operator reading this sentence may expect the rest of the server to come up
with Iceberg REST missing.
Suggestion: "the Gravitino server fails to start" (or "the Iceberg REST
service fails to start, which aborts Gravitino server startup in auxiliary
mode").
Verified by: read `AuxiliaryServiceManager.serviceInit`/`doWithClassLoader`
and `GravitinoEnv.initGravitinoServerComponents` in this run; no try/catch
exists on either path.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java:
##########
@@ -248,6 +251,15 @@ protected void configure() {
}
}
+ static IcebergReadRestrictionLoader loadReadRestrictionLoader(boolean
enableAuthorization) {
+ IcebergReadRestrictionLoader loader = IcebergReadRestrictionLoader.load();
+ if (loader.enabled() && !enableAuthorization) {
+ throw new IllegalStateException(
+ "Iceberg read restrictions require authorization; set
gravitino.authorization.enable=true");
+ }
+ return loader;
Review Comment:
[Nit] A provider that is installed but inert is accepted here silently, so
the operator gets no restrictions and no diagnostic.
`enabled()` is false whenever there is no policy dispatcher
(`ExperimentalIcebergReadRestrictionLoader.java:72-75`), and `load` then
returns the response untouched (`:80-82`). In the standalone Iceberg REST
server that state is permanent: `main` requires authorization to be off
(`iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/server/GravitinoIcebergRESTServer.java:73-75`)
and `initializeBaseComponents` never builds a policy dispatcher
(`core/src/main/java/org/apache/gravitino/GravitinoEnv.java:235-241`,
`882-892`), so an operator who adds the experimental jar to a standalone
deployment gets unrestricted load-table responses for every governed table,
with nothing in the log saying the provider is inert. `docs/policies.md:132`
records the behaviour in one clause, but the server never says it.
This method is now the one place that sees both the discovered provider and
its enabled state, so the fix is a line: when
`IcebergReadRestrictionLoader.load()` returns something other than the no-op
holder and `loader.enabled()` is false, log a warning naming the class and the
missing prerequisite.
Verified by: read the new method, `IcebergReadRestrictionLoader.NoOpHolder`
(`:124-139`), the loader's `enabled()`/`load` entry,
`GravitinoIcebergRESTServer.main`/`initialize` and
`GravitinoEnv.initBaseComponents` in this run;
`IcebergTableOperations.java:359-360` confirms `load` is still called and
simply returns the response unchanged.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java:
##########
@@ -248,6 +251,15 @@ protected void configure() {
}
}
+ static IcebergReadRestrictionLoader loadReadRestrictionLoader(boolean
enableAuthorization) {
+ IcebergReadRestrictionLoader loader = IcebergReadRestrictionLoader.load();
+ if (loader.enabled() && !enableAuthorization) {
Review Comment:
[Important] The prerequisite this validates does not make the authenticated
identity trustworthy, and the default authenticator lets the client choose it.
`gravitino.authenticators` defaults to `simple`
(`core/src/main/java/org/apache/gravitino/Configs.java:309-322`), and
`SimpleAuthenticator.authenticateToken` takes the user name straight from the
client's Basic header with no verification, returning `new
UserPrincipal(userInformation[0], authData)`
(`server-common/src/main/java/org/apache/gravitino/server/authentication/SimpleAuthenticator.java:44-66`)
- the two-argument constructor, so the principal carries no groups.
That identity is exactly what the restrictions are bound to:
`IdentityBindingContext.sessionUser()` returns it
(`iceberg/iceberg-rest-server-experiment/.../ExperimentalIcebergReadRestrictionLoader.java:267-270`)
and `snapshotGroups` reads the principal's groups (`:298-308`). So on a
deployment that satisfies the only prerequisite this check enforces, a row
filter such as `col("owner") == session_user()` is selected by a name the
caller picked, and every `is_group_member` predicate takes the non-member
branch.
I accept that this does not breach the PR's stated reader-enforced trust
model - a client willing to send someone else's name would equally ignore the
returned restrictions. The problem is what an operator concludes from
`docs/policies.md:116-126`: it names `gravitino.authorization.enable=true` as
*the* configuration read restrictions require, and discusses the group gap, so
the natural reading is that setting it makes identity-keyed policies
meaningful. It does not.
Suggestion: make the authenticator part of the same startup story. Read
`Configs.AUTHENTICATORS` here and log a prominent warning (or fail, if you want
the same fail-fast posture) when the loader is enabled and `simple` is the only
authenticator, and add one sentence to that doc paragraph saying read
restrictions are only as trustworthy as the configured authenticator.
Verified by: read `Configs.AUTHENTICATORS` and `SimpleAuthenticator` in full
in this run, plus `AuthenticatorFactory.java:54-58` to confirm that key selects
the authenticator; traced the principal into `IdentityBindingContext`.
--
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]
