From c33fa559931430e9a56ccd86a9f8386861989969 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 4 Nov 2025 19:44:04 -0500 Subject: [PATCH 01/10] Add --timeout (-to) as an option to securityadmin.sh Signed-off-by: Craig Perkins --- .../security/tools/SecurityAdmin.java | 36 +++++++++++-------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/src/main/java/org/opensearch/security/tools/SecurityAdmin.java b/src/main/java/org/opensearch/security/tools/SecurityAdmin.java index b65c50e6f5..f21135c9df 100644 --- a/src/main/java/org/opensearch/security/tools/SecurityAdmin.java +++ b/src/main/java/org/opensearch/security/tools/SecurityAdmin.java @@ -226,6 +226,7 @@ public static int execute(final String[] args) throws Exception { options.addOption( Option.builder("p").longOpt("port").hasArg().argName("port").desc("OpenSearch transport port (default: 9200)").build() ); + options.addOption(Option.builder("to").longOpt("timeout").hasArg().argName("timeout").desc("timeout (default: 30s)").build()); options.addOption( Option.builder("cn") .longOpt("clustername") @@ -336,6 +337,7 @@ public static int execute(final String[] args) throws Exception { String hostname = "localhost"; int port = 9200; + int timeout = 30; String kspass = System.getenv(OPENDISTRO_SECURITY_KS_PASS); String tspass = System.getenv(OPENDISTRO_SECURITY_TS_PASS); String cd = "."; @@ -387,6 +389,7 @@ public static int execute(final String[] args) throws Exception { hostname = line.getOptionValue("h", hostname); port = Integer.parseInt(line.getOptionValue("p", String.valueOf(port))); + timeout = Integer.parseInt(line.getOptionValue("to", String.valueOf(timeout))); promptForPassword = line.hasOption("prompt"); @@ -869,7 +872,7 @@ public static int execute(final String[] args) throws Exception { return (-1); } - boolean success = uploadFile(restHighLevelClient, file, index, type, resolveEnvVars); + boolean success = uploadFile(restHighLevelClient, file, index, type, resolveEnvVars, timeout); if (!success) { System.out.println("ERR: cannot upload configuration, see errors above"); @@ -884,7 +887,7 @@ public static int execute(final String[] args) throws Exception { return (success ? 0 : -1); } - return upload(restHighLevelClient, index, cd, expectedNodeCount, resolveEnvVars); + return upload(restHighLevelClient, index, cd, expectedNodeCount, resolveEnvVars, timeout); } } @@ -959,9 +962,10 @@ private static boolean uploadFile( final String filepath, final String index, final String _id, - boolean resolveEnvVars + boolean resolveEnvVars, + int timeout ) { - return uploadFile(restHighLevelClient, filepath, index, _id, resolveEnvVars, false); + return uploadFile(restHighLevelClient, filepath, index, _id, resolveEnvVars, false, timeout); } private static boolean uploadFile( @@ -970,7 +974,8 @@ private static boolean uploadFile( final String index, final String _id, boolean resolveEnvVars, - final boolean populateEmptyIfMissing + final boolean populateEmptyIfMissing, + int timeout ) { String id = _id; @@ -988,6 +993,7 @@ private static boolean uploadFile( final String content = CharStreams.toString(reader); final String res = restHighLevelClient.index( new IndexRequest(index).id(id) + .timeout(timeout + "s") .setRefreshPolicy(RefreshPolicy.IMMEDIATE) .source(_id, readXContent(resolveEnvVars ? replaceEnvVars(content, Settings.EMPTY) : content, XContentType.YAML)), RequestOptions.DEFAULT @@ -1354,23 +1360,23 @@ private static int backup(RestHighLevelClient tc, String index, File backupDir) return success ? 0 : -1; } - private static int upload(RestHighLevelClient tc, String index, String cd, int expectedNodeCount, boolean resolveEnvVars) + private static int upload(RestHighLevelClient tc, String index, String cd, int expectedNodeCount, boolean resolveEnvVars, int timeout) throws IOException { - boolean success = uploadFile(tc, cd + "config.yml", index, "config", resolveEnvVars); - success = uploadFile(tc, cd + "roles.yml", index, "roles", resolveEnvVars) && success; - success = uploadFile(tc, cd + "roles_mapping.yml", index, "rolesmapping", resolveEnvVars) && success; + boolean success = uploadFile(tc, cd + "config.yml", index, "config", resolveEnvVars, timeout); + success = uploadFile(tc, cd + "roles.yml", index, "roles", resolveEnvVars, timeout) && success; + success = uploadFile(tc, cd + "roles_mapping.yml", index, "rolesmapping", resolveEnvVars, timeout) && success; - success = uploadFile(tc, cd + "internal_users.yml", index, "internalusers", resolveEnvVars) && success; - success = uploadFile(tc, cd + "action_groups.yml", index, "actiongroups", resolveEnvVars) && success; + success = uploadFile(tc, cd + "internal_users.yml", index, "internalusers", resolveEnvVars, timeout) && success; + success = uploadFile(tc, cd + "action_groups.yml", index, "actiongroups", resolveEnvVars, timeout) && success; - success = uploadFile(tc, cd + "tenants.yml", index, "tenants", resolveEnvVars) && success; + success = uploadFile(tc, cd + "tenants.yml", index, "tenants", resolveEnvVars, timeout) && success; - success = uploadFile(tc, cd + "nodes_dn.yml", index, "nodesdn", resolveEnvVars, true) && success; + success = uploadFile(tc, cd + "nodes_dn.yml", index, "nodesdn", resolveEnvVars, true, timeout) && success; if (new File(cd + "audit.yml").exists()) { - success = uploadFile(tc, cd + "audit.yml", index, "audit", resolveEnvVars) && success; + success = uploadFile(tc, cd + "audit.yml", index, "audit", resolveEnvVars, timeout) && success; } if (new File(cd + "allowlist.yml").exists()) { - success = uploadFile(tc, cd + "allowlist.yml", index, "allowlist", resolveEnvVars) && success; + success = uploadFile(tc, cd + "allowlist.yml", index, "allowlist", resolveEnvVars, timeout) && success; } if (!success) { From b664c5d01ae3cbd79051fbd6883d65ef6e90ad91 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 10 Nov 2025 15:14:49 -0500 Subject: [PATCH 02/10] Add to CHANGELOG Signed-off-by: Craig Perkins --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d4df7c4d3e..5f003c757d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - add suggest api to ad read access role ([#5754](https://github.com/opensearch-project/security/pull/5754)) - Get list of headersToCopy from core and use getHeader(String headerName) instead of getHeaders() ([#5769](https://github.com/opensearch-project/security/pull/5769)) - Add support for X509 v3 extensions (SAN) for authentication ([#5701](https://github.com/opensearch-project/security/pull/5701)) +- Add --timeout (-to) as an option to securityadmin.sh ([#5787](https://github.com/opensearch-project/security/pull/5787)) ### Bug Fixes - Create a WildcardMatcher.NONE when creating a WildcardMatcher with an empty string ([#5694](https://github.com/opensearch-project/security/pull/5694)) From 63f0d0be144f5fd07b86e1d795be8404dbb521df Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 24 Nov 2025 07:31:28 -0500 Subject: [PATCH 03/10] Bump kafka_version from 4.1.0 to 4.1.1 (#5806) Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- CHANGELOG.md | 2 +- build.gradle | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 825c0dc203..e112dd3485 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,7 +45,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Bump `org.junit.jupiter:junit-jupiter` from 5.13.4 to 5.14.1 ([#5678](https://github.com/opensearch-project/security/pull/5678), [#5764](https://github.com/opensearch-project/security/pull/5764)) - Bump `ch.qos.logback:logback-classic` from 1.5.18 to 1.5.20 ([#5680](https://github.com/opensearch-project/security/pull/5680), [#5724](https://github.com/opensearch-project/security/pull/5724)) - Bump `org.scala-lang:scala-library` from 2.13.16 to 2.13.17 ([#5682](https://github.com/opensearch-project/security/pull/5682)) -- Bump `kafka_version` from 4.0.0 to 4.1.0 ([#5613](https://github.com/opensearch-project/security/pull/5613)) +- Bump `kafka_version` from 4.0.0 to 4.1.1 ([#5613](https://github.com/opensearch-project/security/pull/5613), [#5806](https://github.com/opensearch-project/security/pull/5806)) - Bump `org.gradle.test-retry` from 1.6.2 to 1.6.4 ([#5706](https://github.com/opensearch-project/security/pull/5706)) - Bump `org.checkerframework:checker-qual` from 3.51.0 to 3.51.1 ([#5705](https://github.com/opensearch-project/security/pull/5705)) - Bump `org.ow2.asm:asm` from 9.8 to 9.9 ([#5707](https://github.com/opensearch-project/security/pull/5707)) diff --git a/build.gradle b/build.gradle index 00c003fc77..5cc04b5206 100644 --- a/build.gradle +++ b/build.gradle @@ -26,7 +26,7 @@ buildscript { common_utils_version = System.getProperty("common_utils.version", '3.2.0.0-SNAPSHOT') - kafka_version = '4.1.0' + kafka_version = '4.1.1' open_saml_version = '5.1.6' open_saml_shib_version = "9.1.4" one_login_java_saml = '2.9.0' From 90d78dd2793f0408985f6549131d845ca7651ae1 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 24 Nov 2025 07:32:04 -0500 Subject: [PATCH 04/10] Bump actions/checkout from 5 to 6 (#5810) Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/auto-release.yml | 2 +- .github/workflows/automatic-merges.yml | 2 +- .github/workflows/changelog_verifier.yml | 2 +- .github/workflows/ci.yml | 28 ++++++++++++------------ .github/workflows/code-hygiene.yml | 10 ++++----- .github/workflows/dependabot_pr.yml | 2 +- .github/workflows/integration-tests.yml | 2 +- .github/workflows/link-checker.yml | 2 +- .github/workflows/maven-publish.yml | 2 +- .github/workflows/plugin_install.yml | 2 +- CHANGELOG.md | 1 + 11 files changed, 28 insertions(+), 27 deletions(-) diff --git a/.github/workflows/auto-release.yml b/.github/workflows/auto-release.yml index 64502b282f..4a1729a4f9 100644 --- a/.github/workflows/auto-release.yml +++ b/.github/workflows/auto-release.yml @@ -21,7 +21,7 @@ jobs: - name: Get tag id: tag uses: dawidd6/action-get-tag@v1 - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 - name: Download release notes run: | curl -s -o release-notes.md https://raw.githubusercontent.com/opensearch-project/security/main/release-notes/opensearch-security.release-notes-${{ steps.tag.outputs.tag }}.md diff --git a/.github/workflows/automatic-merges.yml b/.github/workflows/automatic-merges.yml index e38250a43b..e09130965e 100644 --- a/.github/workflows/automatic-merges.yml +++ b/.github/workflows/automatic-merges.yml @@ -12,7 +12,7 @@ jobs: automatic-merge-version-bumps: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 - id: find-triggering-pr uses: peternied/find-triggering-pr@v1 diff --git a/.github/workflows/changelog_verifier.yml b/.github/workflows/changelog_verifier.yml index 0f793952f3..b4a1a0dc30 100644 --- a/.github/workflows/changelog_verifier.yml +++ b/.github/workflows/changelog_verifier.yml @@ -9,7 +9,7 @@ jobs: if: github.repository == 'opensearch-project/security' runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 with: token: ${{ secrets.GITHUB_TOKEN }} ref: ${{ github.event.pull_request.head.sha }} diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 02b17541ca..d7be63dd3d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -30,7 +30,7 @@ jobs: java-version: 21 - name: Checkout security - uses: actions/checkout@v5 + uses: actions/checkout@v6 - name: Generate list of tasks id: set-matrix @@ -56,7 +56,7 @@ jobs: java-version: ${{ matrix.jdk }} - name: Checkout security - uses: actions/checkout@v5 + uses: actions/checkout@v6 - name: Build and Test uses: gradle/gradle-build-action@v3 @@ -99,7 +99,7 @@ jobs: java-version: ${{ matrix.jdk }} - name: Checkout security - uses: actions/checkout@v5 + uses: actions/checkout@v6 - name: Build and Test uses: gradle/gradle-build-action@v3 @@ -119,7 +119,7 @@ jobs: needs: ["test-windows", "test-linux", "integration-tests-windows", "integration-tests-linux", "sample-plugin-integration-tests-linux", "sample-plugin-integration-tests-windows"] runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 - uses: actions/download-artifact@v6 with: path: downloaded-artifacts @@ -156,7 +156,7 @@ jobs: java-version: ${{ matrix.jdk }} - name: Checkout security - uses: actions/checkout@v5 + uses: actions/checkout@v6 - name: Run Integration Tests uses: gradle/gradle-build-action@v3 @@ -199,7 +199,7 @@ jobs: java-version: ${{ matrix.jdk }} - name: Checkout security - uses: actions/checkout@v5 + uses: actions/checkout@v6 - name: Build and Test uses: gradle/gradle-build-action@v3 @@ -242,7 +242,7 @@ jobs: java-version: ${{ matrix.jdk }} - name: Checkout security - uses: actions/checkout@v5 + uses: actions/checkout@v6 - name: Run SampleResourcePlugin Integration Tests uses: gradle/gradle-build-action@v3 @@ -274,7 +274,7 @@ jobs: java-version: ${{ matrix.jdk }} - name: Checkout security - uses: actions/checkout@v5 + uses: actions/checkout@v6 - name: Run SampleResourcePlugin Integration Tests uses: gradle/gradle-build-action@v3 @@ -307,7 +307,7 @@ jobs: java-version: ${{ matrix.jdk }} - name: Checkout security - uses: actions/checkout@v5 + uses: actions/checkout@v6 - name: Run Resource Tests uses: gradle/gradle-build-action@v3 @@ -325,7 +325,7 @@ jobs: java-version: 21 - name: Checkout Security Repo - uses: actions/checkout@v5 + uses: actions/checkout@v6 - name: Build BWC tests uses: gradle/gradle-build-action@v3 @@ -349,7 +349,7 @@ jobs: java-version: ${{ matrix.jdk }} - name: Checkout Security Repo - uses: actions/checkout@v5 + uses: actions/checkout@v6 - id: build-previous uses: ./.github/actions/run-bwc-suite @@ -363,7 +363,7 @@ jobs: code-ql: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 - uses: actions/setup-java@v5 with: distribution: temurin # Temurin is a distribution of adoptium @@ -377,7 +377,7 @@ jobs: build-health: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 - uses: actions/setup-java@v5 with: distribution: temurin # Temurin is a distribution of adoptium @@ -395,7 +395,7 @@ jobs: runs-on: ubuntu-latest steps: - name: Setup Environment - uses: actions/checkout@v5 + uses: actions/checkout@v6 - name: Configure Java uses: actions/setup-java@v5 diff --git a/.github/workflows/code-hygiene.yml b/.github/workflows/code-hygiene.yml index 1745a88d10..f49f2c3738 100644 --- a/.github/workflows/code-hygiene.yml +++ b/.github/workflows/code-hygiene.yml @@ -8,7 +8,7 @@ jobs: name: Check if all files end in newline steps: - name: Checkout - uses: actions/checkout@v5 + uses: actions/checkout@v6 - name: Linelint uses: fernandrone/linelint@0.0.6 @@ -17,7 +17,7 @@ jobs: runs-on: ubuntu-latest name: Spotless scan steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 - uses: actions/setup-java@v5 with: @@ -33,7 +33,7 @@ jobs: runs-on: ubuntu-latest name: Checkstyle scan steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 - uses: actions/setup-java@v5 with: @@ -49,7 +49,7 @@ jobs: runs-on: ubuntu-latest name: Spotbugs scan steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 - uses: actions/setup-java@v5 with: @@ -65,7 +65,7 @@ jobs: runs-on: ubuntu-latest name: Check permissions orders steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 - run: npm install yaml - name: Check permissions order diff --git a/.github/workflows/dependabot_pr.yml b/.github/workflows/dependabot_pr.yml index 47ca51b505..cd924f2e2b 100644 --- a/.github/workflows/dependabot_pr.yml +++ b/.github/workflows/dependabot_pr.yml @@ -18,7 +18,7 @@ jobs: installation_id: 22958780 - name: Check out code - uses: actions/checkout@v5 + uses: actions/checkout@v6 with: token: ${{ steps.github_app_token.outputs.token }} ref: ${{ github.head_ref }} diff --git a/.github/workflows/integration-tests.yml b/.github/workflows/integration-tests.yml index 49a0546a21..db60498311 100644 --- a/.github/workflows/integration-tests.yml +++ b/.github/workflows/integration-tests.yml @@ -20,7 +20,7 @@ jobs: distribution: temurin # Temurin is a distribution of adoptium java-version: ${{ matrix.jdk }} - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 - run: ./gradlew test diff --git a/.github/workflows/link-checker.yml b/.github/workflows/link-checker.yml index f2bcebb199..72c29557bc 100644 --- a/.github/workflows/link-checker.yml +++ b/.github/workflows/link-checker.yml @@ -9,7 +9,7 @@ jobs: linkchecker: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 - name: lychee Link Checker id: lychee uses: lycheeverse/lychee-action@master diff --git a/.github/workflows/maven-publish.yml b/.github/workflows/maven-publish.yml index 33cfdc5162..1b5a25a3cb 100644 --- a/.github/workflows/maven-publish.yml +++ b/.github/workflows/maven-publish.yml @@ -21,7 +21,7 @@ jobs: with: distribution: temurin # Temurin is a distribution of adoptium java-version: 21 - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 - name: Load secret uses: 1password/load-secrets-action@v3 with: diff --git a/.github/workflows/plugin_install.yml b/.github/workflows/plugin_install.yml index 8f31a3ddd4..3f0653d3d0 100644 --- a/.github/workflows/plugin_install.yml +++ b/.github/workflows/plugin_install.yml @@ -27,7 +27,7 @@ jobs: java-version: ${{ matrix.jdk }} - name: Checkout Branch - uses: actions/checkout@v5 + uses: actions/checkout@v6 - name: Assemble target plugin uses: gradle/gradle-build-action@v3 diff --git a/CHANGELOG.md b/CHANGELOG.md index e112dd3485..0db1a52324 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,6 +66,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Bump `commons-io:commons-io` from 2.20.0 to 2.21.0 ([#5780](https://github.com/opensearch-project/security/pull/5780)) - Bump `com.nimbusds:nimbus-jose-jwt` from 10.5 to 10.6 ([#5782](https://github.com/opensearch-project/security/pull/5782)) - Upgrade to gradle 9.2 and run CI with JDK 25 ([#5786](https://github.com/opensearch-project/security/pull/5786)) +- Bump `actions/checkout` from 5 to 6 ([#5810](https://github.com/opensearch-project/security/pull/5810)) ### Documentation From c8c59c29ef9d1407bbd924f44cf2e0a186c0c3da Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 24 Nov 2025 07:32:38 -0500 Subject: [PATCH 05/10] Bump spring_version from 6.2.12 to 6.2.14 (#5808) Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- CHANGELOG.md | 2 +- build.gradle | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0db1a52324..a4ed681d9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,7 +55,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Bump `github/codeql-action` from 3 to 4 ([#5702](https://github.com/opensearch-project/security/pull/5702)) - Bump `com.github.spotbugs` from 6.4.2 to 6.4.4 ([#5727](https://github.com/opensearch-project/security/pull/5727)) - Bump `com.autonomousapps.build-health` from 3.0.4 to 3.3.0 ([#5726](https://github.com/opensearch-project/security/pull/5726), [#5744](https://github.com/opensearch-project/security/pull/5744)) -- Bump `spring_version` from 6.2.11 to 6.2.12 ([#5725](https://github.com/opensearch-project/security/pull/5725)) +- Bump `spring_version` from 6.2.11 to 6.2.14 ([#5725](https://github.com/opensearch-project/security/pull/5725), [#5808](https://github.com/opensearch-project/security/pull/5808)) - Bump `org.springframework.kafka:spring-kafka-test` from 4.0.0-M5 to 4.0.0-RC1 ([#5742](https://github.com/opensearch-project/security/pull/5742)) - Bump `com.google.errorprone:error_prone_annotations` from 2.42.0 to 2.44.0 ([#5743](https://github.com/opensearch-project/security/pull/5743), [#5779](https://github.com/opensearch-project/security/pull/5779)) - Bump `actions/upload-artifact` from 4 to 5 ([#5740](https://github.com/opensearch-project/security/pull/5740)) diff --git a/build.gradle b/build.gradle index 5cc04b5206..b6e242a401 100644 --- a/build.gradle +++ b/build.gradle @@ -33,7 +33,7 @@ buildscript { jjwt_version = '0.13.0' guava_version = '33.5.0-jre' jaxb_version = '2.3.9' - spring_version = '6.2.12' + spring_version = '6.2.14' if (buildVersionQualifier) { opensearch_build += "-${buildVersionQualifier}" From c48110e71c8a8623935b052fa563eabb1938e98c Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 24 Nov 2025 07:33:12 -0500 Subject: [PATCH 06/10] Bump com.google.googlejavaformat:google-java-format from 1.31.0 to 1.32.0 (#5811) Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- CHANGELOG.md | 2 +- build.gradle | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a4ed681d9a..e9e64a605a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,7 +60,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Bump `com.google.errorprone:error_prone_annotations` from 2.42.0 to 2.44.0 ([#5743](https://github.com/opensearch-project/security/pull/5743), [#5779](https://github.com/opensearch-project/security/pull/5779)) - Bump `actions/upload-artifact` from 4 to 5 ([#5740](https://github.com/opensearch-project/security/pull/5740)) - Bump `actions/download-artifact` from 5 to 6 ([#5739](https://github.com/opensearch-project/security/pull/5739)) -- Bump `com.google.googlejavaformat:google-java-format` from 1.28.0 to 1.31.0 ([#5741](https://github.com/opensearch-project/security/pull/5741), [#5765](https://github.com/opensearch-project/security/pull/5765)) +- Bump `com.google.googlejavaformat:google-java-format` from 1.28.0 to 1.32.0 ([#5741](https://github.com/opensearch-project/security/pull/5741), [#5765](https://github.com/opensearch-project/security/pull/5765), [#5811](https://github.com/opensearch-project/security/pull/5811)) - Bump `com.jayway.jsonpath:json-path` from 2.9.0 to 2.10.0 ([#5767](https://github.com/opensearch-project/security/pull/5767)) - Bump `org.apache.ws.xmlschema:xmlschema-core` from 2.3.1 to 2.3.2 ([#5781](https://github.com/opensearch-project/security/pull/5781)) - Bump `commons-io:commons-io` from 2.20.0 to 2.21.0 ([#5780](https://github.com/opensearch-project/security/pull/5780)) diff --git a/build.gradle b/build.gradle index b6e242a401..2d04179f84 100644 --- a/build.gradle +++ b/build.gradle @@ -812,7 +812,7 @@ dependencies { compileOnly "org.opensearch:opensearch:${opensearch_version}" //spotless - implementation('com.google.googlejavaformat:google-java-format:1.31.0') { + implementation('com.google.googlejavaformat:google-java-format:1.32.0') { exclude group: 'com.google.guava' } } From ba25bb882c79bdbc62cbee3a1a7f1b32ff599600 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 24 Nov 2025 11:56:32 -0500 Subject: [PATCH 07/10] Bump org.scala-lang:scala-library from 2.13.17 to 2.13.18 (#5809) Signed-off-by: dependabot[bot] Signed-off-by: Craig Perkins Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Craig Perkins --- CHANGELOG.md | 2 +- build.gradle | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e9e64a605a..244818df3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,7 +44,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Maintenance - Bump `org.junit.jupiter:junit-jupiter` from 5.13.4 to 5.14.1 ([#5678](https://github.com/opensearch-project/security/pull/5678), [#5764](https://github.com/opensearch-project/security/pull/5764)) - Bump `ch.qos.logback:logback-classic` from 1.5.18 to 1.5.20 ([#5680](https://github.com/opensearch-project/security/pull/5680), [#5724](https://github.com/opensearch-project/security/pull/5724)) -- Bump `org.scala-lang:scala-library` from 2.13.16 to 2.13.17 ([#5682](https://github.com/opensearch-project/security/pull/5682)) +- Bump `org.scala-lang:scala-library` from 2.13.16 to 2.13.18 ([#5682](https://github.com/opensearch-project/security/pull/5682), [#5809](https://github.com/opensearch-project/security/pull/5809)) - Bump `kafka_version` from 4.0.0 to 4.1.1 ([#5613](https://github.com/opensearch-project/security/pull/5613), [#5806](https://github.com/opensearch-project/security/pull/5806)) - Bump `org.gradle.test-retry` from 1.6.2 to 1.6.4 ([#5706](https://github.com/opensearch-project/security/pull/5706)) - Bump `org.checkerframework:checker-qual` from 3.51.0 to 3.51.1 ([#5705](https://github.com/opensearch-project/security/pull/5705)) diff --git a/build.gradle b/build.gradle index 2d04179f84..cb691119e8 100644 --- a/build.gradle +++ b/build.gradle @@ -480,7 +480,7 @@ configurations { resolutionStrategy { force 'commons-codec:commons-codec:1.19.0' force 'org.slf4j:slf4j-api:1.7.36' - force 'org.scala-lang:scala-library:2.13.17' + force 'org.scala-lang:scala-library:2.13.18' force "com.fasterxml.jackson:jackson-bom:${versions.jackson}" force "com.fasterxml.jackson.core:jackson-core:${versions.jackson}" force "com.fasterxml.jackson.datatype:jackson-datatype-jdk8:${versions.jackson}" @@ -796,7 +796,7 @@ dependencies { testRuntimeOnly ("org.springframework:spring-core:${spring_version}") { exclude(group:'org.springframework', module: 'spring-jcl' ) } - testRuntimeOnly 'org.scala-lang:scala-library:2.13.17' + testRuntimeOnly 'org.scala-lang:scala-library:2.13.18' testRuntimeOnly 'com.typesafe.scala-logging:scala-logging_3:3.9.6' testRuntimeOnly('org.apache.zookeeper:zookeeper:3.9.3') { exclude(group:'ch.qos.logback', module: 'logback-classic' ) From e9e341a0b287bf0227536c94710326f9fd3e6661 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 24 Nov 2025 11:56:57 -0500 Subject: [PATCH 08/10] Bump commons-validator:commons-validator from 1.10.0 to 1.10.1 (#5807) Signed-off-by: dependabot[bot] Signed-off-by: Craig Perkins Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Craig Perkins --- CHANGELOG.md | 1 + build.gradle | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 244818df3b..e11f390746 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,6 +66,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Bump `commons-io:commons-io` from 2.20.0 to 2.21.0 ([#5780](https://github.com/opensearch-project/security/pull/5780)) - Bump `com.nimbusds:nimbus-jose-jwt` from 10.5 to 10.6 ([#5782](https://github.com/opensearch-project/security/pull/5782)) - Upgrade to gradle 9.2 and run CI with JDK 25 ([#5786](https://github.com/opensearch-project/security/pull/5786)) +- Bump `commons-validator:commons-validator` from 1.10.0 to 1.10.1 ([#5807](https://github.com/opensearch-project/security/pull/5807)) - Bump `actions/checkout` from 5 to 6 ([#5810](https://github.com/opensearch-project/security/pull/5810)) ### Documentation diff --git a/build.gradle b/build.gradle index cb691119e8..83b2a442d3 100644 --- a/build.gradle +++ b/build.gradle @@ -778,7 +778,7 @@ dependencies { testImplementation "org.apache.kafka:kafka-test-common-runtime:${kafka_version}" testImplementation "org.apache.kafka:kafka-test-common-internal-api:${kafka_version}" testImplementation "org.apache.kafka:kafka-transaction-coordinator:${kafka_version}" - testImplementation 'commons-validator:commons-validator:1.10.0' + testImplementation 'commons-validator:commons-validator:1.10.1' testImplementation "org.springframework.kafka:spring-kafka-test:4.0.0-RC1" testImplementation "org.springframework:spring-beans:${spring_version}" testImplementation 'org.junit.jupiter:junit-jupiter:5.14.1' From ba35e4bc8a94f22b226cb98e60e81958c8445cb0 Mon Sep 17 00:00:00 2001 From: Nils Bandener <33570290+nibix@users.noreply.github.com> Date: Tue, 25 Nov 2025 21:39:27 +0100 Subject: [PATCH 09/10] Cleaned up use of PrivilegesEvaluatorResponse (#5804) Signed-off-by: Nils Bandener --- CHANGELOG.md | 1 + .../security/filter/SecurityFilter.java | 20 +- .../security/filter/SecurityRestFilter.java | 9 +- .../privileges/PitPrivilegesEvaluator.java | 13 +- .../privileges/PrivilegesEvaluatorImpl.java | 39 ++- .../PrivilegesEvaluatorResponse.java | 134 +++++---- .../ProtectedIndexAccessEvaluator.java | 14 +- .../privileges/ResourceAccessEvaluator.java | 11 +- .../RestLayerPrivilegesEvaluator.java | 2 +- .../privileges/SnapshotRestoreEvaluator.java | 29 +- .../SystemIndexAccessEvaluator.java | 57 ++-- .../privileges/TermsAggregationEvaluator.java | 13 +- .../ResourceAccessEvaluatorTest.java | 3 +- .../RestLayerPrivilegesEvaluatorTest.java | 6 +- .../SystemIndexAccessEvaluatorTest.java | 272 ++++++++++++------ 15 files changed, 362 insertions(+), 261 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e11f390746..b2d898f8ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - [GRPC] Fix compilation errors from core protobuf version bump to 0.23.0 ([#5763](https://github.com/opensearch-project/security/pull/5763)) - Modularized PrivilegesEvaluator ([#5791](https://github.com/opensearch-project/security/pull/5791)) - [Resource Sharing] Adds post support for update sharing info API ([#5799](https://github.com/opensearch-project/security/pull/5799)) +- Cleaned up use of PrivilegesEvaluatorResponse ([#5804](https://github.com/opensearch-project/security/pull/5804)) ### Maintenance - Bump `org.junit.jupiter:junit-jupiter` from 5.13.4 to 5.14.1 ([#5678](https://github.com/opensearch-project/security/pull/5678), [#5764](https://github.com/opensearch-project/security/pull/5764)) diff --git a/src/main/java/org/opensearch/security/filter/SecurityFilter.java b/src/main/java/org/opensearch/security/filter/SecurityFilter.java index f5424fd2ad..f4e005d6a2 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityFilter.java @@ -402,18 +402,14 @@ private void ap User finalUser = user; Consumer handleUnauthorized = response -> { auditLog.logMissingPrivileges(action, request, task); - String err; - if (!response.getMissingSecurityRoles().isEmpty()) { - err = String.format("No mapping for %s on roles %s", finalUser, response.getMissingSecurityRoles()); - } else { - err = (injectedRoles != null) - ? String.format( - "no permissions for %s and associated roles %s", - response.getMissingPrivileges(), - context.getMappedRoles() - ) - : String.format("no permissions for %s and %s", response.getMissingPrivileges(), finalUser); - } + String err = (injectedRoles != null) + ? String.format( + "no permissions for %s and associated roles %s", + response.getMissingPrivileges(), + context.getMappedRoles() + ) + : String.format("no permissions for %s and %s", response.getMissingPrivileges(), finalUser); + log.debug(err); listener.onFailure(new OpenSearchSecurityException(err, RestStatus.FORBIDDEN)); }; diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index 53cfdc03e0..d96e704e83 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -282,7 +282,7 @@ void authorizeRequest(RestHandler original, SecurityRequestChannel request, User .findFirst(); final boolean routeSupportsRestAuthorization = handler.isPresent() && handler.get() instanceof NamedRoute; if (routeSupportsRestAuthorization) { - PrivilegesEvaluatorResponse pres = new PrivilegesEvaluatorResponse(); + PrivilegesEvaluatorResponse pres; NamedRoute route = ((NamedRoute) handler.get()); // Check both route.actionNames() and route.name(). The presence of either is sufficient. Set actionNames = ImmutableSet.builder() @@ -299,12 +299,7 @@ void authorizeRequest(RestHandler original, SecurityRequestChannel request, User auditLog.logGrantedPrivileges(user.getName(), request); } else { auditLog.logMissingPrivileges(route.name(), user.getName(), request); - String err; - if (!pres.getMissingSecurityRoles().isEmpty()) { - err = String.format("No mapping for %s on roles %s", user, pres.getMissingSecurityRoles()); - } else { - err = String.format("no permissions for %s and %s", pres.getMissingPrivileges(), user); - } + String err = String.format("no permissions for %s and %s", pres.getMissingPrivileges(), user); log.debug(err); request.queueForSending(new SecurityResponse(HttpStatus.SC_UNAUTHORIZED, err)); diff --git a/src/main/java/org/opensearch/security/privileges/PitPrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PitPrivilegesEvaluator.java index 4fd4141b08..93cdb1d973 100644 --- a/src/main/java/org/opensearch/security/privileges/PitPrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/PitPrivilegesEvaluator.java @@ -40,12 +40,11 @@ public PrivilegesEvaluatorResponse evaluate( final PrivilegesEvaluationContext context, final ActionPrivileges actionPrivileges, final String action, - final PrivilegesEvaluatorResponse presponse, final IndexResolverReplacer irr ) { if (!(request instanceof DeletePitRequest || request instanceof PitSegmentsRequest)) { - return presponse; + return null; } List pitIds = new ArrayList<>(); @@ -58,9 +57,9 @@ public PrivilegesEvaluatorResponse evaluate( } // if request is for all PIT IDs, skip custom pit ids evaluation if (pitIds.size() == 1 && "_all".equals(pitIds.get(0))) { - return presponse; + return null; } else { - return handlePitsAccess(pitIds, context, actionPrivileges, action, presponse, irr); + return handlePitsAccess(pitIds, context, actionPrivileges, action, irr); } } @@ -72,7 +71,6 @@ private PrivilegesEvaluatorResponse handlePitsAccess( PrivilegesEvaluationContext context, ActionPrivileges actionPrivileges, final String action, - PrivilegesEvaluatorResponse presponse, final IndexResolverReplacer irr ) { Map pitToIndicesMap = OpenSearchSecurityPlugin.GuiceHolder.getPitService().getIndicesForPits(pitIds); @@ -87,10 +85,9 @@ private PrivilegesEvaluatorResponse handlePitsAccess( PrivilegesEvaluatorResponse subResponse = actionPrivileges.hasIndexPrivilege(context, ImmutableSet.of(action), pitResolved); // Only if user has access to all PIT's indices, allow operation, otherwise continue evaluation in PrivilegesEvaluator. if (subResponse.isAllowed()) { - presponse.allowed = true; - presponse.markComplete(); + return PrivilegesEvaluatorResponse.ok(); } - return presponse; + return null; } } diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorImpl.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorImpl.java index 3880f16dfe..558fb6e6ad 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorImpl.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorImpl.java @@ -307,7 +307,7 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context) action0 = PutMappingAction.NAME; } - PrivilegesEvaluatorResponse presponse = new PrivilegesEvaluatorResponse(); + PrivilegesEvaluatorResponse presponse; final boolean isDebugEnabled = log.isDebugEnabled(); if (isDebugEnabled) { @@ -328,7 +328,7 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context) presponse = actionPrivileges.hasClusterPrivilege(context, action0); - if (!presponse.allowed) { + if (!presponse.isAllowed()) { log.info( "No cluster-level perm match for {} [Action [{}]] [RolesChecked {}]. No permissions for {}", user, @@ -347,23 +347,26 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context) // check snapshot/restore requests // NOTE: Has to go first as restore request could be for protected and/or system indices and the request may // fail with 403 if system index or protected index evaluators are triggered first - if (snapshotRestoreEvaluator.evaluate(request, task, action0, presponse).isComplete()) { + presponse = snapshotRestoreEvaluator.evaluate(request, task, action0); + if (presponse != null) { return presponse; } // System index access - if (systemIndexAccessEvaluator.evaluate(request, task, action0, requestedResolved, presponse, context, actionPrivileges, user) - .isComplete()) { + presponse = systemIndexAccessEvaluator.evaluate(request, task, action0, requestedResolved, context, actionPrivileges, user); + if (presponse != null) { return presponse; } // Protected index access - if (protectedIndexAccessEvaluator.evaluate(request, task, action0, requestedResolved, presponse, mappedRoles).isComplete()) { + presponse = protectedIndexAccessEvaluator.evaluate(request, task, action0, requestedResolved, mappedRoles); + if (presponse != null) { return presponse; } // check access for point in time requests - if (pitPrivilegesEvaluator.evaluate(request, context, actionPrivileges, action0, presponse, irr).isComplete()) { + presponse = pitPrivilegesEvaluator.evaluate(request, context, actionPrivileges, action0, irr); + if (presponse != null) { return presponse; } @@ -380,7 +383,7 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context) presponse = actionPrivileges.hasClusterPrivilege(context, action0); - if (!presponse.allowed) { + if (!presponse.isAllowed()) { log.info( "No cluster-level perm match for {} {} [Action [{}]] [RolesChecked {}]. No permissions for {}", user, @@ -412,28 +415,25 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context) if (replaceResult.accessDenied) { auditLog.logMissingPrivileges(action0, request, task); } else { - presponse.allowed = true; - presponse.createIndexRequestBuilder = replaceResult.createIndexRequestBuilder; + return PrivilegesEvaluatorResponse.ok().with(replaceResult.createIndexRequestBuilder); } - return presponse; } } log.debug("Allowed because we have cluster permissions for {}", action0); - presponse.allowed = true; - return presponse; + return PrivilegesEvaluatorResponse.ok(); } } } if (checkDocAllowListHeader(user, action0, request)) { - presponse.allowed = true; - return presponse; + return PrivilegesEvaluatorResponse.ok(); } // term aggregations - if (termsAggregationEvaluator.evaluate(requestedResolved, request, context, actionPrivileges, presponse).isComplete()) { + presponse = termsAggregationEvaluator.evaluate(requestedResolved, request, context, actionPrivileges); + if (presponse != null) { return presponse; } @@ -462,9 +462,7 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context) auditLog.logMissingPrivileges(action0, request, task); return PrivilegesEvaluatorResponse.insufficient(action0); } else { - presponse.allowed = true; - presponse.createIndexRequestBuilder = replaceResult.createIndexRequestBuilder; - return presponse; + return PrivilegesEvaluatorResponse.ok().with(replaceResult.createIndexRequestBuilder); } } } @@ -497,8 +495,7 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context) if (presponse.isAllowed()) { if (checkFilteredAliases(requestedResolved, action0, isDebugEnabled)) { - presponse.allowed = false; - return presponse; + return PrivilegesEvaluatorResponse.insufficient(action0); } log.debug("Allowed because we have all indices permissions for {}", action0); diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java index d072ec301c..574200b3d7 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java @@ -26,13 +26,11 @@ package org.opensearch.security.privileges; -import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.HashSet; -import java.util.List; import java.util.Set; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import org.opensearch.action.admin.indices.create.CreateIndexRequestBuilder; @@ -40,19 +38,49 @@ import com.selectivem.collections.CheckTable; public class PrivilegesEvaluatorResponse { - boolean allowed = false; - Set missingSecurityRoles = new HashSet<>(); - PrivilegesEvaluatorResponseState state = PrivilegesEvaluatorResponseState.PENDING; - CreateIndexRequestBuilder createIndexRequestBuilder; - private Set onlyAllowedForIndices = ImmutableSet.of(); - private CheckTable indexToActionCheckTable; + private final boolean allowed; + private final CreateIndexRequestBuilder createIndexRequestBuilder; + private final ImmutableSet onlyAllowedForIndices; + private final CheckTable indexToActionCheckTable; private String privilegeMatrix; - private String reason; + private final String reason; /** * Contains issues that were encountered during privilege evaluation. Can be used for logging. */ - private List evaluationExceptions = new ArrayList<>(); + private ImmutableList evaluationExceptions; + + public PrivilegesEvaluatorResponse( + boolean allowed, + ImmutableSet onlyAllowedForIndices, + CheckTable indexToActionCheckTable, + String privilegeMatrix, + String reason, + ImmutableList evaluationExceptions, + CreateIndexRequestBuilder createIndexRequestBuilder + ) { + this.allowed = allowed; + this.createIndexRequestBuilder = createIndexRequestBuilder; + this.onlyAllowedForIndices = onlyAllowedForIndices; + this.indexToActionCheckTable = indexToActionCheckTable; + this.privilegeMatrix = privilegeMatrix; + this.reason = reason; + this.evaluationExceptions = evaluationExceptions; + } + + public PrivilegesEvaluatorResponse( + boolean allowed, + ImmutableSet onlyAllowedForIndices, + CheckTable indexToActionCheckTable + ) { + this.allowed = allowed; + this.createIndexRequestBuilder = null; + this.onlyAllowedForIndices = onlyAllowedForIndices; + this.indexToActionCheckTable = indexToActionCheckTable; + this.privilegeMatrix = null; + this.reason = null; + this.evaluationExceptions = ImmutableList.of(); + } /** * Returns true if the request can be fully allowed. See also isAllowedForSpecificIndices(). @@ -93,8 +121,15 @@ public String getReason() { } public PrivilegesEvaluatorResponse reason(String reason) { - this.reason = reason; - return this; + return new PrivilegesEvaluatorResponse( + this.allowed, + this.onlyAllowedForIndices, + this.indexToActionCheckTable, + this.privilegeMatrix, + reason, + this.evaluationExceptions, + this.createIndexRequestBuilder + ); } /** @@ -115,8 +150,18 @@ public boolean hasEvaluationExceptions() { } public PrivilegesEvaluatorResponse evaluationExceptions(Collection evaluationExceptions) { - this.evaluationExceptions.addAll(evaluationExceptions); - return this; + if (evaluationExceptions.isEmpty()) { + return this; + } + return new PrivilegesEvaluatorResponse( + this.allowed, + this.onlyAllowedForIndices, + this.indexToActionCheckTable, + this.privilegeMatrix, + this.reason, + ImmutableList.builder().addAll(this.evaluationExceptions).addAll(evaluationExceptions).build(), + this.createIndexRequestBuilder + ); } /** @@ -133,30 +178,24 @@ public String getPrivilegeMatrix() { return result; } - public Set getMissingSecurityRoles() { - return new HashSet<>(missingSecurityRoles); - } - public CreateIndexRequestBuilder getCreateIndexRequestBuilder() { return createIndexRequestBuilder; } - public PrivilegesEvaluatorResponse markComplete() { - this.state = PrivilegesEvaluatorResponseState.COMPLETE; - return this; - } - - public PrivilegesEvaluatorResponse markPending() { - this.state = PrivilegesEvaluatorResponseState.PENDING; - return this; - } - - public boolean isComplete() { - return this.state == PrivilegesEvaluatorResponseState.COMPLETE; - } + public PrivilegesEvaluatorResponse with(CreateIndexRequestBuilder createIndexRequestBuilder) { + if (createIndexRequestBuilder == this.createIndexRequestBuilder) { + return this; + } - public boolean isPending() { - return this.state == PrivilegesEvaluatorResponseState.PENDING; + return new PrivilegesEvaluatorResponse( + this.allowed, + this.onlyAllowedForIndices, + this.indexToActionCheckTable, + this.privilegeMatrix, + this.reason, + this.evaluationExceptions, + createIndexRequestBuilder + ); } @Override @@ -171,36 +210,25 @@ public String toString() { } public static PrivilegesEvaluatorResponse ok() { - PrivilegesEvaluatorResponse response = new PrivilegesEvaluatorResponse(); - response.allowed = true; - return response; + return new PrivilegesEvaluatorResponse(true, ImmutableSet.of(), null); } public static PrivilegesEvaluatorResponse partiallyOk( Set availableIndices, CheckTable indexToActionCheckTable ) { - PrivilegesEvaluatorResponse response = new PrivilegesEvaluatorResponse(); - response.onlyAllowedForIndices = ImmutableSet.copyOf(availableIndices); - response.indexToActionCheckTable = indexToActionCheckTable; - return response; + return new PrivilegesEvaluatorResponse(false, ImmutableSet.copyOf(availableIndices), indexToActionCheckTable); } public static PrivilegesEvaluatorResponse insufficient(String missingPrivilege) { - PrivilegesEvaluatorResponse response = new PrivilegesEvaluatorResponse(); - response.indexToActionCheckTable = CheckTable.create(ImmutableSet.of("_"), ImmutableSet.of(missingPrivilege)); - return response; + return new PrivilegesEvaluatorResponse( + false, + ImmutableSet.of(), + CheckTable.create(ImmutableSet.of("_"), ImmutableSet.of(missingPrivilege)) + ); } public static PrivilegesEvaluatorResponse insufficient(CheckTable indexToActionCheckTable) { - PrivilegesEvaluatorResponse response = new PrivilegesEvaluatorResponse(); - response.indexToActionCheckTable = indexToActionCheckTable; - return response; + return new PrivilegesEvaluatorResponse(false, ImmutableSet.of(), indexToActionCheckTable); } - - public static enum PrivilegesEvaluatorResponseState { - PENDING, - COMPLETE; - } - } diff --git a/src/main/java/org/opensearch/security/privileges/ProtectedIndexAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/ProtectedIndexAccessEvaluator.java index 877e6fd787..05d431ccc8 100644 --- a/src/main/java/org/opensearch/security/privileges/ProtectedIndexAccessEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/ProtectedIndexAccessEvaluator.java @@ -67,16 +67,18 @@ public ProtectedIndexAccessEvaluator(final Settings settings, AuditLog auditLog) this.deniedActionMatcher = WildcardMatcher.from(indexDeniedActionPatterns); } + /** + * @return a PrivilegesEvaluatorResponse if the evaluation process is completed here, null otherwise + */ public PrivilegesEvaluatorResponse evaluate( final ActionRequest request, final Task task, final String action, final IndexResolverReplacer.Resolved requestedResolved, - final PrivilegesEvaluatorResponse presponse, final Set mappedRoles ) { if (!protectedIndexEnabled) { - return presponse; + return null; } if (!requestedResolved.isLocalAll() && indexMatcher.matchAny(requestedResolved.getAllIndices()) @@ -84,15 +86,13 @@ public PrivilegesEvaluatorResponse evaluate( && !allowedRolesMatcher.matchAny(mappedRoles)) { auditLog.logMissingPrivileges(action, request, task); log.warn("{} for '{}' index/indices is not allowed for a regular user", action, indexMatcher); - presponse.allowed = false; - return presponse.markComplete(); + return PrivilegesEvaluatorResponse.insufficient(""); } if (requestedResolved.isLocalAll() && deniedActionMatcher.test(action) && !allowedRolesMatcher.matchAny(mappedRoles)) { auditLog.logMissingPrivileges(action, request, task); log.warn("{} for '_all' indices is not allowed for a regular user", action); - presponse.allowed = false; - return presponse.markComplete(); + return PrivilegesEvaluatorResponse.insufficient(""); } if ((requestedResolved.isLocalAll() || indexMatcher.matchAny(requestedResolved.getAllIndices())) && !allowedRolesMatcher.matchAny(mappedRoles)) { @@ -112,6 +112,6 @@ public PrivilegesEvaluatorResponse evaluate( } } } - return presponse; + return null; } } diff --git a/src/main/java/org/opensearch/security/privileges/ResourceAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/ResourceAccessEvaluator.java index b83e174600..95ff4d6482 100644 --- a/src/main/java/org/opensearch/security/privileges/ResourceAccessEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/ResourceAccessEvaluator.java @@ -78,8 +78,6 @@ public void evaluateAsync( final String action, final ActionListener pResponseListener ) { - PrivilegesEvaluatorResponse pResponse = new PrivilegesEvaluatorResponse(); - log.debug("Evaluating resource access"); // if it reached this evaluator, it is safe to assume that the request if of DocRequest type @@ -87,12 +85,11 @@ public void evaluateAsync( resourceAccessHandler.hasPermission(req.id(), req.type(), action, ActionListener.wrap(hasAccess -> { if (hasAccess) { - pResponse.allowed = true; - pResponseListener.onResponse(pResponse.markComplete()); - return; + pResponseListener.onResponse(PrivilegesEvaluatorResponse.ok()); + } else { + pResponseListener.onResponse(PrivilegesEvaluatorResponse.insufficient(action)); } - pResponseListener.onResponse(PrivilegesEvaluatorResponse.insufficient(action).markComplete()); - }, e -> { pResponseListener.onResponse(pResponse.markComplete()); })); + }, e -> { pResponseListener.onResponse(PrivilegesEvaluatorResponse.insufficient(action)); })); } /** diff --git a/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java index 4890dd5a89..36e32d650a 100644 --- a/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java @@ -38,7 +38,7 @@ public PrivilegesEvaluatorResponse evaluate(final User user, final String routeN PrivilegesEvaluatorResponse result = context.getActionPrivileges().hasAnyClusterPrivilege(context, actions); - if (!result.allowed) { + if (!result.isAllowed()) { log.info( "No permission match for {} [Action [{}]] [RolesChecked {}]. No permissions for {}", user, diff --git a/src/main/java/org/opensearch/security/privileges/SnapshotRestoreEvaluator.java b/src/main/java/org/opensearch/security/privileges/SnapshotRestoreEvaluator.java index ca5cf985d3..ca56f81df4 100644 --- a/src/main/java/org/opensearch/security/privileges/SnapshotRestoreEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/SnapshotRestoreEvaluator.java @@ -68,34 +68,29 @@ public SnapshotRestoreEvaluator( this.isLocalNodeElectedClusterManagerSupplier = isLocalNodeElectedClusterManagerSupplier; } - public PrivilegesEvaluatorResponse evaluate( - final ActionRequest request, - final Task task, - final String action, - final PrivilegesEvaluatorResponse presponse - ) { + /** + * @return a PrivilegesEvaluatorResponse if the evaluation process is completed here, null otherwise + */ + public PrivilegesEvaluatorResponse evaluate(final ActionRequest request, final Task task, final String action) { if (!(request instanceof RestoreSnapshotRequest)) { - return presponse; + return null; } // snapshot restore for regular users not enabled if (!enableSnapshotRestorePrivilege) { log.warn("{} is not allowed for a regular user", action); - presponse.allowed = false; - return presponse.markComplete(); + return PrivilegesEvaluatorResponse.insufficient(action); } // if this feature is enabled, users can also snapshot and restore // the Security index and the global state if (restoreSecurityIndexEnabled) { - presponse.allowed = true; - return presponse; + return null; } if (!isLocalNodeElectedClusterManagerSupplier.get()) { - presponse.allowed = true; - return presponse.markComplete(); + return PrivilegesEvaluatorResponse.ok(); } final RestoreSnapshotRequest restoreRequest = (RestoreSnapshotRequest) request; @@ -104,8 +99,7 @@ public PrivilegesEvaluatorResponse evaluate( if (restoreRequest.includeGlobalState()) { auditLog.logSecurityIndexAttempt(request, action, task); log.warn("{} with 'include_global_state' enabled is not allowed", action); - presponse.allowed = false; - return presponse.markComplete(); + return PrivilegesEvaluatorResponse.insufficient(action).reason("'include_global_state' is not allowed"); } final List rs = SnapshotRestoreHelper.resolveOriginalIndices(restoreRequest); @@ -113,9 +107,8 @@ public PrivilegesEvaluatorResponse evaluate( if (rs != null && (rs.contains(securityIndex) || rs.contains("_all") || rs.contains("*"))) { auditLog.logSecurityIndexAttempt(request, action, task); log.warn("{} for '{}' as source index is not allowed", action, securityIndex); - presponse.allowed = false; - return presponse.markComplete(); + return PrivilegesEvaluatorResponse.insufficient("").reason(securityIndex + " as source index is not allowed"); } - return presponse; + return null; } } diff --git a/src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java index fb28eab972..a2c87b6367 100644 --- a/src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java @@ -123,17 +123,30 @@ private static List deniedActionPatterns() { return securityIndexDeniedActionPatternsList; } + /** + * @return a PrivilegesEvaluatorResponse if the evaluation process is completed here, null otherwise + */ public PrivilegesEvaluatorResponse evaluate( final ActionRequest request, final Task task, final String action, final Resolved requestedResolved, - final PrivilegesEvaluatorResponse presponse, final PrivilegesEvaluationContext context, final ActionPrivileges actionPrivileges, final User user ) { - evaluateSystemIndicesAccess(action, requestedResolved, request, task, presponse, context, actionPrivileges, user); + PrivilegesEvaluatorResponse presponse = evaluateSystemIndicesAccess( + action, + requestedResolved, + request, + task, + context, + actionPrivileges, + user + ); + if (presponse != null && !presponse.isAllowed()) { + return presponse; + } if (requestedResolved.isLocalAll() || requestedResolved.getAllIndices().contains(securityIndex) @@ -234,17 +247,15 @@ private boolean isActionAllowed(String action) { * @param requestedResolved this object contains all indices this request is resolved to * @param request the action request to be used for audit logging * @param task task in which this access check will be performed - * @param presponse the pre-response object that will eventually become a response and returned to the requester * @param context conveys information about user and mapped roles, etc. * @param actionPrivileges the up-to-date ActionPrivileges instance * @param user this user's permissions will be looked up */ - private void evaluateSystemIndicesAccess( + private PrivilegesEvaluatorResponse evaluateSystemIndicesAccess( final String action, final Resolved requestedResolved, final ActionRequest request, final Task task, - final PrivilegesEvaluatorResponse presponse, final PrivilegesEvaluationContext context, final ActionPrivileges actionPrivileges, final User user @@ -269,9 +280,7 @@ private void evaluateSystemIndicesAccess( .collect(Collectors.toList()); log.debug("Service account cannot access regular indices: {}", regularIndices); } - presponse.allowed = false; - presponse.markComplete(); - return; + return PrivilegesEvaluatorResponse.insufficient("").reason("Service account cannot access regular indices"); } boolean containsProtectedIndex = requestContainsAnyProtectedSystemIndices(requestedResolved); if (containsProtectedIndex) { @@ -284,9 +293,7 @@ private void evaluateSystemIndicesAccess( String.join(", ", getAllProtectedSystemIndices(requestedResolved)) ); } - presponse.allowed = false; - presponse.markComplete(); - return; + return PrivilegesEvaluatorResponse.insufficient(""); } else if (containsSystemIndex && !actionPrivileges.hasExplicitIndexPrivilege(context, SYSTEM_INDEX_PERMISSION_SET, requestedResolved).isAllowed()) { auditLog.logSecurityIndexAttempt(request, action, task); @@ -298,9 +305,7 @@ private void evaluateSystemIndicesAccess( String.join(", ", getAllSystemIndices(requestedResolved)) ); } - presponse.allowed = false; - presponse.markComplete(); - return; + return PrivilegesEvaluatorResponse.insufficient(""); } } @@ -313,9 +318,7 @@ private void evaluateSystemIndicesAccess( ); if (requestedResolved.getAllIndices().equals(matchingPluginIndices)) { // plugin is authorized to perform any actions on its own registered system indices - presponse.allowed = true; - presponse.markComplete(); - return; + return PrivilegesEvaluatorResponse.ok(); } else { Set matchingSystemIndices = SystemIndexRegistry.matchesSystemIndexPattern(requestedResolved.getAllIndices()); matchingSystemIndices.removeAll(matchingPluginIndices); @@ -329,17 +332,12 @@ private void evaluateSystemIndicesAccess( matchingPluginIndices ); } - presponse.allowed = false; - presponse.getMissingPrivileges(); - presponse.markComplete(); - return; + return PrivilegesEvaluatorResponse.insufficient(""); } } } else { // no system index protection and request originating from plugin, allow - presponse.allowed = true; - presponse.markComplete(); - return; + return PrivilegesEvaluatorResponse.ok(); } } @@ -359,8 +357,7 @@ private void evaluateSystemIndicesAccess( } else { auditLog.logSecurityIndexAttempt(request, action, task); log.warn("{} for '_all' indices is not allowed for a regular user", action); - presponse.allowed = false; - presponse.markComplete(); + return PrivilegesEvaluatorResponse.insufficient(""); } } // if system index is enabled and system index permissions are enabled we don't need to perform any further @@ -373,9 +370,7 @@ else if (containsSystemIndex && !isSystemIndexPermissionEnabled) { if (log.isDebugEnabled()) { log.debug("Filtered '{}' but resulting list is empty", securityIndex); } - presponse.allowed = false; - presponse.markComplete(); - return; + return PrivilegesEvaluatorResponse.insufficient(""); } irr.replace(request, false, allWithoutSecurity.toArray(new String[0])); if (log.isDebugEnabled()) { @@ -385,10 +380,10 @@ else if (containsSystemIndex && !isSystemIndexPermissionEnabled) { auditLog.logSecurityIndexAttempt(request, action, task); final String foundSystemIndexes = String.join(", ", getAllSystemIndices(requestedResolved)); log.warn("{} for '{}' index is not allowed for a regular user", action, foundSystemIndexes); - presponse.allowed = false; - presponse.markComplete(); + return PrivilegesEvaluatorResponse.insufficient(""); } } } + return null; } } diff --git a/src/main/java/org/opensearch/security/privileges/TermsAggregationEvaluator.java b/src/main/java/org/opensearch/security/privileges/TermsAggregationEvaluator.java index a2cd1c16a7..f9de82f798 100644 --- a/src/main/java/org/opensearch/security/privileges/TermsAggregationEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/TermsAggregationEvaluator.java @@ -61,12 +61,14 @@ public class TermsAggregationEvaluator { public TermsAggregationEvaluator() {} + /** + * @return a PrivilegesEvaluatorResponse if the evaluation process is completed here, null otherwise + */ public PrivilegesEvaluatorResponse evaluate( final Resolved resolved, final ActionRequest request, PrivilegesEvaluationContext context, - ActionPrivileges actionPrivileges, - PrivilegesEvaluatorResponse presponse + ActionPrivileges actionPrivileges ) { try { if (request instanceof SearchRequest) { @@ -102,17 +104,16 @@ public PrivilegesEvaluatorResponse evaluate( sr.source().query(NONE_QUERY); } - presponse.allowed = true; - return presponse.markComplete(); + return PrivilegesEvaluatorResponse.ok(); } } } } } catch (Exception e) { log.warn("Unable to evaluate terms aggregation", e); - return presponse; + return null; } - return presponse; + return null; } } diff --git a/src/test/java/org/opensearch/security/privileges/ResourceAccessEvaluatorTest.java b/src/test/java/org/opensearch/security/privileges/ResourceAccessEvaluatorTest.java index ff70fe8d9f..b6c75ac435 100644 --- a/src/test/java/org/opensearch/security/privileges/ResourceAccessEvaluatorTest.java +++ b/src/test/java/org/opensearch/security/privileges/ResourceAccessEvaluatorTest.java @@ -87,8 +87,7 @@ private void assertEvaluateAsync(boolean hasPermission, boolean expectedAllowed) verify(callback).onResponse(captor.capture()); PrivilegesEvaluatorResponse out = captor.getValue(); - assertThat(out.allowed, equalTo(expectedAllowed)); - assertThat(out.isComplete(), equalTo(true)); + assertThat(out.isAllowed(), equalTo(expectedAllowed)); } @Test diff --git a/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java b/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java index bfb5719be7..311176c9a0 100644 --- a/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java +++ b/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java @@ -101,7 +101,7 @@ public void testEvaluate_Successful_NewPermission() throws Exception { PrivilegesConfiguration privilegesConfiguration = createPrivilegesConfiguration(roles); RestLayerPrivilegesEvaluator restPrivilegesEvaluator = new RestLayerPrivilegesEvaluator(privilegesConfiguration); PrivilegesEvaluatorResponse response = restPrivilegesEvaluator.evaluate(TEST_USER, "route_name", Set.of(action)); - assertThat(response.allowed, equalTo(true)); + assertThat(response.isAllowed(), equalTo(true)); } @Test @@ -113,7 +113,7 @@ public void testEvaluate_Successful_LegacyPermission() throws Exception { PrivilegesConfiguration privilegesConfiguration = createPrivilegesConfiguration(roles); RestLayerPrivilegesEvaluator restPrivilegesEvaluator = new RestLayerPrivilegesEvaluator(privilegesConfiguration); PrivilegesEvaluatorResponse response = restPrivilegesEvaluator.evaluate(TEST_USER, "route_name", Set.of(action)); - assertThat(response.allowed, equalTo(true)); + assertThat(response.isAllowed(), equalTo(true)); } @Test @@ -125,7 +125,7 @@ public void testEvaluate_Unsuccessful() throws Exception { PrivilegesConfiguration privilegesConfiguration = createPrivilegesConfiguration(roles); RestLayerPrivilegesEvaluator restPrivilegesEvaluator = new RestLayerPrivilegesEvaluator(privilegesConfiguration); PrivilegesEvaluatorResponse response = restPrivilegesEvaluator.evaluate(TEST_USER, "route_name", Set.of(action)); - assertThat(response.allowed, equalTo(false)); + assertThat(response.isAllowed(), equalTo(false)); } PrivilegesConfiguration createPrivilegesConfiguration(SecurityDynamicConfiguration roles) { diff --git a/src/test/java/org/opensearch/security/privileges/SystemIndexAccessEvaluatorTest.java b/src/test/java/org/opensearch/security/privileges/SystemIndexAccessEvaluatorTest.java index 17a78501c9..69e94cccca 100644 --- a/src/test/java/org/opensearch/security/privileges/SystemIndexAccessEvaluatorTest.java +++ b/src/test/java/org/opensearch/security/privileges/SystemIndexAccessEvaluatorTest.java @@ -54,11 +54,11 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; import static org.opensearch.security.support.ConfigConstants.SYSTEM_INDEX_PERMISSION; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; @@ -74,8 +74,6 @@ public class SystemIndexAccessEvaluatorTest { @Mock private Task task; @Mock - private PrivilegesEvaluatorResponse presponse; - @Mock private Logger log; @Mock ClusterService cs; @@ -181,7 +179,7 @@ PrivilegesEvaluationContext ctx(String action) { @After public void after() { - verifyNoMoreInteractions(auditLog, irr, request, task, presponse, log); + verifyNoMoreInteractions(auditLog, irr, request, task); } @Test @@ -195,13 +193,11 @@ public void testUnprotectedActionOnRegularIndex_systemIndexDisabled() { null, UNPROTECTED_ACTION, resolved, - presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user ); - verifyNoInteractions(presponse); - assertThat(response, is(presponse)); + assertThat(response, is(nullValue())); } @@ -216,13 +212,11 @@ public void testUnprotectedActionOnRegularIndex_systemIndexPermissionDisabled() null, UNPROTECTED_ACTION, resolved, - presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user ); - verifyNoInteractions(presponse); - assertThat(response, is(presponse)); + assertThat(response, is(nullValue())); } @Test @@ -236,13 +230,11 @@ public void testUnprotectedActionOnRegularIndex_systemIndexPermissionEnabled() { null, UNPROTECTED_ACTION, resolved, - presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user ); - verifyNoInteractions(presponse); - assertThat(response, is(presponse)); + assertThat(response, is(nullValue())); } @Test @@ -256,13 +248,11 @@ public void testUnprotectedActionOnSystemIndex_systemIndexDisabled() { null, UNPROTECTED_ACTION, resolved, - presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user ); - verifyNoInteractions(presponse); - assertThat(response, is(presponse)); + assertThat(response, is(nullValue())); } @Test @@ -276,13 +266,11 @@ public void testUnprotectedActionOnSystemIndex_systemIndexPermissionDisabled() { null, UNPROTECTED_ACTION, resolved, - presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user ); - verifyNoInteractions(presponse); - assertThat(response, is(presponse)); + assertThat(response, is(nullValue())); } @Test @@ -296,13 +284,11 @@ public void testUnprotectedActionOnSystemIndex_systemIndexPermissionEnabled_With null, UNPROTECTED_ACTION, resolved, - presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user ); - verify(presponse).markComplete(); - assertThat(response, is(presponse)); + assertThat(response.isAllowed(), is(false)); verify(auditLog).logSecurityIndexAttempt(request, UNPROTECTED_ACTION, null); verify(log).isInfoEnabled(); @@ -325,14 +311,12 @@ public void testUnprotectedActionOnSystemIndex_systemIndexPermissionEnabled_With null, UNPROTECTED_ACTION, resolved, - presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user ); - assertThat(response, is(presponse)); // unprotected action is not allowed on a system index - assertThat(presponse.allowed, is(false)); + assertThat(response, is(nullValue())); } @Test @@ -344,11 +328,20 @@ public void testDisableCacheOrRealtimeOnSystemIndex_systemIndexDisabled() { final Resolved resolved = createResolved(TEST_SYSTEM_INDEX); // Action - evaluator.evaluate(request, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); - evaluator.evaluate(searchRequest, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); - evaluator.evaluate(realtimeRequest, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); - - verifyNoInteractions(presponse); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + null, + UNPROTECTED_ACTION, + resolved, + ctx(UNPROTECTED_ACTION), + actionPrivileges, + user + ); + assertThat(response, is(nullValue())); + response = evaluator.evaluate(searchRequest, null, UNPROTECTED_ACTION, resolved, ctx(UNPROTECTED_ACTION), actionPrivileges, user); + assertThat(response, is(nullValue())); + response = evaluator.evaluate(realtimeRequest, null, UNPROTECTED_ACTION, resolved, ctx(UNPROTECTED_ACTION), actionPrivileges, user); + assertThat(response, is(nullValue())); } @Test @@ -360,9 +353,20 @@ public void testDisableCacheOrRealtimeOnSystemIndex_systemIndexPermissionDisable final Resolved resolved = createResolved(TEST_SYSTEM_INDEX); // Action - evaluator.evaluate(request, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); - evaluator.evaluate(searchRequest, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); - evaluator.evaluate(realtimeRequest, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + null, + UNPROTECTED_ACTION, + resolved, + ctx(UNPROTECTED_ACTION), + actionPrivileges, + user + ); + assertThat(response, is(nullValue())); + response = evaluator.evaluate(searchRequest, null, UNPROTECTED_ACTION, resolved, ctx(UNPROTECTED_ACTION), actionPrivileges, user); + assertThat(response, is(nullValue())); + response = evaluator.evaluate(realtimeRequest, null, UNPROTECTED_ACTION, resolved, ctx(UNPROTECTED_ACTION), actionPrivileges, user); + assertThat(response, is(nullValue())); verify(searchRequest).requestCache(Boolean.FALSE); verify(realtimeRequest).realtime(Boolean.FALSE); @@ -381,30 +385,30 @@ public void testDisableCacheOrRealtimeOnSystemIndex_systemIndexPermissionEnabled final Resolved resolved = createResolved(TEST_SYSTEM_INDEX); // Action - evaluator.evaluate(request, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); - evaluator.evaluate(searchRequest, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); - evaluator.evaluate(realtimeRequest, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); - - verify(searchRequest).requestCache(Boolean.FALSE); - verify(realtimeRequest).realtime(Boolean.FALSE); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + null, + UNPROTECTED_ACTION, + resolved, + ctx(UNPROTECTED_ACTION), + actionPrivileges, + user + ); + assertThat(response.isAllowed(), is(false)); + response = evaluator.evaluate(searchRequest, null, UNPROTECTED_ACTION, resolved, ctx(UNPROTECTED_ACTION), actionPrivileges, user); + assertThat(response.isAllowed(), is(false)); + response = evaluator.evaluate(realtimeRequest, null, UNPROTECTED_ACTION, resolved, ctx(UNPROTECTED_ACTION), actionPrivileges, user); + assertThat(response.isAllowed(), is(false)); - verify(log, times(2)).isDebugEnabled(); - verify(log).debug("Disable search request cache for this request"); - verify(log).debug("Disable realtime for this request"); verify(auditLog).logSecurityIndexAttempt(request, UNPROTECTED_ACTION, null); verify(auditLog).logSecurityIndexAttempt(searchRequest, UNPROTECTED_ACTION, null); verify(auditLog).logSecurityIndexAttempt(realtimeRequest, UNPROTECTED_ACTION, null); - verify(presponse, times(3)).markComplete(); - verify(log, times(2)).isDebugEnabled(); - verify(log, times(3)).isInfoEnabled(); verify(log, times(3)).info( "No {} permission for user roles {} to System Indices {}", UNPROTECTED_ACTION, user.getSecurityRoles(), TEST_SYSTEM_INDEX ); - verify(log).debug("Disable search request cache for this request"); - verify(log).debug("Disable realtime for this request"); } @Test @@ -416,14 +420,24 @@ public void testDisableCacheOrRealtimeOnSystemIndex_systemIndexPermissionEnabled final Resolved resolved = createResolved(TEST_SYSTEM_INDEX); // Action - evaluator.evaluate(request, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); - evaluator.evaluate(searchRequest, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); - evaluator.evaluate(realtimeRequest, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + null, + UNPROTECTED_ACTION, + resolved, + ctx(UNPROTECTED_ACTION), + actionPrivileges, + user + ); + assertThat(response, is(nullValue())); + response = evaluator.evaluate(searchRequest, null, UNPROTECTED_ACTION, resolved, ctx(UNPROTECTED_ACTION), actionPrivileges, user); + assertThat(response, is(nullValue())); + response = evaluator.evaluate(realtimeRequest, null, UNPROTECTED_ACTION, resolved, ctx(UNPROTECTED_ACTION), actionPrivileges, user); + assertThat(response, is(nullValue())); verify(searchRequest).requestCache(Boolean.FALSE); verify(realtimeRequest).realtime(Boolean.FALSE); - verify(log, times(2)).isDebugEnabled(); verify(log).debug("Disable search request cache for this request"); verify(log).debug("Disable realtime for this request"); } @@ -434,11 +448,18 @@ public void testProtectedActionLocalAll_systemIndexDisabled() { final Resolved resolved = Resolved._LOCAL_ALL; // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); verify(auditLog).logSecurityIndexAttempt(request, PROTECTED_ACTION, task); - assertThat(presponse.allowed, is(false)); - verify(presponse).markComplete(); + assertThat(response.isAllowed(), is(false)); verify(log).warn("{} for '_all' indices is not allowed for a regular user", "indices:data/write"); } @@ -448,11 +469,18 @@ public void testProtectedActionLocalAll_systemIndexPermissionDisabled() { final Resolved resolved = Resolved._LOCAL_ALL; // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); verify(auditLog).logSecurityIndexAttempt(request, PROTECTED_ACTION, task); - assertThat(presponse.allowed, is(false)); - verify(presponse).markComplete(); + assertThat(response.isAllowed(), is(false)); verify(log).warn("{} for '_all' indices is not allowed for a regular user", PROTECTED_ACTION); } @@ -462,11 +490,18 @@ public void testProtectedActionLocalAll_systemIndexPermissionEnabled() { final Resolved resolved = Resolved._LOCAL_ALL; // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); verify(auditLog).logSecurityIndexAttempt(request, PROTECTED_ACTION, task); - assertThat(presponse.allowed, is(false)); - verify(presponse).markComplete(); + assertThat(response.isAllowed(), is(false)); verify(log).warn("{} for '_all' indices is not allowed for a regular user", PROTECTED_ACTION); } @@ -476,9 +511,17 @@ public void testProtectedActionOnRegularIndex_systemIndexDisabled() { final Resolved resolved = createResolved(TEST_INDEX); // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); - assertThat(presponse.allowed, is(false)); + assertThat(response, is(nullValue())); } @Test @@ -487,9 +530,17 @@ public void testProtectedActionOnRegularIndex_systemIndexPermissionDisabled() { final Resolved resolved = createResolved(TEST_INDEX); // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); - assertThat(presponse.allowed, is(false)); + assertThat(response, is(nullValue())); } @Test @@ -498,9 +549,17 @@ public void testProtectedActionOnRegularIndex_systemIndexPermissionEnabled() { final Resolved resolved = createResolved(TEST_INDEX); // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); - assertThat(presponse.allowed, is(false)); + assertThat(response, is(nullValue())); } @Test @@ -509,9 +568,17 @@ public void testProtectedActionOnSystemIndex_systemIndexDisabled() { final Resolved resolved = createResolved(TEST_SYSTEM_INDEX); // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); - assertThat(presponse.allowed, is(false)); + assertThat(response, is(nullValue())); } @Test @@ -520,11 +587,18 @@ public void testProtectedActionOnSystemIndex_systemIndexPermissionDisabled() { final Resolved resolved = createResolved(TEST_SYSTEM_INDEX); // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); verify(auditLog).logSecurityIndexAttempt(request, PROTECTED_ACTION, task); - assertThat(presponse.allowed, is(false)); - verify(presponse).markComplete(); + assertThat(response.isAllowed(), is(false)); verify(log).warn("{} for '{}' index is not allowed for a regular user", PROTECTED_ACTION, TEST_SYSTEM_INDEX); } @@ -534,11 +608,18 @@ public void testProtectedActionOnSystemIndex_systemIndexPermissionEnabled_withou final Resolved resolved = createResolved(TEST_SYSTEM_INDEX); // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); verify(auditLog).logSecurityIndexAttempt(request, PROTECTED_ACTION, task); - assertThat(presponse.allowed, is(false)); - verify(presponse).markComplete(); + assertThat(response.isAllowed(), is(false)); verify(log).isInfoEnabled(); verify(log).info( "No {} permission for user roles {} to System Indices {}", @@ -555,9 +636,17 @@ public void testProtectedActionOnSystemIndex_systemIndexPermissionEnabled_withSy final Resolved resolved = createResolved(TEST_SYSTEM_INDEX); // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); - assertThat(presponse.allowed, is(false)); + assertThat(response, is(nullValue())); } @Test @@ -566,11 +655,18 @@ public void testProtectedActionOnProtectedSystemIndex_systemIndexDisabled() { final Resolved resolved = createResolved(SECURITY_INDEX); // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); verify(auditLog).logSecurityIndexAttempt(request, PROTECTED_ACTION, task); - assertThat(presponse.allowed, is(false)); - verify(presponse).markComplete(); + assertThat(response.isAllowed(), is(false)); verify(log).warn("{} for '{}' index is not allowed for a regular user", PROTECTED_ACTION, SECURITY_INDEX); } @@ -581,11 +677,18 @@ public void testProtectedActionOnProtectedSystemIndex_systemIndexPermissionDisab final Resolved resolved = createResolved(SECURITY_INDEX); // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); verify(auditLog).logSecurityIndexAttempt(request, PROTECTED_ACTION, task); - assertThat(presponse.allowed, is(false)); - verify(presponse).markComplete(); + assertThat(response.isAllowed(), is(false)); verify(log).warn("{} for '{}' index is not allowed for a regular user", PROTECTED_ACTION, SECURITY_INDEX); } @@ -616,11 +719,10 @@ private void testSecurityIndexAccess(String action) { final Resolved resolved = createResolved(SECURITY_INDEX); // Action - evaluator.evaluate(request, task, action, resolved, presponse, ctx(action), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate(request, task, action, resolved, ctx(action), actionPrivileges, user); verify(auditLog).logSecurityIndexAttempt(request, action, task); - assertThat(presponse.allowed, is(false)); - verify(presponse).markComplete(); + assertThat(response.isAllowed(), is(false)); verify(log).isInfoEnabled(); verify(log).info( From 190b7c752871807bfe6f346726e668b10e4028c8 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 25 Nov 2025 15:41:15 -0500 Subject: [PATCH 10/10] Allow overlap of static and custom security configs, but prefer static (#5805) Signed-off-by: Craig Perkins --- CHANGELOG.md | 2 + .../securityconf/DynamicConfigFactory.java | 67 ++++++++++++------- 2 files changed, 43 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b2d898f8ac..cc07ec4c0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Changed - Ensure all restHeaders from ActionPlugin.getRestHeaders are carried to threadContext for tracing ([#5396](https://github.com/opensearch-project/security/pull/5396)) +- Allow overlap of static and custom security configs, but prefer static ([#5805](https://github.com/opensearch-project/security/pull/5805)) + ### Features ### Enhancements diff --git a/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java b/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java index cb124d8c51..f89c215899 100644 --- a/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java +++ b/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java @@ -240,32 +240,9 @@ public void onChange(ConfigurationMap typeToConfig) { final AllowlistingSettings allowlist = cr.getConfiguration(CType.ALLOWLIST).getCEntry("config"); final AuditConfig audit = cr.getConfiguration(CType.AUDIT).getCEntry("config"); - if (roles.containsAny(staticRoles)) { - throw new StaticResourceException("Cannot override static roles"); - } - if (!roles.add(staticRoles) && !staticRoles.getCEntries().isEmpty()) { - throw new StaticResourceException("Unable to load static roles"); - } - - log.debug("Static roles loaded ({})", staticRoles.getCEntries().size()); - - if (actionGroups.containsAny(staticActionGroups)) { - throw new StaticResourceException("Cannot override static action groups"); - } - if (!actionGroups.add(staticActionGroups) && !staticActionGroups.getCEntries().isEmpty()) { - throw new StaticResourceException("Unable to load static action groups"); - } - - log.debug("Static action groups loaded ({})", staticActionGroups.getCEntries().size()); - - if (tenants.containsAny(staticTenants)) { - throw new StaticResourceException("Cannot override static tenants"); - } - if (!tenants.add(staticTenants) && !staticTenants.getCEntries().isEmpty()) { - throw new StaticResourceException("Unable to load static tenants"); - } - - log.debug("Static tenants loaded ({})", staticTenants.getCEntries().size()); + mergeStaticConfigWithWarning("roles", roles, staticRoles, log); + mergeStaticConfigWithWarning("action groups", actionGroups, staticActionGroups, log); + mergeStaticConfigWithWarning("tenants", tenants, staticTenants, log); log.debug( "Static configuration loaded (total roles: {}/total action groups: {}/total tenants: {})", @@ -292,6 +269,44 @@ public void onChange(ConfigurationMap typeToConfig) { initialized.set(true); } + private static void mergeStaticConfigWithWarning( + String typeName, + SecurityDynamicConfiguration dynamicConfig, + SecurityDynamicConfiguration staticConfig, + Logger log + ) { + if (staticConfig == null || staticConfig.isEmpty()) { + return; + } + + // Find overlapping keys between dynamic and static configs + final Map dynamicEntries = dynamicConfig.getCEntries(); + final Map staticEntries = staticConfig.getCEntries(); + + final java.util.List overlaps = dynamicEntries.keySet().stream().filter(staticEntries::containsKey).sorted().toList(); + + if (!overlaps.isEmpty()) { + // Remove overlaps from the dynamic config so static definitions win + dynamicConfig.remove(overlaps); + + log.warn( + "Detected overlap between dynamic {} configuration and static resources for entries: {}. " + + "Dynamic definitions have been removed in favor of static configuration.", + typeName, + overlaps + ); + } + + // Now add static entries; if this fails, it's a real structural problem (version/type) + if (!dynamicConfig.add(staticConfig) && !staticConfig.getCEntries().isEmpty()) { + throw new StaticResourceException("Unable to load static " + typeName); + } + + if (log.isDebugEnabled()) { + log.debug("Static {} loaded ({} entries)", typeName, staticConfig.getCEntries().size()); + } + } + private static ConfigV7 getConfigV7(SecurityDynamicConfiguration sdc) { @SuppressWarnings("unchecked") SecurityDynamicConfiguration c = (SecurityDynamicConfiguration) sdc;