fix: Address security vulnerabilities and configuration issues from PR #3 review#10
fix: Address security vulnerabilities and configuration issues from PR #3 review#10
Conversation
…fe secret handling Co-authored-by: romandidomizio <123122767+romandidomizio@users.noreply.github.com>
Co-authored-by: romandidomizio <123122767+romandidomizio@users.noreply.github.com>
…cy namespace mismatches Co-authored-by: romandidomizio <123122767+romandidomizio@users.noreply.github.com>
…labeling) Co-authored-by: romandidomizio <123122767+romandidomizio@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request addresses critical security vulnerabilities and configuration issues identified in PR #3, focusing on command injection prevention, secret handling improvements, and namespace configuration fixes. The changes enhance the security posture of the deployment scripts by eliminating unsafe patterns and improving operational robustness.
Changes:
- Security hardening: Replaced
eval-based command construction with array-based approach in 3 files to prevent command injection vulnerabilities - Secret management: Transitioned DigitalOcean token from CLI argument passing to Kubernetes Secret requirement, and improved .env file loading safety
- Configuration fixes: Added automatic namespace labeling for NetworkPolicy compatibility, pinned Kubernetes version to prevent drift, removed duplicate functions, and corrected documentation references
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| wordpress/helm/templates/secrets.yaml | Improved comment clarity for WordPress password field |
| wordpress/deploy.sh | Added automatic namespace labeling for NetworkPolicy ingress access compatibility |
| n8n/deploy.sh | Updated documentation references from old script paths to CLI tool, removed duplicate get_external_ip() function, corrected namespace reference in error messages |
| cli/lib/stacks.sh | Replaced eval with array-based Helm command construction, changed DigitalOcean token to require Kubernetes Secret reference |
| cli/lib/helm_utils.sh | Eliminated eval by using direct Helm calls with proper argument quoting |
| cli/lib/do_k8s.sh | Improved .env loading safety using set -a/set +a pattern, pinned K8S_VERSION to 1.33.1-do.0, converted doctl command to array-based construction |
|
@copilot apply changes based on the comments in this thread |
…s, improve .env security Co-authored-by: romandidomizio <123122767+romandidomizio@users.noreply.github.com>
…ents Co-authored-by: romandidomizio <123122767+romandidomizio@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot fix the three issues above that you commented on. the namespace should be ingress-nginx. i'm not sure what infra is supposed to be referencing. identify where infra is referenced, explain why its there, and replace it. |
…ect infra overrides Co-authored-by: romandidomizio <123122767+romandidomizio@users.noreply.github.com>
Fixed in commit 4dd6765. What was "infra" and why it was there:
Changes made:
Now all components use standard namespace naming and NetworkPolicy rules work correctly without overrides. |
Co-authored-by: romandidomizio <123122767+romandidomizio@users.noreply.github.com>
…wording Co-authored-by: romandidomizio <123122767+romandidomizio@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
…passwords Co-authored-by: romandidomizio <123122767+romandidomizio@users.noreply.github.com>
Fixed in commit 0cd03cb. Changes made:
|
|
@copilot apply changes based on the comments in this thread |
… kubectl label Co-authored-by: romandidomizio <123122767+romandidomizio@users.noreply.github.com>
Fixed in commit 5a12917. Changes made:
|
|
@copilot apply changes based on the comments in this thread |
…r messages self-contained Co-authored-by: romandidomizio <123122767+romandidomizio@users.noreply.github.com> Agent-Logs-Url: https://github.com/WeOwnNetwork/ai/sessions/bd4cfc78-078d-491c-b3cc-6fcde72c9f75
Fixed in commit b61c232. Changes made:
|
cli/lib/stacks.sh
Outdated
| log_error "Create the Secret securely using:" | ||
| log_error " kubectl create secret generic <secret-name> --from-literal=digitalocean_api_token=<your-token> -n external-dns" | ||
| log_error "Or use an env file (recommended):" | ||
| log_error " echo 'digitalocean_api_token=<your-token>' > /tmp/do-token.env" | ||
| log_error " kubectl create secret generic <secret-name> --from-env-file=/tmp/do-token.env -n external-dns" | ||
| log_error " rm /tmp/do-token.env" | ||
| log_error "" |
There was a problem hiding this comment.
The ExternalDNS guidance here suggests creating the Secret using --from-literal and writing the token to /tmp. Both patterns can leak secrets (shell history/process list; world-readable temp paths). Update the message to only recommend a secure approach (e.g., using mktemp with a trap cleanup and kubectl create secret ... --from-env-file, or piping from stdin) and avoid mentioning --from-literal or fixed /tmp paths.
| log_error "Create the Secret securely using:" | |
| log_error " kubectl create secret generic <secret-name> --from-literal=digitalocean_api_token=<your-token> -n external-dns" | |
| log_error "Or use an env file (recommended):" | |
| log_error " echo 'digitalocean_api_token=<your-token>' > /tmp/do-token.env" | |
| log_error " kubectl create secret generic <secret-name> --from-env-file=/tmp/do-token.env -n external-dns" | |
| log_error " rm /tmp/do-token.env" | |
| log_error "" | |
| log_error "Create the Secret securely using a temporary env file:" | |
| log_error " AUTH_FILE=\"\$(mktemp)\"" | |
| log_error " trap 'rm -f \"\$AUTH_FILE\"' EXIT" | |
| log_error " cat > \"\$AUTH_FILE\" << 'EOF'" | |
| log_error "digitalocean_api_token=<your-token>" | |
| log_error "EOF" | |
| log_error " kubectl create secret generic <secret-name> --from-env-file=\"\$AUTH_FILE\" -n external-dns" | |
| log_error "" | |
| log_error "Alternatively, you can pipe the value from stdin without writing it to disk:" | |
| log_error " printf 'digitalocean_api_token=' | tr -d '\\n'; read -s TOKEN; echo; \\" | |
| log_error " kubectl create secret generic <secret-name> --from-env-file=<(printf 'digitalocean_api_token=%s\n' \"\$TOKEN\") -n external-dns" | |
| log_error "" |
cli/lib/stacks.sh
Outdated
| if kubectl label namespace "$ns" name=ingress-nginx --overwrite; then | ||
| log_success "Labeled namespace '$ns' with name=ingress-nginx for NetworkPolicy access" | ||
| else | ||
| log_error "Failed to label namespace '$ns' - NetworkPolicy may not work correctly" |
There was a problem hiding this comment.
If labeling the ingress-nginx namespace fails, the function still returns success, even though NetworkPolicies for apps will likely block ingress without the name=ingress-nginx label. Treat this as a hard failure (return non-zero) so callers don’t proceed thinking the infra stack is healthy when it isn’t.
| log_error "Failed to label namespace '$ns' - NetworkPolicy may not work correctly" | |
| log_error "Failed to label namespace '$ns' - NetworkPolicy may not work correctly; treating as hard failure" | |
| return 1 |
cli/lib/do_k8s.sh
Outdated
| # First arg is always cluster_name (with default), remaining args are positional | ||
| local cluster_name=${1:-${CLUSTER_NAME:-weown-cluster}} | ||
| local pool_name=$2 | ||
| local size=$3 | ||
| local count=$4 | ||
| local tags=$5 # Optional setup as "role=value" | ||
|
|
||
| if [ -z "$pool_name" ] || [ -z "$size" ] || [ -z "$count" ]; then | ||
| log_error "Usage: create_node_pool <cluster_name> <pool_name> <size> <count> [label]" | ||
| log_error " (cluster_name defaults to \$CLUSTER_NAME or 'weown-cluster' if not provided)" |
There was a problem hiding this comment.
The usage note says cluster_name “defaults … if not provided”, but with the current positional parsing ($1=cluster_name, $2=pool_name, …) you can’t omit cluster_name while still providing the required pool args (the pool name would be treated as the cluster name). Either (a) make cluster_name truly optional by switching parsing based on arg count (e.g., 3 args => pool_name/size/count; 4+ => cluster_name/pool_name/size/count), or (b) update the usage text to make cluster_name required in practice.
| # First arg is always cluster_name (with default), remaining args are positional | |
| local cluster_name=${1:-${CLUSTER_NAME:-weown-cluster}} | |
| local pool_name=$2 | |
| local size=$3 | |
| local count=$4 | |
| local tags=$5 # Optional setup as "role=value" | |
| if [ -z "$pool_name" ] || [ -z "$size" ] || [ -z "$count" ]; then | |
| log_error "Usage: create_node_pool <cluster_name> <pool_name> <size> <count> [label]" | |
| log_error " (cluster_name defaults to \$CLUSTER_NAME or 'weown-cluster' if not provided)" | |
| # cluster_name is optional: | |
| # - 3 args => <pool_name> <size> <count> (cluster_name from \$CLUSTER_NAME or 'weown-cluster') | |
| # - 4+ args => <cluster_name> <pool_name> <size> <count> [label] | |
| local cluster_name | |
| local pool_name | |
| local size | |
| local count | |
| local tags # Optional setup as "role=value" | |
| if [ "$#" -eq 3 ]; then | |
| # Form: create_node_pool <pool_name> <size> <count> | |
| cluster_name=${CLUSTER_NAME:-weown-cluster} | |
| pool_name=$1 | |
| size=$2 | |
| count=$3 | |
| tags= | |
| elif [ "$#" -ge 4 ]; then | |
| # Form: create_node_pool <cluster_name> <pool_name> <size> <count> [label] | |
| cluster_name=${1:-${CLUSTER_NAME:-weown-cluster}} | |
| pool_name=$2 | |
| size=$3 | |
| count=$4 | |
| tags=$5 | |
| else | |
| log_error "Usage:" | |
| log_error " create_node_pool <pool_name> <size> <count>" | |
| log_error " create_node_pool <cluster_name> <pool_name> <size> <count> [label]" | |
| log_error " (cluster_name defaults to \$CLUSTER_NAME or 'weown-cluster' if not provided)" | |
| return 1 | |
| fi | |
| if [ -z "$pool_name" ] || [ -z "$size" ] || [ -z "$count" ]; then | |
| log_error "Usage:" | |
| log_error " create_node_pool <pool_name> <size> <count>" | |
| log_error " create_node_pool <cluster_name> <pool_name> <size> <count> [label]" | |
| log_error " (cluster_name defaults to \$CLUSTER_NAME or 'weown-cluster' if not provided)" |
|
@copilot apply changes based on the comments in this thread |
…error, make cluster_name truly optional Co-authored-by: romandidomizio <123122767+romandidomizio@users.noreply.github.com> Agent-Logs-Url: https://github.com/WeOwnNetwork/ai/sessions/2f54cc7d-fa5f-466f-92b9-2045c4da7432
Fixed in commit 3656bed. Changes made:
|
Addressing PR Review Comments - Complete ✅
Original Security Fixes (PR #3) ✅
evalwith array-based command construction in 3 filesCode Review Improvements (Round 1) ✅
cli/lib/stacks.shhelm_utils.shsecretName+ proper key name)Code Review Improvements (Round 2) ✅
Namespace Consistency Fix (Round 3) ✅
infrato standard namesname=infraingress-nginxafter deploymentSecurity & Robustness Improvements (Round 4) ✅
--setto--set-stringError Handling & Return Codes (Round 5) ✅
install_selectionfunction for proper error propagationwordpress/deploy.shto check command successcreate_node_poolusage message to match actual function signatureCritical Error Handling & Documentation (Round 6) ✅
create_node_poolusage to reflect actual positional parsing with default noteSecurity Best Practices & Optional Parameters (Round 7) ✅
All changes validated with shellcheck and security best practices enforced throughout.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.