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

Fix security origin for TokenService#findActiveTokensFor... #47418

Merged

Conversation

albertzaharovits
Copy link
Contributor

All internal searches (triggered by APIs) across the .security index must be performed while "under the security origin". Otherwise the search is performed in the context of the caller which most likely does not have privileges to search .security (hopefully).
This commit fixes this in the case of two methods in the TokenService and corrects an overly done such context switch in the ApiKeyService.

In addition, this makes all tests from the client/rest-high-level module execute as an all mighty administrator, but not a literal superuser.

Closes #47151

@elasticmachine
Copy link
Collaborator

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

@albertzaharovits
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/bwc

@albertzaharovits
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/default-distro

@albertzaharovits
Copy link
Contributor Author

The CI failures had :distribution:bwc:staged:buildBwcLinuxTar FAILED and :distribution:bwc:staged:buildBwcOssLinuxTar FAILED without further details.

@albertzaharovits
Copy link
Contributor Author

@elasticmachine update branch

@tvernum
Copy link
Contributor

tvernum commented Oct 4, 2019

Can we add an x-pack test (e.g. a rest yml test) for calling this API without superuser privileges?
I don't like relying on the HLRC to test x-pack API behaviour.

@albertzaharovits
Copy link
Contributor Author

@tvernum @bizybot I have added the suggested tests, may you please take another look?

@albertzaharovits
Copy link
Contributor Author

@elasticmachine test this please

1 similar comment
@albertzaharovits
Copy link
Contributor Author

@elasticmachine test this please

@bizybot
Copy link
Contributor

bizybot commented Oct 21, 2019

Hi @albertzaharovits, Thank you for addressing the review comment.
I think we can also add a rest yml test for token service, in the token service rest yml test (10_basic) the user has superuser role we could check with a limited role to test the bug scenario. Thank you.

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

@albertzaharovits
Copy link
Contributor Author

Cannot create container for service openldap: error creating overlay mount to /var/lib/docker/overlay2/d0a6805f71915982e997588339be1b2443094ef75d584c8c6d3c5b0f1b7e514a/merged: device or resource busy

 @elasticmachine run elasticsearch-ci/2

@albertzaharovits
Copy link
Contributor Author

@bizybot I have pushed the change as you recommended.

@albertzaharovits
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/bwc

@albertzaharovits
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/2

@albertzaharovits
Copy link
Contributor Author

Previous failure (https://gradle-enterprise.elastic.co/s/aulxbxfqrnjxc) tracked under #48258 .

Copy link
Contributor

@bizybot bizybot left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you.

@albertzaharovits albertzaharovits merged commit 9964d89 into elastic:master Oct 21, 2019
@albertzaharovits albertzaharovits deleted the invalidate_for_user_and_realm branch October 21, 2019 09:18
albertzaharovits added a commit that referenced this pull request Oct 21, 2019
…48280)

All internal searches (triggered by APIs) across the .security index
must be performed while "under the security origin". Otherwise,
the search is performed in the context of the caller which most
likely does not have privileges to search .security (hopefully).
This commit fixes this in the case of two methods in the
TokenService and corrects an overly done such context switch
in the ApiKeyService.

In addition, this makes all tests from the client/rest-high-level
module execute as an all mighty administrator,
but not a literal superuser.

Closes #47151
"username": "api_key_user_1"
}
- length: { "invalidated_api_keys" : 1 }
- match: { "invalidated_api_keys.0" : "${user1_key_id}" }
Copy link
Contributor

Choose a reason for hiding this comment

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

The "${user1_key_id}" looks incorrect — all the other "stash" matchers have only $user1_key_id. This breaks for me on the Go client, I'm wondering what the Java runner is doing, so this syntax passes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Users with only manage_token privilege cannot invalidate tokens by username or realmname
6 participants