-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Security Tokens moved to a new separate index #40742
Security Tokens moved to a new separate index #40742
Conversation
Pinging @elastic/es-security |
test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java
Outdated
Show resolved
Hide resolved
@@ -1,5 +1,5 @@ | |||
{ | |||
"index_patterns" : [ ".security-*" ], |
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 index_patterns
are not used because the security index is not managed using templates anymore. The change in name is solely for doc purposes.
} | ||
} | ||
} | ||
} |
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.
Everything here is copy-pasted from the original security-index-template.json
@@ -589,25 +589,25 @@ public void testRemoteMonitoringCollectorRole() { | |||
|
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.
Renames, nothing to see here!
|
||
final TokenService tokenService = new TokenService(settings, Clock.systemUTC(), client, securityIndex.get(), clusterService); | ||
final TokenService tokenService = new TokenService(settings, Clock.systemUTC(), client, securityIndex.get(), | ||
SecurityIndexManager.buildSecurityTokensIndexManager(client, clusterService), clusterService); |
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.
Adds the SecurityIndexManager
instance for the tokens index.
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!
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, Thank you.
@elasticmachine run elasticsearch-ci/1 |
1 similar comment
@elasticmachine run elasticsearch-ci/1 |
@elasticmachine elasticsearch-ci/1 |
@elasticmachine run elasticsearch-ci/1 |
1 similar comment
@elasticmachine run elasticsearch-ci/1 |
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 haven't found the time to do a full pass through this yet, but did want to leave 1 minor comment...
I will try and get through it early next week.
}, | ||
"metadata" : { | ||
"type" : "object", | ||
"dynamic" : true |
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 should be false
(it's changed in .security since you copied it)
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!
// the superseding token document reference is formated as: "<alias>|<document_id>" ; the alias points to a single index | ||
// containing the document with the said id | ||
updateMap.put("superseded_by", | ||
getTokensIndexForVersion(newTokenVersion).aliasName() + "|" + getTokenDocumentId(newUserTokenId)); |
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 feel like this code, and the code to parse the superseded_by should be consistent about whether to use a fixed alias name or not.
This code implies that it's possible to use this alias|id
format with either index, but the parsing code only supports reading from the token index.
I don't mind which way we go, but it's easier to reason about if they make the same assumptions.
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 made it clear in the comments, as well as in the code, that the only valid format of the superseded_by
reference is .security-tokens|<doc_id>
. This code was faking to be generic with regards to the tokens alias.
|
||
private void getSupersedingTokenDocAsync(RefreshTokenStatus refreshTokenStatus, ActionListener<GetResponse> listener) { | ||
final String supersedingDocReference = refreshTokenStatus.getSupersededBy(); | ||
if (supersedingDocReference.startsWith(securityTokensIndex.aliasName() + "|")) { |
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 the parsing code to which I refer.
This commit introduces the `.security-tokens` and `.security-tokens-7` alias-index pair. Because index snapshotting is at the index level granularity (ie you cannot snapshot a subset of an index) snapshoting .`security` had the undesirable effect of storing ephemeral security tokens. The changes herein address this issue by moving tokens "seamlessly" (without user intervention) to another index, so that a "Security Backup" (ie snapshot of `.security`) would not be bloated by ephemeral data.
This commit introduces the `.security-tokens` and `.security-tokens-7` alias-index pair. Because index snapshotting is at the index level granularity (ie you cannot snapshot a subset of an index) snapshoting .`security` had the undesirable effect of storing ephemeral security tokens. The changes herein address this issue by moving tokens "seamlessly" (without user intervention) to another index, so that a "Security Backup" (ie snapshot of `.security`) would not be bloated by ephemeral data.
This commit introduces the `.security-tokens` and `.security-tokens-7` alias-index pair. Because index snapshotting is at the index level granularity (ie you cannot snapshot a subset of an index) snapshoting .`security` had the undesirable effect of storing ephemeral security tokens. The changes herein address this issue by moving tokens "seamlessly" (without user intervention) to another index, so that a "Security Backup" (ie snapshot of `.security`) would not be bloated by ephemeral data.
This commit introduces the `.security-tokens` and `.security-tokens-7` alias-index pair. Because index snapshotting is at the index level granularity (ie you cannot snapshot a subset of an index) snapshoting .`security` had the undesirable effect of storing ephemeral security tokens. The changes herein address this issue by moving tokens "seamlessly" (without user intervention) to another index, so that a "Security Backup" (ie snapshot of `.security`) would not be bloated by ephemeral data.
This commit introduces the
.security-tokens
and.security-tokens-7
alias-index pair. Because index snapshotting is at the index level granularity (ie you cannot snapshot a subset of an index) snapshoting.security
had the undesirable effect of storing ephemeral security tokens. The changes herein address this issue by moving tokens "seamlessly" (without user intervention) to another index, so that a "Security Backup" (ie snapshot of.security
) would not be bloated by ephemeral data.There is no change in field mappings, but the tokens mappings have been copy-pasted in a dedicated mapping file.
The rolling upgrade situation is trappy. This is because newly created tokens (creation or refresh) should be usable by both versions in the rolling upgrade situation. A big part of the change is due to this. For example, token docs will be generated in the
.security
index, until all nodes have been upgraded. Another situation is when we have to refresh a token that was generated by the previous version; in this case the "superseding" doc will be in the new index. I have yet to include this cases in the tests, but I plan to! I will drop comments about newly added tests.Obsoletes #37236
Relates #34454
Implementation details:
SecurityIndexManager
manages two alias-index pairs:.security
-.security-7
and.security-tokens
-.security-tokens-7
. There are twoSecurityIndexManager
instances at theTokenService
level, one for each pair.refresh_tokens
are now versioned in the same way thataccess_tokens
are (new cluster node version generate and return versionedrefresh_tokens
). Concretely, the minimum cluster node version, is prepended to the refresh UUID. In this case, it was required to identify the index where the token doc is stored, given the refresh token. But generally, I think it's beneficial to prepend version headers to packets. These of course have to be usable by the old nodes in the rolling upgrade scenario..security
index..security-tokens
has been created. After that, tokens are searched and removed only from this new index. This works because the new index is created after the rolling upgrade has completed, so no new nodes will create tokens on the old index after that.Note 1: GH stats are misleading! 100% of the main-code changes are in
TokenService
andExpiredTokenRemover
. I did some renamings, mainly around using static names from theRestrictedIndicesNames
instead ofSecurityIndexManager
. This was more "idiomatic" becauseSecurityIndexManager
is aimed at being "generic" and should not define the names that it is generic about... (in hindsight, given the footprint of the PR as it is, I should've refrained from this. Apologies!)Note 2: I plan to add a few more tests to
TokenAuthIntegTests
. I believe I can do without rolling upgrade tests, because I can simulate theTokenService
workings in the bwc mode, as in older nodes are still part of the cluster.