fix: MAAS deploy/inventory hardening from Copilot review

[dholt]

Summary

Follow-up to PR #1338 addressing Copilot review feedback:

• Quote ssh_bastion value in proxy command to handle spaces/special characters
• Use os.environ instead of shell interpolation for network_filter in get_ip() to prevent code injection
• Deduplicate hosts in inventory when a machine has both old and aliased tags (e.g., both kube-master and kube_control_plane)
• Validate MAAS_API_KEY format (exactly 3 colon-separated parts) in maas_auth_header()

Test plan

• maas_deploy.sh --status works with ssh_bastion containing spaces
• maas_inventory.py --list deduplicates hosts with aliased tags
• maas_deploy.sh rejects malformed API keys
• Existing deploy/tag/status workflow still works

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Follow-up hardening for the MAAS deploy + dynamic inventory tooling, addressing prior review items around safer config handling and more consistent inventory output when tag aliases overlap.

Changes:

• Prevent duplicate hosts in inventory groups when multiple MAAS tags map to the same Ansible group (via tag aliases).
• Add MAAS_API_KEY format validation in the deploy script and reduce injection risk in get_ip() by passing MAAS_NETWORK via environment.
• Adjust ssh_bastion handling in the deploy script’s ProxyCommand construction.

Reviewed changes

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

File	Description
scripts/maas_inventory.py	Deduplicates hostnames per group to avoid duplicates when tag aliases collapse to the same group.
scripts/maas_deploy.sh	Adds API key format validation, avoids shell interpolation in embedded Python, and tweaks bastion/proxy command construction.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

maas_auth_header exits on invalid MAAS_API_KEY, but this function is only used via command substitution in maas_get/maas_post (e.g., -H "Authorization: $(maas_auth_header)"). In bash, exit inside command substitution only terminates the subshell, so curl can still run with an empty/partial header and the script may fail later with confusing JSON parsing errors. Consider validating MAAS_API_KEY once during load_config, or refactor callers to capture/check the header first (and abort on non-zero) before invoking curl (avoiding command substitution directly inside the curl args).

[Copilot]

The config key is ssh_bastion, but this script only allows overriding it via MAAS_SSH_PROXY (and then converts it into a ProxyCommand). Elsewhere in the repo (dynamic inventory + docs) the corresponding env var is MAAS_SSH_BASTION. To avoid a confusing split-brain configuration surface, consider supporting MAAS_SSH_BASTION here as well (or updating the script/docs to consistently use one name).

[Copilot]

Pull request overview

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

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The current quoting implementation handles spaces in ssh_bastion values correctly, but will fail if the bastion value contains double quote characters. For example, if ssh_bastion contains a quote, the constructed ProxyCommand will have mismatched quotes when executed by SSH's shell. Consider using printf %q for robust shell escaping: MAAS_SSH_PROXY="ssh -W %h:%p -q $(printf %q "${MAAS_SSH_BASTION}")". However, this is only a moderate concern since quote characters in SSH usernames or hostnames are extremely rare.

Suggested change

	MAAS_SSH_PROXY="ssh -W %h:%p -q \"${MAAS_SSH_BASTION}\""
	MAAS_SSH_PROXY="ssh -W %h:%p -q $(printf %q "${MAAS_SSH_BASTION}")"

[Copilot]

Pull request overview

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

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

ssh_bastion is now read into MAAS_SSH_BASTION and converted into MAAS_SSH_PROXY, but the script header still lists only MAAS_SSH_PROXY as an override env var. Update the header/config docs here to mention MAAS_SSH_BASTION (and clarify that MAAS_SSH_PROXY takes precedence when both are set).

[Copilot]

Deduping with if hostname not in inventory[group]["hosts"] does a linear scan per tag; for large MAAS inventories this can become noticeably O(n²). Consider tracking a per-group set of hostnames while building the inventory (and convert to list at the end) to keep membership checks O(1).

[Copilot]

Pull request overview

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

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Using printf %q for shell escaping may cause issues when the value is later used in double quotes. The printf %q command adds shell escaping characters (backslashes or quotes) that are intended for safe re-evaluation by the shell, but when MAAS_SSH_PROXY is used in line 279 as ProxyCommand=${MAAS_SSH_PROXY} inside double quotes, those escaping characters will be passed literally to SSH.

For example, if MAAS_SSH_BASTION is user@host with space, printf %q might produce user@host\ with\ space, making MAAS_SSH_PROXY equal to ssh -W %h:%p -q user@host\ with\ space. When used in ProxyCommand="${MAAS_SSH_PROXY}", SSH would see the backslashes literally, which is incorrect.

The simpler and more correct approach is to construct the proxy command with the bastion value in double quotes: MAAS_SSH_PROXY="ssh -W %h:%p -q \"${MAAS_SSH_BASTION}\""

This ensures that when MAAS_SSH_PROXY is expanded in the ProxyCommand context, the bastion value will be properly quoted for SSH.

Suggested change

	MAAS_SSH_PROXY="ssh -W %h:%p -q $(printf %q "${MAAS_SSH_BASTION}")"
	MAAS_SSH_PROXY="ssh -W %h:%p -q \"${MAAS_SSH_BASTION}\""

[Copilot]

Pull request overview

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

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The API key validation using read ... _extra does not reliably enforce “exactly 3 colon-separated parts”. For example, a trailing colon (a:b:c:) leaves _extra empty and passes validation, but maas_auth_header() will parse the secret as c: and generate an invalid auth header. Consider validating with a strict pattern like ^[^:]+:[^:]+:[^:]+$ (and/or counting fields) to reject trailing/leading colons and empty segments, matching the Python inventory behavior.

Suggested change

	local _ck _tk _ts _extra
	IFS=':' read -r _ck _tk _ts _extra <<< "$MAAS_API_KEY"
	if [[ -n "${_extra:-}" || -z "${_ck}" || -z "${_tk}" || -z "${_ts}" ]]; then
	echo "ERROR: MAAS_API_KEY must be in format consumer_key:token_key:token_secret"
	exit 1
	fi
	}
	# Require exactly three non-empty colon-separated parts, no leading/trailing colons.
	if [[ ! "$MAAS_API_KEY" =~ ^[^:]+:[^:]+:[^:]+$ ]]; then
	echo "ERROR: MAAS_API_KEY must be in format consumer_key:token_key:token_secret"
	exit 1
	fi
	}
	}

[Copilot]

Pull request overview

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

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.