-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Introduce role description field #107088
Conversation
Hi @slobodanadamovic, I've created a changelog YAML for you. |
…/elasticsearch into sa-add-role-description
…ole-description # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java
…ole-description # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java
The method for `RoleDescriptor` parsing is becoming more complex due to its usage being shared between API keys, file-based and native roles. In some cases we allow 2.X format, in others we disallow restrictions. Having a single method with multiple boolean flags that control inclusion/exclusion of fields is becoming hard to extend. This refactoring aims to allow easier introduction of new fields that should be conditionally supported in some cases but not others. One of such cases is introduction of `description` field that should only be supported for file-based and native roles but not for roles embedded in API keys.
…tor-role-parsing # Conflicts: # x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java
…c/elasticsearch into sa-add-role-description # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/role/PutRoleRequestBuilder.java # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/CrossClusterAccessSubjectInfo.java # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptor.java # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptorsIntersection.java # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptorTests.java # x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java # x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java # x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java # x-pack/plugin/security/src/test/java/org/elasticsearch/test/TestSecurityClient.java # x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java # x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/permission/FieldPermissionsTests.java
…ole-description # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java
The method for `RoleDescriptor` parsing is becoming more complex due to its usage being shared between API keys, file-based and native roles. In some cases we allow 2.X format, in others we disallow restrictions. Having a single method with multiple boolean flags that control inclusion/exclusion of fields is becoming hard to extend. This refactoring aims to allow easier introduction of new fields that should be conditionally supported in some cases but not others. One of such cases is introduction of `description` field that should only be supported for file-based and native roles but not for roles embedded in API keys. Relates to: #107088
…ole-description # Conflicts: # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/apikey/ApiKey.java # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/apikey/BulkUpdateApiKeyRequestTranslator.java # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/apikey/CreateApiKeyRequestBuilder.java # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/apikey/UpdateApiKeyRequestTranslator.java # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/role/PutRoleRequestBuilder.java # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/CrossClusterAccessSubjectInfo.java # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptor.java # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptorsIntersection.java # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/service/GetServiceAccountResponseTests.java # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptorTests.java # x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/role/RoleWithRemoteIndicesPrivilegesRestIT.java # x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java # x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/apikey/ApiKeySingleNodeTests.java # x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java # x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java # x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java # x-pack/plugin/security/src/test/java/org/elasticsearch/test/TestSecurityClient.java # x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java # x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/permission/FieldPermissionsTests.java
…ole-description # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java
## Summary In preparation for the KB UI and ES API to accept `description` for Roles, Kibana `get`, `getAll`, and `put` Roles routes should handle a description. ## Testing Start KB/ES locally In Dev Tools PUT role: ``` PUT kbn:api/security/role/mytestrole { "description": "This is a test role", "metadata": { "version": 1 }, "elasticsearch": { "cluster": [ ], "indices": [ ] }, "kibana": [ { "base": [ ], "feature": { "discover": [ "all" ], "visualize": [ "all" ], "dashboard": [ "all" ], "dev_tools": [ "read" ], "advancedSettings": [ "read" ], "indexPatterns": [ "read" ], "graph": [ "all" ], "apm": [ "read" ], "maps": [ "read" ], "canvas": [ "read" ], "infrastructure": [ "all" ], "logs": [ "all" ], "uptime": [ "all" ] }, "spaces": [ "*" ] } ] } ``` This will fail since ES doesn't accept `descriptions` yet Pull the ES Role Description PR elastic/elasticsearch#107088 and run `yarn es source` and `yarn start` Rerun the PUT above, receive 204! Check the role `description` with a GET: ``` GET kbn:api/security/role/mytestrole ``` <img width="1712" alt="Screenshot 2024-04-30 at 9 43 32 PM" src="https://github.com/elastic/kibana/assets/21210601/20394086-f223-4be8-8660-eb8d3930116c"> It has a limit of 2048 per the requirements: <img width="2248" alt="Screenshot 2024-04-30 at 9 45 13 PM" src="https://github.com/elastic/kibana/assets/21210601/7dc97a8e-1246-49e4-954e-885be433c7c7">
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.
Changes are looking good. A few comments.
(also, this really highlights that we should refactor some of this ... changing 55 files with over 1k lines of change to add a description field is a bit excessive, but nice job finding all the places).
listener.onFailure( | ||
new IllegalStateException( | ||
"all nodes must have transport version [" | ||
+ TransportVersions.SECURITY_ROLE_DESCRIPTION |
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 error message pattern been updated to not include "transport" in message and use .toReleaseVersion()
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.
Good catch! Will fix.
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.
Fixed in: 35a80dc
...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptor.java
Show resolved
Hide resolved
@@ -193,6 +194,7 @@ public void testCrossClusterAccessHeadersSentSingleRemote() throws Exception { | |||
null, | |||
null, | |||
null, | |||
null, |
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.
nit: can you add a comment here that description is intentinally null and never sent across clusters (wouldn't hurt to make a similar comment in org.elasticsearch.xpack.core.security.authz.permission.SimpleRole#getRoleDescriptorsIntersectionForRemoteCluster )
@@ -964,7 +964,8 @@ public ClusterPermission.Builder buildPermission(ClusterPermission.Builder build | |||
new RoleDescriptor.RemoteIndicesPrivileges[] { | |||
RoleDescriptor.RemoteIndicesPrivileges.builder("remote-*", "remote").indices("abc-*", "xyz-*").privileges("read").build(), | |||
RoleDescriptor.RemoteIndicesPrivileges.builder("remote-*").indices("remote-idx-1-*").privileges("read").build(), }, | |||
null | |||
null, |
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.
IIUC, since we don't move the description from role descriptors to roles via the composite role store, the description will never get cached in the role cache. I think that is correct to save memory, but could be problematic if we ever pull from the cache in any CRUD workflows. Is it possible to add a test that it is not part of the cache + a comment that we are explicilty not caching the description ?
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.
That's correct. Thanks for calling it out. From my point of view, the description is only relevant for representation and it's not relevant when constructing internal role models for checking permissions (same as metadata
).
could be problematic if we ever pull from the cache in any CRUD workflows. Is it possible to add a test that it is not part of the cache + a comment that we are explicitly not caching the description ?
Makes sense to add a comment but not sure how I would test this. A single role can be built from multiple role descriptors. What we cache is a Role
data model which per contract does not define a description property.
.setSettings(getMainIndexSettings()) | ||
.setAliasName(SECURITY_MAIN_ALIAS) | ||
.setIndexFormat(INTERNAL_MAIN_INDEX_FORMAT) | ||
.setVersionMetaKey(SECURITY_VERSION_STRING) | ||
.setOrigin(SECURITY_ORIGIN) | ||
.setThreadPools(ExecutorNames.CRITICAL_SYSTEM_INDEX_THREAD_POOLS) | ||
.setPriorSystemIndexDescriptors( | ||
List.of( | ||
SystemIndexDescriptor.builder() |
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.
nit: can you DRY up this builder and only diverge as needed.
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.
Addressed in be602ab
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've also put the remote_cluster under the same new mapping version: 981bec3
@@ -385,6 +402,12 @@ private XContentBuilder getMainIndexMappings() { | |||
builder.field("type", "keyword"); | |||
builder.endObject(); | |||
|
|||
if (version >= SecurityMainIndexMappingVersion.ADD_DESCRIPTION_FIELD) { |
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.
why do we need this conditional ? don't we always want to install the mappings on newer versions ?
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 getMainIndexMappings
method now accepts version
and it is also called with the lower mapping version (1) when creating descriptor of prior versions.
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.
If you want to, you could make use of VersionId
interface to provide a slightly nicer interface to SecurityMainIndexMappingVersion
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.
TIL Implementing VersionId
would fit perfectly here. Thanks for the suggestion.
.setSettings(getMainIndexSettings()) | ||
.setAliasName(SECURITY_MAIN_ALIAS) | ||
.setIndexFormat(INTERNAL_MAIN_INDEX_FORMAT) | ||
.setVersionMetaKey(SECURITY_VERSION_STRING) | ||
.setOrigin(SECURITY_ORIGIN) | ||
.setThreadPools(ExecutorNames.CRITICAL_SYSTEM_INDEX_THREAD_POOLS) | ||
.setPriorSystemIndexDescriptors( |
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 someone help to understand why we need to provide the prior index descriptors ? Shouldn't the logic always be install the new version as soon as possible (at least for passive changes such additions) ?
…ole-description # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/role/PutRoleRequest.java # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptor.java # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStore.java # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/apikey/BulkUpdateApiKeyRequestTests.java # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/apikey/CreateApiKeyRequestTests.java # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/apikey/UpdateApiKeyRequestTests.java # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptorTests.java # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/SimpleRoleTests.java # x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/apikey/ApiKeyRestIT.java # x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/crossclusteraccess/CrossClusterAccessHeadersForCcsRestIT.java # x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java # x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java # x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java # x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java # x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceIntegTests.java # x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java # x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java # x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreTests.java # x-pack/plugin/security/src/test/resources/org/elasticsearch/xpack/security/authz/store/roles.yml # x-pack/qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/ApiKeyBackwardsCompatibilityIT.java
|
||
RemoteClusterPermissions remoteClusters = RemoteClusterPermissions.NONE; | ||
if (allowRemoteClusters && randomBoolean()) { | ||
randomRemoteClusterPermissions(randomIntBetween(1, 5)); |
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.
While refactoring random role descriptor creation, I've noticed that we've never assigned randomRemoteClusterPermissions
to remoteClusters
. This uncovered two failing tests. It is fixed in RoleDescriptorTestHelper
.
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.
whoops ! thanks for fixing.
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.
LGTM
@@ -187,6 +193,7 @@ public RoleDescriptor( | |||
? remoteClusterPermissions | |||
: RemoteClusterPermissions.NONE; | |||
this.restriction = restriction != null ? restriction : Restriction.NONE; | |||
this.description = description; |
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.
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.
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 was undecided here and went with null
. Agreed, using empty string is more consistent and simplifies the checks. I will change this.
&& clusterService.state().getMinTransportVersion().before(TransportVersions.SECURITY_ROLE_DESCRIPTION)) { | ||
listener.onFailure( | ||
new IllegalStateException( | ||
"all nodes must have transport version [" |
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.
nit: all nodes must have version ["
{ | ||
builder.field("type", "object"); | ||
builder.startObject("properties"); | ||
if (mappingVersion.onOrAfter(SecurityMainIndexMappingVersion.ADD_REMOTE_CLUSTER_AND_DESCRIPTION_FIELDS)) { |
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.
thank you for handling this !
|
||
RemoteClusterPermissions remoteClusters = RemoteClusterPermissions.NONE; | ||
if (allowRemoteClusters && randomBoolean()) { | ||
randomRemoteClusterPermissions(randomIntBetween(1, 5)); |
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.
whoops ! thanks for fixing.
import static org.hamcrest.Matchers.equalTo; | ||
import static org.hamcrest.Matchers.notNullValue; | ||
|
||
public class RolesBackwardsCompatibilityIT extends AbstractUpgradeTestCase { |
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 appreciate the effort but writing and running BWC tests for every change to our wire serialization is cost prohibitive (both in terms of developer time and CPU time). For normal additions like this it is not needed. However, I would suggest to leave this as it provides a nice framework for future changes we might want to explicitly test. (IMO, the things we want to test are generally breaking changes or major changes to how things work internally or any migrations triggered on upgrades )
…ole-description # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java
This commit updates Role API docs to include new description field (introduced in #107088) and adds descriptions for all built-in roles.
This PR introduces new
description
field to roles definitions.Role API:
File-based role:
Note: I will address the doc changes in a followup PR.