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.

Sign up for GitHub

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

Jump to bottom

Disallow Password Change when authenticated by Token #49694

Merged
merged 8 commits into from
Jan 13, 2020
Merged

Disallow Password Change when authenticated by Token #49694

merged 8 commits into from
Jan 13, 2020

Conversation

jkakavas
Copy link
Member

@jkakavas jkakavas commented Nov 28, 2019 •
edited by polyfractal
Loading

This commit explicitly disallows Change Password requests
when the user performing the request is authenticated with
an Elasticsearch TokenService access token or an API key.

We should coordinate merging this with kibana since the password change feature would stop working when the token authentication provider is in use in Kibana.

Relates: elastic/kibana#49865
Closes: #48752

@jkakavas
Mark token authN in the authentication object
c781d87 
This commit changes that behavior so that we set the authenticatedBy
realm reference to _es_token_service in the Authentication object
we build and write to the thread context upon successful
authentication with an Elasticsearch Token Service access token.
This alligns our behavior with the API keys implementation and
allows us to make authorization decisions based on the fact that
the current request was authenticated by a bearer token ( i.e.
disallow change password requests ).

The original authentication realm is still available in the token
document.
@jkakavas jkakavas added >enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 v7.6.0 labels Nov 28, 2019
@elasticmachine
Copy link
Collaborator

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

@jkakavas jkakavas mentioned this pull request Nov 28, 2019
Closed
@tvernum
Copy link
Contributor

tvernum commented Nov 29, 2019

Have we considered any other BWC issues with changing this?
I don't specifically know of any, but it's in that grey area where I can imagine there might be some.

We could resolve the password change behaviour by looking at the AuthenticationType as well as the realm (which would avoid needing this change).

@jkakavas
Copy link
Member Author

We could resolve the password change behaviour by looking at the AuthenticationType as well as the realm (which would avoid needing this change).

True, this is not the only way to solve the password change behavior but I like it since it's consistent with our behavior for API keys. I looked for other uses of authenticatedBy and didn't find any potential issues. I can go through them and list them in a way that it makes it easier for us to argue about potential BWC issues (or the lack thereof).

@jkakavas
Copy link
Member Author

jkakavas commented Dec 2, 2019

Let's discuss this on Thursday morning, I want to get a team view on how we treat/should treat the authenticatedBy attribute of the Authentication object when tokens are used by our own realms ( DelegeatedPKI, OIDC, SAML)

@jkakavas
Fix test
5eef8b3
@jkakavas
Disallow based on AuthenticationType
d2f4b97 
Changes the approach for disallowing password changes to check the
AuthenticationType instead of injecting a custom RealmRef for tokens
as the authenticatedBy Realm Reference
@jkakavas jkakavas changed the title Mark token authN in the authentication object Disallow Password Change when authenticated by Token Dec 12, 2019
@jkakavas
Copy link
Member Author

@elasticmachine update branch

tvernum
tvernum reviewed Dec 16, 2019
View reviewed changes
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java Outdated
// and right now only one can exist in the realm configuration - if this changes we should update this check
final Authentication.AuthenticationType authType = authentication.getAuthenticationType();
return (authType.equals(Authentication.AuthenticationType.TOKEN) == false
&& authType.equals(Authentication.AuthenticationType.API_KEY) == false )
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we blacklist rather than whitelist here?
Shouldn't we just check for the positive authType.equals(Authentication.AuthenticationType.REALM) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're absolutely right

@jkakavas jkakavas requested a review from tvernum January 8, 2020 07:47
tvernum
tvernum approved these changes Jan 13, 2020
View reviewed changes
Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

LGTM.
I haven't tracked the status of the Kibana side of the change, so I'll leave it to you to work out whether it should be in 7.6 or wait for 7.7

@jkakavas jkakavas removed the v7.6.0 label Jan 13, 2020
@jkakavas jkakavas merged commit 6543e18 into elastic:master Jan 13, 2020
SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this pull request Jan 23, 2020
@jkakavas @SivagurunathanV
Disallow Password Change when authenticated by Token (elastic#49694)
8327fc8 
Password changes are only allowed when the user is currently
authenticated by a realm (that permits the password to be changed)
and not when authenticated by a bearer token or an API key.
@polyfractal
Copy link
Contributor

@jkakavas Is this still backport_pending for 7.6, or bumped to 7.7?

@jkakavas jkakavas mentioned this pull request Mar 16, 2020
Merged
jkakavas added a commit to jkakavas/elasticsearch that referenced this pull request Mar 16, 2020
@jkakavas
Disallow Password Change when authenticated by Token (elastic#49694)
e4bc958 
Password changes are only allowed when the user is currently
authenticated by a realm (that permits the password to be changed)
and not when authenticated by a bearer token or an API key.
jkakavas added a commit that referenced this pull request Mar 17, 2020
@jkakavas
Disallow Password Change when authenticated by Token (#49694) (#53614)
23af171 
Password changes are only allowed when the user is currently
authenticated by a realm (that permits the password to be changed)
and not when authenticated by a bearer token or an API key.
@codebrain codebrain mentioned this pull request Apr 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tvernum tvernum tvernum approved these changes

Assignees
No one assigned
Labels
>enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v7.7.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change password API authenticating with a bearer token
6 participants
@jkakavas @elasticmachine @tvernum @polyfractal @jakelandis @bpintea