Deprecate security implicitly disabled on trial/basic #72339

jkakavas · 2021-04-27T20:53:47Z

This change deprecates the behavior where security features are
disabled implicitly when the license is basic or trial and the
xpack.security.enabled setting is not explicitly set, as this
behavior will change in 8.0.0.
The recommendation is to be explicit in the configuration and
either enable or disable security in elasticsearch.yml.

Depends on #72300

This change deprecates the behavior where security features are disabled implicitly when the license is basic or trial and the xpack.security.enabled setting is not explicitly set. The recommendation is to be explicit in the configuration and either enable or disable security in elasticsearch.yml.

elasticmachine · 2021-04-27T20:53:50Z

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

…urity-setting

jkakavas · 2021-05-04T06:03:30Z

...ity/src/main/java/org/elasticsearch/xpack/security/support/SecurityStatusChangeListener.java

@@ -45,6 +50,15 @@ public synchronized void licenseStateChanged() {
                logger.warn("Elasticsearch built-in security features are not enabled. Without authentication, your cluster could be " +
                    "accessible to anyone. See https://www.elastic.co/guide/en/elasticsearch/reference/" + Version.CURRENT.major + "." +
                    Version.CURRENT.minor + "/security-minimal-setup.html to enable security.");
+                if (licenseState.getOperationMode().equals(License.OperationMode.BASIC)
+                    || licenseState.getOperationMode().equals(License.OperationMode.TRIAL)) {
+                    deprecationLogger.deprecate(DeprecationCategory.SECURITY, "security_implicitly_disabled",


Happy to get input/feedback/alternative wordings for this

I think there's room to improve the wording, but will iterate on that in a separate review.

tvernum · 2021-05-04T06:24:11Z

.../plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DeprecationChecks.java

     * @return The list of {@link DeprecationIssue} that were found in the cluster
     */
    static <T> List<DeprecationIssue> filterChecks(List<T> checks, Function<T, DeprecationIssue> mapper) {
        return checks.stream().map(mapper).filter(Objects::nonNull).collect(Collectors.toList());
    }
+
+    @FunctionalInterface
+    public interface TriFunction<F, S, T, R> {


Since this interface is local to the class, I think it should have a more specific, meaningful name like NodeDeprecationCheck

tvernum · 2021-05-04T07:51:49Z

...gin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java

+    static DeprecationIssue checkImplicitlyDisabledSecurityOnBasicAndTrial(final Settings settings,
+                                                                           final PluginsAndModules pluginsAndModules,
+                                                                           final XPackLicenseState licenseState) {
+        if ( settings.hasValue(XPackSettings.SECURITY_ENABLED.getKey()) == false


Any reason not to do:

Suggested change

if ( settings.hasValue(XPackSettings.SECURITY_ENABLED.getKey()) == false

if ( XPackSettings.SECURITY_ENABLED.exists(settings) == false

Not really. The former ends up doing a get() on a Map and checks for null, whereas the latter will do a contains on the Set of settings keys. I think this equivalent performance wise but I do agree that your suggestion is less verbose/cleaner

tvernum · 2021-05-04T07:55:53Z

...ity/src/main/java/org/elasticsearch/xpack/security/support/SecurityStatusChangeListener.java

@@ -45,6 +50,15 @@ public synchronized void licenseStateChanged() {
                logger.warn("Elasticsearch built-in security features are not enabled. Without authentication, your cluster could be " +
                    "accessible to anyone. See https://www.elastic.co/guide/en/elasticsearch/reference/" + Version.CURRENT.major + "." +
                    Version.CURRENT.minor + "/security-minimal-setup.html to enable security.");
+                if (licenseState.getOperationMode().equals(License.OperationMode.BASIC)
+                    || licenseState.getOperationMode().equals(License.OperationMode.TRIAL)) {
+                    deprecationLogger.deprecate(DeprecationCategory.SECURITY, "security_implicitly_disabled",


I think there's room to improve the wording, but will iterate on that in a separate review.

tvernum · 2021-05-04T07:57:50Z

...gin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.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";
+            return new DeprecationIssue(
+                DeprecationIssue.Level.WARNING,


Why did we decide WARNING is preferable to CRITICAL?

On the one hand, people who don't resolve this will have a terrible time during (or immediately after) upgrade.
On the other hand, for most people it will be just as easy to fix at/after upgrade as it is before upgrade (change the setting).

I can be definitely swayed. The way I read WARNING / CRITICAL is that the "Failures will occur unless" meant to convey that i.e. the node will fail to start or the cluster won't form etc. So I went with WARNING.

It will probably matter a lot how this is surfaced in the upgrade assistant though and if warning is something users are allowed to click through while upgrading, we should make this CRITICAL. Thank for raising this

I suspect we can revisit this during the final 8.0 upgrade preparations and make sure it's presenting well to the user.

tvernum · 2021-05-04T08:02:42Z

...eprecation/src/test/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecksTests.java

@@ -42,17 +43,19 @@
 public class NodeDeprecationChecksTests extends ESTestCase {

    public void testCheckDefaults() {
-        final Settings settings = Settings.EMPTY;
+        final Settings settings = Settings.builder().put(XPackSettings.SECURITY_ENABLED.getKey(), true).build();


The method is Defaults, but that's not the default setting.
I don't think the method should lie about what it does. Either we:

Rename it

Change the license state to be gold+ (which is still not "default", but better)

Accept that the default now has a deprecation.

I think we want (3)

I was in "mechanically apply change" mode I guess

tvernum · 2021-05-04T08:04:18Z

...eprecation/src/test/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecksTests.java

+        assertThat(issue.getDetails(), containsString("The behavior where the value of [xpack.security.enabled] setting is false for "));
+        assertThat(issue.getUrl(),
+            equalTo("https://www.elastic.co/guide/en/elasticsearch/reference/7.14/deprecated-7.14.html#implicitly-disabled-security"));
+    }


Tests for other cases (explicitly enabled/disabled, gold+) ?

tvernum · 2021-05-04T08:29:56Z

...ity/src/main/java/org/elasticsearch/xpack/security/support/SecurityStatusChangeListener.java

+                    || licenseState.getOperationMode().equals(License.OperationMode.TRIAL)) {
+                    deprecationLogger.deprecate(DeprecationCategory.SECURITY, "security_implicitly_disabled",
+                        "The behavior where the value of [xpack.security.enabled] setting defaults to false for " +
+                            licenseState.getOperationMode() + " licenses is deprecated and will be changed in a future version. " +


The wording on this is hard.

My suggestion is:

The behavior that disables security by default on ${type} licenses is deprecated.
A future version of Elasticsearch will change the default value for [xpack.security.enabled] to "true", for all license types.

I'd like to hear from others as well (Any thoughts @lockewritesdocs?)

Also, I think we should use

licenseState.getOperationMode().description()

so that it says basic rather than BASIC

But I'm open to different views on that.

Thanks, I'll wait for Adam's input and will adjust the message

Thanks for the ping @tvernum 👍 I really like your revision.

I'm proposing two modified options that are slightly different, so choose whichever one you think is best. I agree with Tim that rendering the license name in lowercase is the better choice ("basic license" instead of "BASIC license"):

Option 1
Disabling security on ${type} licenses by default is deprecated. A later version of Elasticsearch will set [xpack.security.enabled] to "true", enabling security for all licenses by default.

Option 2
Disabling security on ${type} licenses by default is deprecated. A later version of Elasticsearch will set [xpack.security.enabled] to "true" for all licenses and enable security by default.

Thanks @lockewritesdocs. My concern with "Disabling security on..." makes it sound like this is something the user does/did/configured and we're deprecating that. Whereas it is something that we do ( as in we change our default configuration when the user does nothing ) . WDYT?

@jkakavas, we could modify the statement a bit to say something like:

Option 1

This release deprecates disabling security on ${type} licenses by default.

However, the change might not be in "this release", correct? For example, users could be on 7.15 but this behavior might be introduced in 7.14. Do we want to include the release where this change occurred?

Option 2

The 7.14 release deprecates disabling security on ${type} licenses by default.

If we really don't want to include any information about a specific release, then we could use language that's more similar to Tim's original statement:

Option 3

The default behavior of disabling security on ${type} licenses is deprecated.

Thanks both! I went with

The default behavior of disabling security on basic licenses is deprecated. A later version of Elasticsearch will set [xpack.security.enabled] to "true" for all licenses and enable security by default.

lockewritesdocs

@jkakavas, I’m suggesting some changes that are non-blocking, but there is a broken URL that must be fixed (I’m adding the required change). I’m approving the PR, but just a heads up that the URL must be fixed or it won’t work.

lockewritesdocs · 2021-05-05T12:22:41Z

...gin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java

+              + "for all licenses and enable security by default."
+              + "See https://www.elastic.co/guide/en/elasticsearch/reference/" + Version.CURRENT.major + "."
+              + Version.CURRENT.minor + "/security-minimal-setup.html to enable security, or explicitly disable security by "
+              + "setting [xpack.security.enabled] to false in elasticsearch.yml";


Suggested change

+ "setting [xpack.security.enabled] to false in elasticsearch.yml";

+ "setting [xpack.security.enabled] to \"false\" in elasticsearch.yml";

Suggest adding quotes around "false" to align with "true" in the previous sentence.

lockewritesdocs · 2021-05-05T12:39:47Z

...gin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java

+            return new DeprecationIssue(
+                DeprecationIssue.Level.CRITICAL,
+                "Security is enabled by default for all licenses in the next major version.",
+                "https://www.elastic.co/guide/en/elasticsearch/reference/7.14/deprecated-7.14.html#implicitly-disabled-security",


Suggested change

"https://www.elastic.co/guide/en/elasticsearch/reference/7.14/deprecated-7.14.html#implicitly-disabled-security",

"https://www.elastic.co/guide/en/elasticsearch/reference/7.14/migrating-7.14.html#implicitly-disabled-security",

See this preview link for the complete URL: https://elasticsearch_72339.docs-preview.app.elstc.co/guide/en/elasticsearch/reference/7.x/migrating-7.14.html#implicitly-disabled-security

lockewritesdocs · 2021-05-05T12:47:57Z

docs/reference/migration/migrate_7_14.asciidoc

+Currently, security features are disabled when operating on a basic or trial
+license when `xpack.security.enabled` has not been explicitly set to `true`.
+
+This behavior is now deprecated. In version 8.0.0, security features will be
+enabled by default for all licenses, unless explicitly disabled (by setting
+`xpack.security.enabled` to `false`).


Suggested change

Currently, security features are disabled when operating on a basic or trial

license when `xpack.security.enabled` has not been explicitly set to `true`.

This behavior is now deprecated. In version 8.0.0, security features will be

enabled by default for all licenses, unless explicitly disabled (by setting

`xpack.security.enabled` to `false`).

.Security features on basic or trial license aren't disabled by default

[%collapsible]

====

*Details* +

Currently, security features are disabled when operating on a basic or trial

license when `xpack.security.enabled` has not been explicitly set to `true`.

This behavior is now deprecated. In version 8.0.0, security features will be

enabled by default for all licenses, unless explicitly disabled (by setting

`xpack.security.enabled` to `false`).

====

This collapsible format isn't required, but I think it's nice to separate the text from the main header, especially when linking to this specific section. See the Mapping changes section in 7.13 for an example of how this formatting renders.

I like this but the title might confuse folks as something we're doing in 7.14( if they don't click to expand the collapsible ). How does "The default behavior of disabling security on basic and trial licenses is deprecated" sound as a title instead @lockewritesdocs ?

👍 works for me @jkakavas

tvernum

LGTM

rjernst · 2021-05-10T21:40:20Z

...gin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java

+            && (licenseState.getOperationMode().equals(License.OperationMode.BASIC)
+            || licenseState.getOperationMode().equals(License.OperationMode.TRIAL))) {
+          String details = "The default behavior of disabling security on " + licenseState.getOperationMode().description()
+              + " licenses is deprecated. A later version of Elasticsearch will set [xpack.security.enabled] to \"true\" "


Could this be better phrased as "will change the default of [xpack.security.enabled] to 'true' regardless of the license level."?

Thanks for the suggestion Ryan. I don't have a strong opinion here, I think both versions convey the meaning equally clearly and as a non-native speaker, I can't say I see an impactful difference. Your version is shorter and I do like that. I'll ping @lockewritesdocs for an opinion and if he's also +1, I'll make the change

I think that "will change the default of [xpack.security.enabled] to 'true'" is a little ambiguous. It almost sounds like we're moving away from the default, which is currently [xpack.security.enabled] to true. I'd suggest the following revision, which incorporates Ryan's shorter version:

A later version of Elasticsearch will set [xpack.security.enabled] to true by default, regardless of the license level.

I was trying to move away from "will set" because it implies the value is actually being set (for example in elasticsearch.yml) when it is not. It is a default value that we will now be applied regardless of other factors.

That's a good callout @rjernst. The behavior will operate as if [xpack.security.enabled] is true. Perhaps we can explain that nuance with something like:

A later version of Elasticsearch will enable security by default and behave as if [xpack.security.enabled] is true, regardless of the license level.

I don't think we're using phrases such as "The behavior will operate as if X is Y" for settings that have default value set to Y anywhere else. I find this confusing to be honest. May I suggest

A later version of Elasticsearch will set [xpack.security.enabled] to \"true\"

-->

In a later version of Elasticsearch, the value of [xpack.security.enabled] will default to \"true\" ?

…urity-setting

jkakavas · 2021-07-15T14:10:22Z

Now that we have a consensus around our oncoming changes for 8.0 ,we will merge this to 7.14 too in time for BC4 so that we get the warning out to our users as soon as possible

jkakavas · 2021-07-15T20:31:59Z

Merging this despite the FIPS failures which are systemic, rather than PR specific

This change deprecates the behavior where security features are disabled implicitly when the license is basic or trial and the xpack.security.enabled setting is not explicitly set. The recommendation is to be explicit in the configuration and either enable or disable security in elasticsearch.yml.

jkakavas added >deprecation :Security/Security Security issues without another label v7.14.0 labels Apr 27, 2021

elasticmachine added the Team:Security Meta label for security team label Apr 27, 2021

fix tests

ad2b84c

jkakavas added 3 commits April 29, 2021 14:14

Merge remote-tracking branch 'origin/7.x' into deprecate-implicit-sec…

eed4977

…urity-setting

typo

3238835

Merge remote-tracking branch 'origin/7.x' into deprecate-implicit-sec…

787aa00

…urity-setting

jkakavas requested a review from tvernum May 4, 2021 06:02

jkakavas commented May 4, 2021

View reviewed changes

tvernum reviewed May 4, 2021

View reviewed changes

Address feedback

e5e9e2a

jkakavas requested a review from tvernum May 4, 2021 09:10

Adust deprecation criticality and message

11c5dc2

jkakavas requested a review from lockewritesdocs May 5, 2021 08:14

lockewritesdocs approved these changes May 5, 2021

View reviewed changes

jkakavas added 3 commits May 5, 2021 16:20

Address feedback

59b85d7

fix string

aa2df0f

update text matcher

b559931

tvernum approved these changes May 7, 2021

View reviewed changes

rjernst reviewed May 10, 2021

View reviewed changes

jkakavas added 4 commits May 11, 2021 00:58

Merge remote-tracking branch 'origin/7.x' into deprecate-implicit-sec…

7be6983

…urity-setting

address feedback

50570d5

Merge remote-tracking branch 'origin/7.x' into deprecate-implicit-sec…

eca4496

…urity-setting

fix test

9207bac

mark-vieira added v7.15.0 and removed v7.14.0 labels Jun 30, 2021

jkakavas added 2 commits July 15, 2021 16:55

Merge remote-tracking branch 'origin/7.x' into deprecate-implicit-sec…

2358a8d

…urity-setting

Finalize wording for deprecation message

1ea2f73

jkakavas added v7.14.0 and removed v7.15.0 labels Jul 15, 2021

jkakavas merged commit d841e79 into elastic:7.x Jul 15, 2021

	if ( settings.hasValue(XPackSettings.SECURITY_ENABLED.getKey()) == false
	if ( XPackSettings.SECURITY_ENABLED.exists(settings) == false

	+ "setting [xpack.security.enabled] to false in elasticsearch.yml";
	+ "setting [xpack.security.enabled] to \"false\" in elasticsearch.yml";

	"https://www.elastic.co/guide/en/elasticsearch/reference/7.14/deprecated-7.14.html#implicitly-disabled-security",
	"https://www.elastic.co/guide/en/elasticsearch/reference/7.14/migrating-7.14.html#implicitly-disabled-security",

Deprecate security implicitly disabled on trial/basic #72339

Deprecate security implicitly disabled on trial/basic #72339

Conversation

jkakavas commented Apr 27, 2021

elasticmachine commented Apr 27, 2021

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

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

lockewritesdocs 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

tvernum left a comment

Choose a reason for hiding this comment

rjernst May 10, 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

jkakavas Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

jkakavas commented Jul 15, 2021

jkakavas commented Jul 15, 2021

rjernst May 10, 2021 •

edited

Loading

jkakavas Jun 8, 2021 •

edited

Loading