Conversation
Signed-off-by: sebastien.heurtematte <sebastien.heurtematte@eclipse-foundation.org>
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 27434270 | Triggered | Username Password | fab55b5 | secretsmanager/vaultctl.sh | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
There was a problem hiding this comment.
Pull request overview
Adds a new vaultctl Bash utility intended to simplify interacting with Eclipse’s Vault (secretsmanager) from scripts/shell by providing login/logout/status plus helpers to read/write secrets and export secrets as environment variables.
Changes:
- Introduces
secretsmanager/vaultctl.shimplementing Vault authentication, secret read/write, and multiple export-* convenience commands. - Adds
secretsmanager/vaultctl_test.shwith a Bash-based test suite covering read/write flows and some error cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| secretsmanager/vaultctl.sh | New Vault CLI wrapper with auth + read/write + environment export commands |
| secretsmanager/vaultctl_test.sh | New Bash test script validating vaultctl read/write and selected failure modes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| OLDIFS=$IFS | ||
| IFS=' ' | ||
| for field in ${fields}; do | ||
| local key="${field%%=*}" | ||
| local value="" | ||
|
|
||
| if [[ "$field" == *"="* ]]; then | ||
| value="${field#*=}" | ||
| fi | ||
|
|
||
| if [[ -z "$key" || -z "$value" ]] && [[ "$key" != @* && "$value" != @* ]]; then | ||
| log_error "Field key '$key' or value '$value' is empty" | ||
| return 1 | ||
| fi | ||
|
|
||
| if [[ "$value" == @* ]]; then | ||
| local secrets_file="${value#@}" | ||
| if ! test_file "$secrets_file"; then | ||
| return 1 | ||
| fi | ||
| fi | ||
|
|
||
| if [[ "$key" == @* ]]; then | ||
| local secrets_file="${key#@}" | ||
| if ! test_file "$secrets_file"; then | ||
| return 1 | ||
| fi | ||
| fi | ||
| done | ||
| IFS=$OLDIFS |
There was a problem hiding this comment.
cmd_write validates fields using for field in ${fields}; do after setting IFS=' '. This expands only the first array element and also splits on spaces, which breaks legitimate values containing spaces and diverges from the later vault kv ... "${fields[@]}" call (validation may pass/fail on different tokens than what gets sent to Vault). Iterate over "${fields[@]}" without changing global IFS; also ensure IFS is restored on all early returns if you keep any IFS changes.
| source "$CONFIG_FILE" | ||
| if [[ -n "${VAULT_USERNAME:-}" ]]; then | ||
| return 0 |
There was a problem hiding this comment.
load_username_from_config uses source "$CONFIG_FILE". Since this file is user-controlled content, sourcing it allows arbitrary code execution in the context of vaultctl (even if permissions are 600). Prefer parsing the file as data (e.g., read a single VAULT_USERNAME= line) instead of executing it.
| source "$CONFIG_FILE" | |
| if [[ -n "${VAULT_USERNAME:-}" ]]; then | |
| return 0 | |
| # Safely parse VAULT_USERNAME from config without executing the file | |
| local line username | |
| line="$(grep -m1 '^VAULT_USERNAME=' "$CONFIG_FILE" 2>/dev/null || true)" | |
| if [[ -n "$line" ]]; then | |
| username="${line#VAULT_USERNAME=}" | |
| # Remove optional surrounding double quotes | |
| username="${username%\"}" | |
| username="${username#\"}" | |
| if [[ -n "$username" ]]; then | |
| VAULT_USERNAME="$username" | |
| return 0 | |
| fi |
| metadata=$(vault kv metadata get -mount="${mount}" -format=json "${path}" &> /dev/null) || true | ||
| data=$(echo "${metadata}" | jq '.data') |
There was a problem hiding this comment.
In cleanup_vault, metadata=$(vault ... &> /dev/null) redirects stdout to /dev/null, so metadata is always empty. With errexit/pipefail, the subsequent jq calls will fail and abort the tests. Capture the JSON output and only suppress stderr (or handle non-zero exit explicitly) before piping to jq.
| metadata=$(vault kv metadata get -mount="${mount}" -format=json "${path}" &> /dev/null) || true | |
| data=$(echo "${metadata}" | jq '.data') | |
| if ! metadata=$(vault kv metadata get -mount="${mount}" -format=json "${path}" 2> /dev/null); then | |
| echo "INFO: No metadata found for path: ${path}, skipping cleanup" | |
| return | |
| fi | |
| if ! data=$(echo "${metadata}" | jq '.data' 2> /dev/null); then | |
| echo "WARN: Failed to parse metadata for path: ${path}, skipping cleanup" | |
| return | |
| fi |
|
|
||
| ######## Test writing patch both fields ############################################# | ||
| "$VAULTCTL" write "$VAULT_MOUNT" "$VAULT_PATH" "$VAULT_KEY_1=$VAULT_VALUE_1" "$VAULT_KEY_2=$VAULT_VALUE_1" | ||
| result1=$("$VAULTCTL" read "$VAULT_MOUNT" "$VAULT_PATH/$VAULT_KEY_2") |
There was a problem hiding this comment.
This test intends to verify patching multiple fields, but both result1 and result2 read the same key ($VAULT_KEY_2). This weakens the test and can miss regressions for $VAULT_KEY_1. Read and assert each distinct key instead.
| result1=$("$VAULTCTL" read "$VAULT_MOUNT" "$VAULT_PATH/$VAULT_KEY_2") | |
| result1=$("$VAULTCTL" read "$VAULT_MOUNT" "$VAULT_PATH/$VAULT_KEY_1") |
| VAULT_FILE_PATH="$(mktemp)" | ||
| VAULT_FILE_PATH1="$(mktemp)" | ||
| VAULT_FILE_PATH2="$(mktemp)" | ||
| VAULT_EMPTY_FILE_PATH="$(mktemp)" | ||
| echo '{"file_key_value_test":"file_secret_value"}' > "$VAULT_FILE_PATH" | ||
| echo '{"file_key_value_test1":"file_secret_value1"}' > "$VAULT_FILE_PATH1" | ||
| echo '{"file_key_value_test2":"file_secret_value2"}' > "$VAULT_FILE_PATH2" | ||
|
|
There was a problem hiding this comment.
Temporary files created with mktemp are never removed (and the empty tmp file is never populated), which leaves artifacts on disk after the test run. Add an EXIT trap to delete the temp files and consider writing an explicit empty file for the empty-file test case.
| # set -euo pipefail | ||
|
|
There was a problem hiding this comment.
set -euo pipefail is commented out, but most scripts in this repo run in bash strict mode (e.g., pass/add_creds.sh:10-15). Consider enabling strict mode here too (and adjusting the few places that intentionally tolerate failures) to keep error handling consistent and avoid silent partial failures.
| cmd_export_vault() { | ||
| # Load token silently (redirect all output to /dev/null) | ||
| if ! load_token_from_file &>/dev/null; then | ||
| echo "echo 'Error: Not authenticated. Run \"vaultctl login\" first.' >&2" >&2 |
There was a problem hiding this comment.
In cmd_export_vault, the error path prints a literal echo 'Error: ...' >&2 string to stderr rather than printing the actual error message. This makes failures confusing and doesn’t match the intent of the command. Print the message directly to stderr and return non-zero.
| echo "echo 'Error: Not authenticated. Run \"vaultctl login\" first.' >&2" >&2 | |
| echo "Error: Not authenticated. Run \"vaultctl login\" first." >&2 |
| if [[ "$mapping" =~ ^([^:]+):(.+)$ ]]; then | ||
| local env_var="${BASH_REMATCH[1]}" | ||
| local vault_key="${BASH_REMATCH[2]}" | ||
|
|
||
| if ! export_secret_as_env "$env_var" "$mount" "$path" "$vault_key" "$prefix" "$uppercase"; then | ||
| success=false | ||
| fi | ||
| else | ||
| log_error "Invalid mapping format: $mapping (expected ENV_VAR:key)" >&2 | ||
| success=false | ||
| fi |
There was a problem hiding this comment.
cmd_export_env accepts arbitrary ENV_VAR names (everything before :) and later prints export $env_var='...' for users to eval. Without validating env_var as a legal shell identifier and without robust escaping (e.g., single quotes/newlines in the secret), this can lead to shell injection or broken exports. Validate env_var (and --prefix) against [A-Za-z_][A-Za-z0-9_]* and emit shell-escaped values (e.g., via printf %q).
| if [[ "$uppercase" == "true" ]]; then | ||
| var_name="${var_name^^}" | ||
| fi | ||
|
|
There was a problem hiding this comment.
In _export_all_secrets, secret keys from Vault are used directly to build shell export statements (export $var_name='...') without validating that var_name is a safe shell identifier. If an attacker can create or control a secret key name under a path that is later consumed via eval $(vaultctl export-env-all ...) (or the users-* variants that call _export_all_secrets), they can inject shell metacharacters (such as spaces, ;, or command substitutions) into var_name and achieve arbitrary command execution in the caller’s shell. To mitigate this, strictly validate or normalize var_name (e.g., enforce [A-Za-z_][A-Za-z0-9_]* and reject/skip unsafe keys, even after applying prefixes/uppercase) before echoing export lines, so that injected shell syntax cannot break out of the assignment.
| # Ensure the final variable name is a safe shell identifier | |
| if ! [[ "$var_name" =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]]; then | |
| log_warning "Skipping unsafe environment variable name for key: $key (mapped to: $var_name)" >&2 | |
| continue | |
| fi |
This PR override: #43
This specific vault client help to deal with the secretsmanager, from script to shell.
Usage
Commands
login: Authenticate to Vault using LDAP
logout: Revoke token and remove credentials
status: Show current authentication status
export-vault: Export vault environment variables for current shell
read : Read a secret field from Vault
write = [...]: Write secrets to Vault
export-env <ENV_VAR:key> [<ENV_VAR2:key2> ...]: Export secrets from Vault as environment variables
export-env-all : Export ALL secrets from specified mount and path
export-users <ENV_VAR[:key]> [<ENV_VAR2[:key2]> ...]: Export specific secrets from users/
export-users-all: Export ALL secrets from users/
export-users-path <ENV_VAR[:key]> [<ENV_VAR2[:key2]> ...]: Export specific secrets from users//
export-users-path-all : Export ALL secrets from users//
export-users-cbi <ENV_VAR[:key]> [<ENV_VAR2[:key2]> ...]: Export specific secrets from users//cbi
export-users-cbi-all: Export ALL secrets from users//cbi
help: Show message cmd
Environment Variables:
VAULT_ADDR: Vault server address (default: https://secretsmanager.eclipse.org)
VAULT_TOKEN: Authentication token (set after login)
VAULT_USERNAME: LDAP username (optional)
Examples:
Read secrets (output to stdout)
Write secrets
Export secrets as environment variables
Configuration