Move security tokens to a separate internal index #37236

albertzaharovits · 2019-01-08T19:04:06Z

This PR moves authn tokens out from .security into their own index. It also includes the upgrade procedure.

The main reason for this change is that tokens are perishable. They should not be having the long lifetime of a password, as this is the shortcoming they were designed to alleviate in the first place. For that matter, they should usually not be included in backups because they would be wasting storage. In addition, restoring the .security on a live cluster will not hamper ongoing clients authn using tokens, because by default, the tokens index will not be restored/snapshoted.

Concretely, after the update the following structure will be effected:

curl -u elastic-admin:elastic-password -X GET "localhost:9200/_aliases?pretty"
{
  ".security-7" : {
    "aliases" : {
      ".security" : { }
    }
  },
  ".security-tokens-7" : {
    "aliases" : {
      ".security-tokens" : { }
    }
  }
}

The upgrade procedure is designed to work on 6.7 . 6.7 nodes understand the previous format as well as the new format. All nodes should be on a version greater than 6.7 for the upgrade assistant to work (_xpack/migration/upgrade/.security-6).

elasticmachine · 2019-01-08T19:04:08Z

Pinging @elastic/es-security

jaymode · 2019-01-08T21:23:03Z

@albertzaharovits can you clarify what part of this change is breaking?

jaymode

I left some comments/questions after a quick review

jaymode · 2019-01-08T21:24:02Z

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

@@ -0,0 +1,151 @@
+{


why is this a new file?

This will be present in 6.7 only. When I backport to 7 new-security-index-template.json will take the place of security-index-template.json .

The rationale is as follows:
In 6.7 nodes will create .security-6 as usual, if the index is missing (fresh install).
_xpack/migration/upgrade/.security-6 should be called before rolling upgrading to 7.
The alternative could have been to create .security-7 on a fresh 6.7 install, but during a rolling upgrade from 6.6 to 6.7, 6.7 nodes might erroneously consider they have to create .security-7.

This will be present in 6.7 only. When I backport to 7

This phrasing confuses me a little - what I assume you mean is that once you've backported this change to 6.7 you will update master (7.0) to have only a single template (rename new-security-index-template.json to security-index-template.json).

I think in that case it might be better to name the json files as security-index-template-6.json and security-index-template-7.json

You're right, will do!

jaymode · 2019-01-08T21:25:02Z

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

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


we should probably exclude the tokens index from getting this template

We don't use these files as templates anymore, so the index_pattern field is only for doc purposes only. I agree it is confusing, I will change it.

jaymode · 2019-01-08T21:26:43Z

...lugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ExpiredTokenRemover.java

-        DeleteByQueryRequest expiredDbq = new DeleteByQueryRequest(SecurityIndexManager.SECURITY_INDEX_NAME);
+        final DeleteByQueryRequest expiredDbq;
+        if (securityTokensIndex.exists()) {
+            expiredDbq = new DeleteByQueryRequest(securityTokensIndex.aliasName());


is this truly a binary choice? Maybe the alias should point to both indices until the upgrade or something like that?

It is a binary choice.

Tokens, expired or not, can sit in a single index at a certain moment in time: either .security-6 or .security-tokens-7. When the remover job starts, it builds the query accordingly. It is possible that after building the query and before its execution, the tokens can move (from .security-6 to .security-tokens-7) and the delete will miss them, but they will be catched by the next removal job.

jaymode · 2019-01-08T21:29:19Z

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

@@ -217,6 +219,14 @@ public static Boolean isTokenServiceEnabled(Settings settings) {
        return XPackSettings.TOKEN_SERVICE_ENABLED_SETTING.get(settings);
    }

+    private SecurityIndexManager indexManager() {
+        if (securityTokensIndex.exists()) {


shouldn't we base this on the index format? also what about tokens created/invalidated while the upgrade is running

shouldn't we base this on the index format?

I find that the alias existence (.security-tokens) is enough to base the decision. Again, this is only a 6.7 thing, in 7 there is no decision to make, the token service will automatically generate its index.

what about tokens created/invalidated while the upgrade is running?

This is an important question!

During a time window during the upgrade, tokens will not be able to be created, refreshed or invalidated, but only to be used. These operations will fail. This is because .security-6, which still has the tokens, is marked as read-only. I tried to minimize this time window. It is comprised of the reindex only of the token documents and the alias creation.

I thought that allowing writing to two indices while a reindex was going was too complicated. For example, the refresh/invalidation case would translate to an update on .security-6 and a create or update on .security-tokens-7, and any of these two can theoretically fail independently. Maybe I don't know enough about versioning to come up with a plan about this...

I had an idea of storing the write token operations against .security-6 while the reindex is going on, and before the alias is shifted, in a bulk request and then replaying the bulk request on the first read against the new .security-tokens-7 index. Though, this is wrong in many aspects...

Do you think this is acceptable? Or do we need to find a solution to the read-only issue. I admit I was influenced by the upgrade method in 5.6, but there were no write intensive objects in .security at that time.

Alternatively, we can make sure that any write operations are retried.

I think the read only aspect during the upgrade is definitely acceptable.

jaymode · 2019-01-08T21:31:58Z

...in/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityIndexManager.java

-    public static final String INTERNAL_SECURITY_INDEX = ".security-" + IndexUpgradeCheckVersion.UPRADE_VERSION;
-    public static final int INTERNAL_INDEX_FORMAT = 6;
+    public static final String SECURITY_ALIAS_NAME = ".security";
+    public static final int INTERNAL_SECURITY_INDEX_FORMAT = 6;


shouldn't we increment the version post upgrade?

Yes, this is done in SecurityIndexUpgradeCheck#upgrade . Again, this is the face of SecurityIndexManager for 6.7 . In the 7 backport the number here will change to 7.

albertzaharovits · 2019-01-09T09:47:50Z

@albertzaharovits can you clarify what part of this change is breaking?

My reasoning was that the forking in itself is a breaking change, but it might not be...
Being an internal index, .security should not be operated upon directly. It is hidden behind APIs, so the change should be transparent. The scenario I had in mind, when I labelled it as such, was that a restore of .security-6 will not mean anything is a cluster with a .security-7 index. Backing up .security is not something documented, so it might not be an external interface to break.

albertzaharovits · 2019-01-09T11:38:13Z

.../plugin/upgrade/src/main/java/org/elasticsearch/xpack/upgrade/SecurityIndexUpgradeCheck.java

+                                    }, removeReadOnlyBlock));
+                        }, removeReadOnlyBlock));
+            }, listener::onFailure));
+        }, listener::onFailure));


@jaymode I think the upgrade method is a good place to deep dive in the PR review.

This does not have tests, I know, I'll add them after a few review rounds.

I think we also should go ahead and remove any documents with a type of invalidated-token. These documents are really not relevant anymore

Also this isn't available yet, but this might be a good place to make use of the StepListener from #37327

albertzaharovits · 2019-01-09T11:58:06Z

Thank you @jaymode for the speedy review round, 'preciate it!

jaymode

@albertzaharovits thanks for the answers. I'm pretty happy with this approach. I think we should add tests and then I can review again.

jaymode · 2019-01-11T16:29:17Z

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

@@ -217,6 +219,14 @@ public static Boolean isTokenServiceEnabled(Settings settings) {
        return XPackSettings.TOKEN_SERVICE_ENABLED_SETTING.get(settings);
    }

+    private SecurityIndexManager indexManager() {
+        if (securityTokensIndex.exists()) {


I think the read only aspect during the upgrade is definitely acceptable.

jaymode · 2019-01-11T16:37:04Z

.../plugin/upgrade/src/main/java/org/elasticsearch/xpack/upgrade/SecurityIndexUpgradeCheck.java

+                                    }, removeReadOnlyBlock));
+                        }, removeReadOnlyBlock));
+            }, listener::onFailure));
+        }, listener::onFailure));


I think we also should go ahead and remove any documents with a type of invalidated-token. These documents are really not relevant anymore

jaymode · 2019-01-11T16:38:22Z

.../plugin/upgrade/src/main/java/org/elasticsearch/xpack/upgrade/SecurityIndexUpgradeCheck.java

+                                    }, removeReadOnlyBlock));
+                        }, removeReadOnlyBlock));
+            }, listener::onFailure));
+        }, listener::onFailure));


Also this isn't available yet, but this might be a good place to make use of the StepListener from #37327

joshdover · 2019-01-14T17:03:38Z

Hey all, I'm implementing the UI side of the Upgrade Assistant for 6.x -> 7.x. Since starting this project, we've been working under the assumption that no internal indices were going to need to be transformed/upgrade beyond a normal reindex that all 5.x indices would need. This change would introduce the first and only internal index upgrade for 7.x.

With 2 weeks until FF for 6.7, I'm worried that adding in this functionality on the Kibana side of things is going to put the upgrade at risk of missing FF. We still have other things to complete on our side and a load of integration testing that needs to be done to ensure a smooth upgrade to 7.0 for customers.

Without much context on this change, it seems that this may not be critical for 7.x. Are there customers who have large snapshots due to tokens being in the same index? If the only reason for doing this is to reduce snapshot sizes for the .security index and there aren't any customers with complaining about .security snapshot size, is it possible we could hold off on this change?

Alternatively, I'm curious what the rationale is behind making this change require user intervention. Is it not possible to apply this transformation when Elasticsearch boots up? In Kibana 6.5, we added a similar migrations mechanism that performs index transformations when Kibana boots. Would it be possible to do something similar for this change in ES so no user intervention is required?

\cc @bleskes @tylersmalley

albertzaharovits · 2019-01-14T18:52:34Z

Without much context on this change, it seems that this may not be critical for 7.x. Are there customers who have large snapshots due to tokens being in the same index? If the only reason for doing this is to reduce snapshot sizes for the .security index and there aren't any customers with complaining about .security snapshot size, is it possible we could hold off on this change?

This is indeed a nice to have. Access tokens take up space and time and are unusable after a restore (i.e. are expired or encrypted with a forgone key).

Alternatively, I'm curious what the rationale is behind making this change require user intervention. Is it not possible to apply this transformation with Elasticsearch boots up? In Kibana 6.5, we added a similar migrations mechanism that performs index transformations when Kibana boots. Would it be possible to do something similar for this change in ES so no user intervention is required?

We can make the upgrade automatic in the background. I've been musing this as well. But we need to decide on a "trigger" for the upgrade. Plus, given the example in 5.6 I was under the impresion that the user intervention is the idiomatic way to do it. I can move tokens to the separate index without updating the mapping on the .security-6 but this obviously lingers unused mappings.

This PR is close, it only needs an integ test for the upgrade proc. I can work on it right know and be done with it soon.

Thoughts @jaymode ?

jaymode · 2019-01-14T19:10:31Z

We can make the upgrade automatic in the background. I've been musing this as well. But we need to decide on a "trigger" for the upgrade. Plus, given the example in 5.6 I was under the impresion that the user intervention is the idiomatic way to do it.

My understanding is that we did not want to have upgrades happen without user intervention; I think @s1monw had some specific concerns with the automatic upgrades.

This PR is close, it only needs an integ test for the upgrade proc. I can work on it right know and be done with it soon.

If you want to get this ready, that's fine but it might be something we push off until 7.x for the upgrade from 7 to 8.

joshdover · 2019-01-14T22:07:47Z

If you want to get this ready, that's fine but it might be something we push off until 7.x for the upgrade from 7 to 8.

Thanks for the context, it sounds like pushing may be our best option. As we're getting closer to FF, I'll have a better idea of whether or not we'll have enough time on the Kibana side to support this in the Upgrade Assistant. Ideally, I'd like to make it happen. I should know better by middle of next week as things start to wrap up.

bleskes · 2019-01-15T10:57:43Z

good catch @joshdover . Thanks for diligently watching other repos :)

I agree that we should avoid building custom reindexing logic this late in the game.

Since tokens are short lived, I wonder if there is an option to naturally let things move from one index to another - i.e., when a session is created it will always be create in the new index while session lookups will be done both in the new index and in the .security index. After some time all sessions will be in the new index alone and we achieve the effect we're after.

albertzaharovits · 2019-01-15T11:38:59Z

I wonder if there is an option to naturally let things move from one index to another - i.e., when a session is created it will always be create in the new index while session lookups will be done both in the new index and in the .security index. After some time all sessions will be in the new index alone and we achieve the effect we're after.

Thanks for digging into alternatives @bleskes ! We could do that. But this would be leaving the mappings for tokens lingering in the old .security-6 index (we cannot remove mapping fields without a reindex). They aren't costing us anything, but some bright day we should still remove them somehow. Also, given that we can full-cluster-restart from any 6.x version we would have to keep the search-in-both-places-and-then-update logic for the 7.x lifetime... I tried hard not to complicate the TokenService anymore.

Instead, I propose we keep the manual intervention for the upgrade and the dual format compatibility in 7.x releases. The dual format compatibility (tokens can be in any index but not in both) in this PR was originally targeted for 6.7 only, and now I suggest we retarget it to 7.x.

This way no migration/upgrade is available/necessary in 6.7, and for the 7.x releases _migration/upgrade is entirely optional. I will document the migration in the context of the Backup .security as an optional but recommended step, before the actual snapshot (we don't have any story about this atm). We could make the migration mandatory in the last minor release in the 7 series.

I believe this alternative achieves both the ease of the 6.7 - 7 upgrade and improves our story about snapshotting/restoring .security (i.e. faster/smaller bakups, decreases the risk of leaking refresh tokens, and maintains the availability of the token service during a restore) , by only slightly complicating the backup procedure guide.

What say you @bleskes @jaymode ?

jaymode · 2019-01-15T15:14:55Z

I think the way @bleskes proposes is similar to what we've done in 6.x with tokens. Pre-6.2 we only indexed invalidation docs; post 6.2 we indexed invalidation and the token doc. For 7.0+ we only store the token doc. Given the power of our search api, if the format of the docs stays the same we can search .security, .security-tokens for matching docs and handle accordingly. If we're updating a doc, then we need to update it in the index that it already exists in. Ideally with this, we should be able to make this transition. I suggest we go this way and target this change for 7.0+ as it will be simpler since we've removed the BWC code for the two separate token docs there.

Ping @jkakavas for thoughts as he has done a good amount of work in this area recently.

tvernum · 2019-04-08T00:59:32Z

@albertzaharovits Should we close this PR now that we have #40742?

albertzaharovits · 2019-04-08T09:07:03Z

No, this should be discontinued, thanks for the ping!

albertzaharovits added 17 commits January 1, 2019 11:42

Fork out the security tokens template

9493da4

No templates in SecurityIndexManager

c4d8a0a

Remove extraneous method from SecurityManager

903dbb7

SecurityIndexManager independent of static

748b8fb

Merge branch 'master' into tokens_on_new_index

a93ada8

Okay, let's go!

ddda03e

TokenService

05de53b

Use alias to resolve the index in the SecurityManager

a433eb9

WIP

ec21cec

WIP

441b971

Wonderfully

24dafb3

Plug in SecurityUpgradeCheck

e46ed31

ExpiredTokenRemover

00c23f7

Nits

7dff216

add creation time for tokens

4be665e

Yay!!

37489fe

Merge branch 'master' into tokens_on_new_index

3eebd01

albertzaharovits added >feature >breaking v7.0.0 :Security/Security Security issues without another label v6.7.0 labels Jan 8, 2019

albertzaharovits self-assigned this Jan 8, 2019

albertzaharovits requested review from tvernum and jaymode January 8, 2019 19:04

jaymode requested changes Jan 8, 2019

View reviewed changes

albertzaharovits commented Jan 9, 2019

View reviewed changes

Merge branch 'master' into tokens_on_new_index

37d2d84

Templates have specific index patterns

2d6e100

joshdover mentioned this pull request Jan 10, 2019

Add reindex feature to Upgrade Assistant elastic/kibana#27457

Merged

6 tasks

jaymode reviewed Jan 11, 2019

View reviewed changes

albertzaharovits added 4 commits January 13, 2019 16:24

Merge branch 'master' into tokens_on_new_index

a626bc1

Renames

e70d0d9

Forgo "invalidated-token"

ff22f81

StepListener

f030a7f

joshdover mentioned this pull request Jan 14, 2019

Upgrade Assistant - Phase 2 - Reindexing elastic/kibana#26368

Closed

19 tasks

jasontedor added v8.0.0 and removed v7.0.0 labels Feb 6, 2019

danielmitterdorfer added v7.2.0 and removed v6.7.0 labels Feb 7, 2019

albertzaharovits mentioned this pull request Apr 2, 2019

Security Tokens moved to a new separate index #40742

Merged

albertzaharovits closed this Apr 8, 2019

albertzaharovits deleted the tokens_on_new_index branch April 8, 2019 09:07

albertzaharovits mentioned this pull request Apr 30, 2019

BACKPORT Security Tokens moved to a new separate index #41673

Merged

jakelandis removed v7.2.0 v8.0.0 labels Jun 18, 2019

Move security tokens to a separate internal index #37236

Move security tokens to a separate internal index #37236

Conversation

albertzaharovits commented Jan 8, 2019 • edited Loading

elasticmachine commented Jan 8, 2019

jaymode commented Jan 8, 2019

jaymode 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits commented Jan 9, 2019 • edited Loading

albertzaharovits Jan 9, 2019 • edited Loading

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 Jan 9, 2019

jaymode 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

joshdover commented Jan 14, 2019 • edited Loading

albertzaharovits commented Jan 14, 2019 • edited Loading

jaymode commented Jan 14, 2019

joshdover commented Jan 14, 2019

bleskes commented Jan 15, 2019

albertzaharovits commented Jan 15, 2019 • edited Loading

jaymode commented Jan 15, 2019

tvernum commented Apr 8, 2019

albertzaharovits commented Apr 8, 2019

albertzaharovits commented Jan 8, 2019 •

edited

Loading

albertzaharovits commented Jan 9, 2019 •

edited

Loading

albertzaharovits Jan 9, 2019 •

edited

Loading

joshdover commented Jan 14, 2019 •

edited

Loading

albertzaharovits commented Jan 14, 2019 •

edited

Loading

albertzaharovits commented Jan 15, 2019 •

edited

Loading