-
Notifications
You must be signed in to change notification settings - Fork 320
Deprecate opendistro_security_roles and add opensearch_security_roles #5113
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
Deprecate opendistro_security_roles and add opensearch_security_roles #5113
Conversation
… for InternalUsersApiAction Signed-off-by: shikharj05 <[email protected]>
@@ -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(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :) ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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?
Signed-off-by: shikharj05 <[email protected]>
Signed-off-by: shikharj05 <[email protected]>
Signed-off-by: shikharj05 <[email protected]>
Signed-off-by: shikharj05 <[email protected]>
Signed-off-by: shikharj05 <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5113 +/- ##
==========================================
- Coverage 71.73% 71.73% -0.01%
==========================================
Files 384 384
Lines 23866 23892 +26
Branches 3646 3652 +6
==========================================
+ Hits 17121 17139 +18
- Misses 4915 4923 +8
Partials 1830 1830
🚀 New features to boost your workflow:
|
Signed-off-by: shikharj05 <[email protected]>
Signed-off-by: shikharj05 <[email protected]>
src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java
Outdated
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java
Outdated
Show resolved
Hide resolved
// Add OpenDistro roles if present | ||
if (openDistroRoles != null) { | ||
mergedRoles.addAll(openDistroRoles); | ||
log.warn( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java
Outdated
Show resolved
Hide resolved
return direct_security_roles; | ||
} | ||
|
||
public void setDirect_security_roles(List<String> direct_security_roles) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
}
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
Signed-off-by: shikharj05 <[email protected]>
Signed-off-by: shikharj05 <[email protected]>
Signed-off-by: shikharj05 <[email protected]>
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; | ||
} |
There was a problem hiding this comment.
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
orPUT
orPATCH
), 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 aPUT
orPATCH
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
-
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.
-
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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: ;-)
- 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.
- 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I am not sure if there's opensearch specific guidance. @cwperks do you know?
The general guidance is to follow Semver. Per semver, if an API contract is to be changed - it needs a new major version. I couldn't find anything explicit about whether it's absolutely required to allow for migration path (deprecation cycle) - just that a new major version is a MUST with changes in API terms. I am okay to keep the behavior as is and update documentation to say opendistro_security_roles
will be replaced by direct_security_roles
in 4.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I've seen a versioned API in opensearch and a quick search of the opensearch-api-specification repo doesn't reveal any either. This is the published APIs: https://opensearch-project.github.io/opensearch-api-specification/
When it comes to renaming an API parameter the steps would be:
- Introduce new param name and support old param name, but mark it deprecated
- Remove it in the next major version
To follow that we would need to mark it deprecated in 3.0 and removed in 4.0 to follow that convention. That being said, there have been efforts historically to remove legacy references like opendistro
and kibana
so there is a general understand that they are being phased out but we should do targeted deprecations where we can. Particular around things like security role names or the security index name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Off the top of my head, there is some awkwardness in the core repo around index templates.
There are 2 index template APIs:
PUT _template/<template name>
(v1)PUT _index_template/<template name>
(v2)
From the code it looks like _index_template
is a replacement for _template
but they renamed it instead of versioning and the old APIs have yet to be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Add OpenDistro roles if present | ||
if (openDistroRoles != null) { | ||
mergedRoles.addAll(openDistroRoles); | ||
log.warn( |
There was a problem hiding this comment.
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?
src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java
Show resolved
Hide resolved
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; | ||
} |
There was a problem hiding this comment.
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.
return direct_security_roles; | ||
} | ||
|
||
public void setDirect_security_roles(List<String> direct_security_roles) { |
There was a problem hiding this comment.
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.
@shikharj05 Would you please resolve merge conflicts? We can also pick this back up once 4.0 is closer? |
@shikharj05 closing this PR as last update was 3 months ago. Feel free to open a new one. |
Description
Deprecate opendistro_security_roles and add opensearch_security_roles for internal-user APIs.
Deprecate legacy language from the security plugin repo in 3.0.0 for removal in 4.0.0 #5092
Issues Resolved
#5098
Testing
Manual testing results above.
Logs
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.