Startup check for security implicit behavior change #76879

jkakavas · 2021-08-24T13:20:33Z

In the security ON by default project, we introduced a breaking change
for the xpack.security.enabled setting. While we do expose necessary
deprecation warnings and release notes, there might still be a case
where a deployment that is

On basic or trial license with xpack.security.enabled not set
A single node cluster or a multi-node, single-host cluster

gets upgraded to 8.x in place without using the Upgrade Assistant or
consulting the release notes. In this case, we elect to stop the node
from starting so that we can notify the user that the implicit behavior
for security has changed. If we don't do that, the upgrade can
seemingly succeed but the user will have no way to interact with the
upgraded cluster as security is enabled and they have no credentials.

This is a best effort check in the sense that:

LicenseState might not be available and/or correct that early in the
node lifecycle, so we might not be able to know if this node was on
basic/trial
A grow-and-shrink upgrade would bypass this check since new
nodes start with empty state on disk
A user might change the configuration and remove the explicit
xpack.security.enabled configuration while upgrading the node
to 8.x

elasticmachine · 2021-08-24T13:20:37Z

Pinging @elastic/es-security (Team:Security)

jkakavas · 2021-08-24T13:20:48Z

cc @colings86

BigPandaToo

LGTM

jkakavas · 2021-09-01T19:21:41Z

@albertzaharovits , @DaveCTurner can you folks please take a look ?

server/src/main/java/org/elasticsearch/env/NodeEnvironment.java

albertzaharovits

I would wait for David to give his advice.

Besides it, I would expect at least some upgrade qa tests to fail, no? It's odd that they don't.

DaveCTurner

The check logic looks good but I think it'll always get skipped when running a real node - see inline comments.

server/src/main/java/org/elasticsearch/env/NodeEnvironment.java

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

…teg test

jkakavas · 2021-10-23T15:33:40Z

...y/src/main/java/org/elasticsearch/xpack/security/SecurityImplicitBehaviorBootstrapCheck.java

+        }
+    }
+
+    public boolean alwaysEnforce() {


I don't love this always enabled Bootstrap check, but this is currently the only way for us to make a check on node startup that has a view ( albeit limited ) to the restored cluster state ( via the BootstrapContext )

I'd forgotten that we expose the metadata read from disk like this, but I think this is fine - at least it's no worse than any of the other places that make decisions based on the contents of the on-disk cluster state despite the fact that this could be stale or even uncommitted.

tvernum

The overall approach LGTM, but I have a few minor comments

tvernum · 2021-10-25T07:03:14Z

server/src/main/java/org/elasticsearch/env/NodeMetadata.java

@@ -68,10 +76,14 @@ public Version nodeVersion() {
        return nodeVersion;
    }

+    public Version previousNodeVersion() {
+        return previousNodeVersion;
+    }


I know I'm fighting against the existing conventions of this class, but is it possible to get some sort of javadoc here?
What does previous mean exactly? I think it's "last time the node started" (or more accurately "the version of the metadata that was read from disk") ... but I'm sure there could be all sorts of nuace in rolling upgrades, master elections, etc, and I'd like to be able to consult javadocs so I can know how to reason about that.

added in 80f893f, @DaveCTurner can keep me honest or suggest enhancements

Docs LGTM 👍

tvernum · 2021-10-25T07:11:01Z

...y/src/main/java/org/elasticsearch/xpack/security/SecurityImplicitBehaviorBootstrapCheck.java

+                    + "."
+                    + Version.CURRENT.minor
+                    + "/security-minimal-setup.html to enable security, or explicitly disable security by "
+                    + "setting [xpack.security.enabled] to \"false\" in elasticsearch.yml before restarting the node"


I'm coming in late, so maybe this has been discussed, but this message feels a bit lacking.

People who get this message don't necessarily realise why they're getting it now, and why it's a fatal error.
I think we can come up with something a bit more helpful that tells them that we've detected that this node was previously running in a configuration that did not have security, and the behaviour has changed so they need to explicitly opt in to the new or old behaviour.
I'm happy to help work on that message if needed.

I will take another attempt at it, I'll ask @lockewritesdocs to weigh-in on the wording too

I've rephrased it, let me know what you think. Open to suggestions

tvernum · 2021-10-25T07:13:29Z

.../test/java/org/elasticsearch/xpack/security/SecurityImplicitBehaviorBootstrapCheckTests.java

+
+public class SecurityImplicitBehaviorBootstrapCheckTests extends AbstractBootstrapCheckTestCase {
+
+    public void testFailureUpgradeFrom7xWithImplicitSecuritySettings() throws Exception {


I think we need 2 methods:

testFailureUpgradeFrom7xWithImplicitSecuritySettingsOnTrialOrBasic

testSuccessfulUpgradeFrom7xWithImplicitSecuritySettingsOnGoldPlus

The 2nd one seems to be missing.

makes sense, will add now

added in 2acbd38

jkakavas · 2021-10-25T11:43:11Z

@DaveCTurner would you be able to take another look please ? 🙏

tvernum

LGTM

DaveCTurner

I left one important comment about not writing the new field to disk and one other comment. Otherwise LGTM.

DaveCTurner · 2021-10-25T13:07:41Z

server/src/main/java/org/elasticsearch/env/NodeMetadata.java

-    public NodeMetadata(final String nodeId, final Version nodeVersion) {
+    private final Version previousNodeVersion;
+
+    public NodeMetadata(final String nodeId, final Version nodeVersion, final Version previousNodeVersion) {


Nit: could we make this private, and construct the instances needed in SecurityImplicitBehaviorBootstrapCheckTests by calling upgradeToCurrentVersion() instead?

DaveCTurner · 2021-10-25T13:08:23Z

server/src/main/java/org/elasticsearch/env/NodeMetadata.java

@@ -68,10 +76,14 @@ public Version nodeVersion() {
        return nodeVersion;
    }

+    public Version previousNodeVersion() {
+        return previousNodeVersion;
+    }


Docs LGTM 👍

DaveCTurner · 2021-10-25T13:10:56Z

server/src/main/java/org/elasticsearch/env/NodeMetadata.java

@@ -125,6 +151,7 @@ public NodeMetadata build() {
            objectParser = new ObjectParser<>("node_meta_data", ignoreUnknownFields, Builder::new);
            objectParser.declareString(Builder::setNodeId, new ParseField(NODE_ID_KEY));
            objectParser.declareInt(Builder::setNodeVersionId, new ParseField(NODE_VERSION_KEY));
+            objectParser.declareInt(Builder::setPreviousNodeVersionId, new ParseField(PREVIOUS_NODE_VERSION_KEY));


Do we need to write this field to disk? I think we just overwrite it before ever using it.

You're right David, we don't need to. I'm amending

DaveCTurner · 2021-10-25T13:13:25Z

...y/src/main/java/org/elasticsearch/xpack/security/SecurityImplicitBehaviorBootstrapCheck.java

+        }
+    }
+
+    public boolean alwaysEnforce() {


I'd forgotten that we expose the metadata read from disk like this, but I think this is fine - at least it's no worse than any of the other places that make decisions based on the contents of the on-disk cluster state despite the fact that this could be stale or even uncommitted.

jkakavas · 2021-10-25T16:47:34Z

Thanks @DaveCTurner , I've addressed your comments

jkakavas · 2021-10-25T16:49:05Z

@elasticmachine update branch

DaveCTurner

Thanks @jkakavas, LGTM

lockewritesdocs

👍 with the latest changes

In the security ON by default project, we introduced a breaking change for the xpack.security.enabled setting. While we do expose necessary deprecation warnings and release notes, there might still be a case where a deployment that is - On basic or trial license with `xpack.security.enabled` not set - A single node cluster or a multi-node, single-host cluster gets upgraded to 8.x in place without using the Upgrade Assistant or consulting the release notes. In this case, we elect to stop the node from starting via a newly introduced BootstrapCheck, so that we can notify the user that the implicit behavior for security has changed. If we don't do that, the upgrade can seemingly succeed but the user will have no way to interact with the upgraded cluster as security is enabled and they have no credentials. This is a best effort check in the sense that: - LicenseState might not be correct that early in the node lifecycle, so we might not be able to know if this node was on basic/trial - A grow-and-shrink upgrade would bypass this check since new nodes start with empty state on disk - A user might change the configuration and remove the explicit xpack.security.enabled configuration _while_ upgrading the node to 8.x

jkakavas added 4 commits August 6, 2021 17:50

working poc

f2bbff6

check license too

1ced464

Improve logic and add some tests

0010034

Merge remote-tracking branch 'origin/master' into check-implicit-upgrade

b4b713c

jkakavas added >enhancement :Security/Security Security issues without another label v8.0.0 labels Aug 24, 2021

jkakavas requested review from albertzaharovits, DaveCTurner and BigPandaToo August 24, 2021 13:20

elasticmachine added the Team:Security Meta label for security team label Aug 24, 2021

fix compilation

cc472a2

fix test

b890c25

BigPandaToo approved these changes Aug 25, 2021

View reviewed changes

albertzaharovits reviewed Sep 2, 2021

View reviewed changes

server/src/main/java/org/elasticsearch/env/NodeEnvironment.java Outdated Show resolved Hide resolved

albertzaharovits reviewed Sep 2, 2021

View reviewed changes

DaveCTurner reviewed Sep 2, 2021

View reviewed changes

server/src/main/java/org/elasticsearch/env/NodeEnvironment.java Outdated Show resolved Hide resolved

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java Outdated Show resolved Hide resolved

jkakavas added 5 commits September 28, 2021 13:24

Merge remote-tracking branch 'origin/master' into check-implicit-upgrade

66d09eb

re-implement as bootstrap check

a8297da

wpi

a7f6421

wrap up tests for the new bootstrap check. Still needs some uprade in…

2eba1d7

…teg test

Merge remote-tracking branch 'origin/master' into check-implicit-upgrade

28491b6

jkakavas requested review from DaveCTurner and tvernum October 21, 2021 10:17

revert unnecessary changes

79da522

jkakavas commented Oct 23, 2021

View reviewed changes

jkakavas added 2 commits October 23, 2021 18:37

Merge remote-tracking branch 'origin/master' into check-implicit-upgrade

1180f45

getter for nodemetadata

399fc96

tvernum reviewed Oct 25, 2021

View reviewed changes

jkakavas added 3 commits October 25, 2021 10:41

add javadoc

80f893f

add missing test

2acbd38

make bootstrap failure message clearer(?)

12bcde6

jkakavas requested a review from tvernum October 25, 2021 07:54

Merge remote-tracking branch 'origin/master' into check-implicit-upgrade

fa33a56

tvernum approved these changes Oct 25, 2021

View reviewed changes

DaveCTurner reviewed Oct 25, 2021

View reviewed changes

address feedback

7bcec6c

jkakavas requested a review from DaveCTurner October 25, 2021 16:47

Merge branch 'master' into check-implicit-upgrade

1006559

DaveCTurner approved these changes Oct 25, 2021

View reviewed changes

lockewritesdocs approved these changes Oct 25, 2021

View reviewed changes

jkakavas merged commit 8b3a615 into elastic:master Oct 25, 2021

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Startup check for security implicit behavior change #76879

Startup check for security implicit behavior change #76879

jkakavas commented Aug 24, 2021

elasticmachine commented Aug 24, 2021

jkakavas commented Aug 24, 2021

BigPandaToo left a comment

jkakavas commented Sep 1, 2021

albertzaharovits left a comment

DaveCTurner left a comment

jkakavas Oct 23, 2021

DaveCTurner Oct 25, 2021 •

edited

Loading

tvernum left a comment

tvernum Oct 25, 2021

jkakavas Oct 25, 2021 •

edited

Loading

DaveCTurner Oct 25, 2021

tvernum Oct 25, 2021

jkakavas Oct 25, 2021

jkakavas Oct 25, 2021

tvernum Oct 25, 2021

jkakavas Oct 25, 2021

jkakavas Oct 25, 2021

jkakavas commented Oct 25, 2021

tvernum left a comment

DaveCTurner left a comment

DaveCTurner Oct 25, 2021

DaveCTurner Oct 25, 2021

DaveCTurner Oct 25, 2021

jkakavas Oct 25, 2021

DaveCTurner Oct 25, 2021 •

edited

Loading

jkakavas commented Oct 25, 2021

jkakavas commented Oct 25, 2021

DaveCTurner left a comment

lockewritesdocs left a comment


		public class SecurityImplicitBehaviorBootstrapCheckTests extends AbstractBootstrapCheckTestCase {

		public void testFailureUpgradeFrom7xWithImplicitSecuritySettings() throws Exception {

Startup check for security implicit behavior change #76879

Startup check for security implicit behavior change #76879

Conversation

jkakavas commented Aug 24, 2021

elasticmachine commented Aug 24, 2021

jkakavas commented Aug 24, 2021

BigPandaToo left a comment

Choose a reason for hiding this comment

jkakavas commented Sep 1, 2021

albertzaharovits left a comment

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

tvernum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkakavas Oct 25, 2021 • 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

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

jkakavas commented Oct 25, 2021

tvernum left a comment

Choose a reason for hiding this comment

DaveCTurner 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

DaveCTurner Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

jkakavas commented Oct 25, 2021

jkakavas commented Oct 25, 2021

DaveCTurner left a comment

Choose a reason for hiding this comment

lockewritesdocs left a comment

Choose a reason for hiding this comment

DaveCTurner Oct 25, 2021 •

edited

Loading

jkakavas Oct 25, 2021 •

edited

Loading

DaveCTurner Oct 25, 2021 •

edited

Loading