Enable run_as for all authentication schemes #79809

ywangd · 2021-10-26T13:43:53Z

This PR removes the restriction for run_as so the authenticated user
does not have to be authenticated by a realm. It can now also be either
an API key, a token or a service account. Note we don't currently have
a service account that has run_as privilege.

This PR also include a change to the existing internal behaviour: the
final authentication object created for run-as now keeps information of
the authenticated user instead of dropping them. For example, metadata
associated with the original authentication is preserved. If the
original authentication is an API key, the final authentication will
have authentication type as "api_key" as well.

elasticmachine · 2021-10-26T13:43:56Z

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

...plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticatorChain.java

...security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java

tvernum · 2021-10-28T02:08:35Z

...security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java

-        } else if (ApiKeyService.isApiKeyAuthentication(authentication)) {
-            getRolesForApiKey(authentication, roleActionListener);
+        if (authentication.getUser().isRunAs() && user == authentication.getUser().authenticatedUser()) {
+            if (ServiceAccountSettings.REALM_TYPE.equals(authentication.getAuthenticatedBy().getType())) {


I think this series of if/else would be clearer as a switch

I still used if/else in the updated version since the logic is now simpler. But please let me know if you still think switch is better.

tvernum · 2021-10-28T02:12:32Z

...security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java

+            if (ServiceAccountSettings.REALM_TYPE.equals(authentication.getAuthenticatedBy().getType())) {
+                getRolesForServiceAccount(authentication.getUser().authenticatedUser().principal(), roleActionListener);
+            } else if ("_es_api_key".equals(authentication.getAuthenticatedBy().getType())) {
+                getRolesForApiKey(authentication, roleActionListener);


I think we need a comment here to explain where we're using the authentication not the authenticatedUser().
It's not obvious that this is reliant of the API Key id being in the authentication metadata.

I added a comment

...rity/src/main/java/org/elasticsearch/xpack/security/authc/service/ServiceAccountService.java

This PR removes the restriction for run_as so the authenticated user does not have to be authenticated by a realm. It can now also be either an API key, a token or a service account. Note we don't currently have a service account that has run_as privilege. This PR also include a change to the existing internal behaviour: the final authentication object created for run-as now keeps information of the authenticated user instead of dropping them. For example, metadata associated with the original authentication is preserved. If the original authentication is an API key, the final authentication will have authentication type as "api_key" as well.

ywangd · 2021-10-29T02:47:45Z

@tvernum The PR is now updated according to our last chat. Sorry to waste some of your time on the initial version. It is now ready for another look. Thanks!

ywangd · 2021-10-29T02:49:47Z

.../src/main/java/org/elasticsearch/xpack/security/action/token/TransportCreateTokenAction.java

-                if (authentication.isServiceAccount()) {
+                if (authentication.isServiceAccount() && false == authentication.getUser().isRunAs()) {
                    listener.onFailure(new ElasticsearchException("OAuth2 token creation is not supported for service accounts"));


This means a service account by itself cannot create oauth tokens. But it can run-as a regular user and create oauth tokens.

I think this comes into my point above - the fact that you need an extra check here for "oh, except if it's run as", implies we have an issue.

I wonder whether we need something more like:

authentication.isServiceAccount(user)

and the answer depends on whether user is the authenticated user or the lookup user.
but then the API is kind of weird.

Maybe

enum UserType { AUTHENTICATING, // the user who provided credentials LOOKUP, // the user who was looked up, if any EFFECTIVE // the lookup user if there was one, otherwise the authenticating user }; boolean isServiceAccount(UserType type);

or

interface UserInfo { boolean isServiceAccount(); boolean isApiKey(); // etc; } public UserInfo getAuthenticatingUserInfo(); @Nullable; public UserInfo getLookupUserInfo(); public UserInfo getEffectiveUserInfo();

ywangd · 2021-10-29T03:39:54Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java

-        String userText = "user [" + authUser.principal() + "]";
+        String userText = (authentication.isServiceAccount() ? "service account" : "user") + " [" + authUser.principal() + "]";


This is visible to end user. So I am not sure if it should be categorized as breaking change. I think no since service account are not meant to be directly user facing. It however creates a RestCompat test failure with 7.16. I think if we agree to make the change here. I'll raise a small PR just to change this wording for 7.16 as a bug fix.

We haven't ever treated error messages as being part of the server's contract, & would not consider this to be a breaking change.

tvernum

I left some comments - we might want to discuss it directly next week.

I like where this is going, but I think there's a concept missing somewhere in our set of Authentication classes, and that makes it more complex than it needs to be.

tvernum · 2021-10-29T07:02:40Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java

-        String userText = "user [" + authUser.principal() + "]";
+        String userText = (authentication.isServiceAccount() ? "service account" : "user") + " [" + authUser.principal() + "]";


We haven't ever treated error messages as being part of the server's contract, & would not consider this to be a breaking change.

tvernum · 2021-10-29T07:11:40Z

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Authentication.java

@@ -114,7 +114,7 @@ public AuthenticationType getAuthenticationType() {
    }

    public boolean isServiceAccount() {
-        return ServiceAccountSettings.REALM_TYPE.equals(getAuthenticatedBy().getType()) && null == getLookedUpBy();
+        return ServiceAccountSettings.REALM_TYPE.equals(getAuthenticatedBy().getType());


I suspect we're going to end up with a bug at some point because this method is a now little ambiguous about what it does.
isServiceAccount is true, if the user authenticated using a service account token (or, in the future another credential), but the effective (run-as) user might not be a service account.

Anything that relies on isServiceAccount to make decisions has the potential to do the wrong thing if they don't take that into account.

I wonder if we can rename the method to make that more clear somehow.

isServiceAccountAuthentication() ?

isAuthenticatedWithServiceAccountCredential() ?

authenticatedAsServiceAccount()

I don't love any of those, but I think we need to do something to mitigate the risk.

tvernum · 2021-10-29T07:37:02Z

.../src/main/java/org/elasticsearch/xpack/security/action/token/TransportCreateTokenAction.java

-                if (authentication.isServiceAccount()) {
+                if (authentication.isServiceAccount() && false == authentication.getUser().isRunAs()) {
                    listener.onFailure(new ElasticsearchException("OAuth2 token creation is not supported for service accounts"));


I think this comes into my point above - the fact that you need an extra check here for "oh, except if it's run as", implies we have an issue.

I wonder whether we need something more like:

authentication.isServiceAccount(user)

and the answer depends on whether user is the authenticated user or the lookup user.
but then the API is kind of weird.

Maybe

enum UserType { AUTHENTICATING, // the user who provided credentials LOOKUP, // the user who was looked up, if any EFFECTIVE // the lookup user if there was one, otherwise the authenticating user }; boolean isServiceAccount(UserType type);

or

interface UserInfo { boolean isServiceAccount(); boolean isApiKey(); // etc; } public UserInfo getAuthenticatingUserInfo(); @Nullable; public UserInfo getLookupUserInfo(); public UserInfo getEffectiveUserInfo();

tvernum · 2021-10-29T07:39:47Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java

+        if (authentication.getUser().isRunAs()
+            || (false == authentication.isServiceAccount() && AuthenticationType.API_KEY != authentication.getAuthenticationType())) {


This is more complex than we'd like - I suspect it's a case of missing the getEffectiveUserInfo concept.

If we could have something like

Suggested change

if (authentication.getUser().isRunAs()

|| (false == authentication.isServiceAccount() && AuthenticationType.API_KEY != authentication.getAuthenticationType())) {

if (authentication.getEffectiveUserInfo().hasRoles()) {

I don't love "UserInfo" though, but I think you get the idea.
Maybe AccountType or UserType or PrincipalType would work

ywangd · 2021-11-01T05:46:36Z

@tvernum and I discussed in a separate channel and agreed that there are work needs to be done for the Authentication class so it's easier to reason about and use. We also agreed that it should be separate PRs and keep this one mostly as is. I raised #80117 and we agree to prioritize it once this PR is merged.

Relates: #80389, #79809

legrego · 2021-11-15T12:46:47Z

@ywangd does this resolve #51910?

ywangd · 2021-11-15T23:09:28Z

@ywangd does this resolve #51910?

Yes it does. Forgot that we have this issue. I will close it. Thanks for the reminder.

This is essentially a follow-up PR for #79809 which enables run-as for API keys. The code still has a few places where it assumes API keys cannot perform run-as. This PR fixes the incorrect assumptions and adds relevant tests. Resolves: #81425

API Key can run-as since elastic#79809. There are places in the code where we assume API key cannot run-as. Most of them are corrected in elastic#81564. But there are still a few things got missed. This PR fixes the methods for checking owner user realm for API key. Note the resource sharing check (canAccessResourcesOf) also needs to be fixed, this will be handled by elastic#84277

This PR removes run-as support for authentication types other than realm and API key. The change essentially makes the behaviour closer to the existing one (in released versions) except for API keys. This is not to say that the existing behaviour is the best. But we need more time to agree on the new behaviour. Relates: elastic#79809

This PR removes run-as support for authentication types other than realm and API key. The change essentially makes the behaviour closer to the existing one (in released versions) except for API keys. This is not to say that the existing behaviour is the best. But we need more time to agree on the new behaviour. Relates: #79809

API Key can run-as since #79809. There are places in the code where we assume API key cannot run-as. Most of them are corrected in #81564. But there are still a few things got missed. This PR fixes the methods for checking owner user realm for API key. This means, when API Keys "running-as" (impersonating other users), we do not expose the authenticating key ID and name to the end-user such as the Authenticate API and the SetSecurityUseringest processor. Only the effective user is revealed, just like in the regular case of a realm user run as. For audit logging, the key's ID and name are not exposed either. But this is mainly because there are no existing fields suitable for these information. We do intend to add them later (#84394) because auditing logging is to consumed by system admin instead of end-users. Note the resource sharing check (canAccessResourcesOf) also needs to be fixed, this will be handled by #84277

) This PR removes run-as support for authentication types other than realm and API key. The change essentially makes the behaviour closer to the existing one (in released versions) except for API keys. This is not to say that the existing behaviour is the best. But we need more time to agree on the new behaviour. Relates: elastic#79809

…84399) This PR removes run-as support for authentication types other than realm and API key. The change essentially makes the behaviour closer to the existing one (in released versions) except for API keys. This is not to say that the existing behaviour is the best. But we need more time to agree on the new behaviour. Relates: #79809

API Key can run-as since elastic#79809. There are places in the code where we assume API key cannot run-as. Most of them are corrected in elastic#81564. But there are still a few things got missed. This PR fixes the methods for checking owner user realm for API key. This means, when API Keys "running-as" (impersonating other users), we do not expose the authenticating key ID and name to the end-user such as the Authenticate API and the SetSecurityUseringest processor. Only the effective user is revealed, just like in the regular case of a realm user run as. For audit logging, the key's ID and name are not exposed either. But this is mainly because there are no existing fields suitable for these information. We do intend to add them later (elastic#84394) because auditing logging is to consumed by system admin instead of end-users. Note the resource sharing check (canAccessResourcesOf) also needs to be fixed, this will be handled by elastic#84277

* Fix owner user realm check for API key authentication (#84325) API Key can run-as since #79809. There are places in the code where we assume API key cannot run-as. Most of them are corrected in #81564. But there are still a few things got missed. This PR fixes the methods for checking owner user realm for API key. This means, when API Keys "running-as" (impersonating other users), we do not expose the authenticating key ID and name to the end-user such as the Authenticate API and the SetSecurityUseringest processor. Only the effective user is revealed, just like in the regular case of a realm user run as. For audit logging, the key's ID and name are not exposed either. But this is mainly because there are no existing fields suitable for these information. We do intend to add them later (#84394) because auditing logging is to consumed by system admin instead of end-users. Note the resource sharing check (canAccessResourcesOf) also needs to be fixed, this will be handled by #84277 * fix test

ywangd added >enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 labels Oct 26, 2021

ywangd requested a review from tvernum October 26, 2021 13:43

elasticmachine added the Team:Security Meta label for security team label Oct 26, 2021

tvernum reviewed Oct 28, 2021

View reviewed changes

ywangd force-pushed the more-general-run-as branch from fe946be to 2dc1796 Compare October 29, 2021 02:39

spotless

59f9204

ywangd requested a review from tvernum October 29, 2021 02:47

ywangd commented Oct 29, 2021

View reviewed changes

ywangd added 2 commits October 29, 2021 13:57

tweak

380116d

fix test failures

a7e1a05

ywangd commented Oct 29, 2021

View reviewed changes

spotless

e9f040a

ywangd added v8.1.0 and removed v8.0.0 labels Oct 29, 2021

tvernum reviewed Oct 29, 2021

View reviewed changes

ywangd mentioned this pull request Nov 1, 2021

Revisit and rationalise the Authentication class #80117

Closed

4 tasks

ywangd added 2 commits November 1, 2021 16:33

Merge remote-tracking branch 'origin/master' into more-general-run-as

9b4e192

address feedback

c1e4e47

ywangd requested a review from tvernum November 1, 2021 05:46

tvernum approved these changes Nov 1, 2021

View reviewed changes

ywangd added 3 commits November 3, 2021 09:56

spotless

8a3f071

Merge remote-tracking branch 'origin/master' into more-general-run-as

9ffc79d

fix tests

2ac1e4f

ywangd merged commit 67f6855 into elastic:master Nov 3, 2021

ywangd mentioned this pull request Nov 8, 2021

[Test] Re-enable v7RestCompatTest for service account #80471

Merged

elasticsearchmachine pushed a commit that referenced this pull request Nov 8, 2021

[Test] Re-enable v7RestCompatTest for service account (#80471)

ba78171

Relates: #80389, #79809

ywangd mentioned this pull request Nov 15, 2021

Allow requests authenticating with an API key to run as a different user #51910

Closed

This was referenced Dec 7, 2021

Review and correct checks for AuthenticationType.API_KEY #81425

Closed

Fix incorrect assumptions for api_key authentication type #81564

Merged

ywangd mentioned this pull request Jan 30, 2022

Authentication under domains #82639

Merged

pugnascotia changed the title ~~Enable run as for all authentication schemes~~ Enable run_as for all authentication schemes Jan 31, 2022

ywangd mentioned this pull request Feb 24, 2022

Fix owner user realm check for API key authentication #84325

Merged

This was referenced Feb 24, 2022

Restrict run-as to realm and api_key authentication types #84336

Merged

Improve auditing for run-as #84394

Open

ywangd mentioned this pull request Feb 28, 2022

Restrict run-as to realm and api_key authentication types (#84336) #84399

Merged

ywangd mentioned this pull request Feb 28, 2022

Fix owner user realm check for API key authentication (#84325) #84401

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable run_as for all authentication schemes #79809

Enable run_as for all authentication schemes #79809

ywangd commented Oct 26, 2021 •

edited

Loading

elasticmachine commented Oct 26, 2021

tvernum Oct 28, 2021

ywangd Oct 29, 2021

tvernum Oct 28, 2021

ywangd Oct 29, 2021

ywangd commented Oct 29, 2021

ywangd Oct 29, 2021

tvernum Oct 29, 2021

ywangd Oct 29, 2021

tvernum Oct 29, 2021

tvernum left a comment

tvernum Oct 29, 2021

tvernum Oct 29, 2021

tvernum Oct 29, 2021

tvernum Oct 29, 2021

ywangd commented Nov 1, 2021

legrego commented Nov 15, 2021

ywangd commented Nov 15, 2021

		String userText = "user [" + authUser.principal() + "]";
		String userText = (authentication.isServiceAccount() ? "service account" : "user") + " [" + authUser.principal() + "]";

		if (authentication.getUser().isRunAs()
		\|\| (false == authentication.isServiceAccount() && AuthenticationType.API_KEY != authentication.getAuthenticationType())) {

	if (authentication.getUser().isRunAs()
	\|\| (false == authentication.isServiceAccount() && AuthenticationType.API_KEY != authentication.getAuthenticationType())) {
	if (authentication.getEffectiveUserInfo().hasRoles()) {

Enable run_as for all authentication schemes #79809

Enable run_as for all authentication schemes #79809

Conversation

ywangd commented Oct 26, 2021 • edited Loading

elasticmachine commented Oct 26, 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

ywangd commented Oct 29, 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

tvernum 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

ywangd commented Nov 1, 2021

legrego commented Nov 15, 2021

ywangd commented Nov 15, 2021

ywangd commented Oct 26, 2021 •

edited

Loading