Skip to content

Fix Has Privilege API check on restricted indices #41226

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

Merged

Conversation

albertzaharovits
Copy link
Contributor

@albertzaharovits albertzaharovits commented Apr 15, 2019

The Has Privileges API allows to tap into the authorization process, to validate privileges without actually running the operations to be authorized. This fixes a bug, uncovered by Tim, in which the Has Privilege API returned spurious results when checking for index privileges over restricted indices (currently .security, .security-6, .security-7). The actual authorization process is not affected by the bug.

Wildcards of index names in the Has Privileges's request do not implicitly cover restricted indices. Concretely, calling:

GET /_security/user/_has_privileges
{
  "index" : [
    {
      "names": [ ".sec*", ".security*" ],
      "privileges": [ "read" ]
    }
  ]
}

and getting back a response such as:

{
  "has_all_requested" : false,
  "index" : {
    ".sec*" : {
      "read" : false
    },
    ".security*" : {
      "read" : true
    }
  }

Does not mean the user is authorized to read the indices .security, .security-6, .security-7 , even though the output says .security*: { "read": true } (and these indices match the wildcard). They might, or they might not, depending on the permissions definition. Implicitly, the API assumes the caller is not interested on privileges over these indices so the wildcard is not considering them.
If the caller is interested in privileges over these restricted indices, it should change the request as such:

GET /_security/user/_has_privileges
{
  "index" : [
    {
      "names": [ ".sec*", ".security*" ],
      "privileges": [ "read" ]
    }
  ],
  "allow_restricted_indieces": true
}

And now the wildcards expand to include the special indices. If the response is true (the same response as above), the the read operation is indeed authorized on .security, .security-6, .security-7 .

Passing the allow_restricted_indices is required even when checking privileges using explicit index names (no wildcards). And this is where the bug manifested: basically the result was bogus when explicitly checking privileges over restricted indices explicitly mentioned, but without setting the allow_restricted_indices flag.

The fix treats explicit restricted indices, without the allow_restricted_indices: true flag, in the request, as an IllegalArgumentException (400 error). If the flag is specified, the API correctly validates the privilege.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@colings86 colings86 added v6.7.3 and removed v6.7.2 labels Apr 17, 2019
@albertzaharovits
Copy link
Contributor Author

albertzaharovits commented Apr 18, 2019

@tvernum you are right that the 400 error is not suitable for a fix in a patch version.

I and @jkakavas gathered and decided it's better to do the proper check on the restricted indices (.security, .security-6 and .security-7) when they are mentioned explicitly, even if the allow_restricted_indices parameter is not explicitly set to true.
The original intention of the allow_restricted_indices parameter was as an escape hatch for wildcard expansions that do not implicitly cover restricted indices, for the cases that the wildcard should do that (very rarely, and not a use case we recommend). It was extended to explicit restricted indices, without thinking it through, on consistency basis.

@albertzaharovits
Copy link
Contributor Author

@jkakavas this is ready for review.

Copy link
Member

@jkakavas jkakavas left a comment

Choose a reason for hiding this comment

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

LGTM

set to `true`. By default this is `false` because restricted indices should
generaly not be "visible" to APIs. For most use cases it is safe to ignore
this parameter.
`allow_restricted_indices`::: (boolean) This needs to be set to `true` (default
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth adding one sentence in the end, something like "If restricted indices are explicitly included in the names list, privileges will be checked against them regardless of the value of allow_restricted_indices" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good idea, I've amended the description as you suggested.

@albertzaharovits
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/1

@albertzaharovits
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/1

@jaymode jaymode added v7.0.2 and removed v7.0.1 labels Apr 24, 2019
@albertzaharovits
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/1

2 similar comments
@albertzaharovits
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/1

@albertzaharovits
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/1

@albertzaharovits albertzaharovits merged commit 6236b3a into elastic:master Apr 25, 2019
@albertzaharovits albertzaharovits deleted the has_privilege_restricted_index branch April 25, 2019 09:02
albertzaharovits added a commit that referenced this pull request Apr 25, 2019
The Has Privileges API allows to tap into the authorization process, to validate
privileges without actually running the operations to be authorized. This commit
fixes a bug, in which the Has Privilege API returned spurious results when checking
for index privileges over restricted indices (currently .security, .security-6,
.security-7). The actual authorization process is not affected by the bug.
albertzaharovits added a commit that referenced this pull request Apr 25, 2019
The Has Privileges API allows to tap into the authorization process, to validate
privileges without actually running the operations to be authorized. This commit
fixes a bug, in which the Has Privilege API returned spurious results when checking
for index privileges over restricted indices (currently .security, .security-6,
.security-7). The actual authorization process is not affected by the bug.
albertzaharovits added a commit that referenced this pull request Apr 25, 2019
The Has Privileges API allows to tap into the authorization process, to validate
privileges without actually running the operations to be authorized. This commit
fixes a bug, in which the Has Privilege API returned spurious results when checking
for index privileges over restricted indices (currently .security, .security-6,
.security-7). The actual authorization process is not affected by the bug.
@colings86 colings86 added v6.7.2 and removed v6.7.3 labels Apr 26, 2019
@jaymode jaymode added v7.0.1 and removed v7.0.2 labels Apr 29, 2019
akhil10x5 pushed a commit to akhil10x5/elasticsearch that referenced this pull request May 2, 2019
The Has Privileges API allows to tap into the authorization process, to validate
privileges without actually running the operations to be authorized. This commit
fixes a bug, in which the Has Privilege API returned spurious results when checking
for index privileges over restricted indices (currently .security, .security-6,
.security-7). The actual authorization process is not affected by the bug.
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
The Has Privileges API allows to tap into the authorization process, to validate
privileges without actually running the operations to be authorized. This commit
fixes a bug, in which the Has Privilege API returned spurious results when checking
for index privileges over restricted indices (currently .security, .security-6,
.security-7). The actual authorization process is not affected by the bug.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants