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 11 commits
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:
direct_security_roles:
- "anomaly_full_access"
description: "Demo anomaly admin user, using internal role"

Expand Down
18 changes: 12 additions & 6 deletions ...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.DIRECT_SECURITY_ROLES;
import static org.opensearch.security.dlic.rest.api.InternalUsersApiAction.OPENDISTRO_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, DIRECT_SECURITY_ROLES));
}

static ToXContentObject defaultServiceUser() {
return serviceUser(null, null, null); // default user is disabled
}
Expand Down Expand Up @@ -413,11 +419,11 @@ void assertInternalUser(
assertThat(actualObjectNode.toPrettyString(), not(actualObjectNode.has("hash")));
assertThat(actualObjectNode.toPrettyString(), actualObjectNode.get("backend_roles"), is(expectedObjectNode.get("backend_roles")));
assertThat(actualObjectNode.toPrettyString(), actualObjectNode.get("attributes"), is(expectedObjectNode.get("attributes")));
assertThat(
actualObjectNode.toPrettyString(),
actualObjectNode.get("opendistro_security_roles"),
is(expectedObjectNode.get("opendistro_security_roles"))
);
// can be either of OPENSEARCH_SECURITY_ROLES or OPENDISTRO_SECURITY_ROLES
JsonNode expectedRoles = expectedObjectNode.get(DIRECT_SECURITY_ROLES) != null
? expectedObjectNode.get(DIRECT_SECURITY_ROLES)
: expectedObjectNode.get(OPENDISTRO_SECURITY_ROLES);
assertThat(actualObjectNode.toPrettyString(), actualObjectNode.get("opendistro_security_roles"), is(expectedRoles));
}

String filterBy(final String value) {
Expand Down
51 changes: 48 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 @@ -12,12 +12,17 @@
package org.opensearch.security.dlic.rest.api;

import java.io.IOException;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

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 @@ -53,8 +58,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 DIRECT_SECURITY_ROLES = "direct_security_roles";

@Override
protected void consumeParameters(final RestRequest request) {
request.param("name");
Expand Down Expand Up @@ -209,8 +223,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 @@ -228,6 +241,37 @@ ValidationResult<SecurityConfiguration> validateSecurityRoles(final SecurityConf
);
}

/**
* This method combines roles from both 'opensearch_security_roles' and the deprecated 'opendistro_security_roles' fields.
* If the deprecated field is used, a warning message is logged.
*
* @param content The SecurityJsonNode containing the security configuration
* @return A List of merged unique roles. Returns an empty list if no roles are found in either field.
*/
List<String> getRoles(SecurityJsonNode content) {
List<String> openSearchRoles = content.get(DIRECT_SECURITY_ROLES).asList();
List<String> openDistroRoles = content.get(OPENDISTRO_SECURITY_ROLES).asList();

Set<String> mergedRoles = new HashSet<>();

// Add OpenSearch roles if present
if (openSearchRoles != null) {
mergedRoles.addAll(openSearchRoles);
}

// Add OpenDistro roles if present
if (openDistroRoles != null) {
mergedRoles.addAll(openDistroRoles);
log.warn(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indirectly related: What's the migration path towards having only the new attribute?

This will only log warnings when the REST API is used. No warnings will be logged for usage of the securityadmin tool or just the existing data in the config index.

Will there be an additional change in the future to address existing data?

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's the migration path towards having only the new attribute?

Looking at other migrations/deprecation of terms, I think we can announce deprecation/removal in docs.
e.g. - https://opensearch.org/docs/latest/breaking-changes/#deprecate-non-inclusive-terms

Will there be an additional change in the future to address existing data?

Yes, I was thinking Security plugin should log a warning during initialization if configs have opendistro_security_role present. will this suffice?

"The field '{}' is deprecated and will be removed in a future release. Please use '{}' instead.",
OPENDISTRO_SECURITY_ROLES,
DIRECT_SECURITY_ROLES
);
}

return mergedRoles.isEmpty() ? List.of() : new ArrayList<>(mergedRoles);
}

ValidationResult<SecurityConfiguration> createOrUpdateAccount(
final RestRequest request,
final SecurityConfiguration securityConfiguration
Expand Down Expand Up @@ -335,7 +379,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(DIRECT_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 @@

// 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();

Check warning on line 363 in src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java#L363 

Added line #L363 was not covered by tests
} else if (!tmp.getDirect_security_roles().isEmpty()) {
return tmp.getDirect_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> direct_security_roles = Collections.emptyList();

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.direct_security_roles = opendistro_security_roles;
}

public List<String> getDirect_security_roles() {
return direct_security_roles;
}

public void setDirect_security_roles(List<String> direct_security_roles) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does the serialized JSON produced from this bean look like? I reckon, it will contain both attributes, even if one is unused, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, I have not removed/modified the old attribute to keep it backwards compatible. Do you think it will lead to confusion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am generally wondering how clients consuming that API should handle this.

This actually already applies to the security Dashboards plugin, which uses this API.

Ideally, an API could just switch the the new attribute and use that. However, in this case, all API clients would also need to have a kind of detection mechanism to see which attribute is the one that is actually in use.

This is especially critical for the PATCH API, which such clients cannot use anymore without making a GET in advance to find out what attribute would be in use.

Generally, that seems to be not a good design of an API.

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 am generally wondering how clients consuming that API should handle this. This actually already applies to the security Dashboards plugin, which uses this API.

Since both the fields are maintained and updated on changes, clients using existing field will not break. As the existing field is being deprecated, guidance to use new field will be documented. As both the fields are available, I don't think a detection mechanism is required. I agree it would have been cleaner to switch to the new attribute, however I am not sure if we can do it without breaking existing clients.

See example below-

curl -k -u admin:<password> 'https://localhost:9200/_plugins/_security/api/internalusers/testuser?pretty' -X PATCH -d '[{"op": "replace", "path": "/opendistro_security_roles", "value": ["notebooks_full_access"]}]' -H 'content-type: application/json'     

{
  "status" : "OK",
  "message" : "'testuser' updated."
}


curl -k -u admin:<password> 'https://localhost:9200/_opendistro/_security/api/user/testuser?pretty'                                             
{
  "testuser" : {
    "hash" : "",
    "reserved" : false,
    "hidden" : false,
    "backend_roles" : [
      "role 1",
      "role 2"
    ],
    "attributes" : {
      "attribute1" : "value1",
      "attribute2" : "value2"
    },
    "opendistro_security_roles" : [
      "manage_snapshots"
    ],
    "opensearch_security_roles" : [
      "manage_snapshots"
    ],
    "static" : false
  }
}

this.opendistro_security_roles = direct_security_roles;
this.direct_security_roles = direct_security_roles;
}

public Map<String, String> getAttributes() {
Expand Down
59 changes: 59 additions & 0 deletions ...test/java/org/opensearch/security/dlic/rest/api/InternalUsersApiActionValidationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.opensearch.security.securityconf.impl.v7.InternalUserV7;
import org.opensearch.security.securityconf.impl.v7.RoleV7;
import org.opensearch.security.support.ConfigConstants;
import org.opensearch.security.support.SecurityJsonNode;
import org.opensearch.security.user.UserService;
import org.opensearch.security.util.FakeRestRequest;

Expand All @@ -39,10 +40,12 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.allOf;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.Matchers.is;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.Mockito.when;

public class InternalUsersApiActionValidationTest extends AbstractApiActionValidationTest {
Expand Down Expand Up @@ -187,6 +190,62 @@ public void validateSecurityRolesWithImmutableRolesMappingConfig() throws Except
assertFalse(result.isValid());
}

@Test
public void testGetRolesOnlyNoRoles() throws IOException {
String jsonString = "{}";
var content = SecurityJsonNode.fromJson(jsonString);
List<String> result = createInternalUsersApiAction().getRoles(content);
assertEquals(0, result.size());
}

@Test
public void testGetRolesOnlyOpenDistroRoles() throws IOException {
String jsonString = """
{
"opendistro_security_roles": ["opendistro_security_role_1", "opendistro_security_role_2"]
}
""";
var content = SecurityJsonNode.fromJson(jsonString);
List<String> result = createInternalUsersApiAction().getRoles(content);

assertEquals(2, result.size());
assertThat(result, hasItem("opendistro_security_role_1"));
assertThat(result, hasItem("opendistro_security_role_2"));
}

@Test
public void testGetRolesOnlyDirectRoles() throws IOException {
String jsonString = """
{
"direct_security_roles": ["direct_security_role_1", "direct_security_role_2"]
}
""";
var content = SecurityJsonNode.fromJson(jsonString);
List<String> result = createInternalUsersApiAction().getRoles(content);

assertEquals(2, result.size());
assertThat(result, hasItem("direct_security_role_1"));
assertThat(result, hasItem("direct_security_role_2"));
}

@Test
public void testGetRolesBothSettingsForRoles() throws IOException {
String jsonString = """
{
"opendistro_security_roles": ["opendistro_security_role_1", "opendistro_security_role_2"],
"direct_security_roles": ["direct_security_role_1", "direct_security_role_2"]
}
""";
List<String> result = createInternalUsersApiAction().getRoles(SecurityJsonNode.fromJson(jsonString));

assertEquals(4, result.size());
assertThat(result, hasItem("direct_security_role_1"));
assertThat(result, hasItem("direct_security_role_2"));
assertThat(result, hasItem("opendistro_security_role_1"));
assertThat(result, hasItem("opendistro_security_role_2"));

}

private InternalUsersApiAction createInternalUsersApiAction() {
return new InternalUsersApiAction(
clusterService,
Expand Down
Loading