Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions x-pack/docs/en/rest-api/security/oidc-authenticate-api.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -31,24 +31,28 @@ and <<security-api-oidc-logout,OpenID Connect logout API>>
==== {api-request-body-title}

`redirect_uri`::
The URL to which the OpenID Connect Provider redirected the User Agent in
(Required, string) The URL to which the OpenID Connect Provider redirected the User Agent in
response to an authentication request, after a successful authentication. This
URL is expected to be provided as-is (URL encoded), taken from the body of the
response or as the value of a `Location` header in the response from the OpenID
Connect Provider.

`state`::
String value used to maintain state between the authentication request and the
(Required, string) Used to maintain state between the authentication request and the
response. This value needs to be the same as the one that was provided to the
call to `/_security/oidc/prepare` earlier, or the one that was generated by {es}
and included in the response to that call.

`nonce`::
String value used to associate a Client session with an ID Token and to mitigate
(Required, string) Used to associate a Client session with an ID Token and to mitigate
replay attacks. This value needs to be the same as the one that was provided to
the call to `/_security/oidc/prepare` earlier, or the one that was generated by
{es} and included in the response to that call.

`realm`::
(Optional, string) Used to identify the name of the OpenID Connect realm that should
be used to authenticate this. Useful when multiple realms have been defined.

[[security-api-oidc-authenticate-example]]
==== {api-examples-title}

Expand All @@ -63,7 +67,8 @@ POST /_security/oidc/authenticate
{
"redirect_uri" : "https://oidc-kibana.elastic.co:5603/api/security/v1/oidc?code=jtI3Ntt8v3_XvcLzCFGq&state=4dbrihtIAt3wBTwo6DxK-vdk-sSyDBV8Yf0AjdkdT5I",
"state" : "4dbrihtIAt3wBTwo6DxK-vdk-sSyDBV8Yf0AjdkdT5I",
"nonce" : "WaBPH0KqPVdG5HHdSxPRjfoZbXMCicm5v1OiAj0DUFM"
"nonce" : "WaBPH0KqPVdG5HHdSxPRjfoZbXMCicm5v1OiAj0DUFM",
"realm" : "oidc1"
}
--------------------------------------------------
// CONSOLE
Expand Down
4 changes: 2 additions & 2 deletions x-pack/docs/en/rest-api/security/oidc-logout-api.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,10 @@ and
==== {api-request-body-title}

`access_token`::
The value of the access token to be invalidated as part of the logout.
(Required, string) The value of the access token to be invalidated as part of the logout.

`refresh_token`::
(Optional) The value of the refresh token to be invalidated as part of the logout.
(Optional, string) The value of the refresh token to be invalidated as part of the logout.


[[security-api-oidc-logout-example]]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,28 +33,28 @@ and <<security-api-oidc-logout,OpenID Connect logout API>>.
The following parameters can be specified in the body of the request:

`realm`::
The name of the OpenID Connect realm in {es} the configuration of which should
(Optional, string) The name of the OpenID Connect realm in {es} the configuration of which should
be used in order to generate the authentication request. Cannot be specified
when `iss` is specified.
when `iss` is specified. One of `realm`, `iss` needs to be specified.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
when `iss` is specified. One of `realm`, `iss` needs to be specified.
when `iss` is specified. One of `realm`, `iss` is required.


`state`::
String value used to maintain state between the authentication request and the
(Optional, string) Value used to maintain state between the authentication request and the
response, typically used as a Cross-Site Request Forgery mitigation. If the
caller of the API doesn't provide a value, {es} will generate one with
sufficient entropy itself and return it in the response.

`nonce`::
String value used to associate a Client session with an ID Token and to mitigate
(Optional, string) Value used to associate a Client session with an ID Token and to mitigate
replay attacks. If the caller of the API doesn't provide a value, {es} will
generate one with sufficient entropy itself and return it in the response.

`issuer`::
In the case of a 3rd Party initiated Single Sign On, this is the Issuer
`iss`::
(Optional, string) In the case of a 3rd Party initiated Single Sign On, this is the Issuer
Identifier for the OP that the RP is to send the Authentication Request to.
Cannot be specified when `realm` is specified.
Cannot be specified when `realm` is specified. One of `realm`, `iss` needs to be specified.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Cannot be specified when `realm` is specified. One of `realm`, `iss` needs to be specified.
Cannot be specified when `realm` is specified. One of `realm`, `iss` is required.


`login_hint`::
In the case of a 3rd Party initiated Single Sign On, a string value to be
(Optional, string) In the case of a 3rd Party initiated Single Sign On, a string value to be
included in the authentication request, as the `login_hint` parameter. This
parameter is not valid when `realm` is specified

Expand Down
7 changes: 5 additions & 2 deletions x-pack/docs/en/security/authentication/oidc-guide.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,9 @@ POST /_security/oidc/prepare
this HTTP GET request, the custom web app will need to make an HTTP POST request to
`_security/oidc/authenticate`, again - authenticating as the `facilitator` user - passing the URL
where the user's browser was redirected to, as a parameter, along with the
values for `nonce` and `state` it had saved in the user's session previously.
values for `nonce` and `state` it had saved in the user's session previously. If more than one
OpenID Connect realms are configured, the custom web app can specify the name of the realm to be
used for handling this, but this parameter is optional.
See {ref}/security-api-oidc-authenticate.html[OIDC Authenticate API] for more details
+
[source,js]
Expand All @@ -658,7 +660,8 @@ POST /_security/oidc/authenticate
{
"redirect_uri" : "https://oidc-kibana.elastic.co:5603/api/security/v1/oidc?code=jtI3Ntt8v3_XvcLzCFGq&state=4dbrihtIAt3wBTwo6DxK-vdk-sSyDBV8Yf0AjdkdT5I",
"state" : "4dbrihtIAt3wBTwo6DxK-vdk-sSyDBV8Yf0AjdkdT5I",
"nonce" : "WaBPH0KqPVdG5HHdSxPRjfoZbXMCicm5v1OiAj0DUFM"
"nonce" : "WaBPH0KqPVdG5HHdSxPRjfoZbXMCicm5v1OiAj0DUFM",
"realm" : "oidc1"
}
-----------------------------------------------------------------------
// CONSOLE
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
package org.elasticsearch.xpack.core.security.action.oidc;

import org.elasticsearch.Version;
import org.elasticsearch.action.ActionRequest;
import org.elasticsearch.action.ActionRequestValidationException;
import org.elasticsearch.common.Strings;
Expand Down Expand Up @@ -38,6 +39,11 @@ public class OpenIdConnectAuthenticateRequest extends ActionRequest {
*/
private String nonce;

/**
* The name of the OIDC Realm that should consume the authentication request
*/
private String realm;

public OpenIdConnectAuthenticateRequest() {

}
Expand All @@ -47,6 +53,10 @@ public OpenIdConnectAuthenticateRequest(StreamInput in) throws IOException {
redirectUri = in.readString();
state = in.readString();
nonce = in.readString();
if (in.getVersion().onOrAfter(Version.V_7_4_0)){
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (in.getVersion().onOrAfter(Version.V_7_4_0)){
if (in.getVersion().onOrAfter(Version.V_7_4_0)) {

realm = in.readOptionalString();
}

}

public String getRedirectUri() {
Expand All @@ -73,6 +83,14 @@ public void setNonce(String nonce) {
this.nonce = nonce;
}

public String getRealm() {
return realm;
}

public void setRealm(String realm) {
this.realm = realm;
}

@Override
public ActionRequestValidationException validate() {
ActionRequestValidationException validationException = null;
Expand All @@ -94,10 +112,13 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeString(redirectUri);
out.writeString(state);
out.writeString(nonce);
if (out.getVersion().onOrAfter(Version.V_7_4_0)){
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (out.getVersion().onOrAfter(Version.V_7_4_0)){
if (out.getVersion().onOrAfter(Version.V_7_4_0)) {

out.writeOptionalString(realm);
}
}

public String toString() {
return "{redirectUri=" + redirectUri + ", state=" + state + ", nonce=" + nonce + "}";
return "{redirectUri=" + redirectUri + ", state=" + state + ", nonce=" + nonce + ", realm=" +realm+"}";
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import org.elasticsearch.action.ActionRequest;
import org.elasticsearch.action.ActionRequestValidationException;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.io.stream.StreamInput;

import java.io.IOException;
Expand All @@ -19,6 +20,8 @@ public final class SamlAuthenticateRequest extends ActionRequest {

private byte[] saml;
private List<String> validRequestIds;
@Nullable
private String realm;

public SamlAuthenticateRequest(StreamInput in) throws IOException {
super(in);
Expand Down Expand Up @@ -47,4 +50,12 @@ public List<String> getValidRequestIds() {
public void setValidRequestIds(List<String> validRequestIds) {
this.validRequestIds = validRequestIds;
}

public String getRealm() {
return realm;
}

public void setRealm(String realm) {
this.realm = realm;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Erk! This class really ought to have serialization logic in it. Can we add that as a followup PR?
It's probably not the only SAML request/response that's missing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

}
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,9 @@ public SamlAuthenticateRequestBuilder validRequestIds(List<String> validRequestI
request.setValidRequestIds(validRequestIds);
return this;
}

public SamlAuthenticateRequestBuilder preferredRealm(String realm) {
request.setRealm(realm);
return this;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import org.elasticsearch.xpack.core.security.action.oidc.OpenIdConnectAuthenticateAction;
import org.elasticsearch.xpack.core.security.authc.Authentication;
import org.elasticsearch.xpack.core.security.authc.AuthenticationResult;
import org.elasticsearch.xpack.core.security.authc.RealmConfig;
import org.elasticsearch.xpack.core.security.authc.oidc.OpenIdConnectRealmSettings;
import org.elasticsearch.xpack.security.authc.AuthenticationService;
import org.elasticsearch.xpack.security.authc.TokenService;
import org.elasticsearch.xpack.security.authc.oidc.OpenIdConnectRealm;
Expand Down Expand Up @@ -59,7 +61,9 @@ protected void doExecute(Task task, OpenIdConnectAuthenticateRequest request,
final ThreadContext threadContext = threadPool.getThreadContext();
Authentication originatingAuthentication = Authentication.getAuthentication(threadContext);
try (ThreadContext.StoredContext ignore = threadContext.stashContext()) {
authenticationService.authenticate(OpenIdConnectAuthenticateAction.NAME, request, token, ActionListener.wrap(
RealmConfig.RealmIdentifier preferredRealm = request.getRealm() == null ? null :
new RealmConfig.RealmIdentifier(OpenIdConnectRealmSettings.TYPE, request.getRealm());
authenticationService.authenticate(OpenIdConnectAuthenticateAction.NAME, request, token, preferredRealm, ActionListener.wrap(
authentication -> {
AuthenticationResult result = threadContext.getTransient(AuthenticationResult.THREAD_CONTEXT_KEY);
if (result == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
import org.elasticsearch.xpack.core.security.action.saml.SamlAuthenticateResponse;
import org.elasticsearch.xpack.core.security.authc.Authentication;
import org.elasticsearch.xpack.core.security.authc.AuthenticationResult;
import org.elasticsearch.xpack.core.security.authc.RealmConfig;
import org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings;
import org.elasticsearch.xpack.security.authc.AuthenticationService;
import org.elasticsearch.xpack.security.authc.TokenService;
import org.elasticsearch.xpack.security.authc.saml.SamlRealm;
Expand Down Expand Up @@ -53,7 +55,10 @@ protected void doExecute(Task task, SamlAuthenticateRequest request, ActionListe
final ThreadContext threadContext = threadPool.getThreadContext();
Authentication originatingAuthentication = Authentication.getAuthentication(threadContext);
try (ThreadContext.StoredContext ignore = threadContext.stashContext()) {
authenticationService.authenticate(SamlAuthenticateAction.NAME, request, saml, ActionListener.wrap(authentication -> {
RealmConfig.RealmIdentifier preferredRealm = request.getRealm() == null ? null :
new RealmConfig.RealmIdentifier(SamlRealmSettings.TYPE, request.getRealm());
authenticationService.authenticate(SamlAuthenticateAction.NAME, request, saml, preferredRealm,
ActionListener.wrap(authentication -> {
AuthenticationResult result = threadContext.getTransient(AuthenticationResult.THREAD_CONTEXT_KEY);
if (result == null) {
listener.onFailure(new IllegalStateException("Cannot find AuthenticationResult on thread context"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.elasticsearch.xpack.core.security.authc.AuthenticationServiceField;
import org.elasticsearch.xpack.core.security.authc.AuthenticationToken;
import org.elasticsearch.xpack.core.security.authc.Realm;
import org.elasticsearch.xpack.core.security.authc.RealmConfig;
import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.EmptyAuthorizationInfo;
import org.elasticsearch.xpack.core.security.support.Exceptions;
import org.elasticsearch.xpack.core.security.user.AnonymousUser;
Expand All @@ -56,6 +57,7 @@
import java.util.concurrent.atomic.AtomicLong;
import java.util.function.BiConsumer;
import java.util.function.Consumer;
import java.util.stream.Collectors;

import static org.elasticsearch.xpack.security.support.SecurityIndexManager.isIndexDeleted;
import static org.elasticsearch.xpack.security.support.SecurityIndexManager.isMoveFromRedToNonRed;
Expand Down Expand Up @@ -140,16 +142,33 @@ public void authenticate(String action, TransportMessage message, User fallbackU
}

/**
* Authenticates the username and password that are provided as parameters. This will not look
* at the values in the ThreadContext for Authentication.
* Authenticates the user based on the contents of the token that is provided as parameter.If {@code preferredRealm} can't
* authenticate the token, no other realms will be attempted.
* This will not look at the values in the ThreadContext for Authentication.
*
* @param action The action of the message
* @param message The message that resulted in this authenticate call
* @param token The token (credentials) to be authenticated
*/
public void authenticate(String action, TransportMessage message,
AuthenticationToken token, ActionListener<Authentication> listener) {
new Authenticator(action, message, null, listener).authenticateToken(token);
new Authenticator(action, message, null, listener).authenticateToken(token, null);
}

/**
* Authenticates the user based on the contents of the token that is provided as parameter only at the realm that is indicated with
* {@code preferredRealm}. If {@code preferredRealm} can't authenticate the token, no other realms will be attempted.
* This will not look at the values in the ThreadContext for Authentication.
*
* @param action The action of the message
* @param message The message that resulted in this authenticate call
* @param token The token (credentials) to be authenticated
* @param preferredRealm The realm that should be used to authenticate the token
*/
public void authenticate(String action, TransportMessage message,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opted to overloading public methods and just adding Nullable parameters to private ones.

AuthenticationToken token, @Nullable RealmConfig.RealmIdentifier preferredRealm,
ActionListener<Authentication> listener) {
new Authenticator(action, message, null, listener).authenticateToken(token, preferredRealm);
}

public void expire(String principal) {
Expand Down Expand Up @@ -345,18 +364,25 @@ void extractToken(Consumer<AuthenticationToken> consumer) {
action.run();
}

private void consumeToken(AuthenticationToken token){
consumeToken(token, null);
}

/**
* Consumes the {@link AuthenticationToken} provided by the caller. In the case of a {@code null} token, {@link #handleNullToken()}
* is called. In the case of a {@code non-null} token, the realms are iterated over and the first realm that returns a non-null
* {@link User} is the authenticating realm and iteration is stopped. This user is then passed to {@link #consumeUser(User, Map)}
* if no exception was caught while trying to authenticate the token
* is called. In the case of a {@code non-null} token, the realms are iterated over in the order defined in the configuration
* while possible also taking into consideration the last realm that authenticated this principal. If {@code preferredRealm} is
* not null, only this realm will be attempted, regardless of its order/applicability. When consulting multiple realms, the
* first realm that returns a non-null {@link User} is the authenticating realm and iteration is stopped. This user is then
* passed to {@link #consumeUser(User, Map)} if no exception was caught while trying to authenticate the token
*/
private void consumeToken(AuthenticationToken token) {
private void consumeToken(AuthenticationToken token, @Nullable RealmConfig.RealmIdentifier preferredRealm) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the AuthenticationService is too generic to handle the particular logic of the OIDC and SAML realms.

This method's core logic is to iterate the realms list and try to authenticate the token. I don't like it that we upset this logic in this case. I suggest we either create a new method in the AuthenticationService which only deals with a single realm, or more preferable for me, would be to incorporate the realm "preference" (which is more than a preference) inside the AuthenticationToken.

For example, in OpenIdConnectRealm:

    public boolean supports(AuthenticationToken token) {
        If (false == (token instanceof OpenIdConnectToken)) {
            return false;
        }
        (OpenIdConnectToken) oidcToken = (OpenIdConnectToken) token;
        if (null == oidcToken.getRealmName()) {
            return true;
        } else {
            return oidcToken.getRealmName().equals(this.name());
        }
    }

This alternative moves the realm "preference" logic to the individual realm, away from the more generic AuthenticationService. This allows, for example, to have OIDC authenticate API calls specify an iss or a realm the same way that OIDC prepareAuthenticate can specify an iss or a realm. You can see how the iss request parameter, which is not relevant for the SAML realm, is handled internally by the OIDC realm.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback @albertzaharovits , appreciated as always!

I don't like it that we upset this logic in this case.

Could you add some context and/or arguments describing what you don't like please ?

I suggest we either create a new method in the AuthenticationService which only deals with a single realm

I think that adding a different consumeToken method which will share much of the logic of the existing one doesn't buy us anything. I believe this fits nicely here, the same way the reordering of the realm list based on the cache fits.

or more preferable for me, would be to incorporate the realm "preference" (which is more than a preference) inside the AuthenticationToken.

I considered dealing with this in the AuthenticationToken but since we want to remove the AuthenticationToken concept entirely, it didn't seem like a good idea to stuff more things in there. Also, why replicate the exact same logic in both realms ?

This allows, for example, to have OIDC authenticate API calls specify an iss or a realm the same way that OIDC prepareAuthenticate can specify an iss or a realm. You can see how the iss request parameter, which is not relevant for the SAML realm, is handled internally by the OIDC realm.

There is no use passing iss or any other parameter. The use case this change tries to solve is similar for all our web SSO realms ( OIDC and SAML for the time being ) and it is that we know which realm needs to consume the AuthenticationToken so we don't need to iterate through the rest.

"preference" (which is more than a preference)

In retrospect, preferredRealm wasn't a good choice for the name, but everything else above still stands.

I'm open to being swayed with more arguments if you feel strongly about this

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add some context and/or arguments describing what you don't like please ?

The AuthenticationServicein the consumeToken method iterates the realms list, checks if each supports the token and tries to authenticate (SUCCESS, CONTINUE, TERMINATE). Crucially, the realms list is private; the caller (to AuthenticationService#authenticate) has no option to reorder or truncate it. For this reason the caller isn't even concerned about what a realm is. The proposed change breaks this encapsulation, because the caller can now "specify a preference about a realm".

On a more pragmatic, lower level, the proposed changes make the code more complex. It probably works, but it is more complex. The method builds the realms list as well as iterates over it (in a very particular way). This can be split into two. For example, one method receives the realms list as a parameter, and there is a different method that builds the realms list and calls the other method.

But, aside from breaking the encapsulation, is it really safe for the caller to alter the realms list? For example, a successful OIDC token authentication changes the realm order for all successive OIDC tokens. Why should a successful authentication when the caller specifies a realm preference affect the order for OIDC token authentications when the caller does NOT specify an order? This is a rhetorical question. I want to point out that whatever assumptions we had mentally when inspecting the code when the realms list was hidden, do not hold anymore when the caller has control on the list. For that matter, only slightly exaggerating for emphasis, I would prefer to just pull the specific OIDC realm by name and call authenticate on it directly, bypassing the AuthenticationService altogether.

I considered dealing with this in the AuthenticationToken but since we want to remove the AuthenticationToken concept entirely, it didn't seem like a good idea to stuff more things in there.

I don't think this is a good idea. The code that is added should fit with what exists right now. How can we know that in the future, when the AuthenticationToken is removed, the proposed code change in the AuthenticationService is still valid? Since the AuthenticationService generates and consumes AuthenticationTokens, it is very probable that it will not be.

Also, why replicate the exact same logic in both realms ?

I think the replicated code (token.getRealm().equals(this.name())) is small enough so that this is not a problem.

There is no use passing iss or any other parameter. The use case this change tries to solve is similar for all our web SSO realms ( OIDC and SAML for the time being ) and it is that we know which realm needs to consume the AuthenticationToken so we don't need to iterate through the rest.

I think I understand this. Though this is not strictly related to the matter being discussed, I think it has an impact on the implementation so I'll ask: The client can call OIDC prepare and OIDC authenticated using the realm request parameter (for both calls). Is it useful for it to be able to call the same APIs using the iss request parameter (for both calls)? If not, why would the client use iss over the realm parameter for the OIDC prepare call?

Copy link
Contributor Author

@jkakavas jkakavas Aug 21, 2019

Choose a reason for hiding this comment

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

Crucially, the realms list is private; the caller (to AuthenticationService#authenticate) has no option to reorder or truncate it. For this reason the caller isn't even concerned about what a realm is. The proposed change breaks this encapsulation, because the caller can now "specify a preference about a realm".

Previously, the caller wasn't actually concerned about the realm. Now the caller is a caller that is aware of what a realm is and which one should be used. I see what you're saying but I think that breaking this encapsulation by only filtering isn't problematic

On a more pragmatic, lower level, the proposed changes make the code more complex. It probably works, but it is more complex. The method builds the realms list as well as iterates over it (in a very particular way).

Didn't it do that before the proposed changes also?

This can be split into two. For example, one method receives the realms list as a parameter, and there is a different method that builds the realms list and calls the other method.

Not really opposed to breaking this up but filtering first, gains us to not have to hit the cache and reorder the list if we don't have to.

Why should a successful authentication when the caller specifies a realm preference affect the order for OIDC token authentications when the caller does NOT specify an order?

Not sure where you're getting this from. Why would this happen ? Wouldn't subsequent OIDC token authentications still call getRealmList() with a Null preferredRealm and get the behavior they get today? What am I missing here?

I don't think this is a good idea. The code that is added should fit with what exists right now. How can we know that in the future

I would agree with you if the AuthenticationToken solution was the obvious best solution, but I don't believe it is. This gets a little too theoretical for the sake of argumentation, but I have always seen us being forward looking when it comes to changes we know we want to do and that this was a valid argument

I think the replicated code (token.getRealm().equals(this.name())) is small enough so that this is not a problem.

Fair enough

I think I understand this. Though this is not strictly related to the matter being discussed, I think it has an impact on the implementation so I'll ask: The client can call OIDC prepare and OIDC authenticated using the realm request parameter (for both calls). Is it useful for it to be able to call the same APIs using the iss request parameter (for both calls)? If not, why would the client use iss over the realm parameter for the OIDC prepare call?

The iss is a parameter in the OIDC prepare endpoint as it is used to handle 3rd party initiated login. It is used there since the initiator of the login knows what the OPs Issuer is but doesn't know what we call the Elasticsearch realm (or what an Elasticsearch realm is ) and the caller of the API ( kibana or a custom web app) can't make the connection between iss and realm as it doesn't have the OP configuration ( only Elasticsearch has it )
iss can't be used in the authenticate endpoint as it has no meaning there, the OP needs to be sending the code that we will exchange for an ID Token ( authorization code grant ) or the ID Token itself ( implicit flow ). We can't do anything if it sends the issuer name again to us. edit: It theoretically could be sent to us by the caller of the API in order to implicitly denote the realm to be used ( 1-1 mapping between OPs and realms ) , but there is no reason for that. Kibana or a custom web app would know which realm to use and won't have iss available anyway.

see:

I don't feel like we should put much more time/energy in dueling over this one. I'll go ahead with a solution that doesn't break this encapsulation and ping for a review

if (token == null) {
handleNullToken();
} else {
authenticationToken = token;
final List<Realm> realmsList = getRealmList(authenticationToken.principal());
final List<Realm> realmsList = getRealmList(authenticationToken.principal(), preferredRealm);
logger.debug("Realms [{}] will be used for authentication in the specified order.", realmsList);
final long startInvalidation = numInvalidation.get();
final Map<Realm, Tuple<String, Exception>> messages = new LinkedHashMap<>();
final BiConsumer<Realm, ActionListener<User>> realmAuthenticatingConsumer = (realm, userListener) -> {
Expand Down Expand Up @@ -411,8 +437,20 @@ private void consumeToken(AuthenticationToken token) {
}
}

private List<Realm> getRealmList(String principal) {
/**
* Possible filters the realm list that is defined in the node's configuration based on {@code preferredRealm} and also
* possibly reorders the realm list depending on whether this principal has been recently authenticated by a specific realm
* @param principal The principal of the {@link AuthenticationToken} to be authenticated by a realm
* @param preferredRealm The realm that should authenticate the current {@link AuthenticationToken}
* @return a list of realms ordered based on which realm should authenticate the current {@link AuthenticationToken}
*/
private List<Realm> getRealmList(String principal, @Nullable RealmConfig.RealmIdentifier preferredRealm) {
final List<Realm> orderedRealmList = this.defaultOrderedRealmList;
if (preferredRealm != null){
return orderedRealmList.stream()
.filter(realm -> realm.name().equals(preferredRealm.getName()) && realm.type().equals(preferredRealm.getType()))
.collect(Collectors.toUnmodifiableList());
}
if (lastSuccessfulAuthCache != null) {
final Realm lastSuccess = lastSuccessfulAuthCache.get(principal);
if (lastSuccess != null) {
Expand Down Expand Up @@ -519,7 +557,7 @@ private void consumeUser(User user, Map<Realm, Tuple<String, Exception>> message
* names of users that exist using a timing attack
*/
private void lookupRunAsUser(final User user, String runAsUsername, Consumer<User> userConsumer) {
final RealmUserLookup lookup = new RealmUserLookup(getRealmList(runAsUsername), threadContext);
final RealmUserLookup lookup = new RealmUserLookup(getRealmList(runAsUsername, null), threadContext);
final long startInvalidationNum = numInvalidation.get();
lookup.lookup(runAsUsername, ActionListener.wrap(tuple -> {
if (tuple == null) {
Expand Down Expand Up @@ -572,8 +610,8 @@ void writeAuthToContext(Authentication authentication) {
action.run();
}

private void authenticateToken(AuthenticationToken token) {
this.consumeToken(token);
private void authenticateToken(AuthenticationToken token, @Nullable RealmConfig.RealmIdentifier preferredRealm) {
this.consumeToken(token, preferredRealm);
}
}

Expand Down
Loading