-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Make reserved built-in roles queryable #117581
Merged
elasticsearchmachine
merged 68 commits into
elastic:main
from
slobodanadamovic:sa-queryable-built-in-roles
Dec 16, 2024
Merged
Changes from 20 commits
Commits
Show all changes
68 commits
Select commit
Hold shift + click to select a range
73f0307
Make reserved built-in roles queryable
slobodanadamovic 784a922
Merge branch 'main' into sa-queryable-built-in-roles
slobodanadamovic 3c78126
export queryable classes
slobodanadamovic 5a1c233
Merge branch 'main' into sa-queryable-built-in-roles
slobodanadamovic 252e140
suppress 'this-escape' warning
slobodanadamovic 9bba1aa
Merge branch 'main' into sa-queryable-built-in-roles
slobodanadamovic c3b41ab
introduce a factory interface in order to be able to inject different…
slobodanadamovic b83410c
export org.elasticsearch.xpack.security.authz.store
slobodanadamovic cb70eff
allow returning all file role definitions
slobodanadamovic 31b4fb2
mark query roles API as public in serverless
slobodanadamovic 4d6bd57
imports are important
slobodanadamovic 8ca36ad
remove assertion as it's not true in all use cases
slobodanadamovic 7b278e6
remove unused import
slobodanadamovic 8c71c73
test reserved roles are all indexed and cannot be modified via API
slobodanadamovic 58f132e
Merge branch 'main' of github.com:elastic/elasticsearch into sa-query…
slobodanadamovic c679261
Merge branch 'main' of github.com:elastic/elasticsearch into sa-query…
slobodanadamovic 9cae158
document feature flag
slobodanadamovic 57cac14
code cleanup
slobodanadamovic 1330265
Merge branch 'main' of github.com:elastic/elasticsearch into sa-query…
slobodanadamovic e821710
test that hash calculation produces consistent hash digest
slobodanadamovic d27c91b
update log message
slobodanadamovic 9e7076d
simple unit test for reserved roles provider
slobodanadamovic 0e85b77
Merge branch 'main' of github.com:elastic/elasticsearch into sa-query…
slobodanadamovic 1b56b95
make collections immutable
slobodanadamovic f35ba07
fix failing test
slobodanadamovic 71ed79b
remove unused import
slobodanadamovic f4e0292
Update docs/changelog/117581.yaml
slobodanadamovic d94da64
mark query API as internal for now
slobodanadamovic ada74e1
mark correct API as internal
slobodanadamovic 2defd9b
Merge branch 'main' of github.com:elastic/elasticsearch into sa-query…
slobodanadamovic 32d4494
test get reserved roles
slobodanadamovic ba7541c
Merge branch 'main' of github.com:elastic/elasticsearch into sa-query…
slobodanadamovic 56d49b3
Merge branch 'main' into sa-queryable-built-in-roles
slobodanadamovic 21646f5
remove QueryableBuiltInRolesStore and depend on NativeRolesStore
slobodanadamovic a229e82
move concrete index name resolution after index gets created
slobodanadamovic 8e551b7
revert changes to QueryRoleIT
slobodanadamovic c633037
test that bulk delete of built-in roles fails
slobodanadamovic d4f99f2
log 'expected' errors at info level
slobodanadamovic 8cc05ab
use delegateFailureAndWrap
slobodanadamovic 085755d
sanity check rolesToDelete and rolesToUpsert
slobodanadamovic c3ee0b5
return empty query result if native roles are disabled
slobodanadamovic a6bc077
naming nit validateRoles -> allowReservedRoleNames
slobodanadamovic e12c3e8
trace -> debug
slobodanadamovic 70181d6
change role hashing implementation to hash ordered and flattened JSON…
slobodanadamovic aa64c4d
Merge branch 'main' of github.com:elastic/elasticsearch into sa-query…
slobodanadamovic 7dce761
avoid magic numbers in assertion
slobodanadamovic d400669
introduce a test plugin in order to test reserved roles change
slobodanadamovic 78ea695
ignore javadoc in test plugin
slobodanadamovic cde5382
test closing and deleting .security index
slobodanadamovic 1b54b5c
imports
slobodanadamovic f0d4810
wait a bit longer after cluster restart
slobodanadamovic 1b88a5d
move static methods to utility class
slobodanadamovic fec25ae
better exception handling and code cleanup
slobodanadamovic a771b52
Merge branch 'main' of github.com:elastic/elasticsearch into sa-query…
slobodanadamovic 1fa03d7
fix logger usage
slobodanadamovic 36f1085
unit test rolesToUpsert and rolesToDelete
slobodanadamovic 5aacc30
wait for the index to be deleted
slobodanadamovic ac5c837
handle cases when .security index gets deleted in the mean time
slobodanadamovic 9d5982b
Merge branch 'main' into sa-queryable-built-in-roles
slobodanadamovic 123c890
switch to LinkedHashSet to keep ordering same as before (when List wa…
slobodanadamovic ee484b3
validateRoleDescriptors
slobodanadamovic 5cd4e81
deduplicate log messages
slobodanadamovic 7d09bbf
validateRoleNames
slobodanadamovic ca6485a
test with randomized metadata order
slobodanadamovic d9f69f7
test no updates with different digests instances
slobodanadamovic d691466
test no updates needed with randomized role and its copy
slobodanadamovic 8343448
Merge branch 'main' into sa-queryable-built-in-roles
slobodanadamovic a253252
Merge branch 'main' into sa-queryable-built-in-roles
slobodanadamovic File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
79 changes: 79 additions & 0 deletions
79
...asic/src/javaRestTest/java/org/elasticsearch/xpack/security/QueryableReservedRolesIT.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
package org.elasticsearch.xpack.security; | ||
|
||
import org.elasticsearch.client.Request; | ||
import org.elasticsearch.client.ResponseException; | ||
import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore; | ||
import org.elasticsearch.xpack.security.support.SecurityMigrations; | ||
import org.junit.BeforeClass; | ||
|
||
import static org.elasticsearch.xpack.security.QueryRoleIT.assertQuery; | ||
import static org.elasticsearch.xpack.security.QueryRoleIT.waitForMigrationCompletion; | ||
import static org.hamcrest.Matchers.containsString; | ||
import static org.hamcrest.Matchers.equalTo; | ||
import static org.hamcrest.Matchers.is; | ||
import static org.hamcrest.Matchers.iterableWithSize; | ||
import static org.hamcrest.Matchers.oneOf; | ||
|
||
public class QueryableReservedRolesIT extends SecurityInBasicRestTestCase { | ||
|
||
@BeforeClass | ||
public static void setup() { | ||
new ReservedRolesStore(); | ||
} | ||
|
||
public void testQueryDeleteOrUpdateReservedRoles() throws Exception { | ||
waitForMigrationCompletion(adminClient(), SecurityMigrations.ROLE_METADATA_FLATTENED_MIGRATION_VERSION); | ||
|
||
final String[] allReservedRoles = ReservedRolesStore.names().toArray(new String[0]); | ||
assertQuery(""" | ||
{ "query": { "bool": { "must": { "term": { "metadata._reserved": true } } } }, "size": 100 } | ||
""", 31, roles -> { | ||
assertThat(roles, iterableWithSize(31)); | ||
for (var role : roles) { | ||
assertThat((String) role.get("name"), is(oneOf(allReservedRoles))); | ||
} | ||
}); | ||
|
||
final String roleName = randomFrom(allReservedRoles); | ||
assertQuery(String.format(""" | ||
{ "query": { "bool": { "must": { "term": { "name": "%s" } } } } } | ||
""", roleName), 1, roles -> { | ||
assertThat(roles, iterableWithSize(1)); | ||
assertThat((String) roles.get(0).get("name"), equalTo(roleName)); | ||
}); | ||
|
||
assertDeleteReservedRole(roleName); | ||
assertCreateOrUpdateReservedRole(roleName); | ||
} | ||
|
||
private void assertDeleteReservedRole(String roleName) throws Exception { | ||
slobodanadamovic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Request request = new Request("DELETE", "/_security/role/" + roleName); | ||
slobodanadamovic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
var e = expectThrows(ResponseException.class, () -> adminClient().performRequest(request)); | ||
assertThat(e.getMessage(), containsString("role [" + roleName + "] is reserved and cannot be deleted")); | ||
} | ||
|
||
private void assertCreateOrUpdateReservedRole(String roleName) throws Exception { | ||
Request request = new Request(randomBoolean() ? "PUT" : "POST", "/_security/role/" + roleName); | ||
request.setJsonEntity(""" | ||
{ | ||
"cluster": ["all"], | ||
"indices": [ | ||
{ | ||
"names": ["*"], | ||
"privileges": ["all"] | ||
} | ||
] | ||
} | ||
"""); | ||
var e = expectThrows(ResponseException.class, () -> adminClient().performRequest(request)); | ||
assertThat(e.getMessage(), containsString("Role [" + roleName + "] is reserved and may not be used.")); | ||
} | ||
|
||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Using ordered
TreeMap
here to produce consistent hash.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! Just in case you've considered the same thing: instead of imposing this constraint here, I wonder if it makes sense to confine it to
QueryableBuiltInRolesUtils
by copying the role descriptor. That would avoid data copying on each role descriptor creation. Just a thought though, good to leave as is.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 thinking of a similar(ish) approach: serialize to JSON, flatten all properties, sort them and then hash strings. This way I would avoid imposing any order in the constructor or during parsing of roles, but rather during hashing. Also, we would avoid copying role descriptors (not really true as we would copy a json map) and sorting would be confined in
QueryableBuiltInRolesUtils
. LMWYTThere 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 could look something like this:
Edit: I pushed the change 70181d6. I think this is a better approach as it should be future proof in case we add new map properties.