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 16 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
65 changes: 47 additions & 18 deletions .../InternalUsersRestApiIntegrationTest.java → ...tInternalUsersRestApiIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
import org.opensearch.security.dlic.rest.api.Endpoint;
import org.opensearch.test.framework.TestSecurityConfig;
import org.opensearch.test.framework.cluster.TestRestClient;
import org.opensearch.test.framework.cluster.TestRestClient.HttpResponse;

import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.not;
Expand All @@ -43,9 +42,11 @@
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 {
public abstract class AbstractInternalUsersRestApiIntegrationTest extends AbstractConfigEntityApiIntegrationTest {

private final static String REST_API_ADMIN_INTERNAL_USERS_ONLY = "rest_api_admin_iternal_users_only";

Expand All @@ -67,12 +68,12 @@ public class InternalUsersRestApiIntegrationTest extends AbstractConfigEntityApi
);
}

public InternalUsersRestApiIntegrationTest() {
public AbstractInternalUsersRestApiIntegrationTest() {
super("internalusers", new TestDescriptor() {

@Override
public ToXContentObject entityPayload(Boolean hidden, Boolean reserved, Boolean _static) {
return internalUser(hidden, reserved, _static, randomAsciiAlphanumOfLength(10), null, null, null);
return internalUserInitWithoutRoles(hidden, reserved, _static, randomAsciiAlphanumOfLength(10));
}

@Override
Expand All @@ -92,11 +93,37 @@ public Optional<String> restAdminLimitedUser() {
});
}

static ToXContentObject internalUserWithPassword(final String password) {
static ToXContentObject internalUserInitWithoutRoles(
final Boolean hidden,
final Boolean reserved,
final Boolean _static,
final String password
) {
return (builder, params) -> {
builder.startObject();
if (hidden != null) {
builder.field("hidden", hidden);
}
if (reserved != null) {
builder.field("reserved", reserved);
}
if (_static != null) {
builder.field("static", _static);
}
if (password == null) {
builder.field("password").nullValue();
} else {
builder.field("password", password);
}
return builder.endObject();
};
}

ToXContentObject internalUserWithPassword(final String password) {
return internalUser(null, null, null, password, null, null, null);
}

static ToXContentObject internalUser(
ToXContentObject internalUser(
final Boolean hidden,
final Boolean reserved,
final String password,
Expand All @@ -107,7 +134,7 @@ static ToXContentObject internalUser(
return internalUser(hidden, reserved, null, password, backendRoles, attributes, securityRoles);
}

static ToXContentObject internalUser(
ToXContentObject internalUser(
final String password,
final ToXContentObject backendRoles,
final ToXContentObject attributes,
Expand All @@ -116,7 +143,7 @@ static ToXContentObject internalUser(
return internalUser(null, null, null, password, backendRoles, attributes, securityRoles);
}

static ToXContentObject internalUser(
ToXContentObject internalUser(
final Boolean hidden,
final Boolean reserved,
final Boolean _static,
Expand Down Expand Up @@ -149,13 +176,15 @@ 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();
};
}

protected abstract String getRoleField();

static ToXContentObject defaultServiceUser() {
return serviceUser(null, null, null); // default user is disabled
}
Expand Down Expand Up @@ -413,11 +442,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 All @@ -435,7 +464,7 @@ public void filters() throws Exception {
});
}

void assertFilterByUsers(final HttpResponse response, final boolean hasServiceUser, final boolean hasInternalUser) {
void assertFilterByUsers(final TestRestClient.HttpResponse response, final boolean hasServiceUser, final boolean hasInternalUser) {
assertThat(response.getBody(), response.bodyAsJsonNode().has(SERVICE_ACCOUNT_USER), is(hasServiceUser));
assertThat(response.getBody(), response.bodyAsJsonNode().has(NEW_USER), is(hasInternalUser));
}
Expand Down Expand Up @@ -683,10 +712,10 @@ void settingOfUnknownRoleIsNotAllowed(final String predefinedUserName, final Tes
public void parallelPutRequests() throws Exception {
withUser(ADMIN_USER_NAME, client -> {
final var userName = randomAsciiAlphanumOfLength(10);
final var httpResponses = new HttpResponse[10];
final var httpResponses = new TestRestClient.HttpResponse[10];

try (final var executorService = Executors.newFixedThreadPool(httpResponses.length)) {
final var futures = new ArrayList<Future<HttpResponse>>(httpResponses.length);
final var futures = new ArrayList<Future<TestRestClient.HttpResponse>>(httpResponses.length);
for (int i = 0; i < httpResponses.length; i++) {
futures.add(
executorService.submit(
Expand All @@ -702,7 +731,7 @@ public void parallelPutRequests() throws Exception {
}
}
boolean created = false;
for (HttpResponse response : httpResponses) {
for (TestRestClient.HttpResponse response : httpResponses) {
int sc = response.getStatusCode();
switch (sc) {
case HttpStatus.SC_CREATED:
Expand Down
21 changes: 21 additions & 0 deletions ...Test/java/org/opensearch/security/api/InternalUsersRestApiDirectRolesIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*
* Modifications Copyright OpenSearch Contributors. See
* GitHub history for details.
*/

package org.opensearch.security.api;

import static org.opensearch.security.dlic.rest.api.InternalUsersApiAction.DIRECT_SECURITY_ROLES;

public class InternalUsersRestApiDirectRolesIntegrationTest extends AbstractInternalUsersRestApiIntegrationTest {
@Override
protected String getRoleField() {
return DIRECT_SECURITY_ROLES;
}
}
21 changes: 21 additions & 0 deletions .../java/org/opensearch/security/api/InternalUsersRestApiOpenDistroRolesIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*
* Modifications Copyright OpenSearch Contributors. See
* GitHub history for details.
*/

package org.opensearch.security.api;

import static org.opensearch.security.dlic.rest.api.InternalUsersApiAction.OPENDISTRO_SECURITY_ROLES;

public class InternalUsersRestApiOpenDistroRolesIntegrationTest extends AbstractInternalUsersRestApiIntegrationTest {
@Override
protected String getRoleField() {
return OPENDISTRO_SECURITY_ROLES;
}
}
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 direct_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 'direct_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?

Copy link
Member

Choose a reason for hiding this comment

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

Can we do a clean break with a deprecation message that clearly states that this will be remove in 3.0 GA?

"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)
Copy link
Member

Choose a reason for hiding this comment

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

Do we want either to be set or both should be allowed for now?

What is the plan to remove this deprecated key from allowed_keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The plan is to mark these deprecated in 3.0, removal in 4.0. Hence, we need to keep both in 3.0 or decide on whether we just document in 3.0 and remove cleanly in 4.0

.put(DIRECT_SECURITY_ROLES, DataType.ARRAY)
.put("hash", DataType.STRING)
.put("password", DataType.STRING)
.build();
Expand Down
29 changes: 26 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 @@ -308,6 +308,8 @@

private static class InternalUsersModelV7 extends InternalUsersModel {

protected final Logger log = LogManager.getLogger(InternalUsersModelV7.class);

private final SecurityDynamicConfiguration<InternalUserV7> internalUserV7SecurityDynamicConfiguration;

private final SecurityDynamicConfiguration<RoleV7> rolesV7SecurityDynamicConfiguration;
Expand Down Expand Up @@ -357,14 +359,35 @@
public List<String> getSecurityRoles(String user) {
InternalUserV7 tmp = internalUserV7SecurityDynamicConfiguration.getCEntry(user);

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
}

// Log opendistro_security_roles regardless of which path we take
if (tmp.getOpendistro_security_roles() != null && !tmp.getOpendistro_security_roles().isEmpty()) {
log.warn(
"Deprecated configuration opendistro_security_roles set for: {} "
+ "opendistro_security_roles will not be used in favor of direct_security_roles.",
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.getDirect_security_roles() != null && !tmp.getDirect_security_roles().isEmpty()) {
return tmp.getDirect_security_roles()
.stream()
.filter(role -> !isRolesMappingHidden(role) && rolesV7SecurityDynamicConfiguration.exists(role))
.collect(ImmutableList.toImmutableList());
}

if (tmp.getOpendistro_security_roles() != null && !tmp.getOpendistro_security_roles().isEmpty()) {
return tmp.getOpendistro_security_roles()
Copy link
Member

Choose a reason for hiding this comment

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

nit: line 368 logger can be moved here.

.stream()

Check warning on line 386 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#L385-L386 

Added lines #L385 - L386 were not covered by tests
.filter(role -> !isRolesMappingHidden(role) && rolesV7SecurityDynamicConfiguration.exists(role))
.collect(ImmutableList.toImmutableList());

Check warning on line 388 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#L388 

Added line #L388 was not covered by tests
}
return ImmutableList.of();
}

// Remove any hidden rolesmapping from the security roles
Expand Down
16 changes: 16 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 @@ -33,13 +33,17 @@

import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

import org.opensearch.security.securityconf.Hashed;
import org.opensearch.security.securityconf.Hideable;
import org.opensearch.security.securityconf.StaticDefinable;

public class InternalUserV7 implements Hideable, Hashed, StaticDefinable {

private final Logger log = LogManager.getLogger(InternalUserV7.class);

private String hash;
private boolean reserved;
private boolean hidden;
Expand All @@ -51,6 +55,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 @@ -116,7 +121,18 @@ public List<String> getOpendistro_security_roles() {
}

public void setOpendistro_security_roles(List<String> opendistro_security_roles) {
log.warn("Deprecated configuration opendistro_security_roles set. Kindly use direct_security_roles instead.");
this.opendistro_security_roles = opendistro_security_roles;
this.direct_security_roles = opendistro_security_roles;
}
Comment on lines 123 to +127
Copy link
Collaborator

@nibix nibix Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for the delay, I was a bit busy with other stuff. I have thought about this log message again.

This method will be called by Jackson when it de-serializes the internal user JSON. It will be called for each user object.

As in the security config index, all user records are stored as one big JSON in a single document, Jackson needs to de-serialize all user objects, even the security plugin needs only one (for example for a REST request). This will have the effect that whenever the user config is being loaded from the index, you'll get this log message repeated for each user stored in that index. On clusters with many users, these will be quite a lot repeated log messages at once.

This would happen in these cases:

  • On each node: On cluster startup, when the index is initially read
  • On each node: Whenever the configuration is updated
  • On a single node: Whenever internalusers REST API is used (be it GET or PUT or PATCH), the index is freshly read initially, so these logs would also appear for all users, even if the REST API only addresses a single user. If this is a PUT or PATCH request, you'd see the logs repeated twice, once for reading the existing users, once for updating the changed user information.

I think such repeated log messages won't be helpful to the user. I would guess that we need a different solution to notify the user about the deprecated attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries. Thanks for the detailed explanation. My understanding was that the log lines would be added only during initial read and during updates (limited to a specific user); And that once the configuration is in memory, it won't be fetched from the index unless a flush was called.

I will remove the log line to avoid excessive logging.

I would guess that we need a different solution to notify the user about the deprecated attribute.

Do you think documentation should be enough? something like- https://opensearch.org/docs/latest/breaking-changes/#deprecate-non-inclusive-terms

Copy link
Collaborator

@nibix nibix Mar 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Do you think documentation should be enough?

This is actually a question which I cannot answer just on my own. This really depends on the deprecation philosophy/strategy of the whole project. I am not sure if there is actually something like this formally defined.

In my own humble opinion, a good solution would be like following (disclaimer: I really do not want to give a direction here, I am just sharing my private opinion):

There are two main alternative options:

  1. Change name of the attribute only along with a major update of the config REST APIs. The config REST APIs have many more shortcomings, like very limited validation capabilities, no bulk update capabilities, unusual configuration of privileges, etc. This could be seen as a reason to rebuild the config REST APIs in a major effort. In that effort, this attribute could be also renamed. Up until this point, the "obsolete" name of this attribute is such a minor disturbance that it can be tolerated.

  2. Support both attribute names, but have already at the beginning a solution that can be kept for the longer future. That means that any retrieval of the configuration will only show one attribute name, either the opendistro one or the new one. Which one is shown is determined by the last update: If the client used the new attribute name in the update, the new one is also used when retrieving the document. If the client used the old attribute name in the update, the old one is also used when retrieving the document. This state can be kept for the foreseeable future. Because there will be never a time when an admin needs to get active and convert the config, there is also no need for a deprecation message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which one is shown is determined by the last update: If the client used the new attribute name in the update, the new one is also used when retrieving the document. If the client used the old attribute name in the update, the old one is also used when retrieving the document.

nit: This might get problematic with number of clients? For e.g. client A using old attribute v/s client B using the new attribute.

I am more aligned with option 1 - i.e. track with a major update if required. Alternatively, we could introduce new APIs to deprecate old functionality. i.e. new APIs return new attribute names, existing ones use old one. (would have been cleaner if this was done when APIs moved to _plugins/_security from _opendistro API paths)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This might get problematic with number of clients? For e.g. client A using old attribute v/s client B using the new attribute.

Yes, good point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cwperks @nibix - how do you think we should proceed here?

Copy link
Member

Choose a reason for hiding this comment

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

Can we show both for now with deprecation documented on the website? We do set the direct_security_roles atttribute even when opendistro_security_roles is set, which should not affect any roles being missed in the new key when populating the deprecated key. Apologies if I didn't understand the problem correctly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The following is really just IMHO, so please be invited to have different opinions: ;-)

  1. The change of the attribute names provides too little value to justify the investment of significant effort. Meanwhile, like stated above, the security config APIs have many shortcomings. Thus, one option would be to postpone the renaming until the security config APIs get a new version. I could offer to write an RFC on new security config APIs.
  2. Alternatively, of course, we can have the changes as in this PR. The log message in its present form won't be possible, though. We can also document this as a deprecation in the user docs. Still, I am then unsure of the path forward. Is there a possibility to drop the old attribute name at some point? And what would be the strategy for that? I tried to find a document on OpenSearch deprecation strategies to get some guidance, but I could not find one.


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
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

are opensearch_security_roles and direct_security_roles same? Asking, as i see direct_security_roles being used in config/internal_users.yml file in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The initial name was opensearch_security_roles, changed to direct_security_roles as per discussion on the PR. The test results are from before the rename :)

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

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