Internalise run-as user and simplify the regular User class #86246

ywangd · 2022-04-28T01:45:25Z

Today the run-as information is spread between User (authenticatedUser) and Authentication (lookup realm). They have to be configured consistently to work correctly. Previously there was also no inherent logic to ensure the consistency. Recent refacotring of Authentication class has made the situation better by favour Authentication creation with dedicated convenient methods over free-for-all constructors. #86206 is the ongoing PR that will finally remove public access of Authentication constructors. Now that the Authentication class is being tightly controlled, it makes possible to clean-up the User class. Specifically, the run-as information is already provided by the Authentication class, there is no need for the User class to also keep track of it. In fact, the way User class tracking the authenticating user with an inner user is less straightforward and not friendly to serialisation. Also, conceptually run-as is an information at Authentication level instead of User level.

This PR refactors the User class so that it no longer keeps track of run-as information so that there is a single consistent way to check whether an Authentication object is run-as. The essential changes are:

Removes User.isRunAs() method
Removes User.authenticatedUser() method and its backing instance variable
Removes all User constructors that take authenticatedUser as an argument
Adds a new private RunAsUser class inside Authentication

Note that this RunAsUser class is not really necessary in long term, the plan is to remove it in the later refactoring of Authentication class (where Subject variable and methods will become primary over the current User). It is added mostly to make the refactoring easier and reduce the change to how things work conceptually. This class is not exposed and there should be no need to use this class outside of Authentication itself. This tight scope should make it relatively easy to remove it later.

Relates: #86206
Relates: #80117

elasticmachine · 2022-04-29T00:10:59Z

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

ywangd · 2022-04-29T00:24:35Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/user/User.java

-    public static User readFrom(StreamInput input) throws IOException {
-        final boolean isInternalUser = input.readBoolean();
-        assert isInternalUser == false : "should always return false. Internal users should use the InternalUserSerializationHelper";
-        final String username = input.readString();
-        return partialReadFrom(username, input);
-    }


This method is replaced by the new AuthenticationSerializationHelper#readUserFrom. The replacement is basically the old InternalUserSerializationHelper.readFrom.

Note that this method assert that the user cannot be internal. I don't think it is necessary at this method level. I have replaced it with assertions at the caller sites (there are only 2 of them).

ywangd · 2022-04-29T00:25:09Z

...n/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/GetUsersResponse.java

@@ -29,7 +30,9 @@ public GetUsersResponse(StreamInput in) throws IOException {
        } else {
            users = new User[size];
            for (int i = 0; i < size; i++) {
-                users[i] = User.readFrom(in);
+                final User user = Authentication.AuthenticationSerializationHelper.readUserFrom(in);
+                assert false == User.isInternal(user) : "should not get internal users";


This is the new call site assertion metioned above.

ywangd · 2022-04-29T00:25:27Z

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

+            boolean hasInnerUser = input.readBoolean();
+            if (hasInnerUser) {
+                User innerUser = readUserFrom(input);
+                assert false == User.isInternal(innerUser) : "authenticating user cannot be internal";


This is the other call site assertion.

ywangd · 2022-04-29T00:26:50Z

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/user/UserTests.java

-        user = new User("u1", new String[] { "r1" }, new User("u2", "r2", "r3"));
-        assertThat(
-            user.toString(),
-            is(
-                "User[username=u1,roles=[r1],fullName=null,email=null,metadata={},"
-                    + "authenticatedUser=[User[username=u2,roles=[r2,r3],fullName=null,email=null,metadata={}]]]"
-            )
-        );


No need to test anymore because User no longer has inner user.

ywangd · 2022-04-29T00:33:48Z

A large number of changes to test classes, e.g. LoggingAuditTrailFilterTests, are due to:

They randomise whether a user has inner user and ensure the effective user is picked up
They randomise authentication based on whether the randomised user has an inner user

For 1, the randomisation is simply removed.
For 2, I refactored the method to take two User arguments instead of one so that the inner user is provided as its own argument, e.g. previously createAuthentication(User user, String realmName) where user can have an inner user to indicate the Authentication should be created with run-as. This is changed to createAuthentication(User user, @Nullable User authenticatingUser, String realmName). The call sites are also updated accordingly.

ywangd · 2022-04-29T00:35:32Z

...c/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailFilterTests.java

-        final List<User> filteredUsers = filteredUsernames.stream().map(u -> {
-            if (randomBoolean()) {
-                return new User(u);
-            } else {
-                return new User(new User(u), new User(UNFILTER_MARKER + randomAlphaOfLengthBetween(1, 4)));
-            }
-        }).collect(Collectors.toList());


This is an example where it is no longer necessary to randomise whether User has an inner user.

ywangd · 2022-04-29T00:37:06Z

...c/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailFilterTests.java

-            authentication = createAuthentication(
-                new User("user1", new String[] { "r1" }, new User("authUsername", new String[] { "r2" })),
-                "effectiveRealmName"
-            );
+            authentication = createAuthentication(new User("user1", "r1"), new User("authUsername", "r2"), "effectiveRealmName");


This is an example where a single User (with inner user) argument is split into 2 User arguments.

ywangd · 2022-04-29T00:40:00Z

...n/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticatorChainTests.java

-        authentication = mock(Authentication.class);
-        when(authentication.getUser()).thenReturn(user);
+        authentication = AuthenticationTestHelper.builder().user(user).build(false);


Mocking Authentication is no longer recommended. In future PPs, I plan to remove all mocking usage from tests and make Authentication final.

tvernum · 2022-05-02T05:08:38Z

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

+            } else if (XPackSecurityUser.is(user)) {
+                output.writeBoolean(true);
+                output.writeString(XPackSecurityUser.NAME);
+            } else if (SecurityProfileUser.is(user)) {


This isn't an issue with this PR, but I noticed it here.
How do we handle BWC/mixed-nodes here? If output has a version before 8.3, then the other side won't know about, and won't be able to deserialize the SecurityProfileUser.

Do we need BWC for this given the feature is still beta? I had assumed that we don't need it. For example, the profile index mappings changed in both 8.2 and 8.3 in a non-compatible way. It is unlikely things would work in a mixed cluster even without this new internal user. That said, maybe we should have phased the changes better so that they are BWC all the way. Unfortunately it was not the case. What do you think?

But what happens when someone does a rolling upgrade from 7.17 to 8.7 ? Will the new internal user get passed around in cross-node requests and cause failures on the 7.17 nodes?

If they call User Profile APIs on the 8.7 node, then yes, it is possible that they can see the failure on a 7.17 node. A BWC for this internal user will not fix the failure entirely. But one may argue that the error message could be slightly more friendy?

Instead of a mysterious serialisation failure (user [_security_profile] is not an internal user), it could be a 404 index_not_found (no such index [_security]) or 400 (request body is required) or some other errors. Now I actually don't know which one is less friendly. The serialisation failure is probably more mysterious but it fails consistently and mentions profile in the failure message, while the other option could be more unpredictable.

Sorry I have to correct myself: the 400 or 404 error is what user can get on the REST layer. Since this talks about transport layer, I think the error should always be something along the line of action handler not found, which is arguably a more friend error message. I think this is enough to convince me to handle BWC for this user. I will raise a separate PR. Thanks for the discussion.

I raised #86345
Btw, the BWC with the above PR is better than I expected. As long as the request hits nodes with newer version first, it should just work because requests that actually got sent across nodes are basic index and search. Feature specific actions are all handled locally.

tvernum · 2022-05-02T06:10:32Z

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

-        assert false == runAs.isRunAs();
-        assert false == getUser().isRunAs();
+        assert false == runAs instanceof RunAsUser;
+        assert false == getUser() instanceof RunAsUser;
        assert AuthenticationType.REALM == getAuthenticationType() || AuthenticationType.API_KEY == getAuthenticationType();


Should we also check that runAs.getAuthenticationType() == AuthenticationType.REALM ?

Not sure if I miss something, but runAs is a User, not an Authentication. So it does not have an AuthenticationType. Do you mean we should check lookUpRealmRef is not one of the sythetic realms? Unfortunately we cannot because of custom realms and we don't retrict their names or types.

Sorry, yeah - too many classes with things in the wrong place.

Logically we ought to check that runAs is a realm based used, and not an Internal user or an API Key, or ServiceAccount.

Since the proposal is an assert, I don't think we need to worry about custom realms. ES doesn't run with assertions in production, and if it interferes with someone's development environment then it just serves as a reminder that they picked a terrible name & type for their realm.

I added assertion for lookupRealmRef not having synthetic realm name or type.

PS: I also plan to consolidate all the assertions from different Authentication methods into its own private method and call it inside the constructors. This method will also serve a good reference for the internal logic of authentication object.

tvernum · 2022-05-02T06:32:40Z

...ck/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java

        final String authenticationType = changePasswordRequest
            ? randomFrom(ReservedRealm.TYPE, NativeRealmSettings.TYPE)
            : randomAlphaOfLengthBetween(4, 12);


I don't think we need this conditional do we?
This is used for the authUser - we don't care what realm type it has, we only care which realm user was lookedUpBy.

Suggested change

final String authenticationType = changePasswordRequest

? randomFrom(ReservedRealm.TYPE, NativeRealmSettings.TYPE)

: randomAlphaOfLengthBetween(4, 12);

final String authenticationType = randomAlphaOfLengthBetween(4, 12);

Though authenticationRealmType is probably a better name.

You are right. It does not matter. I inlined it with simply randomAlphaOfLengthBetween(4, 12).

tvernum

LGTM

ywangd · 2022-05-04T05:59:26Z

@elasticmachine update branch

Another refactor leftover. Relates: elastic#86246 Resolves: elastic#86421

Another refactor leftover. Relates: #86246 Resolves: #86421

This PR is a follow-up of elastic#86246 to further clean up the Authentication class by: * Promoting usage of the Subject class. The User field and a few other related fields are now removed from Authentication. Relevant methods have their implementation replaced by using Subjects and same behaviours are retained. * Removed the temporary internal RunAsUser class. It essence is about wire serialisation which is now merged into Authentication itself. * Simplify serialisation of regular User object. All the complexities of handling inner user is now completely within the Authentication class itself. * Cosnolidate assertions in different places into a single method that is called in constructors. Also removed a few assertions because there is no RunAsUser class anymore and a User object is just a simple user. Relates: elastic#86246 Relates: elastic#86544

This PR is a follow-up of #86246 to further clean up the Authentication class by: * Promote usage of the Subject class. The User field and a few other related fields are now removed from Authentication. Relevant methods have their implementation replaced by using Subjects and same behaviours are retained. * Remove the temporary internal RunAsUser class. Its essence is about wire serialisation which is now merged into Authentication itself. * Simplify serialisation of regular User object. All the complexities of handling inner user is now completely within the Authentication class itself. * Consolidate assertions in different places into a single method that is called in constructors. Also removed a few assertions because there is no RunAsUser class anymore and a User object is just a simple user. Relates: #86246 Relates: #86544 Resolves: #80117

elasticsearchmachine added the v8.3.0 label Apr 28, 2022

wip

e621d44

ywangd force-pushed the authentication-run-as-user branch from 180eb22 to e621d44 Compare April 28, 2022 01:51

ywangd added 6 commits April 28, 2022 15:22

fix tests

0c91c0e

fix test

4fbc90b

fix more usages

f16fed7

Remove User.isRunAs

ee8bd68

fix tests

5d93e71

Remove authenticatedUser

87d3174

ywangd changed the title ~~Internalise run-as user~~ Internalise run-as user and simplify the regular User class Apr 28, 2022

ywangd marked this pull request as ready for review April 29, 2022 00:10

ywangd added >refactoring :Security/Security Security issues without another label labels Apr 29, 2022

ywangd requested a review from tvernum April 29, 2022 00:10

elasticmachine added the Team:Security Meta label for security team label Apr 29, 2022

ywangd commented Apr 29, 2022

View reviewed changes

tvernum reviewed May 2, 2022

View reviewed changes

ywangd added 3 commits May 2, 2022 17:19

address feedback

8b0d0c6

address feedback

deaabf9

address feedback: add assertion for synthetic realms

06eb4e3

ywangd requested a review from tvernum May 3, 2022 01:14

tvernum approved these changes May 4, 2022

View reviewed changes

Merge branch 'master' into authentication-run-as-user

d55bb6c

ywangd merged commit cc7a5a8 into elastic:master May 4, 2022

breskeby mentioned this pull request May 4, 2022

[CI] CustomAuthorizationEngine.java:50: error: cannot find symbol authentication.getUser().isRunAs() #86421

Closed

ywangd added a commit to ywangd/elasticsearch that referenced this pull request May 4, 2022

[Test] Replace removed User methods

15164db

Another refactor leftover. Relates: elastic#86246 Resolves: elastic#86421

ywangd mentioned this pull request May 4, 2022

[Test] Fixing compilation error by replacing removed User methods #86422

Merged

elasticsearchmachine pushed a commit that referenced this pull request May 4, 2022

[Test] Replace removed User methods (#86422)

286cb2b

Another refactor leftover. Relates: #86246 Resolves: #86421

ywangd mentioned this pull request May 4, 2022

User APIs allow use of internal usernames #86398

Merged

ywangd mentioned this pull request Jul 13, 2022

Promote usage of Subjects in Authentication class #88494

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Internalise run-as user and simplify the regular User class #86246

Internalise run-as user and simplify the regular User class #86246

ywangd commented Apr 28, 2022 •

edited

Loading

elasticmachine commented Apr 29, 2022

ywangd Apr 29, 2022

ywangd Apr 29, 2022

ywangd Apr 29, 2022

ywangd Apr 29, 2022

ywangd commented Apr 29, 2022

ywangd Apr 29, 2022

ywangd Apr 29, 2022

ywangd Apr 29, 2022

tvernum May 2, 2022

ywangd May 2, 2022

tvernum May 2, 2022

ywangd May 2, 2022

ywangd May 2, 2022

ywangd May 2, 2022

tvernum May 2, 2022

ywangd May 2, 2022

tvernum May 2, 2022

ywangd May 2, 2022

tvernum May 2, 2022

ywangd May 2, 2022

tvernum left a comment

ywangd commented May 4, 2022

Internalise run-as user and simplify the regular User class #86246

Internalise run-as user and simplify the regular User class #86246

Conversation

ywangd commented Apr 28, 2022 • edited Loading

elasticmachine commented Apr 29, 2022

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 Apr 29, 2022

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

tvernum left a comment

Choose a reason for hiding this comment

ywangd commented May 4, 2022

ywangd commented Apr 28, 2022 •

edited

Loading