anchela commented on code in PR #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094#discussion_r1356877584
##########
oak-api/src/main/java/org/apache/jackrabbit/oak/api/jmx/QueryEngineSettingsMBean.java:
##########
@@ -188,4 +188,21 @@ void setIgnoredClassNamesInCallTrace(
// @Description("Get the Java package / fully qualified class names to
ignore when finding the caller of query")
@NotNull
String[] getIgnoredClassNamesInCallTrace();
+
+ /**
+ * Set the name of a JCR privilege that grants a repository permission to
use insecure query options.
+ *
+ * @param privilegeName the insecure query options privilege name
+ * @since 1.60
+ */
+ @Description("Set the name of a JCR privilege that grants a repository
permission to use insecure query options.")
+ void setInsecureQueryOptionsPrivilegeName(@NotNull String privilegeName);
+
+ /**
+ * Get the configured name of a JCR privilege that grants a repository
permission to use insecure query options.
Review Comment:
same as above.
##########
oak-api/src/main/java/org/apache/jackrabbit/oak/api/Result.java:
##########
@@ -74,6 +74,15 @@ public interface Result {
* count is higher than max, it returns Long.MAX_VALUE.
*/
long getSize(SizePrecision precision, long max);
+
+ /**
+ * Indicates whether the query specified the INSECURE RESULT SIZE option.
+ *
+ * @return true if the INSECURE RESULT SIZE query option was set
+ */
+ default boolean isQueryOptionInsecureResultSize() {
Review Comment:
from a security point of view, i find it questionable to add public API to
make something insecure by design.
i wonder if there is any, remote possiblity to introduce the feature without
altering public API contract that we cannot revert or adjust later on.
##########
oak-api/src/main/java/org/apache/jackrabbit/oak/api/jmx/QueryEngineSettingsMBean.java:
##########
@@ -188,4 +188,21 @@ void setIgnoredClassNamesInCallTrace(
// @Description("Get the Java package / fully qualified class names to
ignore when finding the caller of query")
@NotNull
String[] getIgnoredClassNamesInCallTrace();
+
+ /**
+ * Set the name of a JCR privilege that grants a repository permission to
use insecure query options.
+ *
+ * @param privilegeName the insecure query options privilege name
+ * @since 1.60
+ */
+ @Description("Set the name of a JCR privilege that grants a repository
permission to use insecure query options.")
Review Comment:
the description and the javadoc of this method is not entirely accurate. if
a custom privilege is registered (and by doing so relying on application to
verify that privilege to be present), we can no longer talk about a repository
permission. the repository and it's permission evaluation is agnostic to this
custom privilege and will not have the ability to enforce it.... so, i would
like the phrasing here to accurately reflect on how the 'insecure-query-option'
is being mitigated to some extend
##########
oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/QueryLimits.java:
##########
@@ -43,4 +43,14 @@ default String getStrictPathRestriction() {
default @NotNull String[] getIgnoredClassNamesInCallTrace() {
return new String[] {};
}
+
+ /**
+ * Retrieve the configured name of a JCR privilege that grants a
repository permission to use insecure query
Review Comment:
see comment above about being accurate with respect to custom privilege vs
repository enforced permissions.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java:
##########
@@ -95,6 +95,9 @@
import org.apache.jackrabbit.oak.spi.query.QueryIndex.OrderEntry;
import org.apache.jackrabbit.oak.spi.query.QueryIndex.OrderEntry.Order;
import org.apache.jackrabbit.oak.spi.query.QueryIndexProvider;
+import org.apache.jackrabbit.oak.spi.query.QueryLimits;
+import
org.apache.jackrabbit.oak.spi.security.authorization.permission.PermissionProvider;
Review Comment:
this import is unused -> remove. spotted when looking at the code in my IDE
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java:
##########
@@ -1421,6 +1425,43 @@ public long getSize(SizePrecision precision, long max) {
return Math.min(limit.orElse(Long.MAX_VALUE),
source.getSize(context.getBaseState(), precision, max));
}
+ /**
+ * Check whether the execution context has been granted the privilege
represented by
+ * {@link
org.apache.jackrabbit.oak.api.jmx.QueryEngineSettingsMBean#getInsecureQueryOptionsPrivilegeName()}
as a
+ * repository permission.
+ *
+ * @return true if the context has the insecure query options privilege
granted as a repository permission
+ */
+ boolean isAllowedInsecureOptions() {
+ return
Optional.ofNullable(context.getSettings().getInsecureQueryOptionsPrivilegeName())
+ .filter(name -> !name.isEmpty())
+ .flatMap(privilegeName -> Optional.ofNullable(context)
+ .map(ExecutionContext::getPermissionProvider)
+ .map(provider -> provider.hasPrivileges(null,
privilegeName)))
+ .orElse(false);
+ }
+
+ private boolean hasInsecureQueryOptions() {
+ return this.queryOptions != null &&
(this.queryOptions.insecureResultSize || this.queryOptions.insecureFacets);
+ }
+
+ @Override
+ public void verifyInsecureOptions() {
+ if (hasInsecureQueryOptions() && !isAllowedInsecureOptions()) {
+ throw new IllegalArgumentException("Insecure query options are not
allowed.");
Review Comment:
serious note: this exception may actually bubble up unfiltered like the one
recently spotted by a customer and reported by @joerghoh in
[OAK-10473](https://issues.apache.org/jira/browse/OAK-10473).
again not a query expert but i would rather avoid that.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java:
##########
@@ -1421,6 +1425,43 @@ public long getSize(SizePrecision precision, long max) {
return Math.min(limit.orElse(Long.MAX_VALUE),
source.getSize(context.getBaseState(), precision, max));
}
+ /**
+ * Check whether the execution context has been granted the privilege
represented by
+ * {@link
org.apache.jackrabbit.oak.api.jmx.QueryEngineSettingsMBean#getInsecureQueryOptionsPrivilegeName()}
as a
+ * repository permission.
+ *
+ * @return true if the context has the insecure query options privilege
granted as a repository permission
+ */
+ boolean isAllowedInsecureOptions() {
+ return
Optional.ofNullable(context.getSettings().getInsecureQueryOptionsPrivilegeName())
+ .filter(name -> !name.isEmpty())
+ .flatMap(privilegeName -> Optional.ofNullable(context)
+ .map(ExecutionContext::getPermissionProvider)
+ .map(provider -> provider.hasPrivileges(null,
privilegeName)))
Review Comment:
this means that the privilege can only be granted as a repository-level
privilege. a given subject (as associated with every jcr session) either has
the privilege granted or not for the whole repository.
in other words: this applies to all queries and there is no way to turn it
on/off for particular query or query results. right?
that would be crucial information on the public facing configuration options
and osgi configurations.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java:
##########
@@ -1421,6 +1425,43 @@ public long getSize(SizePrecision precision, long max) {
return Math.min(limit.orElse(Long.MAX_VALUE),
source.getSize(context.getBaseState(), precision, max));
}
+ /**
+ * Check whether the execution context has been granted the privilege
represented by
+ * {@link
org.apache.jackrabbit.oak.api.jmx.QueryEngineSettingsMBean#getInsecureQueryOptionsPrivilegeName()}
as a
+ * repository permission.
+ *
+ * @return true if the context has the insecure query options privilege
granted as a repository permission
+ */
+ boolean isAllowedInsecureOptions() {
+ return
Optional.ofNullable(context.getSettings().getInsecureQueryOptionsPrivilegeName())
+ .filter(name -> !name.isEmpty())
+ .flatMap(privilegeName -> Optional.ofNullable(context)
+ .map(ExecutionContext::getPermissionProvider)
+ .map(provider -> provider.hasPrivileges(null,
privilegeName)))
+ .orElse(false);
+ }
+
+ private boolean hasInsecureQueryOptions() {
+ return this.queryOptions != null &&
(this.queryOptions.insecureResultSize || this.queryOptions.insecureFacets);
+ }
+
+ @Override
+ public void verifyInsecureOptions() {
+ if (hasInsecureQueryOptions() && !isAllowedInsecureOptions()) {
Review Comment:
ouch
that is hard to read
##########
oak-api/src/main/java/org/apache/jackrabbit/oak/api/jmx/QueryEngineSettingsMBean.java:
##########
@@ -188,4 +188,21 @@ void setIgnoredClassNamesInCallTrace(
// @Description("Get the Java package / fully qualified class names to
ignore when finding the caller of query")
@NotNull
String[] getIgnoredClassNamesInCallTrace();
+
+ /**
+ * Set the name of a JCR privilege that grants a repository permission to
use insecure query options.
+ *
+ * @param privilegeName the insecure query options privilege name
+ * @since 1.60
+ */
+ @Description("Set the name of a JCR privilege that grants a repository
permission to use insecure query options.")
+ void setInsecureQueryOptionsPrivilegeName(@NotNull String privilegeName);
+
+ /**
+ * Get the configured name of a JCR privilege that grants a repository
permission to use insecure query options.
+ *
+ * @return the insecure query options privilege name
+ * @since 1.60
+ */
+ String getInsecureQueryOptionsPrivilegeName();
Review Comment:
that method name and it's associated 'setter' hurt my security heart.
'insecure' + 'privilegeName' really doesn't got together. i understand your
intention but i would strongly recommend to reconsider the naming here.
given that this class is public API and not just an implementation detail, i
would like you to put reconsider the naming convention used for this feature.
once we have it in the API we cannot get rid of it easily.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java:
##########
@@ -95,6 +95,9 @@
import org.apache.jackrabbit.oak.spi.query.QueryIndex.OrderEntry;
import org.apache.jackrabbit.oak.spi.query.QueryIndex.OrderEntry.Order;
import org.apache.jackrabbit.oak.spi.query.QueryIndexProvider;
+import org.apache.jackrabbit.oak.spi.query.QueryLimits;
+import
org.apache.jackrabbit.oak.spi.security.authorization.permission.PermissionProvider;
+import
org.apache.jackrabbit.oak.spi.security.authorization.permission.Permissions;
Review Comment:
this import is unused -> remove. spotted when looking at the code in my IDE
##########
oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/QueryLimits.java:
##########
@@ -43,4 +43,14 @@ default String getStrictPathRestriction() {
default @NotNull String[] getIgnoredClassNamesInCallTrace() {
return new String[] {};
}
+
+ /**
+ * Retrieve the configured name of a JCR privilege that grants a
repository permission to use insecure query
+ * options.
+ * @return the insecure query options privilege name
+ * @since 1.60
+ */
+ default String getInsecureQueryOptionsPrivilegeName() {
Review Comment:
ok.... the same option now is present in
- QueryEngineSettings
- QueryOptions
- QueryLimits
that looks wrong to me. i am mentioning this here as this particular
configuration option doesn't sound like a 'limit' to me.
i am not a query expert but adding the same option to 3 different interfaces
in API/Spi range does look like a good design. on top it ends up in the
Oak.java class.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java:
##########
@@ -95,6 +95,9 @@
import org.apache.jackrabbit.oak.spi.query.QueryIndex.OrderEntry;
import org.apache.jackrabbit.oak.spi.query.QueryIndex.OrderEntry.Order;
import org.apache.jackrabbit.oak.spi.query.QueryIndexProvider;
+import org.apache.jackrabbit.oak.spi.query.QueryLimits;
Review Comment:
this import is unused -> remove. spotted when looking at the code in my IDE
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java:
##########
@@ -1421,6 +1425,43 @@ public long getSize(SizePrecision precision, long max) {
return Math.min(limit.orElse(Long.MAX_VALUE),
source.getSize(context.getBaseState(), precision, max));
}
+ /**
+ * Check whether the execution context has been granted the privilege
represented by
+ * {@link
org.apache.jackrabbit.oak.api.jmx.QueryEngineSettingsMBean#getInsecureQueryOptionsPrivilegeName()}
as a
+ * repository permission.
+ *
+ * @return true if the context has the insecure query options privilege
granted as a repository permission
+ */
+ boolean isAllowedInsecureOptions() {
Review Comment:
as far as i can see this method is only used inside QueryImpl itself. is
there a reason for not marking it as private?
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java:
##########
@@ -1421,6 +1425,43 @@ public long getSize(SizePrecision precision, long max) {
return Math.min(limit.orElse(Long.MAX_VALUE),
source.getSize(context.getBaseState(), precision, max));
}
+ /**
+ * Check whether the execution context has been granted the privilege
represented by
+ * {@link
org.apache.jackrabbit.oak.api.jmx.QueryEngineSettingsMBean#getInsecureQueryOptionsPrivilegeName()}
as a
+ * repository permission.
+ *
+ * @return true if the context has the insecure query options privilege
granted as a repository permission
+ */
+ boolean isAllowedInsecureOptions() {
+ return
Optional.ofNullable(context.getSettings().getInsecureQueryOptionsPrivilegeName())
+ .filter(name -> !name.isEmpty())
+ .flatMap(privilegeName -> Optional.ofNullable(context)
+ .map(ExecutionContext::getPermissionProvider)
+ .map(provider -> provider.hasPrivileges(null,
privilegeName)))
Review Comment:
i haven't looked at PermissionProvider.hasPrivieges in a while. so my
question would be: what happens if an invalid privilege name has been
configured? e.g. one that never got registered. will the method return true or
false?
i remember from recent work on OAK-9674 that i had to introduce a way to
validate privilege names and i suspect it's not validated for internal calls.
the main usage for `PermissionProvider.hasPrivileges` is the JCR API calls
AccessControlManager.hasPrivileges which takes a privilege object and not a
simple string.
what i suspect (but have not verified): the assertion will return true if an
unknown privilege name has been configured.
please check that.
--
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: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
