fix: Phase 3 — command injection, unwrap/expect panics, unsigned image acceptance

- VPN key gen: replaced sh -c with format string (command injection) with
  safe stdin piping to wg pubkey
- Secrets manager: replaced .unwrap() on path.parent() with proper error
- Tor proxy: replaced .expect("valid proxy") with continue on error
- Image verifier: added require_signatures flag, strict mode rejects
  unsigned images and missing cosign binary

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

core/archipelago/src/api/rpc/tor.rs

@@ -522,7 +522,11 @@ async fn notify_federation_peers_address_change(
                         });
                         let url = format!("http://{}/rpc/v1", &peer.onion);
                         let client = match reqwest::Client::builder()
-                            .proxy(reqwest::Proxy::all(format!("socks5h://{}", proxy)).unwrap_or_else(|_| reqwest::Proxy::all("socks5h://127.0.0.1:9050").expect("valid proxy")))
+                            .proxy(match reqwest::Proxy::all(format!("socks5h://{}", proxy))
+                                .or_else(|_| reqwest::Proxy::all("socks5h://127.0.0.1:9050")) {
+                                Ok(p) => p,
+                                Err(_) => continue,
+                            })
                             .timeout(std::time::Duration::from_secs(30))
                             .build()
                         {

core/archipelago/src/vpn.rs

@@ -121,30 +121,29 @@ pub async fn generate_wireguard_keypair() -> Result<(String, String)> {
         .trim()
         .to_string();
 
-    let _pubkey_output = tokio::process::Command::new("wg")
+    let mut child = tokio::process::Command::new("wg")
         .arg("pubkey")
         .stdin(std::process::Stdio::piped())
         .stdout(std::process::Stdio::piped())
+        .stderr(std::process::Stdio::piped())
         .spawn()
         .context("Failed to spawn wg pubkey")?;
 
-    // Use echo + pipe approach instead
-    let pubkey_output = tokio::process::Command::new("sh")
-        .arg("-c")
-        .arg(format!("echo '{}' | wg pubkey", private_key))
-        .output()
-        .await
-        .context("Failed to derive public key")?;
-
-    if !pubkey_output.status.success() {
-        anyhow::bail!(
-            "wg pubkey failed: {}",
-            String::from_utf8_lossy(&pubkey_output.stderr)
-        );
+    if let Some(mut stdin) = child.stdin.take() {
+        use tokio::io::AsyncWriteExt;
+        stdin.write_all(private_key.as_bytes()).await
+            .context("Failed to write private key to wg stdin")?;
     }
 
-    let public_key = String::from_utf8(pubkey_output.stdout)
-        .context("Invalid UTF-8 from wg pubkey")?
+    let output = child.wait_with_output().await
+        .context("wg pubkey process failed")?;
+
+    if !output.status.success() {
+        anyhow::bail!("wg pubkey failed: {}", String::from_utf8_lossy(&output.stderr));
+    }
+
+    let public_key = String::from_utf8(output.stdout)
+        .context("wg pubkey output is not valid UTF-8")?
         .trim()
         .to_string();
 

core/security/src/image_verifier.rs

@@ -6,28 +6,46 @@ use std::process::Command;
 use tracing::{info, warn};
 
 pub struct ImageVerifier {
-    cosign_public_key: Option<String>, // Public key for verification
+    cosign_public_key: Option<String>,
+    require_signatures: bool,
 }
 
 impl ImageVerifier {
     pub fn new(cosign_public_key: Option<String>) -> Self {
-        Self { cosign_public_key }
+        Self { cosign_public_key, require_signatures: false }
     }
-    
+
+    /// Create a verifier that requires all images to be signed.
+    pub fn new_strict(cosign_public_key: Option<String>) -> Self {
+        Self { cosign_public_key, require_signatures: true }
+    }
+
     /// Verify a container image signature
     pub async fn verify_image(&self, image: &str, signature: Option<&str>) -> Result<bool> {
         if signature.is_none() && self.cosign_public_key.is_none() {
+            if self.require_signatures {
+                return Err(anyhow::anyhow!(
+                    "Image '{}' has no signature and no cosign key is configured. \
+                     All container images must be signed for production use.",
+                    image
+                ));
+            }
             warn!("No signature provided for image: {}", image);
             return Ok(false);
         }
-        
+
         // Check if cosign is available
         let cosign_available = Command::new("cosign")
             .arg("version")
             .output()
             .is_ok();
-        
+
         if !cosign_available {
+            if self.require_signatures {
+                return Err(anyhow::anyhow!(
+                    "Cosign binary not found. Install cosign to verify container image signatures."
+                ));
+            }
             warn!("Cosign not available, skipping signature verification");
             return Ok(false);
         }

core/security/src/secrets_manager.rs

@@ -109,7 +109,9 @@ impl SecretsManager {
             .join(app_id)
             .join(format!("{}.secret", secret_id));
 
-        fs::create_dir_all(secret_path.parent().unwrap()).await?;
+        let parent = secret_path.parent()
+            .ok_or_else(|| anyhow::anyhow!("Invalid secret path: no parent directory for {:?}", secret_path))?;
+        fs::create_dir_all(parent).await?;
 
         let encrypted = self
             .encrypt(value.as_bytes())

loop/plan.md

@@ -190,7 +190,7 @@
 > server (command injection) or crash your entire node at will (unwrap panic). These are the most
 > dangerous code-level bugs found.
 
-- [ ] **Fix command injection in VPN key generation**: In `core/archipelago/src/vpn.rs`, find lines 132-137 where `sh -c` is used with `format!("echo '{}' | wg pubkey", private_key)`. This is a textbook command injection vulnerability. Replace the entire block with safe stdin piping:
+- [x] **Fix command injection in VPN key generation**: In `core/archipelago/src/vpn.rs`, find lines 132-137 where `sh -c` is used with `format!("echo '{}' | wg pubkey", private_key)`. This is a textbook command injection vulnerability. Replace the entire block with safe stdin piping:
   ```rust
   let mut child = tokio::process::Command::new("wg")
       .arg("pubkey")
@@ -223,7 +223,7 @@
   Build: `cd ~/archy/core && cargo clippy --all-targets --all-features`.
   Test: If VPN setup is available in the UI, test generating a WireGuard key.
 
-- [ ] **Fix unwrap crash in secrets manager**: In `core/security/src/secrets_manager.rs`, find line 112 with `secret_path.parent().unwrap()`. Replace with:
+- [x] **Fix unwrap crash in secrets manager**: In `core/security/src/secrets_manager.rs`, find line 112 with `secret_path.parent().unwrap()`. Replace with:
   ```rust
   let parent = secret_path.parent()
       .ok_or_else(|| anyhow::anyhow!("Invalid secret path: no parent directory for {:?}", secret_path))?;
@@ -232,7 +232,7 @@
   Search for ALL `.unwrap()` calls in the file: `grep -n "unwrap()" core/security/src/secrets_manager.rs`. For each one in a non-test function, evaluate whether it can actually fail and replace with `?` or `.ok_or_else()` if so. Common safe unwraps (e.g., after a `.is_some()` check) can stay but should get a comment explaining why they're safe.
   Build and deploy.
 
-- [ ] **Fix expect crash in Tor proxy fallback**: In `core/archipelago/src/api/rpc/tor.rs`, find line ~525 with `.expect("valid proxy")`. Replace the entire proxy chain with proper error handling:
+- [x] **Fix expect crash in Tor proxy fallback**: In `core/archipelago/src/api/rpc/tor.rs`, find line ~525 with `.expect("valid proxy")`. Replace the entire proxy chain with proper error handling:
   ```rust
   let proxy_url = format!("socks5h://{}", proxy);
   let proxy = reqwest::Proxy::all(&proxy_url)
@@ -242,7 +242,7 @@
   Search for ALL `.expect(` calls in non-test code: `grep -rn "\.expect(" core/archipelago/src/ --include="*.rs" | grep -v "#\[cfg(test)\]" | grep -v "mod tests"`. List them and fix any that could realistically fail in production.
   Build: `cargo clippy --all-targets --all-features`.
 
-- [ ] **Fix image verifier accepting unsigned images**: In `core/security/src/image_verifier.rs`, find lines 18-22 where the verifier returns `Ok(false)` for unsigned images. Change to:
+- [x] **Fix image verifier accepting unsigned images**: In `core/security/src/image_verifier.rs`, find lines 18-22 where the verifier returns `Ok(false)` for unsigned images. Change to:
   ```rust
   if signature.is_none() && self.cosign_public_key.is_none() {
       return Err(anyhow::anyhow!(
@@ -262,7 +262,7 @@
   ```
   Build and test. Note: this may cause existing unsigned images to fail verification. If the system doesn't use cosign yet, add a config flag `require_signatures: bool` that defaults to `false` for now but can be flipped to `true` when cosign is deployed.
 
-- [ ] **Verify Phase 3 — No more crash vectors**: Run these checks:
+- [x] **Verify Phase 3 — No more crash vectors**: Run these checks:
   1. `grep -rn 'Command::new("sh")' core/ --include="*.rs"` — should return zero results.
   2. `grep -rn "\.unwrap()" core/security/src/secrets_manager.rs | grep -v test` — should be minimal/commented.
   3. `grep -rn "\.expect(" core/archipelago/src/api/ --include="*.rs" | grep -v test | grep -v "// SAFE:"` — review each remaining expect.