Security and performance hardening from rust-engineer audit

- Verify command checks agent.id matches requested domain
- Zeroize private key bytes in decode/encode_private_key
- Magic link: min code length 20, hint reduced to 2 chars, 1h max expiry
- CORS restricted to agent.json endpoint only
- Use exec() on Unix in sign command (no lingering process memory)
- write_secure uses random temp file suffix (collision-safe)
- agent_json save uses write_secure (0o600 perms)
- Added Content-Security-Policy: default-src 'none' header
- opt-level = 3 for better crypto throughput
- Rate limiter HashMap pre-allocated
- Deposit handler avoids unnecessary label clone
- Background cleanup logs warnings on failure

Author: hiteshjoshi

Cargo.toml

@@ -64,7 +64,7 @@ dirs = "6"
 zeroize = { version = "1.8.2", features = ["derive"] }
 
 [profile.release]
-opt-level = "s"
+opt-level = 3
 lto = true
 codegen-units = 1
 panic = "abort"

src/agent_json.rs

@@ -29,12 +29,7 @@ impl AgentJson {
     pub fn save(&self, path: &Path) -> Result<()> {
         let json =
             serde_json::to_string_pretty(self).context("Failed to serialize agent.json")?;
-        // Atomic write: tmp + rename to prevent corruption on crash
-        let tmp_path = path.with_extension("tmp");
-        std::fs::write(&tmp_path, &json)
-            .with_context(|| format!("Failed to write {}", tmp_path.display()))?;
-        std::fs::rename(&tmp_path, path)
-            .with_context(|| format!("Failed to rename {} to {}", tmp_path.display(), path.display()))
+        crate::config::write_secure(path, json.as_bytes())
     }
 
     pub fn load(path: &Path) -> Result<Self> {

src/config.rs

@@ -1,14 +1,18 @@
 use anyhow::{Context, Result};
+use rand::RngCore;
 use std::path::{Path, PathBuf};
 
 pub fn write_secure(path: &Path, data: &[u8]) -> Result<()> {
     use std::io::Write;
     use std::os::unix::fs::OpenOptionsExt;
 
-    // Atomic write: create .tmp with 0600 from the start, then rename.
+    // Atomic write: create temp file with 0600 from the start, then rename.
     // Using OpenOptionsExt::mode() sets permissions at creation time,
     // eliminating the TOCTOU window where the file could be world-readable.
-    let tmp_path = path.with_extension("tmp");
+    // Random suffix prevents collisions between concurrent writers.
+    let mut suffix = [0u8; 8];
+    rand::rngs::OsRng.fill_bytes(&mut suffix);
+    let tmp_path = path.with_extension(format!("tmp.{}", hex::encode(suffix)));
     let mut file = std::fs::OpenOptions::new()
         .write(true)
         .create(true)

src/crypto/signing.rs

@@ -3,6 +3,7 @@ use base64::engine::general_purpose::STANDARD as BASE64;
 use base64::Engine;
 use ed25519_dalek::{Signature, Signer, SigningKey, Verifier, VerifyingKey};
 use rand::rngs::OsRng;
+use zeroize::Zeroizing;
 
 pub fn generate_keypair() -> (SigningKey, VerifyingKey) {
     let signing_key = SigningKey::generate(&mut OsRng);
@@ -37,14 +38,17 @@ pub fn decode_public_key(encoded: &str) -> Result<VerifyingKey> {
 }
 
 pub fn encode_private_key(signing_key: &SigningKey) -> String {
-    BASE64.encode(signing_key.to_bytes())
+    let key_bytes = Zeroizing::new(signing_key.to_bytes());
+    BASE64.encode(&*key_bytes)
 }
 
 pub fn decode_private_key(encoded: &str) -> Result<SigningKey> {
-    let bytes = BASE64.decode(encoded).context("Invalid base64 in private key")?;
-    let key_bytes: [u8; 32] = bytes
-        .try_into()
-        .map_err(|_| anyhow::anyhow!("Private key must be 32 bytes"))?;
+    let bytes = Zeroizing::new(BASE64.decode(encoded).context("Invalid base64 in private key")?);
+    if bytes.len() != 32 {
+        anyhow::bail!("Private key must be 32 bytes");
+    }
+    let mut key_bytes = Zeroizing::new([0u8; 32]);
+    key_bytes.copy_from_slice(&bytes);
     Ok(SigningKey::from_bytes(&key_bytes))
 }
 

src/magic_link.rs

@@ -11,15 +11,17 @@ fn hash_code(code: &str) -> String {
 }
 
 pub fn host(code: &str, expires: &str) -> Result<()> {
-    if code.len() < 8 {
-        anyhow::bail!("Code must be at least 8 characters (got {}). Use a longer code to prevent brute-force.", code.len());
+    if code.len() < 20 {
+        anyhow::bail!("Code must be at least 20 characters (got {}). Use a longer code to prevent brute-force.", code.len());
     }
     let duration = parse_duration(expires)?;
+    let max_secs: u64 = 3600; // 1 hour max for magic links
+    let capped_secs = duration.as_secs().min(max_secs);
     let expires_at = chrono::Utc::now().timestamp()
-        + i64::try_from(duration.as_secs()).map_err(|_| anyhow::anyhow!("Duration too large"))?;
+        + i64::try_from(capped_secs).map_err(|_| anyhow::anyhow!("Duration too large"))?;
 
     let code_hash = hash_code(code);
-    let hint = &code[..4.min(code.len())];
+    let hint = &code[..2.min(code.len())];
 
     let conn = db::open()?;
     conn.execute(
@@ -97,7 +99,7 @@ mod tests {
 
     fn insert_code(conn: &rusqlite::Connection, code: &str, expires_at: i64) {
         let code_hash = hash_code(code);
-        let hint = &code[..4.min(code.len())];
+        let hint = &code[..2.min(code.len())];
         conn.execute(
             "INSERT INTO magic_links (code_hash, hint, expires_at) VALUES (?1, ?2, ?3)",
             rusqlite::params![code_hash, hint, expires_at],

src/main.rs

@@ -121,7 +121,10 @@ async fn main() -> Result<()> {
             println!("  Public Key: {}", agent.public_key);
             println!("  Status:     {}", agent.status);
             println!("  Created:    {}", agent.created_at);
-            if agent.status == "active" {
+            if agent.id != domain {
+                println!("  WARNING:    agent.id '{}' does not match requested domain '{}'", agent.id, domain);
+                println!("  Result:     DOMAIN MISMATCH");
+            } else if agent.status == "active" {
                 println!("  Result:     VALID");
             } else {
                 println!("  Result:     {}", agent.status.to_uppercase());

src/server.rs

@@ -104,7 +104,7 @@ pub async fn run_server(credentials: Credentials) -> Result<()> {
         db: std::sync::Mutex::new(db_conn),
         tls_active,
         behind_proxy,
-        rate_limiter: std::sync::Mutex::new(HashMap::new()),
+        rate_limiter: std::sync::Mutex::new(HashMap::with_capacity(256)),
     });
 
     // Background task: clean expired magic links, old deposit nonces, and stale rate limiter entries
@@ -116,9 +116,13 @@ pub async fn run_server(credentials: Credentials) -> Result<()> {
             let _ = tokio::task::spawn_blocking(move || {
                 if let Ok(conn) = db_ref.db.lock() {
                     let now = chrono::Utc::now().timestamp();
-                    let _ = conn.execute("DELETE FROM magic_links WHERE expires_at <= ?1", [now]);
+                    if let Err(e) = conn.execute("DELETE FROM magic_links WHERE expires_at <= ?1", [now]) {
+                        tracing::warn!("Failed to clean expired magic links: {e}");
+                    }
                     let cutoff = now - 7 * 86400;
-                    let _ = conn.execute("DELETE FROM used_deposits WHERE used_at < ?1", [cutoff]);
+                    if let Err(e) = conn.execute("DELETE FROM used_deposits WHERE used_at < ?1", [cutoff]) {
+                        tracing::warn!("Failed to clean old deposit nonces: {e}");
+                    }
                 }
                 // Evict stale rate limiter entries
                 if let Ok(mut map) = db_ref.rate_limiter.lock() {
@@ -129,14 +133,20 @@ pub async fn run_server(credentials: Credentials) -> Result<()> {
         }
     });
 
+    // CORS only on the public agent.json endpoint; deposit and magic link
+    // endpoints are called by servers, not browsers, and don't need CORS.
     let cors = CorsLayer::new()
         .allow_origin(Any)
         .allow_methods(Any)
         .allow_headers(Any);
 
+    let public_routes = Router::new()
+        .route("/.well-known/agent.json", get(serve_agent_json))
+        .layer(cors);
+
     let app = Router::new()
         .route("/", get(root_redirect))
-        .route("/.well-known/agent.json", get(serve_agent_json))
+        .merge(public_routes)
         .route("/d/{token}", post(handle_deposit))
         .route("/m/{code}", get(handle_magic_link))
         .fallback(handle_404)
@@ -145,7 +155,6 @@ pub async fn run_server(credentials: Credentials) -> Result<()> {
             state.clone(),
             security_headers,
         ))
-        .layer(cors)
         .with_state(state);
 
     let pid_path = config::pid_path()?;
@@ -245,40 +254,39 @@ async fn handle_deposit(
     // DB operations in spawn_blocking.
     // Access vault_key via the Arc<AppState> reference inside the closure
     // to avoid copying the key out of its Zeroizing wrapper.
-    let label = payload.label.clone();
     let state_clone = state.clone();
     let body_clone = body;
     let deposit_result = tokio::task::spawn_blocking(move || {
         let conn = state_clone.db.lock()
             .map_err(|e| anyhow::anyhow!("DB mutex poisoned: {e}"))?;
         crate::deposit::claim_nonce_with_conn(&payload, &conn)?;
         crate::vault::vault_set_with_conn(&conn, &payload.label, &body_clone, state_clone.vault_key())?;
-        crate::deposit::log_deposit(&conn, &payload.label, &source_ip, &user_agent)
+        crate::deposit::log_deposit(&conn, &payload.label, &source_ip, &user_agent)?;
+        Ok::<_, anyhow::Error>(payload.label)
     }).await;
 
     match deposit_result {
-        Ok(Ok(())) => {},
+        Ok(Ok(label)) => {
+            info!("Deposit received: '{label}'");
+            let resp = DepositResponse { status: "deposited", label };
+            match serde_json::to_string(&resp) {
+                Ok(json) => (StatusCode::OK, [(header::CONTENT_TYPE, "application/json")], json).into_response(),
+                Err(_) => StatusCode::INTERNAL_SERVER_ERROR.into_response(),
+            }
+        }
         Ok(Err(e)) => {
             let err_msg = format!("{e}");
             if err_msg.contains("replay") || err_msg.contains("Nonce already used") {
                 return StatusCode::NOT_FOUND.into_response();
             }
             tracing::error!("Deposit failed: {}", e);
-            return StatusCode::INTERNAL_SERVER_ERROR.into_response();
+            StatusCode::INTERNAL_SERVER_ERROR.into_response()
         }
         Err(e) => {
             tracing::error!("Deposit task panicked: {}", e);
-            return StatusCode::INTERNAL_SERVER_ERROR.into_response();
+            StatusCode::INTERNAL_SERVER_ERROR.into_response()
         }
     }
-
-    info!("Deposit received: '{label}'");
-
-    let resp = DepositResponse { status: "deposited", label };
-    match serde_json::to_string(&resp) {
-        Ok(json) => (StatusCode::OK, [(header::CONTENT_TYPE, "application/json")], json).into_response(),
-        Err(_) => StatusCode::INTERNAL_SERVER_ERROR.into_response(),
-    }
 }
 
 async fn handle_magic_link(
@@ -342,6 +350,10 @@ async fn security_headers(
         header::REFERRER_POLICY,
         HeaderValue::from_static("no-referrer"),
     );
+    headers.insert(
+        header::CONTENT_SECURITY_POLICY,
+        HeaderValue::from_static("default-src 'none'"),
+    );
     if state.tls_active {
         headers.insert(
             header::STRICT_TRANSPORT_SECURITY,

src/sign.rs

@@ -1,4 +1,6 @@
-use anyhow::{Context, Result};
+use anyhow::Result;
+#[cfg(not(unix))]
+use anyhow::Context;
 
 use crate::config;
 use crate::credentials::Credentials;
@@ -40,29 +42,30 @@ pub fn run(command: &[String], dry_run: bool) -> Result<()> {
         return Ok(());
     }
 
-    // Drop sensitive data before process::exit (which skips destructors)
-    drop(signing_key);
-    drop(creds);
-
-    let status = std::process::Command::new(&new_cmd[0])
-        .args(&new_cmd[1..])
-        .status()
-        .with_context(|| format!("Failed to execute: {}", new_cmd[0]))?;
-
-    let exit_code = if let Some(code) = status.code() {
-        code
-    } else {
-        #[cfg(unix)]
-        {
-            use std::os::unix::process::ExitStatusExt;
-            128 + status.signal().unwrap_or(1)
-        }
-        #[cfg(not(unix))]
-        {
-            1
-        }
-    };
-    std::process::exit(exit_code);
+    // On Unix, exec replaces the process image entirely — no memory lingers,
+    // so there is no need to manually drop/zeroize sensitive data.
+    #[cfg(unix)]
+    {
+        use std::os::unix::process::CommandExt;
+        let err = std::process::Command::new(&new_cmd[0])
+            .args(&new_cmd[1..])
+            .exec();
+        // exec() only returns on error
+        anyhow::bail!("Failed to exec {}: {}", new_cmd[0], err);
+    }
+
+    #[cfg(not(unix))]
+    {
+        drop(signing_key);
+        drop(creds);
+
+        let status = std::process::Command::new(&new_cmd[0])
+            .args(&new_cmd[1..])
+            .status()
+            .with_context(|| format!("Failed to execute: {}", new_cmd[0]))?;
+
+        std::process::exit(status.code().unwrap_or(1));
+    }
 }
 
 // Pull the body from -d, --data, or --data-raw flags.