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.

Sign up for GitHub

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

Deprecate opendistro_security_roles and add opensearch_security_roles #5113

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Deprecate opendistro_security_roles and add opensearch_security_roles #5113

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
8c6601b
Deprecate opendistro_security_roles and add opensearch_security_roles…
shikharj05 Feb 18, 2025
54c72fa
Fix rest integ tests
shikharj05 Feb 18, 2025
ab26317
Get combined list of roles while validating the roles
shikharj05 Feb 18, 2025
4b27f5c
spotless apply
shikharj05 Feb 18, 2025
afe19d8
Merge branch 'opensearch-project:main' into opendistro-api-req-resp
shikharj05 Feb 19, 2025
cb3e1cd
Rename opensearch_security_roles to direct_security_roles
shikharj05 Feb 19, 2025
879f575
spotless apply
shikharj05 Feb 19, 2025
23b5916
Adding tests for getRoles method
shikharj05 Feb 19, 2025
47d755a
Remove test log configuration
shikharj05 Feb 19, 2025
ba574b6
Merge branch 'main' into opendistro-api-req-resp
shikharj05 Feb 19, 2025
20768b0
Merge branch 'opensearch-project:main' into opendistro-api-req-resp
shikharj05 Feb 25, 2025
736dc78
Update integ tests for both roles
shikharj05 Mar 3, 2025
7e0fcc9
Adding log line for depreacted opendistro_security_roles
shikharj05 Mar 3, 2025
5f10ad6
Add more logs for deprecation notice
shikharj05 Mar 3, 2025
9e28622
Merge branch 'main' into opendistro-api-req-resp
shikharj05 Mar 5, 2025
0674469
Merge branch 'main' into opendistro-api-req-resp
shikharj05 Mar 6, 2025
5b85ba6
Merge branch 'main' into opendistro-api-req-resp
DarshitChanpura Mar 8, 2025
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
2 changes: 1 addition & 1 deletion config/internal_users.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ admin:
anomalyadmin:
hash: "$2y$12$TRwAAJgnNo67w3rVUz4FIeLx9Dy/llB79zf9I15CKJ9vkM4ZzAd3."
reserved: false
opendistro_security_roles:
opensearch_security_roles:
- "anomaly_full_access"
description: "Demo anomaly admin user, using internal role"

Expand Down
8 changes: 7 additions & 1 deletion ...integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@
import static org.opensearch.security.api.PatchPayloadHelper.addOp;
import static org.opensearch.security.api.PatchPayloadHelper.patch;
import static org.opensearch.security.api.PatchPayloadHelper.replaceOp;
import static org.opensearch.security.dlic.rest.api.InternalUsersApiAction.OPENDISTRO_SECURITY_ROLES;
import static org.opensearch.security.dlic.rest.api.InternalUsersApiAction.OPENSEARCH_SECURITY_ROLES;
import static org.opensearch.security.dlic.rest.api.InternalUsersApiAction.RESTRICTED_FROM_USERNAME;

public class InternalUsersRestApiIntegrationTest extends AbstractConfigEntityApiIntegrationTest {
Expand Down Expand Up @@ -149,13 +151,17 @@ static ToXContentObject internalUser(
builder.field("attributes", attributes);
}
if (securityRoles != null) {
builder.field("opendistro_security_roles");
builder.field(getRoleField());
securityRoles.toXContent(builder, params);
}
return builder.endObject();
};
}

private static String getRoleField() {
return randomFrom(List.of(OPENDISTRO_SECURITY_ROLES, OPENSEARCH_SECURITY_ROLES));
}

static ToXContentObject defaultServiceUser() {
return serviceUser(null, null, null); // default user is disabled
}
Expand Down
33 changes: 30 additions & 3 deletions src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.fasterxml.jackson.databind.node.ObjectNode;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

import org.opensearch.cluster.service.ClusterService;
import org.opensearch.common.inject.Inject;
Expand Down Expand Up @@ -51,8 +53,17 @@

public class InternalUsersApiAction extends AbstractApiAction {

private static final Logger log = LogManager.getLogger(InternalUsersApiAction.class);
private final PasswordHasher passwordHasher;

/**
* @deprecated Use opensearch_security_roles instead. This field will be removed in a future release.
*/
@Deprecated
public static final String OPENDISTRO_SECURITY_ROLES = "opendistro_security_roles";

public static final String OPENSEARCH_SECURITY_ROLES = "opensearch_security_roles";

@Override
protected void consumeParameters(final RestRequest request) {
request.param("name");
Expand Down Expand Up @@ -183,8 +194,7 @@ ValidationResult<SecurityConfiguration> validateSecurityRoles(final SecurityConf
rolesConfiguration -> loadConfiguration(CType.ROLESMAPPING, false, false).map(roleMappingsConfiguration -> {
final var contentAsNode = (ObjectNode) securityConfiguration.requestContent();
final var securityJsonNode = new SecurityJsonNode(contentAsNode);
var securityRoles = securityJsonNode.get("opendistro_security_roles").asList();
securityRoles = securityRoles == null ? List.of() : securityRoles;
var securityRoles = getRoles(securityJsonNode);
final var rolesValid = endpointValidator.validateRoles(securityRoles, rolesConfiguration);
if (!rolesValid.isValid()) {
return ValidationResult.error(rolesValid.status(), rolesValid.errorMessage());
Expand All @@ -202,6 +212,22 @@ ValidationResult<SecurityConfiguration> validateSecurityRoles(final SecurityConf
);
}

private List<String> getRoles(SecurityJsonNode content) {
List<String> roles = content.get(OPENSEARCH_SECURITY_ROLES).asList();
// Fall back to deprecated field if new field is not present
if (roles == null) {
roles = content.get(OPENDISTRO_SECURITY_ROLES).asList();
if (roles != null) {
log.warn(
"The field '{}' is deprecated and will be removed in a future release. Please use '{}' instead.",
OPENDISTRO_SECURITY_ROLES,
OPENSEARCH_SECURITY_ROLES
);
}
}
return roles != null ? roles : List.of();
}

ValidationResult<SecurityConfiguration> createOrUpdateAccount(
final RestRequest request,
final SecurityConfiguration securityConfiguration
Expand Down Expand Up @@ -309,7 +335,8 @@ public Map<String, RequestContentValidator.DataType> allowedKeys() {
return allowedKeys.put("backend_roles", DataType.ARRAY)
.put("attributes", DataType.OBJECT)
.put("description", DataType.STRING)
.put("opendistro_security_roles", DataType.ARRAY)
.put(OPENDISTRO_SECURITY_ROLES, DataType.ARRAY)
.put(OPENSEARCH_SECURITY_ROLES, DataType.ARRAY)
.put("hash", DataType.STRING)
.put("password", DataType.STRING)
.build();
Expand Down
13 changes: 10 additions & 3 deletions src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,12 +359,19 @@ public List<String> getSecurityRoles(String user) {

// Security roles should only contain roles that exist in the roles dynamic config.
// We should filter out any roles that have hidden rolesmapping.
return tmp == null
? ImmutableList.of()
: tmp.getOpendistro_security_roles()
if (tmp == null) {
return ImmutableList.of();
} else if (!tmp.getOpensearch_security_roles().isEmpty()) {
return tmp.getOpensearch_security_roles()
.stream()
.filter(role -> !isRolesMappingHidden(role) && rolesV7SecurityDynamicConfiguration.exists(role))
.collect(ImmutableList.toImmutableList());
} else {
return tmp.getOpendistro_security_roles()
.stream()
.filter(role -> !isRolesMappingHidden(role) && rolesV7SecurityDynamicConfiguration.exists(role))
.collect(ImmutableList.toImmutableList());
}
}

// Remove any hidden rolesmapping from the security roles
Expand Down
11 changes: 11 additions & 0 deletions src/main/java/org/opensearch/security/securityconf/impl/v7/InternalUserV7.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ public class InternalUserV7 implements Hideable, Hashed, StaticDefinable {
private Map<String, String> attributes = Collections.emptyMap();
private String description;
private List<String> opendistro_security_roles = Collections.emptyList();
private List<String> opensearch_security_roles = Collections.emptyList();
Copy link
Collaborator

Choose a reason for hiding this comment

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

One high-level remark:

IMHO, config property names should be as self-descriptive as possible. However, neither opendistro_security_roles nor opensearch_security_roles are really self-descriptive.

The function of this property is as follows: Roles which are specified via role property first go through role-mapping so that they are mapped onto the effective roles of the user at runtime. Roles specified via opendistro_security_roles do not go through role mapping, but get effective immediately.

Can we find a property name that is (kind of) self-descriptive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you think about opensearch_security_inline_roles/opensearch_security_static_roles/opensearch_security_direct_roles or any other name :) ?

Copy link
Collaborator

@nibix nibix Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I do not think the opensearch_security prefix is necessary, as the other attribute is also just called roles.

So, that would be inline_roles or static_roles or direct_roles. I would not recommend static_roles, as this could be confused with the static role configuration in https://github.com/opensearch-project/security/blob/main/src/main/resources/static_config/static_roles.yml .

I could also imagine directly_effective_roles, that would be most telling.

@cwperks @DarshitChanpura @derek-ho wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Personally, I think we should consider removing this attribute from the InternalUser object and only do roles mappings with RolesMappings.

What is the advantage of having multiple ways to map roles to usernames?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the advantage of having multiple ways to map roles to usernames?

I would consider these roles as default permissions that a user has when you create such an user. Also, it's simpler to setup for small number of users.

Using role mappings is better for more centralized and flexible management, better for managing similar users, etc.

I think it should be okay to keep this feature for it's simplicity (single API setup/configuration) - also, maybe it will prove helpful with service/system accounts feature in the plugin - i.e. using inline_roles or such directly_effective_roles for accounts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the advantage of having multiple ways to map roles to usernames?

I would agree with @shikharj05 that one advantage is simplicity. On simple clusters without any external auth systems, this can improve the first user experience - under one constraint: It must be clear to the user which option has which purpose. If it is not clear to the user, this possibility actually makes first user experience worse, as this will cause confusion.

To make things even more complicated: There's also another config option plugins.security.roles_mapping_resolution which can be used to use the roles property of the internal users database without roles mapping.

My gist from this is: IMHO, on medium term, the property should be kept with a telling name and a crystal clear documentation. On long term, one might need to re-think the whole role mapping concept to address its larger issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have renamed the property to direct_security_roles - let me know your thoughts @nibix @cwperks .

@cwperks should we create an issue to track the long-term re-evaluation of role mappings.

Copy link
Member

Choose a reason for hiding this comment

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

@cwperks should we create an issue to track the long-term re-evaluation of role mappings.

Sounds like we are going to keep supporting inline roles based on this conversation. I think we can add an issue for migration assistance here. In past versions, there was a Migrate API to help migrating the security config so should we start implementing one to account for these upcoming changes as well?


private InternalUserV7(String hash, boolean reserved, boolean hidden, List<String> backend_roles, Map<String, String> attributes) {
super();
Expand Down Expand Up @@ -117,6 +118,16 @@ public List<String> getOpendistro_security_roles() {

public void setOpendistro_security_roles(List<String> opendistro_security_roles) {
this.opendistro_security_roles = opendistro_security_roles;
this.opensearch_security_roles = opendistro_security_roles;
}

public List<String> getOpensearch_security_roles() {
return opensearch_security_roles;
}

public void setOpensearch_security_roles(List<String> opensearch_security_roles) {
this.opendistro_security_roles = opensearch_security_roles;
this.opensearch_security_roles = opensearch_security_roles;
}

public Map<String, String> getAttributes() {
Expand Down
Loading