Skip to content

Commit

Permalink
Improve "Has Privilege" performance for boolean-only response (#86685)
Browse files Browse the repository at this point in the history
Boolean-only privilege checks, i.e. the ones currently used in the
"profile has privilege" API, now benefit from a performance improvement,
because the check will now stop upon first encountering a privilege that
is NOT granted over a resource (and return `false` overall). Previously,
all the privileges were always checked over all the resources in order
to assemble a comprehensive response with all the privileges that are
not granted.
  • Loading branch information
albertzaharovits committed May 24, 2022
1 parent 3fa82fa commit 346abf9
Show file tree
Hide file tree
Showing 27 changed files with 1,057 additions and 579 deletions.
5 changes: 5 additions & 0 deletions docs/changelog/86685.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 86685
summary: Improve "Has Privilege" performance for boolean-only response
area: Authorization
type: enhancement
issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,8 @@ private PrivilegesCheckResult getPrivilegesCheckResult(PrivilegesToCheck privile
}
privilegesByApplication.put(applicationName, appPrivilegesByResource.values());
}
return new PrivilegesCheckResult(authorized, clusterPrivMap, indices, privilegesByApplication);
return new PrivilegesCheckResult(authorized,
new PrivilegesCheckResult.Details(clusterPrivMap, indices, privilegesByApplication));
}

private GetUserPrivilegesResponse getUserPrivilegesResponse(boolean isSuperuser) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,8 @@ public void testFollowIndex() throws Exception {
e.getMessage(),
containsString(
"insufficient privileges to follow index [unallowed-index], "
+ "privilege for action [indices:monitor/stats] is missing, "
+ "privilege for action [indices:data/read/xpack/ccr/shard_changes] is missing"
+ "privilege for action [indices:data/read/xpack/ccr/shard_changes] is missing, "
+ "privilege for action [indices:monitor/stats] is missing"
)
);
// Verify that the follow index has not been created and no node tasks are running
Expand All @@ -121,8 +121,8 @@ public void testFollowIndex() throws Exception {
e.getMessage(),
containsString(
"insufficient privileges to follow index [unallowed-index], "
+ "privilege for action [indices:monitor/stats] is missing, "
+ "privilege for action [indices:data/read/xpack/ccr/shard_changes] is missing"
+ "privilege for action [indices:data/read/xpack/ccr/shard_changes] is missing, "
+ "privilege for action [indices:monitor/stats] is missing"
)
);
assertBusy(() -> assertThat(getCcrNodeTasks(), empty()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ public class HasPrivilegesRequest extends ActionRequest implements UserRequest {
private String[] clusterPrivileges;
private IndicesPrivileges[] indexPrivileges;
private ApplicationResourcePrivileges[] applicationPrivileges;
// this is hard-coded for now, but it doesn't have to be
private final boolean runDetailedCheck = true;

public HasPrivilegesRequest() {}

Expand All @@ -40,9 +42,14 @@ public HasPrivilegesRequest(StreamInput in) throws IOException {
applicationPrivileges = in.readArray(ApplicationResourcePrivileges::new, ApplicationResourcePrivileges[]::new);
}

public AuthorizationEngine.PrivilegesToCheck getPrivilegesToCheck() {
return new AuthorizationEngine.PrivilegesToCheck(clusterPrivileges, indexPrivileges, applicationPrivileges, runDetailedCheck);
}

@Override
public ActionRequestValidationException validate() {
return new AuthorizationEngine.PrivilegesToCheck(clusterPrivileges, indexPrivileges, applicationPrivileges).validate(null);
assert getPrivilegesToCheck().runDetailedCheck();
return getPrivilegesToCheck().validate(null);
}

/**
Expand Down Expand Up @@ -88,6 +95,13 @@ public void applicationPrivileges(ApplicationResourcePrivileges... appPrivileges
this.applicationPrivileges = appPrivileges;
}

public void privilegesToCheck(AuthorizationEngine.PrivilegesToCheck privilegesToCheck) {
assert privilegesToCheck.runDetailedCheck() == runDetailedCheck;
clusterPrivileges(privilegesToCheck.cluster());
indexPrivileges(privilegesToCheck.index());
applicationPrivileges(privilegesToCheck.application());
}

@Override
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,12 @@ public HasPrivilegesRequestBuilder username(String username) {
public HasPrivilegesRequestBuilder source(String username, BytesReference source, XContentType xContentType) throws IOException {
final AuthorizationEngine.PrivilegesToCheck privilegesToCheck = RoleDescriptor.parsePrivilegesToCheck(
username + "/has_privileges",
true, // hard-coded for now, but it doesn't have to be
source,
xContentType
);
request.username(username);
request.clusterPrivileges(privilegesToCheck.cluster());
request.indexPrivileges(privilegesToCheck.index());
request.applicationPrivileges(privilegesToCheck.application());
request.privilegesToCheck(privilegesToCheck);
return this;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public class ProfileHasPrivilegesRequest extends ActionRequest {
PARSER.declareStringArray(constructorArg(), Fields.UIDS);
PARSER.declareField(
constructorArg(),
parser -> RoleDescriptor.parsePrivilegesToCheck("profile_has_privileges_request", parser),
parser -> RoleDescriptor.parsePrivilegesToCheck("profile_has_privileges_request", false, parser),
Fields.PRIVILEGES,
ObjectParser.ValueType.OBJECT
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,8 @@ public interface AuthorizationEngine {
* This could include retrieval of permissions from an index or external system.
* See also {@link #resolveAuthorizationInfo(RequestInfo, ActionListener)}, for which this method is the more general
* sibling. This returns the {@code AuthorizationInfo} that is used for access checks outside the context of
* authorizing a specific request, i.e. {@link #checkPrivileges(AuthorizationInfo, PrivilegesToCheck, Collection, ActionListener)}
* authorizing a specific request, i.e.
* {@link #checkPrivileges(AuthorizationInfo, PrivilegesToCheck, Collection, ActionListener)}
*
* @param subject object representing the effective user
* @param listener the listener to be notified of success using {@link ActionListener#onResponse(Object)}
Expand Down Expand Up @@ -253,23 +254,35 @@ default AuthorizationInfo getAuthenticatedUserAuthorizationInfo() {
}
}

/**
* This encapsulates the privileges that can be checked for access. It's intentional that the privileges to be checked are specified
* in the same manner that they are granted in the {@link RoleDescriptor}. The privilege check can be detailed or not, per the
* {@link #runDetailedCheck} parameter. The detailed response {@link PrivilegesCheckResult} of a check run, also shows which privileges
* are NOT granted.
*/
record PrivilegesToCheck(
String[] cluster,
RoleDescriptor.IndicesPrivileges[] index,
RoleDescriptor.ApplicationResourcePrivileges[] application
RoleDescriptor.ApplicationResourcePrivileges[] application,
boolean runDetailedCheck
) {
public static PrivilegesToCheck readFrom(StreamInput in) throws IOException {
return new PrivilegesToCheck(
in.readOptionalStringArray(),
in.readOptionalArray(RoleDescriptor.IndicesPrivileges::new, RoleDescriptor.IndicesPrivileges[]::new),
in.readOptionalArray(RoleDescriptor.ApplicationResourcePrivileges::new, RoleDescriptor.ApplicationResourcePrivileges[]::new)
in.readOptionalArray(
RoleDescriptor.ApplicationResourcePrivileges::new,
RoleDescriptor.ApplicationResourcePrivileges[]::new
),
in.readBoolean()
);
}

public void writeTo(StreamOutput out) throws IOException {
out.writeOptionalStringArray(cluster);
out.writeOptionalArray(RoleDescriptor.IndicesPrivileges::write, index);
out.writeOptionalArray(RoleDescriptor.ApplicationResourcePrivileges::write, application);
out.writeBoolean(runDetailedCheck);
}

public ActionRequestValidationException validate(ActionRequestValidationException validationException) {
Expand Down Expand Up @@ -306,12 +319,16 @@ public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
PrivilegesToCheck that = (PrivilegesToCheck) o;
return Arrays.equals(cluster, that.cluster) && Arrays.equals(index, that.index) && Arrays.equals(application, that.application);
return runDetailedCheck == that.runDetailedCheck
&& Arrays.equals(cluster, that.cluster)
&& Arrays.equals(index, that.index)
&& Arrays.equals(application, that.application);
}

@Override
public int hashCode() {
int result = Arrays.hashCode(cluster);
int result = Objects.hash(runDetailedCheck);
result = 31 * result + Arrays.hashCode(cluster);
result = 31 * result + Arrays.hashCode(index);
result = 31 * result + Arrays.hashCode(application);
return result;
Expand All @@ -329,16 +346,67 @@ public String toString() {
+ ","
+ "application="
+ Arrays.toString(application)
+ ","
+ "detailed="
+ runDetailedCheck
+ "}";
}
}

record PrivilegesCheckResult(
boolean allMatch,
Map<String, Boolean> cluster,
Map<String, ResourcePrivileges> index,
Map<String, Collection<ResourcePrivileges>> application
) {}
/**
* The result of a (has) privilege check. This is not to be used as an Elasticsearch authorization result (though clients can base their
* authorization decisions on this response). The {@link #allChecksSuccess} field tells if all the privileges are granted over
* all the resources. The {@link #details} field is only present (non-null) if the check has been run in a detailed mode
* {@link PrivilegesToCheck#runDetailedCheck}, and contains a run-down of which privileges are granted over which resources or not.
*/
final class PrivilegesCheckResult {

public static final PrivilegesCheckResult ALL_CHECKS_SUCCESS_NO_DETAILS = new PrivilegesCheckResult(true, null);
public static final PrivilegesCheckResult SOME_CHECKS_FAILURE_NO_DETAILS = new PrivilegesCheckResult(false, null);

private final boolean allChecksSuccess;

@Nullable
private final Details details;

public PrivilegesCheckResult(boolean allChecksSuccess, Details details) {
this.allChecksSuccess = allChecksSuccess;
this.details = details;
}

public boolean allChecksSuccess() {
return allChecksSuccess;
}

public @Nullable Details getDetails() {
return details;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
PrivilegesCheckResult that = (PrivilegesCheckResult) o;
return allChecksSuccess == that.allChecksSuccess && Objects.equals(details, that.details);
}

@Override
public int hashCode() {
return Objects.hash(allChecksSuccess, details);
}

public record Details(
Map<String, Boolean> cluster,
Map<String, ResourcePrivileges> index,
Map<String, Collection<ResourcePrivileges>> application
) {
public Details {
Objects.requireNonNull(cluster);
Objects.requireNonNull(index);
Objects.requireNonNull(application);
}
}
}

/**
* Implementation of authorization info that is used in cases where we were not able to resolve
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,8 @@ private static String[] readStringArray(String roleName, XContentParser parser,
}
}

public static PrivilegesToCheck parsePrivilegesToCheck(String description, XContentParser parser) throws IOException {
public static PrivilegesToCheck parsePrivilegesToCheck(String description, boolean runDetailedCheck, XContentParser parser)
throws IOException {
if (parser.currentToken() != XContentParser.Token.START_OBJECT) {
throw new ElasticsearchParseException(
"failed to parse privileges check [{}]. expected an object but found [{}] instead",
Expand Down Expand Up @@ -435,12 +436,20 @@ public static PrivilegesToCheck parsePrivilegesToCheck(String description, XCont
return new PrivilegesToCheck(
clusterPrivileges != null ? clusterPrivileges : Strings.EMPTY_ARRAY,
indexPrivileges != null ? indexPrivileges : IndicesPrivileges.NONE,
applicationPrivileges != null ? applicationPrivileges : ApplicationResourcePrivileges.NONE
applicationPrivileges != null ? applicationPrivileges : ApplicationResourcePrivileges.NONE,
runDetailedCheck
);
}

public static PrivilegesToCheck parsePrivilegesToCheck(String description, BytesReference source, XContentType xContentType)
throws IOException {
/**
* Parses the privileges to be checked, from the same syntax used for granting privileges in a {@code RoleDescriptor}.
*/
public static PrivilegesToCheck parsePrivilegesToCheck(
String description,
boolean runDetailedCheck,
BytesReference source,
XContentType xContentType
) throws IOException {
try (
InputStream stream = source.streamInput();
XContentParser parser = xContentType.xContent()
Expand All @@ -455,7 +464,7 @@ public static PrivilegesToCheck parsePrivilegesToCheck(String description, Bytes
token
);
}
return parsePrivilegesToCheck(description, parser);
return parsePrivilegesToCheck(description, runDetailedCheck, parser);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.apache.lucene.util.automaton.Automaton;
import org.apache.lucene.util.automaton.Operations;
import org.elasticsearch.common.util.set.Sets;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.core.Tuple;
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilege;
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor;
Expand Down Expand Up @@ -98,15 +99,19 @@ public boolean grants(ApplicationPrivilege other, String resource) {
* @param checkForPrivilegeNames check permission grants for the set of privilege names
* @param storedPrivileges stored {@link ApplicationPrivilegeDescriptor} for an application against which the access checks are
* performed
* @return an instance of {@link ResourcePrivilegesMap}
* @param resourcePrivilegesMapBuilder out-parameter for returning the details on which privilege over which resource is granted or not.
* Can be {@code null} when no such details are needed so the method can return early, after
* encountering the first privilege that is not granted over some resource.
* @return {@code true} when all the privileges are granted over all the resources, or {@code false} otherwise
*/
public ResourcePrivilegesMap checkResourcePrivileges(
public boolean checkResourcePrivileges(
final String applicationName,
Set<String> checkForResources,
Set<String> checkForPrivilegeNames,
Collection<ApplicationPrivilegeDescriptor> storedPrivileges
Collection<ApplicationPrivilegeDescriptor> storedPrivileges,
@Nullable ResourcePrivilegesMap.Builder resourcePrivilegesMapBuilder
) {
final ResourcePrivilegesMap.Builder resourcePrivilegesMapBuilder = ResourcePrivilegesMap.builder();
boolean allMatch = true;
for (String checkResource : checkForResources) {
for (String checkPrivilegeName : checkForPrivilegeNames) {
final Set<String> nameSet = Collections.singleton(checkPrivilegeName);
Expand All @@ -116,16 +121,22 @@ public ResourcePrivilegesMap checkResourcePrivileges(
assert Automatons.predicate(applicationName).test(checkPrivilege.getApplication())
: "Privilege " + checkPrivilege + " should have application " + applicationName;
assert checkPrivilege.name().equals(nameSet) : "Privilege " + checkPrivilege + " should have name " + nameSet;

if (grants(checkPrivilege, checkResource)) {
resourcePrivilegesMapBuilder.addResourcePrivilege(checkResource, checkPrivilegeName, Boolean.TRUE);
if (resourcePrivilegesMapBuilder != null) {
resourcePrivilegesMapBuilder.addResourcePrivilege(checkResource, checkPrivilegeName, Boolean.TRUE);
}
} else {
resourcePrivilegesMapBuilder.addResourcePrivilege(checkResource, checkPrivilegeName, Boolean.FALSE);
if (resourcePrivilegesMapBuilder != null) {
resourcePrivilegesMapBuilder.addResourcePrivilege(checkResource, checkPrivilegeName, Boolean.FALSE);
allMatch = false;
} else {
return false;
}
}
}
}
}
return resourcePrivilegesMapBuilder.build();
return allMatch;
}

@Override
Expand Down

0 comments on commit 346abf9

Please sign in to comment.