Security Tokens moved to a new separate index #40742

albertzaharovits · 2019-04-02T15:00:44Z

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 two SecurityIndexManager instances at the TokenService level, one for each pair.
refresh_tokens are now versioned in the same way that access_tokens are (new cluster node version generate and return versioned refresh_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.
Token docs are created on the new index, only after all the nodes have been upgraded. Until then, new nodes will still create token docs in the .security index.
Searching for tokens by realm or username inevitably has to search both indices, at least for some period of time (The same situation for removing expired token docs). The proposed approach is to search both, only for 24hrs (maximum lifetime of a token), after the .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 and ExpiredTokenRemover . I did some renamings, mainly around using static names from the RestrictedIndicesNames instead of SecurityIndexManager. This was more "idiomatic" because SecurityIndexManager 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 the TokenService workings in the bwc mode, as in older nodes are still part of the cluster.

elasticmachine · 2019-04-02T15:00:47Z

Pinging @elastic/es-security

test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java

albertzaharovits · 2019-04-03T10:49:39Z

x-pack/plugin/core/src/main/resources/security-index-template-7.json

@@ -1,5 +1,5 @@
 {
-  "index_patterns" : [ ".security-*" ],


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.

albertzaharovits · 2019-04-03T10:50:46Z

x-pack/plugin/core/src/main/resources/security-tokens-index-template-7.json

+      }
+    }
+  }
+}


Everything here is copy-pasted from the original security-index-template.json

albertzaharovits · 2019-04-03T10:51:28Z

...src/test/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStoreTests.java

@@ -589,25 +589,25 @@ public void testRemoteMonitoringCollectorRole() {



Renames, nothing to see here!

albertzaharovits · 2019-04-03T10:56:14Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java


-        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);


Adds the SecurityIndexManager instance for the tokens index.

albertzaharovits · 2019-04-17T14:51:43Z

@jkakavas , @bizybot thanks for the feedback ! I think I've addressed all your comments.

jkakavas

LGTM!

bizybot

LGTM, Thank you.

albertzaharovits · 2019-04-24T20:17:32Z

@elasticmachine run elasticsearch-ci/1

albertzaharovits · 2019-04-25T06:17:35Z

@elasticmachine run elasticsearch-ci/1

albertzaharovits · 2019-04-25T07:25:57Z

@elasticmachine elasticsearch-ci/1

albertzaharovits · 2019-04-25T07:26:14Z

@elasticmachine run elasticsearch-ci/1

albertzaharovits · 2019-04-25T08:42:42Z

@elasticmachine run elasticsearch-ci/1

tvernum

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.

tvernum · 2019-04-26T05:50:36Z

x-pack/plugin/core/src/main/resources/security-tokens-index-template-7.json

+                },
+                "metadata" : {
+                  "type" : "object",
+                  "dynamic" : true


This should be false (it's changed in .security since you copied it)

Good catch!

tvernum · 2019-04-30T03:24:14Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java

+                // 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));


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.

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.

tvernum · 2019-04-30T03:25:11Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java

+
+    private void getSupersedingTokenDocAsync(RefreshTokenStatus refreshTokenStatus, ActionListener<GetResponse> listener) {
+        final String supersedingDocReference = refreshTokenStatus.getSupersededBy();
+        if (supersedingDocReference.startsWith(securityTokensIndex.aliasName() + "|")) {


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.

albertzaharovits added 13 commits March 27, 2019 20:09

Rename drudgery

1e6b323

WIP TokenService, templates done

bf65bd4

WIP getting closer

b2dd6b8

InnerRefresh is next, then find*, then token remover

27d5679

TokenService done

48a3b31

Fix tests compilation

de02138

TokenServiceTests without anything new

f0e886a

mistakes

56e5832

versionCompatibilityOverride

90b51c5

Invalidation carry-over

68ccfd4

TokenServiceAuth

8e0c518

Old tests finally working

7842104

Checkstyle

2ef0581

albertzaharovits added >enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 v7.2.0 labels Apr 2, 2019

albertzaharovits self-assigned this Apr 2, 2019

albertzaharovits requested review from bizybot, tvernum and jkakavas April 2, 2019 15:00

Merge branch 'master' into tokens_on_separate_index

8bcafd3

tvernum reviewed Apr 3, 2019

View reviewed changes

test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java Outdated Show resolved Hide resolved

albertzaharovits added 2 commits April 3, 2019 12:34

Checkstyle

f886142

Checkstyle

07eb32f

albertzaharovits commented Apr 3, 2019

View reviewed changes

jkakavas self-requested a review April 18, 2019 09:49

jkakavas approved these changes Apr 18, 2019

View reviewed changes

bizybot approved these changes Apr 22, 2019

View reviewed changes

albertzaharovits added 2 commits April 24, 2019 14:39

Merge branch 'master' into tokens_on_separate_index

3703a0a

Merge branch 'master' into tokens_on_separate_index

ab378ac

tvernum reviewed Apr 26, 2019

View reviewed changes

tvernum reviewed Apr 30, 2019

View reviewed changes

albertzaharovits added 3 commits April 30, 2019 08:43

Merge branch 'master' into tokens_on_separate_index

ba9b4e5

Clarify the superseded_by format

01d0129

dynamic: false

46ffb00

albertzaharovits requested a review from tvernum April 30, 2019 06:05

tvernum approved these changes Apr 30, 2019

View reviewed changes

albertzaharovits merged commit b26fe5d into elastic:master Apr 30, 2019

albertzaharovits deleted the tokens_on_separate_index branch April 30, 2019 09:01

albertzaharovits added the backport pending label Apr 30, 2019

albertzaharovits mentioned this pull request Apr 30, 2019

BACKPORT Security Tokens moved to a new separate index #41673

Merged

azasypkin mentioned this pull request May 2, 2019

Remove tokens from .security-tokens index instead of .security in tests and re-enable affected API integration tests. elastic/kibana#35931

Merged

albertzaharovits removed the backport pending label May 2, 2019

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Security Tokens moved to a new separate index #40742

Security Tokens moved to a new separate index #40742

albertzaharovits commented Apr 2, 2019 •

edited

elasticmachine commented Apr 2, 2019

albertzaharovits Apr 3, 2019

albertzaharovits Apr 3, 2019

albertzaharovits Apr 3, 2019

albertzaharovits Apr 3, 2019

albertzaharovits commented Apr 17, 2019

jkakavas left a comment

bizybot left a comment

albertzaharovits commented Apr 24, 2019

albertzaharovits commented Apr 25, 2019

albertzaharovits commented Apr 25, 2019

albertzaharovits commented Apr 25, 2019

albertzaharovits commented Apr 25, 2019

tvernum left a comment

tvernum Apr 26, 2019

albertzaharovits Apr 30, 2019

tvernum Apr 30, 2019

albertzaharovits Apr 30, 2019

tvernum Apr 30, 2019

		@@ -589,25 +589,25 @@ public void testRemoteMonitoringCollectorRole() {

Security Tokens moved to a new separate index #40742

Security Tokens moved to a new separate index #40742

Conversation

albertzaharovits commented Apr 2, 2019 • edited

elasticmachine commented Apr 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits commented Apr 17, 2019

jkakavas left a comment

Choose a reason for hiding this comment

bizybot left a comment

Choose a reason for hiding this comment

albertzaharovits commented Apr 24, 2019

albertzaharovits commented Apr 25, 2019

albertzaharovits commented Apr 25, 2019

albertzaharovits commented Apr 25, 2019

albertzaharovits commented Apr 25, 2019

tvernum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits commented Apr 2, 2019 •

edited