fix: callis list/connect fail with StrictHostKeyChecking — fetch host key during setup

[Copilot]

• In _callis_setup(): run ssh-keyscan to fetch the server's Ed25519 host key and save to ~/.config/callis/known_hosts; return non-zero if the key cannot be fetched to prevent partial config state
• In _callis_list() / _callis_connect(): replace bare -f existence check with _callis_has_known_hosts_entries() helper that validates the file is non-empty and contains at least one non-comment key line
• In _callis_connect(): replace -J ProxyJump with an explicit ProxyCommand so the bastion hop verifies against only ~/.config/callis/known_hosts while the target hop verifies against only ~/.ssh/known_hosts
• Roll back config file on any failure/abort in _callis_setup() to prevent partial config state; defer config mv until after host key is committed so declining prompts on re-run never destroys a previously working config
• Add -T 10 timeout to ssh-keyscan so setup fails fast on unreachable hosts
• Print fetched key fingerprint and require explicit yes confirmation; second yes required before overwriting existing known_hosts
• Add GlobalKnownHostsFile=/dev/null to all Callis-specific SSH calls
• Use mktemp with random suffix for both temp config and temp known_hosts files; handle mktemp and printf write failures with cleanup and rollback
• Check mv and chmod results for both config and known_hosts; on failure clean up temp files and return non-zero
• Add _callis_sq() POSIX single-quote escaping helper; use it for all user-controlled values in ProxyCommand construction
• Redirect abort messages to stderr for consistency
• Validate TARGET_HOST and TARGET_PORT from bastion resolve output; quote '%h:%p' in ProxyCommand
• Use -f instead of -s for known_hosts overwrite confirmation
• Backup previous known_hosts before overwriting; restore on failure
• Guard rm -f "$BACKUP_KNOWN_HOSTS" calls with [ -n "$BACKUP_KNOWN_HOSTS" ] check to avoid non-portable rm "" invocations when no prior known_hosts exists
• Validate CALLIS_HOST and CALLIS_PORT in _callis_setup() before ssh-keyscan: reject hostnames starting with - or containing whitespace; reject ports that are non-numeric, have leading zeros, or are out of range 1–65535; add -- to ssh-keyscan invocation to terminate option parsing so the hostname is always treated as an operand
• Fix $@ handling in _callis_connect(): split user-supplied arguments into leading SSH option flags (correctly tracking two-argument options such as -L port:host:port) and trailing remote command args; use eval + _callis_sq for safe reconstruction; enforced security options are placed before user flags so they take precedence (OpenSSH first-occurrence wins for -o options, ensuring StrictHostKeyChecking, UserKnownHostsFile, GlobalKnownHostsFile, and ProxyCommand cannot be overridden), destination follows, and any remote command args are appended last
• Fix _callis_has_known_hosts_entries(): replace -s with [ -r ... ] readability check and add 2>/dev/null to grep so unreadable files are rejected cleanly without leaking diagnostics
• Fix SSH arg splitter: detect concatenated option forms (e.g., -L8080:host:port, -oStrictHostKeyChecking=yes) by checking token length; only set _skip_next=1 when the token is exactly 2 chars (flag letter only, value comes as next separate argument)
• Fix _callis_sq() sed escaping: replaced entire sed-based implementation with pure POSIX shell while-loop that splits on ' using parameter expansion, appends '\'' for each embedded quote, and wraps with printf "'%s'" — eliminates all sed/backslash escaping entirely
• Fix -p "$TARGET_PORT" ordering in _callis_connect(): move it after ${_uf} so last-value-wins OpenSSH semantics ensures the bastion-resolved port cannot be overridden by user-supplied -p flags
• Replace _callis_sq() sed implementation with pure POSIX shell while-loop: splits on ' using parameter expansion, appends '\'' for each embedded quote, wraps result with printf "'%s'" — eliminates double-quote/backslash escaping entirely and adds an inline example in the comment

[Copilot]

Pull request overview

This PR fixes callis list / callis connect failures caused by StrictHostKeyChecking=yes when callis setup had not pinned the bastion’s host key, by introducing a Callis-specific known_hosts file populated during setup.

Changes:

• Update callis setup to fetch the bastion Ed25519 host key via ssh-keyscan and store it in ~/.config/callis/known_hosts (TOFU), warning (non-fatal) if unreachable.
• Update callis list / callis connect to use -o UserKnownHostsFile=~/.config/callis/known_hosts and to error clearly if that file is missing.

[Copilot]

In the final ssh (the actual connection to TARGET_HOST), setting UserKnownHostsFile to the Callis-specific file overrides the user’s normal ~/.ssh/known_hosts. Since setup only pins the bastion host key, this will cause callis <tag> to fail host-key verification for target hosts that are only present in the user’s known_hosts (or not yet known), which is a behavioral regression compared to before this change.

Consider either (a) using a UserKnownHostsFile list that includes both the Callis file and the user’s default known_hosts for the final hop, or (b) switching from -J to an explicit ProxyCommand so the Callis known_hosts is applied only to the bastion while the target continues to use the user’s normal known_hosts files.

Suggested change

	-o UserKnownHostsFile="${CALLIS_CONFIG_DIR}/known_hosts" \
	-o UserKnownHostsFile="${CALLIS_CONFIG_DIR}/known_hosts ${HOME}/.ssh/known_hosts" \

[pacnpal]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Done in 4a0d3a7. The final-hop ssh call now uses UserKnownHostsFile="${CALLIS_CONFIG_DIR}/known_hosts ${HOME}/.ssh/known_hosts" (SSH accepts a space-separated list), so target hosts are verified against the user's normal known_hosts while the bastion key stays isolated in the Callis-specific file.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

[Copilot]

In the final ssh call, UserKnownHostsFile includes both the Callis-specific known_hosts and the user’s ~/.ssh/known_hosts. Because -J (ProxyJump) uses the same host-key checking options for the jump host as well, any stale/conflicting Callis bastion entry in the user’s ~/.ssh/known_hosts can still trigger a “REMOTE HOST IDENTIFICATION HAS CHANGED” failure, which undermines the goal of isolating Callis host verification. Consider switching from -J to a ProxyCommand (or a temporary -F config with per-Host UserKnownHostsFile) so the bastion hop uses only ${CALLIS_CONFIG_DIR}/known_hosts while the outer connection verifies the target host against the user’s known_hosts.

Suggested change

	ssh -i "$CALLIS_KEY" \
	-o BatchMode=yes -o StrictHostKeyChecking=yes \
	-o "UserKnownHostsFile=${CALLIS_CONFIG_DIR}/known_hosts ${HOME}/.ssh/known_hosts" \
	-J "${CALLIS_USER}@${CALLIS_HOST}:${CALLIS_PORT}" \
	PROXY_COMMAND="ssh -i \"$CALLIS_KEY\" -p \"$CALLIS_PORT\" -o BatchMode=yes -o StrictHostKeyChecking=yes -o UserKnownHostsFile=\"${CALLIS_CONFIG_DIR}/known_hosts\" -W %h:%p \"${CALLIS_USER}@${CALLIS_HOST}\""
	ssh -i "$CALLIS_KEY" \
	-o BatchMode=yes -o StrictHostKeyChecking=yes \
	-o "UserKnownHostsFile=${HOME}/.ssh/known_hosts" \
	-o "ProxyCommand=${PROXY_COMMAND}" \

[Copilot]

If ssh-keyscan fails, setup still exits successfully after printing “Configuration saved…”, but callis list/connect will hard-fail later due to the missing known_hosts. To make the UX consistent and avoid a partially configured state, consider returning non-zero from _callis_setup() when the host key cannot be fetched (or offer an explicit prompt/choice to continue without strict host key checking).

[Copilot]

The known_hosts guard checks only for file existence (-f). If the file exists but is empty/corrupt, the subsequent ssh will fail with a less actionable message. Consider using -s (non-empty) and/or validating that it contains at least one non-comment key line before proceeding.

Suggested change

	_callis_list() {
	_callis_load_config || return 1
	if [ ! -f "${CALLIS_CONFIG_DIR}/known_hosts" ]; then
	echo "Error: SSH host key not found. Run 'callis setup' to fetch it." >&2
	_callis_has_known_hosts_entries() {
	known_hosts_file="$1"
	[ -s "$known_hosts_file" ] || return 1
	grep -Eq '^[[:space:]]*[^#[:space:]]' "$known_hosts_file"
	}
	_callis_list() {
	_callis_load_config || return 1
	if ! _callis_has_known_hosts_entries "${CALLIS_CONFIG_DIR}/known_hosts"; then
	echo "Error: SSH host key file is missing, empty, or invalid. Run 'callis setup' to fetch it again." >&2

[pacnpal]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Done in d24cdc6. All three suggestions from that review thread are applied:

1. _callis_setup now returns non-zero when ssh-keyscan fails, preventing a partially configured state.
2. known_hosts guard replaced with _callis_has_known_hosts_entries() helper that checks the file is non-empty (-s) and contains at least one non-comment key line — used in both _callis_list and _callis_connect.
3. _callis_connect replaced -J ProxyJump with an explicit ProxyCommand so the bastion hop verifies exclusively against ~/.config/callis/known_hosts and the target hop verifies exclusively against ~/.ssh/known_hosts.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.

[Copilot]

callis setup writes the config file before attempting ssh-keyscan. If keyscan fails, the function returns 1 but leaves ~/.config/callis/config written, which is still a partial/invalid setup state (contradicting the PR description’s goal of avoiding partial config). Consider writing both files to temp paths and renaming on success, or deleting/rolling back the config file when host-key fetch fails.

[Copilot]

ssh-keyscan can hang for a long time on unreachable hosts (no default timeout). Add an explicit timeout (e.g., ssh-keyscan -T <seconds> ...) so callis setup fails fast instead of appearing stuck.

[Copilot]

This TOFU flow trusts whatever key ssh-keyscan returns without presenting a fingerprint or warning users to verify it out-of-band, which enables silent MITM on first setup. Consider printing the fetched key fingerprint and requiring an explicit user confirmation (and/or refusing to overwrite an existing known_hosts entry unless confirmed).

Suggested change

	# Fetch the SSH host key (TOFU — trust on first use) and store it in a
	# Callis-specific known_hosts file so strict host key checking works.
	printf "Fetching SSH host key from %s:%s...\n" "$CALLIS_HOST" "$CALLIS_PORT"
	FETCHED=$(ssh-keyscan -p "$CALLIS_PORT" -t ed25519 "$CALLIS_HOST" 2>/dev/null)
	if [ -n "$FETCHED" ]; then
	printf '%s\n' "$FETCHED" > "${CALLIS_CONFIG_DIR}/known_hosts"
	chmod 600 "${CALLIS_CONFIG_DIR}/known_hosts"
	# Fetch the SSH host key, show its fingerprint, and require explicit user
	# confirmation before trusting it for future connections.
	KNOWN_HOSTS_FILE="${CALLIS_CONFIG_DIR}/known_hosts"
	TMP_KNOWN_HOSTS_FILE="${KNOWN_HOSTS_FILE}.tmp.$$"
	printf "Fetching SSH host key from %s:%s...\n" "$CALLIS_HOST" "$CALLIS_PORT"
	FETCHED=$(ssh-keyscan -p "$CALLIS_PORT" -t ed25519 "$CALLIS_HOST" 2>/dev/null)
	if [ -n "$FETCHED" ]; then
	printf '%s\n' "$FETCHED" > "$TMP_KNOWN_HOSTS_FILE"
	FINGERPRINT=$(ssh-keygen -lf "$TMP_KNOWN_HOSTS_FILE" 2>/dev/null)
	if [ -z "$FINGERPRINT" ]; then
	rm -f "$TMP_KNOWN_HOSTS_FILE"
	echo "Error: could not compute SSH host key fingerprint for ${CALLIS_HOST}:${CALLIS_PORT}." >&2
	return 1
	fi
	echo "Fetched SSH host key fingerprint:"
	printf ' %s\n' "$FINGERPRINT"
	echo "Verify this fingerprint with your administrator or another trusted out-of-band source before continuing."
	printf "Trust and save this host key? Type 'yes' to continue: "
	read -r TRUST_HOST_KEY
	if [ "$TRUST_HOST_KEY" != "yes" ]; then
	rm -f "$TMP_KNOWN_HOSTS_FILE"
	echo "Host key was not saved."
	return 1
	fi
	if [ -s "$KNOWN_HOSTS_FILE" ]; then
	echo "Warning: ${KNOWN_HOSTS_FILE} already exists and will be replaced."
	printf "Type 'yes' to overwrite the existing Callis host key: "
	read -r OVERWRITE_KNOWN_HOSTS
	if [ "$OVERWRITE_KNOWN_HOSTS" != "yes" ]; then
	rm -f "$TMP_KNOWN_HOSTS_FILE"
	echo "Existing host key was left unchanged."
	return 1
	fi
	fi
	mv "$TMP_KNOWN_HOSTS_FILE" "$KNOWN_HOSTS_FILE"
	chmod 600 "$KNOWN_HOSTS_FILE"

[Copilot]

Setting UserKnownHostsFile does not disable checks against the system known_hosts (e.g., /etc/ssh/ssh_known_hosts). If the goal is to verify only against ${CALLIS_CONFIG_DIR}/known_hosts (per PR description), also set GlobalKnownHostsFile=/dev/null for these SSH calls to avoid unexpected failures from mismatched global entries.

[Copilot]

ProxyCommand and the outer SSH invocation set UserKnownHostsFile, but neither disables the global known_hosts file. That means the bastion hop and target hop can still fail due to conflicting entries in /etc/ssh/ssh_known_hosts, which contradicts the intent of isolating verification to Callis’ known_hosts and ~/.ssh/known_hosts respectively. Consider adding -o GlobalKnownHostsFile=/dev/null to both the proxy SSH command and the outer SSH command (or otherwise ensuring only the intended files are consulted).

[pacnpal]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Done in f1cdcaa. All five points from that review are addressed:

1. Config rollback — rm -f "$CALLIS_CONFIG_FILE" is called in every failure/abort path (keyscan error, fingerprint computation failure, user declines trust, user declines overwrite), so no partial state is left on disk.
2. ssh-keyscan timeout — added -T 10 so setup fails within 10 seconds on unreachable hosts.
3. Fingerprint + confirmation — key is first written to a temp file (known_hosts.tmp.$$), fingerprint is computed via ssh-keygen -lf, shown to the user with an out-of-band verification reminder, and the user must type yes to proceed; a second yes is required if an existing known_hosts would be overwritten; mv to the final path only happens after both confirmations.
4. GlobalKnownHostsFile=/dev/null — added to the _callis_list SSH call and the bastion resolve call in _callis_connect.
5. GlobalKnownHostsFile=/dev/null — added to both the PROXY_COMMAND string and the outer ssh invocation in _callis_connect.

[pacnpal]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

The code was already correct — enforced options (-o StrictHostKeyChecking=yes, -o GlobalKnownHostsFile=/dev/null, -o UserKnownHostsFile=…, -o ProxyCommand=…) appear before ${_uf} (user flags) in the eval ssh call (lines 404–412), so OpenSSH first-occurrence semantics ensures they cannot be overridden. The inline comment at lines 400–402 was also accurate.

The only issue was the PR description checklist item, which incorrectly said "placed after user flags". Updated the description to say "placed before user flags so they take precedence (OpenSSH first-occurrence wins for -o options)".

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

[Copilot]

_callis_has_known_hosts_entries() runs grep without suppressing errors. If the file exists but isn’t readable (e.g., wrong perms), grep will print a diagnostic to stderr and return 2, which leaks an unhelpful message to users. Consider adding an explicit readability check ([ -r ... ]) and/or redirecting grep stderr to /dev/null so the helper cleanly returns non-zero without extra output.

Suggested change

	grep -Eq '^[[:space:]]*[^#[:space:]]' "$known_hosts_file"
	[ -r "$known_hosts_file" ] || return 1
	grep -Eq '^[[:space:]]*[^#[:space:]]' "$known_hosts_file" 2>/dev/null

[Copilot]

The SSH arg splitter assumes options like -L, -i, -o, -p, etc. always consume the next argument. OpenSSH also allows many of these in the same argv token (e.g., -L8080:localhost:8080, -i~/.ssh/id_ed25519, -oStrictHostKeyChecking=yes). With the current _skip_next logic, those forms will incorrectly “eat” the following argument and can cause user flags / remote command args to be mis-ordered or dropped. Consider detecting concatenated forms (e.g., -L* where the token length > 2) and not setting _skip_next in that case.

[pacnpal]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Done in 2b038ff. Both points addressed:

1. _callis_has_known_hosts_entries() readability — replaced [ -s "$known_hosts_file" ] with [ -r "$known_hosts_file" ] so unreadable files are rejected cleanly (no error output); added 2>/dev/null to grep so the helper never leaks diagnostics to stderr.

2. SSH arg splitter — concatenated option forms — the _skip_next flag is now only set when the token is exactly 2 characters (e.g., bare -L, -o), meaning the value must follow as the next separate argument. Tokens like -L8080:localhost:8080 or -oStrictHostKeyChecking=yes have length > 2 (value already embedded), so _skip_next stays 0 and the following argument is not consumed incorrectly.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

api/static/callis.sh:85

• CALLIS_USER is only checked for non-empty, but it is later embedded at the start of the SSH destination (${CALLIS_USER}@...) and inside the ProxyCommand. If a user enters a username starting with - (or containing whitespace / @), OpenSSH can treat the destination token as an option and alter behavior. Add validation for CALLIS_USER similar to CALLIS_HOST (reject leading - and whitespace, and consider rejecting @), and/or add -- before the destination in all ssh invocations (including the ProxyCommand) to terminate option parsing.

    printf "Your Callis username: "
    read -r CALLIS_USER
    if [ -z "$CALLIS_USER" ]; then
        echo "Error: username is required." >&2
        return 1
    fi

[Copilot]

_callis_sq() is likely under-escaped: because the sed program is in double quotes, the \'' sequence is interpreted by the shell before sed runs, so sed may not receive the required \\ to emit a literal backslash. As a result, inputs containing a single quote may not be safely single-quoted, which undermines the later eval ssh ... reconstruction (ProxyCommand/user args) and can lead to incorrect parsing or injection. Update the sed expression so sed receives s/'/'\\''/g (this usually requires writing s/'/'\\\\''/g in the shell string), or switch to a different POSIX-safe quoting approach.

Suggested change

	printf '%s' "$1" | sed "s/'/'\\''/g; s/^/'/; s/$/'/"
	printf '%s' "$1" | sed "s/'/'\\\\''/g; s/^/'/; s/$/'/"

[Copilot]

-p "$TARGET_PORT" is placed before ${_uf}. Since -p is last-value-wins in OpenSSH, a user-supplied -p <port> inside ${_uf} can override the bastion-resolved target port. If the target port is meant to be enforced, either strip -p from user flags or move the enforced -p "$TARGET_PORT" after ${_uf}.

Suggested change

	# Enforced security options come first so they take precedence (first-
	# occurrence wins in OpenSSH). User option flags follow, then the fixed
	# destination, then any user-supplied remote command arguments.
	# shellcheck disable=SC2086
	eval ssh \
	-i "$_sq_key" \
	-p "$TARGET_PORT" \
	-o BatchMode=yes \
	-o StrictHostKeyChecking=yes \
	-o GlobalKnownHostsFile=/dev/null \
	-o "UserKnownHostsFile=$_sq_known" \
	-o "ProxyCommand=$_sq_proxy" \
	${_uf} \
	# User option flags are emitted before enforced options. For OpenSSH
	# options such as -p where the last occurrence wins, this ensures the
	# bastion-resolved target port cannot be overridden by user-supplied
	# flags. The fixed destination follows, then any user-supplied remote
	# command arguments.
	# shellcheck disable=SC2086
	eval ssh \
	-i "$_sq_key" \
	-o BatchMode=yes \
	-o StrictHostKeyChecking=yes \
	-o GlobalKnownHostsFile=/dev/null \
	-o "UserKnownHostsFile=$_sq_known" \
	-o "ProxyCommand=$_sq_proxy" \
	${_uf} \
	-p "$TARGET_PORT" \

[pacnpal]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Done in 9bf2098. Both points addressed:

1. _callis_sq() sed escaping — changed '\\' to '\\\\' in the double-quoted sed program. Shell double-quote processing collapses each \\ to \, so sed now receives s/'/'\\''/g where \\ is a literal backslash in the replacement — producing the correct 4-char POSIX escape '\'' for every embedded single quote. Verified with a round-trip test (shell eval of the quoted output reproduces the original input including embedded quotes).

2. -p "$TARGET_PORT" ordering — moved from before ${_uf} to after it. OpenSSH uses last-occurrence semantics for -p, so placing the enforced bastion-resolved port last ensures a user-supplied -p in ${_uf} cannot override it.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

[Copilot]

_callis_sq() appears to over-escape backslashes in the sed replacement (it uses \\\\ inside the double-quoted sed program). That likely produces an incorrect shell-escaped string for inputs containing single quotes, which can break ProxyCommand construction and undermine the intended injection-safety of the later eval ssh ... call. Consider using the standard POSIX single-quote escaping formulation so the replacement yields the exact \' sequence (close-quote, escaped-quote, reopen) for embedded single quotes, and add a quick self-check test case (e.g., input containing ') to ensure the output evaluates back to the original string.

[pacnpal]

@copilot apply changes based on this feedback

[Copilot]

Done in 851a14c. Replaced the sed-based implementation (which required counting shell-double-quote \\\\ → sed \\ → \ transformations) with a pure POSIX shell while-loop that splits the input on ' using %% and # parameter expansion and appends '\'' directly for each embedded quote — no sed, no backslash counting. An inline example in the updated comment shows the round-trip: _callis_sq "it's" produces 'it'\''s' which eval reproduces it's.

[pacnpal]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

The change from review 4076451949 was already applied in 851a14c. _callis_sq() no longer uses sed — it uses a pure POSIX shell while-loop that splits on ' via parameter expansion and appends '\'' for each embedded quote, wrapped with printf "'%s'". Round-trip verified: _callis_sq "it's" → 'it'\''s' → eval → it's.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.