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

Restrict direct use of ApplicationPrivilege constructor #91176

Merged
merged 9 commits into from
Nov 2, 2022

Conversation

n1v0lg
Copy link
Contributor

@n1v0lg n1v0lg commented Oct 28, 2022

The canonical way to construct application privileges is to use ApplicationPrivilege::get which accounts for stored privilege look-up. This PR restricts and reduces the direct usage of the ApplicationPrivilege constructor, to enforce this. To ease testing, the PR introduces a new utility function that matches the constructor's signature.

I plan to follow this up with a final PR that ports the test helper method to use ApplicationPrivilege::get, and make the constructor private.

Relates: #91152 (review)

@n1v0lg n1v0lg added >refactoring :Security/Security Security issues without another label v8.6.0 labels Oct 28, 2022
@n1v0lg n1v0lg self-assigned this Oct 28, 2022
@n1v0lg n1v0lg changed the title Lock down application privilege constructor Restrict direct use of application privilege constructor Oct 28, 2022
@n1v0lg n1v0lg changed the title Restrict direct use of application privilege constructor Restrict direct use of ApplicationPrivilege constructor Oct 28, 2022
@@ -257,10 +264,6 @@ private ApplicationPrivilege randomPrivilege() {
patterns[i] = randomAlphaOfLengthBetween(2, 5) + "/" + suffix;
}

final Map<String, Object> metadata = new HashMap<>();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was dead code

@n1v0lg n1v0lg requested a review from ywangd November 1, 2022 10:48
@n1v0lg n1v0lg marked this pull request as ready for review November 1, 2022 10:48
@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Nov 1, 2022
@elasticsearchmachine
Copy link
Collaborator

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

Copy link
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

@n1v0lg n1v0lg merged commit d5fb604 into elastic:main Nov 2, 2022
@n1v0lg n1v0lg deleted the lock-down-application-priv-constructors branch November 2, 2022 09:49
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Nov 3, 2022
* main: (1300 commits)
  update c2id/c2id-server-demo docker image to support ARM (elastic#91144)
  Allow legacy index settings on legacy indices (elastic#90264)
  Skip prevoting if single-node discovery (elastic#91255)
  Chunked encoding for snapshot status API (elastic#90801)
  Allow different decay values depending on the score function (elastic#91195)
  Fix handling indexed envelopes crossing the dateline in mvt API (elastic#91105)
  Ensure cleanups succeed in JoinValidationService (elastic#90601)
  Add overflow behaviour test for RecyclerBytesStreamOutput (elastic#90638)
  More actionable error for ancient indices (elastic#91243)
  Fix APM configuration file delete (elastic#91058)
  Clean up handshake test class (elastic#90966)
  Improve H3#hexRing logic and add H3#areNeighborCells method (elastic#91140)
  Restrict direct use of `ApplicationPrivilege` constructor (elastic#91176)
  [ML] Allow NLP truncate option to be updated when span is set (elastic#91224)
  Support multi-intersection for FieldPermissions (elastic#91169)
  Support intersecting multi-sets of queries with DocumentPermissions (elastic#91151)
  Ensure TermsEnum action works correctly with API keys (elastic#91170)
  Fix NPE in auditing authenticationSuccess for non-existing run-as user (elastic#91171)
  Ensure PKI's delegated_by_realm metadata respect run-as (elastic#91173)
  [ML] Update API documentation for anomaly score explanation (elastic#91177)
  ...

# Conflicts:
#	x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackClientPlugin.java
#	x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/RollupShardIndexer.java
#	x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/TransportRollupIndexerAction.java
#	x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/v2/RollupActionSingleNodeTests.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>refactoring :Security/Security Security issues without another label 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