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

Introduce role description field #107088

Merged
Merged
Show file tree
Hide file tree
Changes from 62 commits
Commits
Show all changes
68 commits
Select commit Hold shift + click to select a range
248c615
Introduce role description field
slobodanadamovic Apr 4, 2024
9b540d0
Update docs/changelog/107088.yaml
slobodanadamovic Apr 4, 2024
42dbaab
Fix serialization issue
slobodanadamovic Apr 4, 2024
ad6e48f
Merge branch 'sa-add-role-description' of github.com:slobodanadamovic…
slobodanadamovic Apr 4, 2024
fca71bb
Fix failing bwc test
slobodanadamovic Apr 4, 2024
906d537
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic Apr 11, 2024
ca60fb3
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic Apr 12, 2024
b22fe9a
Refactor role descriptor parsing
slobodanadamovic Apr 12, 2024
546602f
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic Apr 12, 2024
23d3396
Fix failing tests (API key roles do not support description field)
slobodanadamovic Apr 12, 2024
36a3e5c
Refactor role descriptor parsing
slobodanadamovic Apr 12, 2024
621959b
Merge branch 'main' of github.com:elastic/elasticsearch into sa-refac…
slobodanadamovic Apr 15, 2024
c3ebf46
Merge branch 'sa-refactor-role-parsing' of github.com:slobodanadamovi…
slobodanadamovic Apr 16, 2024
ce3baab
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic Apr 16, 2024
c2e5a36
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic Apr 17, 2024
b2a9d7d
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic Apr 17, 2024
48b069f
Fix failing API key tests
slobodanadamovic Apr 17, 2024
c3ed4ba
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic Apr 17, 2024
9dbcfaa
Fix another failing test
slobodanadamovic Apr 17, 2024
e74a90d
spotless
slobodanadamovic Apr 17, 2024
8da5db4
Relax cross cluster role parsing - we have checks in place
slobodanadamovic Apr 17, 2024
9437d19
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic Apr 17, 2024
082c0c1
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic Apr 22, 2024
ab65161
fix npe
slobodanadamovic Apr 22, 2024
83f66ba
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic Apr 22, 2024
168fa09
Add missing role description mapping
slobodanadamovic Apr 22, 2024
59631a1
Do not persist limited-by role descriptions
slobodanadamovic Apr 22, 2024
9ab9409
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic Apr 24, 2024
eaeeaf8
Add API key tests
slobodanadamovic Apr 25, 2024
6ba46b5
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic Apr 25, 2024
ef744de
More serialization/deserialization tests
slobodanadamovic Apr 25, 2024
8b8bf29
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic Apr 25, 2024
2974c61
Validate and test file roles
slobodanadamovic Apr 25, 2024
2dc0203
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic Apr 25, 2024
4b41525
Test invalid role descriptions
slobodanadamovic Apr 25, 2024
38556c8
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic Apr 25, 2024
526d4be
Test role creation and update with description
slobodanadamovic Apr 25, 2024
6e2e69d
add bwc tests
slobodanadamovic Apr 25, 2024
a18bff8
fix bwc test
slobodanadamovic Apr 25, 2024
da76594
avoid failing when same role is created in 2/3 upgrade test
slobodanadamovic Apr 25, 2024
825c840
Add YAML test
slobodanadamovic Apr 26, 2024
4de6d35
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic Apr 29, 2024
7259f4d
Bump main .security index's mapping format version
slobodanadamovic Apr 29, 2024
dc51317
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic Apr 29, 2024
d4bd948
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic Apr 29, 2024
dd893a2
Add missing "priorSystemIndexDescriptor"
slobodanadamovic Apr 29, 2024
113d83f
Fix SecurityIndexManagerTests
slobodanadamovic Apr 29, 2024
72bbd76
Define constants for .security index mapping versions
slobodanadamovic Apr 29, 2024
6aa7c12
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic Apr 29, 2024
9368be1
Lower the max description length to 1000 chars
slobodanadamovic Apr 30, 2024
e04e992
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic Apr 30, 2024
05aa831
Fix failing tests
slobodanadamovic Apr 30, 2024
35a80dc
TransportVersions.SECURITY_ROLE_DESCRIPTION.toReleaseVersion
slobodanadamovic May 6, 2024
1fb938a
nit: comment why description is never send across clusters
slobodanadamovic May 6, 2024
be602ab
Implement VersionId and dry up getSecurityMainIndexDescriptor
slobodanadamovic May 6, 2024
491b410
remove comment
slobodanadamovic May 6, 2024
52fd232
Remove unnecessary INTERNAL_MAIN_INDEX_MAPPINGS_FORMAT constant
slobodanadamovic May 7, 2024
619e335
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic May 7, 2024
981bec3
Update security index mapping version to include remote_cluster
slobodanadamovic May 7, 2024
e20ca64
Fix issue with overloaded createRole method
slobodanadamovic May 7, 2024
5ceb492
Fix failing test
slobodanadamovic May 7, 2024
981b203
Another test fix
slobodanadamovic May 7, 2024
4507041
remove "transport" from exception message
slobodanadamovic May 7, 2024
e768c8b
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic May 7, 2024
0c56fef
null -> empty string
slobodanadamovic May 7, 2024
990d9c3
Fix toString test
slobodanadamovic May 8, 2024
91ccbfb
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic May 8, 2024
6a5c379
Merge branch 'main' of github.com:elastic/elasticsearch into sa-add-r…
slobodanadamovic May 8, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/107088.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 107088
summary: Introduce role description field
area: Authorization
type: enhancement
issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ static TransportVersion def(int id) {
public static final TransportVersion NO_GLOBAL_RETENTION_FOR_SYSTEM_DATA_STREAMS = def(8_650_00_0);
public static final TransportVersion SHUTDOWN_REQUEST_TIMEOUTS_FIX = def(8_651_00_0);
public static final TransportVersion INDEXING_PRESSURE_REQUEST_REJECTIONS_COUNT = def(8_652_00_0);
public static final TransportVersion SECURITY_ROLE_DESCRIPTION = def(8_653_00_0);

/*
* STOP! READ THIS FIRST! No, really,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ public class PutRoleRequest extends ActionRequest {
private List<RoleDescriptor.RemoteIndicesPrivileges> remoteIndicesPrivileges = new ArrayList<>();
private RemoteClusterPermissions remoteClusterPermissions = RemoteClusterPermissions.NONE;
private boolean restrictRequest = false;
private String description;

public PutRoleRequest() {}

Expand All @@ -63,6 +64,10 @@ public void name(String name) {
this.name = name;
}

public void description(String description) {
this.description = description;
}

public void cluster(String... clusterPrivilegesArray) {
this.clusterPrivileges = clusterPrivilegesArray;
}
Expand Down Expand Up @@ -164,6 +169,10 @@ public String name() {
return name;
}

public String description() {
return description;
}

public String[] cluster() {
return clusterPrivileges;
}
Expand Down Expand Up @@ -213,7 +222,8 @@ public RoleDescriptor roleDescriptor() {
Collections.emptyMap(),
remoteIndicesPrivileges.toArray(new RoleDescriptor.RemoteIndicesPrivileges[0]),
remoteClusterPermissions,
null
null,
description
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
*/
public class PutRoleRequestBuilder extends ActionRequestBuilder<PutRoleRequest, PutRoleResponse> {

private static final RoleDescriptor.Parser ROLE_DESCRIPTOR_PARSER = RoleDescriptor.parserBuilder().build();
private static final RoleDescriptor.Parser ROLE_DESCRIPTOR_PARSER = RoleDescriptor.parserBuilder().allowDescription(true).build();

public PutRoleRequestBuilder(ElasticsearchClient client) {
super(client, PutRoleAction.INSTANCE, new PutRoleRequest());
Expand All @@ -43,6 +43,7 @@ public PutRoleRequestBuilder source(String name, BytesReference source, XContent
request.addApplicationPrivileges(descriptor.getApplicationPrivileges());
request.runAs(descriptor.getRunAs());
request.metadata(descriptor.getMetadata());
request.description(descriptor.getDescription());
return this;
}

Expand All @@ -51,6 +52,11 @@ public PutRoleRequestBuilder name(String name) {
return this;
}

public PutRoleRequestBuilder description(String description) {
request.description(description);
return this;
}

public PutRoleRequestBuilder cluster(String... cluster) {
request.cluster(cluster);
return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege;
import org.elasticsearch.xpack.core.security.authz.restriction.WorkflowResolver;
import org.elasticsearch.xpack.core.security.support.MetadataUtils;
import org.elasticsearch.xpack.core.security.support.Validation;

import java.util.Arrays;
import java.util.Set;
Expand Down Expand Up @@ -102,6 +103,12 @@ public static ActionRequestValidationException validate(
}
}
}
if (roleDescriptor.getDescription() != null) {
Validation.Error error = Validation.Roles.validateRoleDescription(roleDescriptor.getDescription());
if (error != null) {
validationException = addValidationError(error.toString(), validationException);
}
}
return validationException;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,10 @@ public static final class RoleDescriptorsBytes implements Writeable {

public static final RoleDescriptorsBytes EMPTY = new RoleDescriptorsBytes(new BytesArray("{}"));

private static final RoleDescriptor.Parser ROLE_DESCRIPTOR_PARSER = RoleDescriptor.parserBuilder().build();
private static final RoleDescriptor.Parser ROLE_DESCRIPTOR_PARSER = RoleDescriptor.parserBuilder()
.allowRestriction(true)
.allowDescription(true)
.build();

private final BytesReference rawBytes;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@
import java.util.Map;
import java.util.Objects;

import static org.elasticsearch.common.xcontent.XContentHelper.createParserNotCompressed;

/**
* A holder for a Role that contains user-readable information about the Role
* without containing the actual Role object.
Expand All @@ -70,6 +72,7 @@ public class RoleDescriptor implements ToXContentObject, Writeable {
private final Restriction restriction;
private final Map<String, Object> metadata;
private final Map<String, Object> transientMetadata;
private final String description;

/**
* Needed as a stop-gap measure because {@link FieldPermissionsCache} has state (settings) but we need to use one
Expand All @@ -93,7 +96,7 @@ public RoleDescriptor(

/**
* @deprecated Use {@link #RoleDescriptor(String, String[], IndicesPrivileges[], ApplicationResourcePrivileges[],
* ConfigurableClusterPrivilege[], String[], Map, Map, RemoteIndicesPrivileges[], RemoteClusterPermissions, Restriction)}
* ConfigurableClusterPrivilege[], String[], Map, Map, RemoteIndicesPrivileges[], RemoteClusterPermissions, Restriction, String)}
*/
@Deprecated
public RoleDescriptor(
Expand All @@ -108,7 +111,7 @@ public RoleDescriptor(

/**
* @deprecated Use {@link #RoleDescriptor(String, String[], IndicesPrivileges[], ApplicationResourcePrivileges[],
* ConfigurableClusterPrivilege[], String[], Map, Map, RemoteIndicesPrivileges[], RemoteClusterPermissions, Restriction)}
* ConfigurableClusterPrivilege[], String[], Map, Map, RemoteIndicesPrivileges[], RemoteClusterPermissions, Restriction, String)}
*/
@Deprecated
public RoleDescriptor(
Expand All @@ -130,7 +133,8 @@ public RoleDescriptor(
transientMetadata,
RemoteIndicesPrivileges.NONE,
RemoteClusterPermissions.NONE,
Restriction.NONE
Restriction.NONE,
null
);
}

Expand All @@ -155,7 +159,8 @@ public RoleDescriptor(
transientMetadata,
RemoteIndicesPrivileges.NONE,
RemoteClusterPermissions.NONE,
Restriction.NONE
Restriction.NONE,
null
);
}

Expand All @@ -170,7 +175,8 @@ public RoleDescriptor(
@Nullable Map<String, Object> transientMetadata,
@Nullable RemoteIndicesPrivileges[] remoteIndicesPrivileges,
@Nullable RemoteClusterPermissions remoteClusterPermissions,
@Nullable Restriction restriction
@Nullable Restriction restriction,
@Nullable String description
) {
this.name = name;
this.clusterPrivileges = clusterPrivileges != null ? clusterPrivileges : Strings.EMPTY_ARRAY;
Expand All @@ -187,6 +193,7 @@ public RoleDescriptor(
? remoteClusterPermissions
: RemoteClusterPermissions.NONE;
this.restriction = restriction != null ? restriction : Restriction.NONE;
this.description = description;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I have slight preference to check for null and if null then set to empty string. It can eliminate a class of problems and things to check and better matches the other @ Nullable -> non-null class members.

Copy link
Contributor Author

@slobodanadamovic slobodanadamovic May 7, 2024

Choose a reason for hiding this comment

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

I was undecided here and went with null. Agreed, using empty string is more consistent and simplifies the checks. I will change this.

}

public RoleDescriptor(StreamInput in) throws IOException {
Expand Down Expand Up @@ -218,12 +225,21 @@ public RoleDescriptor(StreamInput in) throws IOException {
} else {
this.remoteClusterPermissions = RemoteClusterPermissions.NONE;
}
if (in.getTransportVersion().onOrAfter(TransportVersions.SECURITY_ROLE_DESCRIPTION)) {
this.description = in.readOptionalString();
} else {
this.description = null;
}
}

public String getName() {
return this.name;
}

public String getDescription() {
return description;
}

public String[] getClusterPrivileges() {
return this.clusterPrivileges;
}
Expand Down Expand Up @@ -338,6 +354,7 @@ public String toString() {
sb.append(group.toString()).append(",");
}
sb.append("], restriction=").append(restriction);
sb.append(", description=").append(description);
sb.append("]");
return sb.toString();
}
Expand All @@ -358,7 +375,8 @@ public boolean equals(Object o) {
if (Arrays.equals(runAs, that.runAs) == false) return false;
if (Arrays.equals(remoteIndicesPrivileges, that.remoteIndicesPrivileges) == false) return false;
if (remoteClusterPermissions.equals(that.remoteClusterPermissions) == false) return false;
return restriction.equals(that.restriction);
if (restriction.equals(that.restriction) == false) return false;
return Objects.equals(description, that.description);
}

@Override
Expand All @@ -373,6 +391,7 @@ public int hashCode() {
result = 31 * result + Arrays.hashCode(remoteIndicesPrivileges);
result = 31 * result + remoteClusterPermissions.hashCode();
result = 31 * result + restriction.hashCode();
result = 31 * result + Objects.hashCode(description);
return result;
}

Expand Down Expand Up @@ -431,6 +450,9 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params, boolea
if (hasRestriction()) {
builder.field(Fields.RESTRICTION.getPreferredName(), restriction);
}
if (description != null) {
builder.field(Fields.DESCRIPTION.getPreferredName(), description);
}
jakelandis marked this conversation as resolved.
Show resolved Hide resolved
return builder.endObject();
}

Expand All @@ -456,18 +478,23 @@ public void writeTo(StreamOutput out) throws IOException {
if (out.getTransportVersion().onOrAfter(TransportVersions.ROLE_REMOTE_CLUSTER_PRIVS)) {
remoteClusterPermissions.writeTo(out);
}
if (out.getTransportVersion().onOrAfter(TransportVersions.SECURITY_ROLE_DESCRIPTION)) {
out.writeOptionalString(description);
}
}

public static Parser.Builder parserBuilder() {
return new Parser.Builder();
}

public record Parser(boolean allow2xFormat, boolean allowRestriction) {
public record Parser(boolean allow2xFormat, boolean allowRestriction, boolean allowDescription) {

public static final class Builder {
private boolean allow2xFormat = false;
private boolean allowRestriction = false;

private boolean allowDescription = false;

private Builder() {}

public Builder allow2xFormat(boolean allow2xFormat) {
Expand All @@ -480,8 +507,13 @@ public Builder allowRestriction(boolean allowRestriction) {
return this;
}

public Builder allowDescription(boolean allowDescription) {
this.allowDescription = allowDescription;
return this;
}

public Parser build() {
return new Parser(allow2xFormat, allowRestriction);
return new Parser(allow2xFormat, allowRestriction, allowDescription);
}

}
Expand Down Expand Up @@ -565,6 +597,8 @@ public RoleDescriptor parse(String name, XContentParser parser) throws IOExcepti
remoteClusterPermissions = parseRemoteCluster(name, parser);
} else if (allowRestriction && Fields.RESTRICTION.match(currentFieldName, parser.getDeprecationHandler())) {
restriction = Restriction.parse(name, parser);
} else if (allowDescription && Fields.DESCRIPTION.match(currentFieldName, parser.getDeprecationHandler())) {
description = parser.text();
} else if (Fields.TYPE.match(currentFieldName, parser.getDeprecationHandler())) {
// don't need it
} else {
Expand All @@ -586,7 +620,8 @@ public RoleDescriptor parse(String name, XContentParser parser) throws IOExcepti
null,
remoteIndicesPrivileges,
remoteClusterPermissions,
restriction
restriction,
description
);

}
Expand Down Expand Up @@ -686,7 +721,7 @@ public static PrivilegesToCheck parsePrivilegesToCheck(
}

private static XContentParser createParser(BytesReference source, XContentType xContentType) throws IOException {
return XContentHelper.createParserNotCompressed(LoggingDeprecationHandler.XCONTENT_PARSER_CONFIG, source, xContentType);
return createParserNotCompressed(LoggingDeprecationHandler.XCONTENT_PARSER_CONFIG, source, xContentType);
}

public static RoleDescriptor.IndicesPrivileges[] parseIndices(String roleName, XContentParser parser, boolean allow2xFormat)
Expand Down Expand Up @@ -1821,5 +1856,6 @@ public interface Fields {
ParseField TYPE = new ParseField("type");
ParseField RESTRICTION = new ParseField("restriction");
ParseField WORKFLOWS = new ParseField("workflows");
ParseField DESCRIPTION = new ParseField("description");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@ public record RoleDescriptorsIntersection(Collection<Set<RoleDescriptor>> roleDe

public static RoleDescriptorsIntersection EMPTY = new RoleDescriptorsIntersection(Collections.emptyList());

private static final RoleDescriptor.Parser ROLE_DESCRIPTOR_PARSER = RoleDescriptor.parserBuilder().allowRestriction(true).build();
private static final RoleDescriptor.Parser ROLE_DESCRIPTOR_PARSER = RoleDescriptor.parserBuilder()
.allowRestriction(true)
.allowDescription(true)
.build();

public RoleDescriptorsIntersection(RoleDescriptor roleDescriptor) {
this(List.of(Set.of(roleDescriptor)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,7 @@ static RoleDescriptor kibanaSystem(String name) {
getRemoteIndicesReadPrivileges("traces-apm.*"),
getRemoteIndicesReadPrivileges("traces-apm-*") },
null,
null,
null
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ public class ReservedRolesStore implements BiConsumer<Set<String>, ActionListene
new String[] { "*" }
)
),
null,
null
);

Expand Down Expand Up @@ -201,6 +202,7 @@ private static Map<String, RoleDescriptor> initializeReservedRoles() {
getRemoteIndicesReadPrivileges("/metrics-(beats|elasticsearch|enterprisesearch|kibana|logstash).*/"),
getRemoteIndicesReadPrivileges("metricbeat-*") },
null,
null,
null
)
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/
package org.elasticsearch.xpack.core.security.support;

import org.elasticsearch.common.Strings;
import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.xpack.core.security.authc.esnative.ClientReservedRealm;
Expand Down Expand Up @@ -204,10 +205,19 @@ public static Error validatePassword(SecureString password) {

public static final class Roles {

public static final int MAX_DESCRIPTION_LENGTH = 1000;

public static Error validateRoleName(String roleName, boolean allowReserved) {
return validateRoleName(roleName, allowReserved, MAX_NAME_LENGTH);
}

public static Error validateRoleDescription(String description) {
if (description != null && description.length() > MAX_DESCRIPTION_LENGTH) {
return new Error(Strings.format("Role description must be less than %s characters.", MAX_DESCRIPTION_LENGTH));
}
return null;
}

static Error validateRoleName(String roleName, boolean allowReserved, int maxLength) {
if (roleName == null) {
return new Error("role name is missing");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ public class SystemUser extends InternalUser {
null,
null,
null,
null,
null
);

Expand Down
Loading