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

Support 3rd party initiated login for OpenID Connect #38474

Merged
merged 32 commits into from
Feb 25, 2019

Conversation

jkakavas
Copy link
Member

@jkakavas jkakavas commented Feb 5, 2019

- Use nimbus oidc sdk
- JWE not handled
- UserInfo requests not handled
- Make the calls to TokenEndpoint and UserInfoEndpoint asynchrously
- Move IdTokenValidator to an instance variable
Access Token will not be returned in the implicit flow if the
response type is set to "id_token" (as opposed to "id_token token")
In such cases, there is no access token to validate and we cannot
make requests to the UserInfo endpoint, even if the user has
configured the Userinfo endpoint in the configuration
@jkakavas jkakavas added >feature :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels Feb 5, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@albertzaharovits
Copy link
Contributor

This does not cover the spec you've pointed to:

login_hint
OPTIONAL. Hint to the Authorization Server about the login identifier the End-User might use to log in. If the client receives a value for this string-valued parameter, it MUST include it in the Authentication Request as the login_hint parameter value.

The transport request for the prepare authentication request does not carry the login_hint parameter.

target_link_uri
OPTIONAL. URL that the RP is requested to redirect to after authentication. RPs MUST verify the value of the target_link_uri to prevent being used as an open redirector to external sites.

Do we have to keep this in the flow context and return it in the OpenIdConnectAuthenticateResponse ? 😒

@jkakavas
Copy link
Member Author

jkakavas commented Feb 11, 2019

The transport request for the prepare authentication request does not carry the login_hint parameter.

Yeap, thanks for keeping me honest here. I worked mostly from experience here and missed adding the login hint. It's rarely used AFAIK but it's easy to support so I'll add it.

Do we have to keep this in the flow context and return it in the OpenIdConnectAuthenticateResponse ? unamused

I don't think we should support target_link_uri. There aren't many use cases where the 3rd party would necessarily know a deep link within Kibana so that it would point the UA there from the beginning. Even if we do support this in the future, this should be a URI pointing to the facilitator only and as such, it can very well be stored in the user's session with the facilitator

@albertzaharovits
Copy link
Contributor

I don't think we should support target_link_uri

I totally agree. This is the kind of flexibility we should refrain from following.

@jkakavas jkakavas force-pushed the 3rd-party-initiated branch from 8cee04c to 4e0788e Compare February 11, 2019 18:11
Copy link
Contributor

@albertzaharovits albertzaharovits left a comment

Choose a reason for hiding this comment

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

LGTM after updating the rest-transport PARSER

@@ -36,6 +36,7 @@

static {
PARSER.declareString(OpenIdConnectPrepareAuthenticationRequest::setRealmName, new ParseField("realm"));
PARSER.declareString(OpenIdConnectPrepareAuthenticationRequest::setIssuer, new ParseField("iss"));
Copy link
Contributor

@albertzaharovits albertzaharovits Feb 13, 2019

Choose a reason for hiding this comment

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

add:

PARSER.declareString(OpenIdConnectPrepareAuthenticationRequest::setLoginHint, new ParseField("login_hint"));

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦‍♂️

Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

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

left some minor comments, otherwise LGTM

/**
* In case of a
* <a href="https://openid.net/specs/openid-connect-core-1_0.html#ThirdPartyInitiatedLogin">3rd party initiated authentication</a>, the
* issuer to the UA needs to be redirected for authentication
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* issuer to the UA needs to be redirected for authentication
* issuer that the User Agent needs to be redirected to for authentication.

return loginHint;
}


Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

if (null == realm || realm instanceof OpenIdConnectRealm == false) {
Realm realm = null;
if (Strings.hasText(request.getIssuer())) {
List<OpenIdConnectRealm> matchingRealms = this.realms.stream().filter(r -> r instanceof OpenIdConnectRealm)
Copy link
Member

Choose a reason for hiding this comment

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

can we use a single filter?

.filter(r -> r instanceof OpenIdConnectRealm && ((OpenIdConnectRealm) r).isIssuerValid(request.getIssuer()))

@jkakavas jkakavas merged commit 53c3061 into elastic:feature-oidc-realm Feb 25, 2019
jkakavas added a commit that referenced this pull request Apr 4, 2019
This commit adds an OpenID Connect authentication realm to
elasticsearch. Elasticsearch (with the assistance of kibana or
another web component) acts as an OpenID Connect Relying
Party and supports the Authorization Code Grant and Implicit
flows as described in http://ela.st/oidc-spec. It adds support
for consuming and verifying signed ID Tokens, both RP
initiated and 3rd party initiated Single Sign on and RP
initiated signle logout.
It also adds an OpenID Connect Provider in the idp-fixture to
be used for the associated integration tests.

The code in this commit has been tracked in a feature branch
and has been previously reviewed and approved in :

#37009
#37787
#38474
#38475
#40262
jkakavas added a commit to jkakavas/elasticsearch that referenced this pull request Apr 14, 2019
This commit adds an OpenID Connect authentication realm to
elasticsearch. Elasticsearch (with the assistance of kibana or
another web component) acts as an OpenID Connect Relying
Party and supports the Authorization Code Grant and Implicit
flows as described in http://ela.st/oidc-spec. It adds support
for consuming and verifying signed ID Tokens, both RP
initiated and 3rd party initiated Single Sign on and RP
initiated signle logout.
It also adds an OpenID Connect Provider in the idp-fixture to
be used for the associated integration tests.

The code in this commit has been tracked in a feature branch
and has been previously reviewed and approved in :

elastic#37009
elastic#37787
elastic#38474
elastic#38475
elastic#40262
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
This commit adds an OpenID Connect authentication realm to
elasticsearch. Elasticsearch (with the assistance of kibana or
another web component) acts as an OpenID Connect Relying
Party and supports the Authorization Code Grant and Implicit
flows as described in http://ela.st/oidc-spec. It adds support
for consuming and verifying signed ID Tokens, both RP
initiated and 3rd party initiated Single Sign on and RP
initiated signle logout.
It also adds an OpenID Connect Provider in the idp-fixture to
be used for the associated integration tests.

The code in this commit has been tracked in a feature branch
and has been previously reviewed and approved in :

elastic#37009
elastic#37787
elastic#38474
elastic#38475
elastic#40262
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>feature :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants