Skip to content

Assert wildcards are not expanded as specified by request options #90641

Merged
elasticsearchmachine merged 12 commits intoelastic:mainfrom
albertzaharovits:replace_wildcards
Oct 6, 2022
Merged

Assert wildcards are not expanded as specified by request options #90641
elasticsearchmachine merged 12 commits intoelastic:mainfrom
albertzaharovits:replace_wildcards

Conversation

@albertzaharovits
Copy link
Copy Markdown
Contributor

@albertzaharovits albertzaharovits commented Oct 4, 2022

This modifies some assertions in the authz to cater for
the cases where wildcards are not expanded because
of request options.

@albertzaharovits albertzaharovits added :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC >non-issue labels Oct 4, 2022
@albertzaharovits albertzaharovits marked this pull request as ready for review October 4, 2022 12:56
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Oct 4, 2022
// expand to hidden
return expandWildcardsOpen() || expandWildcardsClosed();
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've added this new options-test method, to make it easier to track all the code places that rely on wildcards being expanded or not.

+ requestInfo.getAction()
+ "] contains unexpanded wildcards "
+ Arrays.stream(indices).filter(Regex::isSimpleMatchPattern).toList();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This does not really represent the conditions under which a proper child-action contains unexpanded wildcards.
The condition reflects the fact that an action can be double-authorized.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would like to avoid double-authorization in a follow-up PR, but I won't promise.

@albertzaharovits
Copy link
Copy Markdown
Contributor Author

To the reviewers:
A request can contain wildcards which will not be expanded due to request options (and will also be authorized) by Security and will then percolate down to Core where wildcards will be ignored or trigger a "resource not found" exception.
But there was this assertion in RBACEngine#isChildActionAuthorizedByParent about a child request that checked that names are expanded wildcards and which didn't account for this case. This PR mainly adjusts that assertion.

BUT, that assertion didn't actually trip for a child action authorization scenario. It tripped for a double-authorization known anomaly. Consequently the assertion adjusting doesn't really make sense when considering child actions only.
Moreover, the fact that authorization passes for unexpanded wildcard names and that some access control is populated for such is not asserted because I would like to reconsider the case of access control permissions for non-concrete indices, which is maybe coming in a future episode.

Copy link
Copy Markdown
Member

@ywangd ywangd 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 albertzaharovits added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Oct 6, 2022
@elasticsearchmachine elasticsearchmachine merged commit ca32963 into elastic:main Oct 6, 2022
@albertzaharovits albertzaharovits deleted the replace_wildcards branch October 6, 2022 16:53
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Oct 10, 2022
* main: (150 commits)
  Remove ToXContent interface from ChunkedToXContent (elastic#90409)
  Remove extra SearchService constructor (elastic#90733)
  Update min version for the diagnosis yaml test (elastic#90731)
  Use the AggTestConfig object in testCase (elastic#90699)
  [DOCS] Add links to clear trained model deployment cache API (elastic#90727)
  Assert wildcards are not expanded as specified by request options  (elastic#90641)
  [TEST] Fix exit snapshot restore exit condition (elastic#90696)
  [TEST] Change to atomic file contents save (elastic#90695)
  Update forbiddenapis to 3.4 (elastic#90624)
  [Tests] Don't use concurrent search in scripted field type tests (elastic#90712)
  [ML] Move scaling is possible check for starting trained model (elastic#90706)
  Add new base test case for chunked xcontent types  (elastic#90707)
  Fix testRedNoBlockedIndicesAndRedAllRoleNodes (elastic#90671)
  Fix nullpointer in docs test setup (elastic#90660)
  Don't produce build logs artifact when in a composite build
  Fixing a race condition in EnrichCoordinatorProxyAction that can leave an item stuck in its queue (elastic#90688)
  docs: update fleet/agent pipeline docs (elastic#90659)
  [HealthAPI] Use plural consistently in resource types (elastic#90682)
  [Testing] Enable bwc and fix sorting for 500_date_range (elastic#90681)
  Add profiling and documentation for dfs phase (elastic#90536)
  ...

# Conflicts:
#	x-pack/plugin/mapper-aggregate-metric/src/test/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapperTests.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >non-issue :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team v8.6.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants