Skip to content
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

Warn users if security is implicitly disabled #70114

Merged
merged 64 commits into from
Apr 13, 2021

Conversation

BigPandaToo
Copy link
Contributor

Elasticsearch has security features implicitly disabled by default for
Basic and Trial licenses, unless explicitly set in the configuration
file.
This may be good for onboarding, but it also lead to unintended insecure
clusters.
This change introduces clear warnings when security features are
implicitly disabled.

  • a warning header in each REST response if security is implicitly
    disabled;
  • a log message during cluster boot.

Elasticsearch has security features implicitly disabled by default for
Basic and Trial licenses, unless explicitly set in the configuration
file.
This may be good for onboarding, but it also lead to unintended insecure
 clusters.
 This change introduces clear warnings when security features are
 implicitly disabled.
 - a warning header in each REST response if security is implicitly
 disabled;
 - a log message during cluster boot.
@BigPandaToo
Copy link
Contributor Author

@elasticmachine update branch

@elasticmachine
Copy link
Collaborator

merge conflict between base and head

@BigPandaToo BigPandaToo requested a review from tvernum March 8, 2021 21:11
@romseygeek romseygeek added the :Security/Security Security issues without another label label Mar 9, 2021
@elasticmachine elasticmachine added the Team:Security Meta label for security team label Mar 9, 2021
@elasticmachine
Copy link
Collaborator

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

@BigPandaToo
Copy link
Contributor Author

@elasticmachine update branch

@BigPandaToo
Copy link
Contributor Author

@elasticmachine update branch

@BigPandaToo
Copy link
Contributor Author

@elasticmachine update branch

@BigPandaToo
Copy link
Contributor Author

@elasticmachine update branch

@BigPandaToo
Copy link
Contributor Author

@elasticmachine update branch

HeaderWarning.addWarning("Elasticsearch built-in security features are not enabled, your cluster may be accessible " +
"without authentication. Read https://www.elastic.co/guide/en/elasticsearch/reference/" + Version.CURRENT.major + "." +
Version.CURRENT.minor + "/get-started-enable-security.html for more information");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we checked with the docs team (maybe @lockewritesdocs ?) about whether we should point directly to this URL?
It might be better to have a redirect in place and link to that - I don't know, they're the experts.

Whatever we put here is likely to need to be maintained for 2+ years, while 7.x is still a supported version, so we should take a few minutes to make the best possible choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to find out

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linked page was removed as part of the recent security documentation overhaul. However, there is a redirect in place to Set up minimal security for the Elastic Stack, so I would use that link instead.

Hardcoded links can be brittle, but if the page is moved or changed, we implement redirects like the one for the get-started-enable-security.html page. The Kibana team uses a link service to manage doc links instead of hardcoding them, but that's a much heavier lift than adding a single link.

@BigPandaToo
Copy link
Contributor Author

@elasticmachine update branch

Copy link
Contributor

@lockewritesdocs lockewritesdocs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BigPandaToo, I added a suggested change to update the doc link and slightly modified the language.

Comment on lines 94 to 96
HeaderWarning.addWarning("Elasticsearch built-in security features are not enabled, your cluster may be accessible " +
"without authentication. Read https://www.elastic.co/guide/en/elasticsearch/reference/" + Version.CURRENT.major + "." +
Version.CURRENT.minor + "/get-started-enable-security.html for more information");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
HeaderWarning.addWarning("Elasticsearch built-in security features are not enabled, your cluster may be accessible " +
"without authentication. Read https://www.elastic.co/guide/en/elasticsearch/reference/" + Version.CURRENT.major + "." +
Version.CURRENT.minor + "/get-started-enable-security.html for more information");
containsString("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."));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the link with this suggested change and also modified the language a bit. Let me know if those changes are 👍

@BigPandaToo
Copy link
Contributor Author

@elasticmachine update branch

@BigPandaToo
Copy link
Contributor Author

@elasticmachine update branch

@BigPandaToo BigPandaToo merged commit 3b0b794 into elastic:master Apr 13, 2021
BigPandaToo added a commit to BigPandaToo/elasticsearch that referenced this pull request Apr 13, 2021
* Warn users if security is implicitly disabled

Elasticsearch has security features implicitly disabled by default for
Basic and Trial licenses, unless explicitly set in the configuration
file.
This may be good for onboarding, but it also lead to unintended insecure
 clusters.
 This change introduces clear warnings when security features are
 implicitly disabled.
 - a warning header in each REST response if security is implicitly
 disabled;
 - a log message during cluster boot.
BigPandaToo added a commit that referenced this pull request Apr 13, 2021
* Warn users if security is implicitly disabled (#70114)

Elasticsearch has security features implicitly disabled by default for
Basic and Trial licenses, unless explicitly set in the configuration
file.
This may be good for onboarding, but it also lead to unintended insecure
 clusters.
 This change introduces clear warnings when security features are
 implicitly disabled.
 - a warning header in each REST response if security is implicitly
 disabled;
 - a log message during cluster boot.
@jkakavas
Copy link
Member

In retrospect this would have been a good candidate for our :Security/FIPS label that would have run tests in fips mode and would have caught all YAML tests failing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Security/Security Security issues without another label Team:Security Meta label for security team v7.13.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants