feat(installer): bootstrap discovery datasources - #152
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for configuring discovery and SSH datasources during installation via the deploy/install.sh script, along with corresponding documentation in README.md. The review feedback highlights several critical and medium-severity issues in the shell script, including a missing datasources: key in the generated YAML configuration, insecure directory permissions for SSH keys, an incorrectly scoped cleanup trap for temporary files, and global scope pollution from undeclared local variables.
| DATASOURCES=discovery | ||
| fi | ||
| local key_path item | ||
| DATASOURCE_YAML="" |
There was a problem hiding this comment.
The generated configuration is missing the datasources: key, which will result in invalid YAML and cause the agent to fail to parse the configuration. Prepend the datasources: key to DATASOURCE_YAML when initializing it.
| DATASOURCE_YAML="" | |
| DATASOURCE_YAML="datasources:\n" |
| [ -n "$source" ] || { err "SSH private key file is required for datasource '$name'"; return 1; } | ||
| [ -r "$source" ] || { err "SSH private key file is not readable: $source"; return 1; } | ||
| [ -s "$source" ] || { err "SSH private key file is empty: $source"; return 1; } | ||
| mkdir -p "${DATA_DIR}/ssh" |
There was a problem hiding this comment.
Using mkdir -p to create the SSH key directory leaves it owned by root:root with default permissions (usually 0755), which is a security risk for private keys and prevents the nudgebee user from managing the directory. Use install -d with secure permissions (0700) and the correct owner/group instead.
| mkdir -p "${DATA_DIR}/ssh" | |
| install -d -o nudgebee -g nudgebee -m 0700 "${DATA_DIR}/ssh" |
| tmp_pack="$(mktemp)" | ||
| trap 'rm -f "$tmp_pack"; trap - RETURN' RETURN | ||
| if command -v curl &>/dev/null; then curl -fsSL -o "$tmp_pack" "$url"; elif command -v wget &>/dev/null; then wget -q -O "$tmp_pack" "$url"; else err "Neither curl nor wget found"; return 1; fi | ||
| [ -s "$tmp_pack" ] || { rm -f "$tmp_pack"; err "Downloaded inventory pack is empty"; return 1; } | ||
| "$INSTALL_DIR/$BINARY_NAME" pack verify "$tmp_pack" --key "$PACK_PUBLIC_KEY" >/dev/null | ||
| install -o nudgebee -g nudgebee -m 0644 "$tmp_pack" "${PACK_DIR}/linux-inventory-v2.yaml" |
There was a problem hiding this comment.
When defining a trap to clean up a temporary file stored in a local variable within a function, use double quotes (e.g., trap "rm -f \"${tmp_pack}\"" EXIT) instead of single quotes. Single quotes defer variable expansion until the trap runs, at which point the local variable is out of scope if the function has already returned, causing the cleanup to fail. Additionally, use the EXIT trap instead of RETURN to ensure cleanup runs even if the script exits prematurely due to set -e.
| tmp_pack="$(mktemp)" | |
| trap 'rm -f "$tmp_pack"; trap - RETURN' RETURN | |
| if command -v curl &>/dev/null; then curl -fsSL -o "$tmp_pack" "$url"; elif command -v wget &>/dev/null; then wget -q -O "$tmp_pack" "$url"; else err "Neither curl nor wget found"; return 1; fi | |
| [ -s "$tmp_pack" ] || { rm -f "$tmp_pack"; err "Downloaded inventory pack is empty"; return 1; } | |
| "$INSTALL_DIR/$BINARY_NAME" pack verify "$tmp_pack" --key "$PACK_PUBLIC_KEY" >/dev/null | |
| install -o nudgebee -g nudgebee -m 0644 "$tmp_pack" "${PACK_DIR}/linux-inventory-v2.yaml" | |
| local tmp_pack | |
| tmp_pack="$(mktemp)" | |
| trap "rm -f \"${tmp_pack}\"" EXIT |
References
- In bash scripts, when defining a trap to clean up a temporary directory stored in a local variable within a function, use double quotes (e.g., trap "rm -rf "${tmpdir}"" EXIT) instead of single quotes. Single quotes defer variable expansion until the trap runs, at which point the local variable is out of scope if the function has already returned, causing the cleanup to fail on successful execution paths.
| } | ||
|
|
||
| validate_list() { | ||
| local value="$1" label="$2" item |
There was a problem hiding this comment.
The items array variable is not declared as local, which pollutes the global shell scope. Declare it as a local array.
| local value="$1" label="$2" item | |
| local value="$1" label="$2" item | |
| local -a items |
| } | ||
|
|
||
| prompt_discovery() { | ||
| local tty=/dev/tty |
There was a problem hiding this comment.
The value variable used in prompt_discovery is not declared as local, which pollutes the global shell scope. Declare it as local.
| local tty=/dev/tty | |
| local tty=/dev/tty value |
| prompt_discovery | ||
| DATASOURCES=discovery | ||
| fi | ||
| local key_path item |
There was a problem hiding this comment.
The items array variable is not declared as local, which pollutes the global shell scope. Declare it as a local array.
| local key_path item | |
| local key_path item | |
| local -a items |
1b66072 to
0d0a21a
Compare
0d0a21a to
9d2bb19
Compare
1 participant
Description
Add optional Linux Forager datasource bootstrap for discovery and SSH installations. The installer validates inputs, securely copies private keys, downloads and verifies the v2 inventory pack, and preserves existing configuration unless replacement is explicit.
Type of change
How Has This Been Tested?
Checklist