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

do_not_fail_on_forbidden mode introduces inconsistencies for mget, msearch and similar actions #4485

Closed
nibix opened this issue Jun 24, 2024 · 5 comments · Fixed by #4486
Closed
Labels
bug Something isn't working triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.

Comments

@nibix
Copy link
Collaborator

nibix commented Jun 24, 2024

This is a facet of the work done for #3870. A corresponding PR will immediately follow up, which is a part of the changes done in #4380

The implementation of the do_not_fail_on_forbidden mode in conjunction with multi-part requests like mget, msearch, mtv and other index-independent read operations like scroll seems to be inconsistent on several levels.

The PrivilegesEvaluator class gives certain cluster actions a special treatment for do_not_fail_on_forbidden mode at

if (dnfofEnabled && (action0.startsWith("indices:data/read/")) && !requestedResolved.getAllIndices().isEmpty()) {
.

The affected actions are determined by action0.startsWith("indices:data/read/") and the conditions that determine whether an action is a cluster action:

public static boolean isClusterPerm(String action0) {

This results in the following actions being handled here:

  • indices:data/read/scroll
  • indices:data/read/mget
  • indices:data/read/msearch
  • indices:data/read/mtv
  • indices:data/read/search/template/render

In any case, these permissions are checked as cluster permission with the method securityRoles.impliesClusterPermissionPermission(action0).

The special treatment for do_not_fail_on_forbidden which is executed afterwards looks like this:

Set<String> reduced = securityRoles.reduce(
                            requestedResolved,
                            user,
                            new String[] { action0 },
                            resolver,
                            clusterService
                        );

                        if (reduced.isEmpty()) {
                            presponse.allowed = false;
                            return presponse;
                        }

                        if (irr.replace(request, true, reduced.toArray(new String[0]))) {
                            presponse.missingPrivileges.clear();
                            presponse.allowed = true;
                            return presponse;
                        }

This results in some inconsistencies described in the following.

Privileges need to be specified both in cluster_privileges and index_privileges

As described before, the affected actions such as indices:data/read/mget are considered as cluster actions, so these need to be specified in the cluster_privileges section of a role. securityRoles.reduce() now however is an index-specific operation, so it checks the index_privileges of a role. Thus, in do_not_fail_on_forbidden mode, the privileges for indices:data/read/mget need to be present in index_privileges as well. This is inconsistent to the mode where do_not_fail_on_forbidden is turned off, the index privileges for indices:data/read/mget are not needed in that case.

It is also confusing and surprising to the user. The docs at https://opensearch.org/docs/latest/security/access-control/permissions/#cluster-permissions say on one hand:

These permissions are for the cluster and can’t be applied granularly. For example, you either have permissions to take snapshots (cluster:admin/snapshot/create) or you don’t. The cluster permission, therefore, cannot grant a user privileges to take snapshots of a select set of indexes while preventing the user from taking snapshots of others.

But in the table below it mentions for mget and related actions:

Permission to run multiple GET operations in one request. This setting must be configured as both a cluster- and index-level permission

This is a contradiction to the first sentence. Additionally, it is not true when do_not_fail_on_forbidden is turned off.

_mget gains an invalid index pattern support in do_not_fail_on_forbidden mode

In OpenSearch, the _get API and indivual items in _mget API requests always refer to exactly one index, so index patterns are not supported here. If you try to use an index pattern, you will usually get an error:

{
    "docs": [
        {
            "_index": "test_*",
            "_id": "1",
            "error": {
                "root_cause": [
                    {
                        "type": "index_not_found_exception",
                        "reason": "no such index [test_*]",
                        "index": "test_*",
                        "resource.id": "test_*",
                        "resource.type": "index_expression",
                        "index_uuid": "_na_"
                    }
                ],
                "type": "index_not_found_exception",
                "reason": "no such index [test_*]",
                "index": "test_*",
                "resource.id": "test_*",
                "resource.type": "index_expression",
                "index_uuid": "_na_"
            }
        }
   ]
}

When having do_not_fail_on_forbidden enabled, however, index patterns are supported at least in some cases like the test at

MultiGetRequest request = new MultiGetRequest().add(BOTH_INDEX_PATTERN, ID_1).add(BOTH_INDEX_PATTERN, ID_4);
proves.

This is caused by the irr.replace() call in the special handling which resolves index patterns for the MultiGetRequest.Item objects.

The indices:data/read/scroll privilege is also required as index privilege, but any index will do

Note: This might sound like a security issue first, but it is actually not, as the indices:data/read/search privileges are still effective to limit access.

Like indices:data/read/mget, the indices:data/scroll privileges need to be provided both as cluster_permissions and index_permissions. The special handling for indices:data/scroll also includes the attempt to gather the requested indices from the request. As the scroll request does not carry index information, the requested indices will be however assumed to be * (all indices). The securityRoles.reduce() call will then calculate the intersection between * and the indices with privileges and return all indices with available privileges. With these, the irr.replace() method is called. This however is just a no-op for scroll requests:

That means effectively, that a indices:data/read/scroll index privilege is necessary, but any non-empty set of indices is sufficient to satisfy the special handling code. While this is not a security issue, as there are further safe-guards (see above), this is an inconsistent behaviour.

Proposed solution

The special handling code for do_not_fail_on_forbidden mode on cluster actions is actually unnecessary and can be completely removed.

For the multi actions like mget, msearch, mtv, and search template actions, the action handling will always trigger sub-actions which will be routed again through the privilege evaluation code - then, the proper access controls will be enforced.

For the scroll action, proper access control has been already enforced before when issuing the search request.

Thus, the special handling code actually introduces no benefits and only issues.

@nibix nibix added bug Something isn't working untriaged Require the attention of the repository maintainers and may need to be prioritized labels Jun 24, 2024
@nibix nibix changed the title [BUG] do_not_fail_on_forbidden mode introduces inconsistencies for mget, msearch and similar actions Jun 24, 2024
nibix added a commit to nibix/security that referenced this issue Jun 24, 2024
This special handling does not provide any benefits - rather it just causes some inconsistent behavior. Fixes opensearch-project#4485

Signed-off-by: Nils Bandener <[email protected]>
nibix added a commit to nibix/security that referenced this issue Jun 24, 2024
This special handling does not provide any benefits - rather it just causes some inconsistent behavior. Fixes opensearch-project#4485

Signed-off-by: Nils Bandener <[email protected]>
@stephen-crawford
Copy link
Contributor

[Triage] Hi Nils, thanks for filing this issue. Looks good, will mark as triaged.

@stephen-crawford stephen-crawford added triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. and removed untriaged Require the attention of the repository maintainers and may need to be prioritized labels Jun 24, 2024
nibix added a commit to nibix/security that referenced this issue Jun 24, 2024
This special handling does not provide any benefits - rather it just causes some inconsistent behavior. Fixes opensearch-project#4485

Signed-off-by: Nils Bandener <[email protected]>
nibix added a commit to nibix/security that referenced this issue Jun 24, 2024
This special handling does not provide any benefits - rather it just causes some inconsistent behavior. Fixes opensearch-project#4485

Signed-off-by: Nils Bandener <[email protected]>
nibix added a commit to nibix/security that referenced this issue Jun 24, 2024
This special handling does not provide any benefits - rather it just causes some inconsistent behavior. Fixes opensearch-project#4485

Signed-off-by: Nils Bandener <[email protected]>
@cwperks
Copy link
Member

cwperks commented Jun 24, 2024

@nibix Thank you very much for the detailed issue. It would definitely be good to implement a fix for this to avoid having to duplicate action names across cluster_permissions and index_permissions.

@DarshitChanpura
Copy link
Member

+1 @nibix This is very detailed, and would definitely help alleviate pains with DNFOF.

In OpenSearch, the _get API and indivual items in _mget API requests always refer to exactly one index, so index patterns are not supported here

Will the implementation of this issue address this by adding support for multiple indices?

@nibix
Copy link
Collaborator Author

nibix commented Jun 24, 2024

@DarshitChanpura

Will the implementation of this issue address this by adding support for multiple indices?

No, it is a fundamental propery of get that only exactly one document in one index can be referenced. The change in #4486 just adjusts the behavior for do_not_fail_on_forbidden: true to the fundamental behavior that can be observed when do_not_fail_on_forbidden: false or when there is no security plugin alltogether.

@DarshitChanpura
Copy link
Member

Thank you @nibix. I should have been more clearer by mentioning mget. Regardless, it seems like you have already raised a PR to fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants