Skip to content

Copy scripts from tools to bin/ directory in assembly and make scripts agnostic to directory#6023

Open
cwperks wants to merge 10 commits intoopensearch-project:mainfrom
cwperks:move-tools-to-bin
Open

Copy scripts from tools to bin/ directory in assembly and make scripts agnostic to directory#6023
cwperks wants to merge 10 commits intoopensearch-project:mainfrom
cwperks:move-tools-to-bin

Conversation

@cwperks
Copy link
Copy Markdown
Member

@cwperks cwperks commented Mar 19, 2026

Description

This PR fixes the packaging of the OpenSearch Security tools scripts so they retain executable permissions after plugin installation.

Today, scripts under <OPENSEARCH_HOME>/plugins/opensearch-security/tools can lose their execute bit during plugin installation. As a result, cluster administrators must manually update file permissions before they can run them.

i.e.

> ./install_demo_configuration.sh
zsh: permission denied: ./install_demo_configuration.sh
> sudo chmod 755 *
> ./install_demo_configuration.sh

### OpenSearch Security Demo Installer
### ** Warning: Do not use on production or public reachable systems **
...

With the changes in this PR we are going to copy the tools into another directory called bin/ on assembly which OpenSearch core nows how to handle on plugin install to copy these to <OPENSEARCH_HOME>/bin/opensearch-security/ and keep the file's permissions as they are (755).

In order to do this, I also have to make the scripts agnostic to directory since they are now one level less deep so there's logic introduced to identify OPENSEARCH_HOME directory by looking for the lib directory and OpenSearch jar.

Yes this means there will be copies of each script (left intentionally until all places around the ecosystem are updated like Docker entrypoint).

  • Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
  • Why these changes are required?
  • What is the old behavior before changes and new behavior after changes?

Issues Resolved

Enhancement

Check List

  • New functionality includes testing
  • New functionality has been documented
  • New Roles/Permissions have a corresponding security dashboards plugin PR
  • API changes companion pull request created
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…s agnostic to directory

Signed-off-by: Craig Perkins <cwperx@amazon.com>
cwperks added 5 commits March 19, 2026 12:22
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.78%. Comparing base (88c407c) to head (a5f85ee).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6023      +/-   ##
==========================================
- Coverage   74.80%   74.78%   -0.02%     
==========================================
  Files         447      447              
  Lines       28463    28474      +11     
  Branches     4328     4330       +2     
==========================================
+ Hits        21291    21295       +4     
- Misses       5176     5184       +8     
+ Partials     1996     1995       -1     
Files with missing lines Coverage Δ
...pensearch/security/tools/democonfig/Installer.java 74.01% <100.00%> (+1.12%) ⬆️

... and 8 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

cwperks added 2 commits March 19, 2026 22:27
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
@cwperks cwperks requested a review from finnegancarroll as a code owner April 1, 2026 10:59
@github-actions
Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: Make shell and batch scripts directory-agnostic using OPENSEARCH_HOME discovery

Relevant files:

  • tools/install_demo_configuration.sh
  • tools/securityadmin.sh
  • tools/hash.sh
  • tools/audit_config_migrater.sh
  • tools/install_demo_configuration.bat
  • tools/securityadmin.bat
  • tools/hash.bat
  • tools/audit_config_migrater.bat

Sub-PR theme: Update Installer.java BASE_DIR logic to use SCRIPT_DIR directly

Relevant files:

  • src/main/java/org/opensearch/security/tools/democonfig/Installer.java
  • src/test/java/org/opensearch/security/tools/democonfig/InstallerTests.java

⚡ Recommended focus areas for review

Path Separator Bug

The bundled JDK path is set as %OPENSEARCH_HOME%jdk\bin\java.exe (missing backslash between OPENSEARCH_HOME and jdk). Since OPENSEARCH_HOME is set from %DIR% which ends with a backslash (as %~dp0 always includes trailing backslash), this may work in some cases, but it's inconsistent with the old code that used %OPENSEARCH_HOME%\jdk\bin\java.exe. Similarly, PLUGIN_DIR and classpath entries like %OPENSEARCH_HOME%lib\* are missing a backslash separator, which could cause failures if OPENSEARCH_HOME does not end with a backslash.

  set "JAVA=%OPENSEARCH_HOME%jdk\bin\java.exe"
  set "JAVA_HOME=%OPENSEARCH_HOME%jdk"
  set JAVA_TYPE=bundled jdk
)

if not exist "%JAVA%" (
  echo could not find java in %JAVA_TYPE% at %JAVA% 1>&2
  exit /b 1
)

set "OPENSEARCH_HOME_ARG=%OPENSEARCH_HOME%"
if "%OPENSEARCH_HOME_ARG:~-1%" == "\" set "OPENSEARCH_HOME_ARG=%OPENSEARCH_HOME_ARG:~0,-1%"

"%JAVA%" -Dorg.apache.logging.log4j.simplelog.StatusLogger.level=OFF -cp "%PLUGIN_DIR%\*;%PLUGIN_DIR%\deps\*;%OPENSEARCH_HOME%lib\*" org.opensearch.security.tools.democonfig.Installer "%OPENSEARCH_HOME_ARG%" %* 2> nul
Path Separator Bug

The classpath uses %OPENSEARCH_HOME%lib\* and PLUGIN_DIR is set as %OPENSEARCH_HOME%plugins\opensearch-security — both missing a backslash between OPENSEARCH_HOME and the subdirectory. This is inconsistent and may cause classpath resolution failures if OPENSEARCH_HOME does not end with a backslash.

set "PLUGIN_DIR=%OPENSEARCH_HOME%plugins\opensearch-security"

if defined OPENSEARCH_JAVA_HOME (
  set BIN_PATH="%OPENSEARCH_JAVA_HOME%\bin\java.exe"
) else if defined JAVA_HOME (
  set BIN_PATH="%JAVA_HOME%\bin\java.exe"
) else (
  echo Unable to find java runtime
  echo OPENSEARCH_JAVA_HOME or JAVA_HOME must be defined
  exit /b 1
)

%BIN_PATH% -Dorg.apache.logging.log4j.simplelog.StatusLogger.level=OFF -cp "%PLUGIN_DIR%\*;%PLUGIN_DIR%\deps\*;%OPENSEARCH_HOME%lib\*" org.opensearch.security.tools.SecurityAdmin %* 2> nul
Path Separator Bug

Same issue as other .bat files: %OPENSEARCH_HOME%lib\* and %OPENSEARCH_HOME%plugins\opensearch-security are missing a backslash separator. While %~dp0 ends with a backslash, the traversal logic using %%~dpI may or may not preserve the trailing backslash consistently across Windows versions.

set "PLUGIN_DIR=%OPENSEARCH_HOME%plugins\opensearch-security"

if defined OPENSEARCH_JAVA_HOME (
  set BIN_PATH="%OPENSEARCH_JAVA_HOME%\bin\java.exe"
) else if defined JAVA_HOME (
  set BIN_PATH="%JAVA_HOME%\bin\java.exe"
) else (
  echo Unable to find java runtime
  echo OPENSEARCH_JAVA_HOME or JAVA_HOME must be defined
  exit /b 1
)

%BIN_PATH% -cp "%PLUGIN_DIR%\*;%PLUGIN_DIR%\deps\*;%OPENSEARCH_HOME%lib\*" org.opensearch.security.tools.AuditConfigMigrater %*
Existing OPENSEARCH_HOME Not Validated

When OPENSEARCH_HOME is already set (e.g., by the user or environment), the script skips the discovery loop entirely and proceeds without verifying that the provided path actually contains lib/opensearch-*.jar. This could lead to confusing runtime errors if an incorrect OPENSEARCH_HOME is set externally.

if [ -z "$OPENSEARCH_HOME" ]; then
  OPENSEARCH_HOME="$DIR"
  while [ "$OPENSEARCH_HOME" != "/" ] && [ -z "$(ls "$OPENSEARCH_HOME/lib/opensearch-"*.jar 2>/dev/null)" ]; do
    OPENSEARCH_HOME="$(dirname "$OPENSEARCH_HOME")"
  done
  if [ "$OPENSEARCH_HOME" = "/" ]; then
    echo "Could not locate OpenSearch home. Set OPENSEARCH_HOME manually." >&2
    exit 1
  fi
fi
BASE_DIR Logic Change

The setBaseDir() method now sets BASE_DIR directly to SCRIPT_DIR (the directory of the script itself) rather than navigating three levels up. This assumes the script is always run from OPENSEARCH_HOME. If the script is invoked from the old tools/ location (which still exists as a copy), BASE_DIR will point to the tools/ directory rather than OPENSEARCH_HOME, causing all downstream path constructions (config, bin, plugins, lib) to be incorrect.

File baseDirFile = new File(SCRIPT_DIR);
BASE_DIR = baseDirFile.isDirectory() ? baseDirFile.getAbsolutePath() : null;

if (BASE_DIR == null || !new File(BASE_DIR).isDirectory()) {
    System.out.println("DEBUG: basedir does not exist");
    exitHandler.exit(-1);

@github-actions
Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Strip trailing slash from path argument

The script now passes $OPENSEARCH_HOME as the first argument to Installer, which the
Java code uses as SCRIPT_DIR and then directly as BASE_DIR. This is correct only if
OPENSEARCH_HOME is the OpenSearch root. However, if OPENSEARCH_HOME has a trailing
slash (e.g., from dirname output), the isDirectory() check will still pass but
BASE_DIR may have inconsistent trailing slash behavior compared to what downstream
path construction expects. Consider stripping any trailing slash from
$OPENSEARCH_HOME before passing it.

tools/install_demo_configuration.sh [71]

-"$JAVA" -Dorg.apache.logging.log4j.simplelog.StatusLogger.level=OFF -cp "$PLUGIN_DIR/*:$PLUGIN_DIR/deps/*:$OPENSEARCH_HOME/lib/*" org.opensearch.security.tools.democonfig.Installer "$OPENSEARCH_HOME" "$@" 2>/dev/null
+OPENSEARCH_HOME_ARG="${OPENSEARCH_HOME%/}"
+"$JAVA" -Dorg.apache.logging.log4j.simplelog.StatusLogger.level=OFF -cp "$PLUGIN_DIR/*:$PLUGIN_DIR/deps/*:$OPENSEARCH_HOME/lib/*" org.opensearch.security.tools.democonfig.Installer "$OPENSEARCH_HOME_ARG" "$@" 2>/dev/null
Suggestion importance[1-10]: 4

__

Why: This is a valid concern — trailing slashes in $OPENSEARCH_HOME could cause inconsistent path construction in the Java code. The install_demo_configuration.bat already handles this with OPENSEARCH_HOME_ARG, making it reasonable to apply the same pattern in the .sh script for consistency.

Low
Ensure consistent path separator in classpath

The classpath uses %OPENSEARCH_HOME%lib* but OPENSEARCH_HOME is set with a trailing
backslash (from %~dp0), so the path becomes correct. However, to be consistent with
the PLUGIN_DIR usage (which uses %OPENSEARCH_HOME%plugins... without an extra
separator), this is fine — but note that %OPENSEARCH_HOME%lib* is missing a
backslash separator between OPENSEARCH_HOME and lib if OPENSEARCH_HOME ever lacks a
trailing backslash. Add an explicit separator to be safe.

tools/audit_config_migrater.bat [30]

+%BIN_PATH% -cp "%PLUGIN_DIR%\*;%PLUGIN_DIR%\deps\*;%OPENSEARCH_HOME%lib\*" org.opensearch.security.tools.AuditConfigMigrater %*
 
-
Suggestion importance[1-10]: 2

__

Why: The existing_code and improved_code are identical, meaning no actual change is proposed. The concern about missing backslash is valid in theory, but %~dp0 always includes a trailing backslash, making this a non-issue in practice.

Low
Safely change directory with existence check

The cd %CUR_DIR% command is executed after the find_home loop, but CUR_DIR is set to
%DIR% (the script directory). The cd call changes the working directory to the
script's directory, which may interfere with relative paths used by the caller. More
critically, if OPENSEARCH_HOME was already defined and the goto find_home_done jumps
past the loop, cd %CUR_DIR% still executes — this is likely intentional but should
be verified that it doesn't break when OPENSEARCH_HOME is pre-set to a path without
a trailing backslash.

tools/install_demo_configuration.bat [18]

-cd %CUR_DIR%
+if exist "%CUR_DIR%" cd /d "%CUR_DIR%"
Suggestion importance[1-10]: 2

__

Why: The cd %CUR_DIR% is a pre-existing pattern and CUR_DIR is set to %DIR% which is always the script directory. The suggestion to add an existence check adds minimal value and the concern about trailing backslash is not a real issue here.

Low
Possible issue
Verify SCRIPT_DIR is OpenSearch home root

The new logic sets BASE_DIR directly to SCRIPT_DIR if it is a directory, but the
downstream code appends subdirectory names like config, bin, plugins, and lib
directly to BASE_DIR. This means SCRIPT_DIR must now be the OpenSearch home root.
Previously it was 3 levels up from the script. Ensure that the value passed as
SCRIPT_DIR (i.e., the first argument from the shell script) is indeed the OpenSearch
home directory and not a subdirectory, otherwise all derived paths will be wrong.

src/main/java/org/opensearch/security/tools/democonfig/Installer.java [261-262]

+File baseDirFile = new File(SCRIPT_DIR);
+BASE_DIR = baseDirFile.isDirectory() ? baseDirFile.getAbsolutePath() : null;
 
-
Suggestion importance[1-10]: 1

__

Why: The existing_code and improved_code are identical — no actual code change is proposed. The suggestion only asks to verify behavior, and the shell script change already confirms $OPENSEARCH_HOME is passed as the argument.

Low

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant