Remove last usage of Authentication constructor from production code

[ywangd]

Authentication object has many internal logic on what values can or
cannot be used together. Hence it is better to instantiate it using
convenient methods that is tailored for specific situations instead of
the free usage of constructors.

This PR adds a new convenient method for incorporating anonymous roles
so that all production usages of constructors are removed (other than by
the class itself). The next step is to remove usages in test code and
ultimately locking down how Authentication can be created.

PS: Strictly speaking, the Authentication(StreamInput) constructor is
still used in production code. But this one is special, less used and
not convenient to use. So we can let it stay for now. In long term, we
can also lock it down.

[elasticmachine]

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

[albertzaharovits]

I think we should be using the new method in the authorization logic as well.

It's good that we move the logic inside a method, and avoid calling the Authentication constructor. It's even better if we reuse the method.

Something like the following:

diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Subject.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Subject.java
index 509f33c93a0..c174bbf095f 100644
--- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Subject.java
+++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Subject.java
@@ -11,12 +11,9 @@ import org.elasticsearch.ElasticsearchSecurityException;
 import org.elasticsearch.Version;
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.bytes.BytesReference;
-import org.elasticsearch.common.util.ArrayUtils;
-import org.elasticsearch.core.Nullable;
 import org.elasticsearch.xpack.core.security.authc.service.ServiceAccountSettings;
 import org.elasticsearch.xpack.core.security.authz.store.RoleReference;
 import org.elasticsearch.xpack.core.security.authz.store.RoleReferenceIntersection;
-import org.elasticsearch.xpack.core.security.user.AnonymousUser;
 import org.elasticsearch.xpack.core.security.user.User;
 
 import java.util.Map;
@@ -86,10 +83,10 @@ public class Subject {
         return metadata;
     }
 
-    public RoleReferenceIntersection getRoleReferenceIntersection(@Nullable AnonymousUser anonymousUser) {
+    public RoleReferenceIntersection getRoleReferenceIntersection() {
         switch (type) {
             case USER:
-                return buildRoleReferencesForUser(anonymousUser);
+                return buildRoleReferencesForUser();
             case API_KEY:
                 return buildRoleReferencesForApiKey();
             case SERVICE_ACCOUNT:
@@ -157,21 +154,8 @@ public class Subject {
             + '}';
     }
 
-    private RoleReferenceIntersection buildRoleReferencesForUser(AnonymousUser anonymousUser) {
-        if (user.equals(anonymousUser)) {
-            return new RoleReferenceIntersection(new RoleReference.NamedRoleReference(user.roles()));
-        }
-        final String[] allRoleNames;
-        if (anonymousUser == null || false == anonymousUser.enabled()) {
-            allRoleNames = user.roles();
-        } else {
-            // TODO: should we validate enable status and length of role names on instantiation time of anonymousUser?
-            if (anonymousUser.roles().length == 0) {
-                throw new IllegalStateException("anonymous is only enabled when the anonymous user has roles");
-            }
-            allRoleNames = ArrayUtils.concat(user.roles(), anonymousUser.roles());
-        }
-        return new RoleReferenceIntersection(new RoleReference.NamedRoleReference(allRoleNames));
+    private RoleReferenceIntersection buildRoleReferencesForUser() {
+        return new RoleReferenceIntersection(new RoleReference.NamedRoleReference(user.roles()));
     }
 
     private RoleReferenceIntersection buildRoleReferencesForApiKey() {
diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java
index bba75559dc2..8110316a064 100644
--- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java
+++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java
@@ -170,7 +170,8 @@ public class CompositeRolesStore {
     }
 
     public void getRoles(Authentication authentication, ActionListener<Tuple<Role, Role>> roleActionListener) {
-        final AuthenticationContext authenticationContext = AuthenticationContext.fromAuthentication(authentication);
+        Authentication effectiveAuthentication = authentication.maybeAddAnonymousRoles(this.anonymousUser);
+        final AuthenticationContext authenticationContext = AuthenticationContext.fromAuthentication(effectiveAuthentication);
         getRole(authenticationContext.getEffectiveSubject(), ActionListener.wrap(role -> {
             if (authenticationContext.isRunAs()) {
                 getRole(
@@ -195,7 +196,7 @@ public class CompositeRolesStore {
 
         assert false == User.isInternal(subject.getUser()) : "Internal user should not pass here";
 
-        final RoleReferenceIntersection roleReferenceIntersection = subject.getRoleReferenceIntersection(anonymousUser);
+        final RoleReferenceIntersection roleReferenceIntersection = subject.getRoleReferenceIntersection();
         roleReferenceIntersection.buildRole(this::buildRoleFromRoleReference, roleActionListener);
     }
 
@@ -345,7 +346,7 @@ public class CompositeRolesStore {
         tryGetRoleDescriptorForInternalUser(subject).ifPresentOrElse(
             roleDescriptor -> listener.onResponse(List.of(Set.of(roleDescriptor))),
             () -> {
-                final List<RoleReference> roleReferences = subject.getRoleReferenceIntersection(anonymousUser).getRoleReferences();
+                final List<RoleReference> roleReferences = subject.getRoleReferenceIntersection().getRoleReferences();
                 final GroupedActionListener<Set<RoleDescriptor>> groupedActionListener = new GroupedActionListener<>(
                     listener,
                     roleReferences.size()

What do you think? Was this the plan for a follow-up or there were other plans?

[ywangd]

I think we should be using the new method in the authorization logic as well.

I intentionally left it out in this PR and will be raising it as a separate one. I talked about it during the sync but I may not have been clear.

The initial plan was indeed to reuse this method in CompositeRolesStore so that Subject.getRoleReferenceIntersection no longer needs to take AnonymousUser as the argument. I went with that initially and found the method also needs to be reused in ApiKeyGenerator.

This made me thinking that dropping AnonymousUser from Subject.getRoleReferenceIntersection has the assumption that the Subject must be "prepared" correctly, otherwise the result could miss the anonymous roles. My concern is that it is easier to be misused because next author needs to know Authentication must be prepared first before using the Subject. The possible mitigation is to make CompositeRolesStore the only place where this preparation needs to happen. This likely means refactor CompositeRolesStore.getRoleDescriptorsList method so that ApiKeyGenerator does not have to "prepare" the Authentication. All these is doable but has cascading changes which are not essential to and in fact blur (due to amount of changes) the main focus of this PR. So I prefer to have it as a follow-up.

[albertzaharovits]

Okay, I'm content that you've got it on your radar.

The premise is that the Authentication is initially constructed without the anonymous roles and we cannot easily change that.
This limitation demands that the anonymous roles are "injected" when resolving the roles during authz 1) and when creating API keys 2).
It also requires that the anonymous roles are again "injected" in the Authentication that we present in the response to the _security/authenticate call 3).

So, we need a method that does the "anonymous roles inject" that can be reused in the above 3 cases, which will influence its signature. I think it is a bonus, as I understand you're saying, if we manage to make this method such that it never has to be invoked anywhere else, in the future.

My overall point is that the method that replaces the Authentication constructor should've been thought out to be reused in all the above 3 circumstances, and have a fitting signature for it, all in one PR. That's how I would've personally approached it.
But I OK with your proposal to tackle it incrementally.

[albertzaharovits]

LGTM

[ywangd]

Thanks Albert. You summarized the usage nicely.

Since the future change will remove theanonymousUser argument from Subject.getRoleReferenceIntersection, anything related to role or role name resolving must begin with Authentication. Currently CompositeRolesStore has some public methods, getRole and buildRoleFromRoleReference, that do not take Authentication as argument and hence can possibly be misused (and therefore missing anonymous roles). We should be able to make these public methods private or at least protected because they are public only for test usages. This is also a piece of work that I intend for the follow-up PR. Just to say this also adds to potential amount of changes which made me decide to separate this one out.