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

Revisit and rationalise the Authentication class #80117

Closed
2 of 4 tasks
ywangd opened this issue Nov 1, 2021 · 5 comments · Fixed by #88494
Closed
2 of 4 tasks

Revisit and rationalise the Authentication class #80117

ywangd opened this issue Nov 1, 2021 · 5 comments · Fixed by #88494
Assignees
Labels
>enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Security Meta label for security team

Comments

@ywangd
Copy link
Member

ywangd commented Nov 1, 2021

We would like to revisit the Authentication class to make it provide better and easier to use interface to the consumers.

#79809 enables run-as for all authentication schemes in addition to realm which is already supported. This adds more complexity to an Authentication object and how it should be used. For an example, building the role associated to an user now has to consider whether the user is the authenticated user or run-as user and the authentication scheme of the authenticated user. The existing Authentication class can answer all these question today. But its interface and internals are not aligned for these questions and the usage is error prone. For example, the Authentication object itself does not know whether the user has run-as. This information is kept by the User object. If the User object has an authenticatedUser, it's the run-as user. Otherwise it is not. But the information about realms for each User is directly kept in Authentication. Therefore, for full picture, the caller must check both User and Authentication realms and understands the nuance of their differenent combinations.

@ywangd ywangd added >enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels Nov 1, 2021
@ywangd ywangd self-assigned this Nov 1, 2021
@elasticmachine elasticmachine added the Team:Security Meta label for security team label Nov 1, 2021
@elasticmachine
Copy link
Collaborator

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

@tvernum
Copy link
Contributor

tvernum commented Nov 1, 2021

My first draft of a suggested model is something more like this:

class AuthenticationContext {

    private Version version;

    private ResolvedUser effectiveUser;
    private ResolvedUser authenticatedUser;

    private AuthenticationInfo authInfo;

    boolean hasLookupUser() {
        return effectiveUser != authenticatedUser;
    }

}

class ResolvedUser {
    User user;
    RealmRef realm;

    boolean isServiceAccount() { .. }
    boolean isApiKey() { .. }

    boolean hasNamedRoles() { return isServiceAccount() == false && isApiKey() == false; }

    // Not sure if this is really needed / useful
    RoleReference roles() {
        if(hasNamedRoles()) {
            return NamedRoles(user.roles());
        } else {
            // ...
        }
    }

}
    
class AuthenticationInfo {
    AuthenticationType type;
    Map<String,Object> metadata;
}

I haven't put a lot of thought into the class names, but I think the overall structure would probably work.

The only question is whether to also use this as an opportunity to do something with authorization_realms and represent them in the model as well (I haven't done that here, but I would like to).

@ywangd
Copy link
Member Author

ywangd commented Nov 3, 2021

The only question is whether to also use this as an opportunity to do something with authorization_realms and represent them in the model as well (I haven't done that here, but I would like to).

This is a good point. I also would like to. The challenge is that this function is currently managed internally by the supporting realms and buried under the common Realm#authenticate interface which only cares about the User and not realms. IIUC, this is by design because we don't want the realm to tell us directly about its realm information which is managed externally (previously by AuthenticationService and now AuthenticationChain).

Assuming this is still the principal, it's likely that we need to formalise the interface for delegated authorization and externalise the delegation process similar to "run-as" lookup, maybe something like:

realm.authenticate(token, ActionListener.wrap( user -> {
  maybeDelegateAuthorization(user, realm.getDelegatedRealms());
}));

It could also be an opportunity to support "authorization delegation" for native realms (we have an ER for it).

@ywangd
Copy link
Member Author

ywangd commented Nov 10, 2021

@tvernum and I discussed in a separate channel and we settled down on the following design (it can still be tweaked during the implementation phase)

UPDATED: 2021-11-24

public static class Subject {

    public enum Type {
        USER,
        API_KEY,
        SERVICE_ACCOUNT,
    }

    private final User user;
    private final Authentication.RealmRef realm;
    private final Type type;
    private final Map<String, Object> metadata;

    public List<RoleReference> getRoleReferences(AnonymousUser anonymousUser) { }
}

public class AuthenticationContext {

    private final Version version;
    private final Subject effectiveSubject;
    private final Subject authenticatingSubject;
    private final Authentication.AuthenticationType type;

    public boolean isRunAs() { }

    public Authentication toAuthentication() { }

    public static AuthenticationContext fromAuthentication(Authentication authentication) { }
}

public interface RoleReference {

    RoleKey cacheKey();
    void resolve(RoleReferenceResolver resolver, ActionListener<RolesRetrievalResult> listener);

    final class NamedRoleReference implements RoleReference {
        public final String[] roleNames;
    }

    final class ApiKeyRoleReference implements RoleReference {
        private final String apiKeyId;
        private final BytesReference roleDescriptorsBytes;
    }

    final class BwcApiKeyRoleReference implements RoleReference {
        private final String apiKeyId;
        private final Map<String, Object> roleDescriptorsMap;
    }

    final class ServiceAccountRoleReference implements RoleReference {
        private final String principal;
    }
}

public interface RoleReferenceResolver {

    void resolveNamedRoleReference(RoleReference.NamedRoleReference namedRoleReference, ActionListener<RolesRetrievalResult> listener);

    void resolveApiKeyRoleReference(RoleReference.ApiKeyRoleReference apiKeyRoleReference, ActionListener<RolesRetrievalResult> listener);

    void resolveBwcApiKeyRoleReference( RoleReference.BwcApiKeyRoleReference bwcApiKeyRoleReference, ActionListener<RolesRetrievalResult> listener);

    void resolveServiceAccountRoleReference(ServiceAccountRoleReference roleReference, ActionListener<RolesRetrievalResult> listener);
}

@ywangd
Copy link
Member Author

ywangd commented Nov 23, 2021

Updated the class design based on the latest development.

ywangd added a commit to ywangd/elasticsearch that referenced this issue Dec 2, 2021
The process of role resolving is to build the Role object from an
Authentication object. The high level steps of this process is as the follows:

1. Locate the role reference for the Authentication, e.g. for regular user,
this means a collection of role names.

2. Retrieve the role descriptors for the role reference, e.g. search the
security index to get the role descriptors for the role name.

3. Build the Role object based on the role descriptors.  There are also special
cases in the above process. For example, API keys do not have role names, but
two byteReference representing the role descriptors. API keys also have a
nested Role structure for limiting the key's actual privileges based on the
owner's.

Today, this process is managed by a single CompositeRolesStore class and the
steps are not clearly separated. This PR improves the situation by introducing
a new RoleReferenceResolver class that is responsible for turning roleReference
into role descriptors. CompositeRolesStore is now only responsible for buiding
the Role from the descriptors.

The RoleReference is also a new concept introduced by this PR, along with
AuthenticationContext and Subject. They technically belong to the issue of
revisiting Authentication class (elastic#80117). But they are needed here to faciliate
the changes. Their usage will be expanded once we start working on elastic#80117.

A Subject knows how to return a list of RoleReference and the final Role is the
intersection of all the RoleReference. This was a concept for API keys. It is
now generalised in this PR in the light on potential expanded usage of
limitedBy roles.

Relates: elastic#80117
ywangd added a commit that referenced this issue Dec 7, 2021
The process of role resolving is to build the Role object from an
Authentication object. The high level steps of this process is as the follows:

1. Locate the role reference for the Authentication, e.g. for regular user,
this means a collection of role names.

2. Retrieve the role descriptors for the role reference, e.g. search the
security index to get the role descriptors for the role name.

3. Build the Role object based on the role descriptors.  There are also special
cases in the above process. For example, API keys do not have role names, but
two byteReference representing the role descriptors. API keys also have a
nested Role structure for limiting the key's actual privileges based on the
owner's.

Today, this process is managed by a single CompositeRolesStore class and the
steps are not clearly separated. This PR improves the situation by introducing
a new RoleReferenceResolver class that is responsible for turning roleReference
into role descriptors. CompositeRolesStore is now only responsible for buiding
the Role from the descriptors.

The RoleReference is also a new concept introduced by this PR, along with
AuthenticationContext and Subject. They technically belong to the issue of
revisiting Authentication class (#80117). But they are needed here to faciliate
the changes. Their usage will be expanded once we start working on #80117.

A Subject knows how to return a list of RoleReference and the final Role is the
intersection of all the RoleReference. This was a concept for API keys. It is
now generalised in this PR in the light on potential expanded usage of
limitedBy roles.

Relates: #80117
ywangd added a commit to ywangd/elasticsearch that referenced this issue Dec 7, 2021
This PR extract an interface from the Role class. This helped to rework
the LimitedRole class so it no longer has the constraint of one level of
limiting.

Resolves: elastic#81192
Relates: elastic#80117
ywangd added a commit that referenced this issue Jan 19, 2022
This PR extract an interface from the Role class. This helped to rework
the LimitedRole class so it no longer has the constraint of one level of
limiting.

Resolves: #81192
Relates: #80117
ywangd added a commit that referenced this issue May 4, 2022
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 refactoring 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
elasticsearchmachine pushed a commit that referenced this issue Jul 14, 2022
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
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants