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

Remove HTTPS check for API Keys & Service Accounts #76801

Merged
merged 6 commits into from Sep 21, 2021

Conversation

tvernum
Copy link
Contributor

@tvernum tvernum commented Aug 23, 2021

This commit removes the checks that prevented the use of API Keys and
Service Account (Service Tokens) on nodes without HTTPS
(xpack.security.http.ssl.enabled)

As a consequence of removing this check, the API Key service is now
automatically enabled, but can be explicitly disabled with

xpack.security.authc.api_key.enabled: false

This commit removes the checks that prevented the use of API Keys and
Service Account (Service Tokens) on nodes without HTTPS
(xpack.security.http.ssl.enabled)

As a consequence of removing this check, the API Key service is not
automatically enabled, but can be explicitly disabled with

    xpack.security.authc.api_key.enabled: false
@tvernum tvernum added >enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 v7.16.0 labels Aug 23, 2021
@tvernum
Copy link
Contributor Author

tvernum commented Aug 23, 2021

@elastic/es-security I've marked this as a draft while we discuss exactly what behaviour we want here.

Questions:

  • Do we want to include this in 7.16? (I've labelled as if we do, but it's open for discussion)
  • Do we want to automatically turn on the API Key Service even if HTTPS is disabled? (I've assumed we do, but opinions welcome).

@bytebilly
Copy link
Contributor

Do we want to include this in 7.16? (I've labelled as if we do, but it's open for discussion)

I'm in favor of doing that. Users that have to stay on 7.x will be able to fully leverage Kibana Alerting and other similar features even if their environment requires HTTPS off, for example service mesh setups.

Do we want to automatically turn on the API Key Service even if HTTPS is disabled? (I've assumed we do, but opinions welcome).

In 8.x I'm in favor of turning on by default, since security is also on by default.
I'm not against turning on by default in 7.x, but we could keep it off if we want to make it "less breaking".

@jkakavas
Copy link
Member

Do we want to include this in 7.16?

I agree we should and I think that previous team discussions also came to the same conclusion

Do we want to automatically turn on the API Key Service even if HTTPS is disabled?

I think that for 8.x, since we are deciding to decouple API Key Service from HTTPS configuration, it wouldn't make sense to turn off API Key service if HTTPS is disabled. In 7.x though, this has the potential of being a "breaking change" for some of our users. If an administrator willingly wants API Key service to be disabled and now depends on it being disabled because TLS for HTTP is not enabled, will have API Key service enabled (unknowingly) on upgrade to 7.16. I can't think of a use case where this becomes breaking in itself, but consider the case where we ( or an application that uses elasticsearch ) has a bug where API keys can be created and leaked and can be used for data exfiltration.
It feels also consistent with what we did with enabling the file realm by default. We could have done it in 7.x too but this would be "breaking" in a similar way, and we did it only in 8.0

Copy link
Member

@jkakavas jkakavas left a comment

Choose a reason for hiding this comment

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

We also need to update

docs/reference/settings/security-settings.asciidoc#L174
x-pack/docs/en/security/authentication/service-accounts.asciidoc#L61

@ywangd
Copy link
Member

ywangd commented Aug 25, 2021

I second on @jkakavas's comment. Also a few more docs to update:

  • x-pack/docs/en/rest-api/security/create-api-keys.asciidoc#L33
  • x-pack/docs/en/security/securing-communications/tls-http.asciidoc#L11
  • x-pack/docs/en/security/securing-communications/security-basic-setup-https.asciidoc#L8

@tvernum
Copy link
Contributor Author

tvernum commented Sep 9, 2021

If an administrator willingly wants API Key service to be disabled and now depends on it being disabled because TLS for HTTP is not enabled, will have API Key service enabled (unknowingly) on upgrade to 7.16.

My counter point would be that if we follow this line of reasoning, then the first time something like API Key Service is introduced it should be disabled by default because we didn't get administrators to opt-in to it.
Moving from disabled by default to enabled by default shouldn't be any more of a risk or breaking change than moving from does not exist to enabled by default.

I appreciate the analogy to file realm though, so I'm happy to not change the default in 7.16, but I'm cautious about setting too much of a precedent here - we shouldn't be afraid to add features in a minor.

@jkakavas
Copy link
Member

jkakavas commented Sep 9, 2021

My counter point would be that if we follow this line of reasoning, then the first time something like API Key Service is introduced it should be disabled by default because we didn't get administrators to opt-in to it.
Moving from disabled by default to enabled by default shouldn't be any more of a risk or breaking change than moving from does not exist to enabled by default.

but I'm cautious about setting too much of a precedent here - we shouldn't be afraid to add features in a minor.

True, I see your point.

@tvernum tvernum marked this pull request as ready for review September 9, 2021 07:19
@elasticmachine elasticmachine added the Team:Security Meta label for security team label Sep 9, 2021
Copy link
Member

@jkakavas jkakavas left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

LGTM

I appreciate the analogy to file realm though, so I'm happy to not change the default in 7.16

Do you plan to change the default when backporting? If so, all relevant docs/javadocs need to be changed as well. Do we also want to give users heads up in 7.16 docs about the default will be changed again in 8.0?

@ywangd
Copy link
Member

ywangd commented Sep 20, 2021

Just a thought crossed my mind: Should API keys be always enabled in 8.0? That is, they cannot be disabled similar to service accounts? It's often used as an alternative to avoid directly serialising user credentials. It is also long lived. So it is potentially useful to any long running tasks.

I am not suggesting to make the change for this PR. But I think it is worth discussing before finalising 8.0.

@tvernum
Copy link
Contributor Author

tvernum commented Sep 21, 2021

Do you plan to change the default when backporting? If so, all relevant docs/javadocs need to be changed as well.

Yes, the backport will require a few tweaks. Does anyone specifically want to review that PR before it merges?

Do we also want to give users heads up in 7.16 docs about the default will be changed again in 8.0?

I'm not sure - writing docs that talk about the future is risky. Probably we can get away with "A future release will ... "

Should API keys be always enabled in 8.0?

I think we probably want to work towards them always being enabled. I don't know whether we want to do that in 8.0, or just deprecating the setting and remove it in a future release. We should discuss.

@tvernum
Copy link
Contributor Author

tvernum commented Sep 21, 2021

@elasticmachine update branch

@bytebilly
Copy link
Contributor

Should API keys be always enabled in 8.0?

I did some telemetry analysis and the number of clusters with API keys "explicitly" disabled (HTTPS on, API keys off) is irrelevant.

However, since removing the configuration setting is a breaking change, I suggest to deprecate it in 8.0 and remove later.

@tvernum tvernum merged commit ea0dc45 into elastic:master Sep 21, 2021
tvernum added a commit to tvernum/elasticsearch that referenced this pull request Sep 22, 2021
This commit removes the checks that prevented the use of API Keys and
Service Account (Service Tokens) on nodes without HTTPS
(xpack.security.http.ssl.enabled)

The API Key service is only enabled by default if HTTPS is also enabled
however it can be explicitly enabled (regardless of HTTPS) with

     xpack.security.authc.api_key.enabled: false

Backport of: elastic#76801
ywangd added a commit to ywangd/elasticsearch that referenced this pull request Sep 22, 2021
Since elastic#76801, the API Key feature is enabled by default and no longer
ties to HTTPS setting. This PR fixes the test to reflect the new
default.

Resolves: elastic#78162
ywangd added a commit that referenced this pull request Sep 22, 2021
Since #76801, the API Key feature is enabled by default and no longer
ties to HTTPS setting. This PR fixes the test to reflect the new
default.

Resolves: #78162
arteam pushed a commit to arteam/elasticsearch that referenced this pull request Sep 22, 2021
This commit removes the checks that prevented the use of API Keys and
Service Account (Service Tokens) on nodes without HTTPS
(xpack.security.http.ssl.enabled)

As a consequence of removing this check, the API Key service is now
automatically enabled, but can be explicitly disabled with

     xpack.security.authc.api_key.enabled: false
arteam pushed a commit to arteam/elasticsearch that referenced this pull request Sep 22, 2021
Since elastic#76801, the API Key feature is enabled by default and no longer
ties to HTTPS setting. This PR fixes the test to reflect the new
default.

Resolves: elastic#78162
tvernum added a commit that referenced this pull request Sep 23, 2021
This commit removes the checks that prevented the use of API Keys and
Service Account (Service Tokens) on nodes without HTTPS
(xpack.security.http.ssl.enabled)

The API Key service is only enabled by default if HTTPS is also enabled
however it can be explicitly enabled (regardless of HTTPS) with

     xpack.security.authc.api_key.enabled: false

Backport of: #76801
tvernum added a commit to tvernum/elasticsearch that referenced this pull request Sep 23, 2021
tvernum added a commit that referenced this pull request Sep 23, 2021
@t-indumathy
Copy link

t-indumathy commented Dec 8, 2021

Is this applicable for xpack.security.authc.token.enabled: true too ? Because when I try to use Istio and apply OIDC realm, it expects xpack.security.authc.token.enabled: true which makes xpack.security.http.ssl.enabled: true to be mandatory. Whereas the mesh layer takes care of mTLS within mesh.

@tvernum
Copy link
Contributor Author

tvernum commented Dec 8, 2021

It does not apply to xpack.security.authc.token.enabled

The Elasticsearch token service tries (where possible) to conform to OAuth2 specs, which require https.
We might change that position in the future, but this PR did not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Security Meta label for security team v7.16.0 v8.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants