From 1e597559a247f7ea305968b7635f0263edc28aee Mon Sep 17 00:00:00 2001 From: voson Date: Sun, 22 Mar 2026 15:40:02 +0800 Subject: [PATCH] feat(core): FK for user_id columns; MCP search requires user - Add fk_entries_user_id, fk_entries_history_user_id, fk_audit_log_user_id (ON DELETE SET NULL) - Add scripts/cleanup-orphan-user-ids.sql for pre-deploy orphan user_id cleanup - Remove deprecated SERVER_MASTER_KEY / per-user key wrap helpers from secrets-core - secrets-mcp: require authenticated user for secrets_search; improve body-read failure response - Bump secrets-mcp to 0.2.1 Made-with: Cursor --- Cargo.lock | 2 +- crates/secrets-core/src/crypto.rs | 74 ----------------------------- crates/secrets-core/src/db.rs | 31 ++++++++++++ crates/secrets-mcp/Cargo.toml | 2 +- crates/secrets-mcp/src/logging.rs | 25 +++++++--- crates/secrets-mcp/src/tools.rs | 12 ++--- scripts/cleanup-orphan-user-ids.sql | 22 +++++++++ 7 files changed, 80 insertions(+), 88 deletions(-) create mode 100644 scripts/cleanup-orphan-user-ids.sql diff --git a/Cargo.lock b/Cargo.lock index a201b32..d423c35 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1968,7 +1968,7 @@ dependencies = [ [[package]] name = "secrets-mcp" -version = "0.2.0" +version = "0.2.1" dependencies = [ "anyhow", "askama", diff --git a/crates/secrets-core/src/crypto.rs b/crates/secrets-core/src/crypto.rs index 14b2990..48d0473 100644 --- a/crates/secrets-core/src/crypto.rs +++ b/crates/secrets-core/src/crypto.rs @@ -55,35 +55,6 @@ pub fn decrypt_json(master_key: &[u8; 32], data: &[u8]) -> Result { serde_json::from_slice(&bytes).context("deserialize decrypted JSON") } -// ─── Per-user key management (DEPRECATED — kept only for migration) ─────────── - -/// Generate a new random 32-byte per-user encryption key. -#[allow(dead_code)] -pub fn generate_user_key() -> [u8; 32] { - use aes_gcm::aead::rand_core::RngCore; - let mut key = [0u8; 32]; - OsRng.fill_bytes(&mut key); - key -} - -/// Wrap a per-user key with the server master key using AES-256-GCM. -#[allow(dead_code)] -pub fn wrap_user_key(server_master_key: &[u8; 32], user_key: &[u8; 32]) -> Result> { - encrypt(server_master_key, user_key.as_ref()) -} - -/// Unwrap a per-user key using the server master key. -#[allow(dead_code)] -pub fn unwrap_user_key(server_master_key: &[u8; 32], wrapped: &[u8]) -> Result<[u8; 32]> { - let bytes = decrypt(server_master_key, wrapped)?; - if bytes.len() != 32 { - bail!("unwrapped user key has unexpected length {}", bytes.len()); - } - let mut key = [0u8; 32]; - key.copy_from_slice(&bytes); - Ok(key) -} - // ─── Client-supplied key extraction ────────────────────────────────────────── /// Parse a 64-char hex string (from X-Encryption-Key header) into a 32-byte key. @@ -100,33 +71,6 @@ pub fn extract_key_from_hex(hex_str: &str) -> Result<[u8; 32]> { Ok(key) } -// ─── Server master key ──────────────────────────────────────────────────────── - -/// Load the server master key from `SERVER_MASTER_KEY` environment variable (64 hex chars). -pub fn load_master_key_auto() -> Result<[u8; 32]> { - let hex_str = std::env::var("SERVER_MASTER_KEY").map_err(|_| { - anyhow::anyhow!( - "SERVER_MASTER_KEY is not set. \ - Generate one with: openssl rand -hex 32" - ) - })?; - - if hex_str.is_empty() { - bail!("SERVER_MASTER_KEY is set but empty"); - } - - let bytes = hex::decode_hex(hex_str.trim())?; - if bytes.len() != 32 { - bail!( - "SERVER_MASTER_KEY must be 64 hex chars (32 bytes), got {} bytes", - bytes.len() - ); - } - let mut key = [0u8; 32]; - key.copy_from_slice(&bytes); - Ok(key) -} - // ─── Public hex helpers ─────────────────────────────────────────────────────── pub mod hex { @@ -186,22 +130,4 @@ mod tests { let dec = decrypt_json(&key, &enc).unwrap(); assert_eq!(dec, value); } - - #[test] - fn user_key_wrap_unwrap_roundtrip() { - let server_key = [0xABu8; 32]; - let user_key = [0xCDu8; 32]; - let wrapped = wrap_user_key(&server_key, &user_key).unwrap(); - let unwrapped = unwrap_user_key(&server_key, &wrapped).unwrap(); - assert_eq!(unwrapped, user_key); - } - - #[test] - fn user_key_wrap_wrong_server_key_fails() { - let server_key1 = [0xABu8; 32]; - let server_key2 = [0xEFu8; 32]; - let user_key = [0xCDu8; 32]; - let wrapped = wrap_user_key(&server_key1, &user_key).unwrap(); - assert!(unwrap_user_key(&server_key2, &wrapped).is_err()); - } } diff --git a/crates/secrets-core/src/db.rs b/crates/secrets-core/src/db.rs index f22e270..4ec576b 100644 --- a/crates/secrets-core/src/db.rs +++ b/crates/secrets-core/src/db.rs @@ -156,6 +156,37 @@ pub async fn migrate(pool: &PgPool) -> Result<()> { CREATE INDEX IF NOT EXISTS idx_oauth_accounts_user ON oauth_accounts(user_id); CREATE UNIQUE INDEX IF NOT EXISTS idx_oauth_accounts_user_provider ON oauth_accounts(user_id, provider); + + -- FK: user_id columns -> users(id) (nullable = legacy rows; ON DELETE SET NULL) + DO $$ BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint WHERE conname = 'fk_entries_user_id' + ) THEN + ALTER TABLE entries + ADD CONSTRAINT fk_entries_user_id + FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE SET NULL; + END IF; + END $$; + + DO $$ BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint WHERE conname = 'fk_entries_history_user_id' + ) THEN + ALTER TABLE entries_history + ADD CONSTRAINT fk_entries_history_user_id + FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE SET NULL; + END IF; + END $$; + + DO $$ BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint WHERE conname = 'fk_audit_log_user_id' + ) THEN + ALTER TABLE audit_log + ADD CONSTRAINT fk_audit_log_user_id + FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE SET NULL; + END IF; + END $$; "#, ) .execute(pool) diff --git a/crates/secrets-mcp/Cargo.toml b/crates/secrets-mcp/Cargo.toml index b9ef236..8b0e268 100644 --- a/crates/secrets-mcp/Cargo.toml +++ b/crates/secrets-mcp/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "secrets-mcp" -version = "0.2.0" +version = "0.2.1" edition.workspace = true [[bin]] diff --git a/crates/secrets-mcp/src/logging.rs b/crates/secrets-mcp/src/logging.rs index fc89056..c50dc34 100644 --- a/crates/secrets-mcp/src/logging.rs +++ b/crates/secrets-mcp/src/logging.rs @@ -5,11 +5,11 @@ use axum::{ body::{Body, Bytes, to_bytes}, extract::{ConnectInfo, Request}, http::{ - HeaderMap, Method, + HeaderMap, Method, StatusCode, header::{CONTENT_LENGTH, CONTENT_TYPE, USER_AGENT}, }, middleware::Next, - response::Response, + response::{IntoResponse, Response}, }; /// Axum middleware that logs structured info for every HTTP request. @@ -68,10 +68,23 @@ pub async fn request_logging_middleware(req: Request, next: Next) -> Response { } Err(e) => { tracing::warn!(path, error = %e, "failed to buffer MCP request body for logging"); - // Reconstruct with empty body; request was consumed — return 500. - // This branch is highly unlikely in practice. - let resp = next.run(Request::from_parts(parts, Body::empty())).await; - return resp; + let elapsed = start.elapsed().as_millis(); + tracing::info!( + method = method.as_str(), + path, + status = StatusCode::INTERNAL_SERVER_ERROR.as_u16(), + elapsed_ms = elapsed, + client_ip = ip.as_deref(), + ua = ua.as_deref(), + content_length = content_len, + mcp_session = mcp_session.as_deref(), + "mcp request", + ); + return ( + StatusCode::INTERNAL_SERVER_ERROR, + "failed to read request body", + ) + .into_response(); } } } diff --git a/crates/secrets-mcp/src/tools.rs b/crates/secrets-mcp/src/tools.rs index 767b9dc..d6ec5a9 100644 --- a/crates/secrets-mcp/src/tools.rs +++ b/crates/secrets-mcp/src/tools.rs @@ -298,8 +298,8 @@ struct EnvMapInput { #[tool_router] impl SecretsService { #[tool( - description = "Search entries in the secrets store. Returns entries with metadata and \ - secret field names (not values). Use secrets_get to decrypt secret values.", + description = "Search entries in the secrets store. Requires Bearer API key. Returns \ + entries with metadata and secret field names (not values). Use secrets_get to decrypt secret values.", annotations( title = "Search Secrets", read_only_hint = true, @@ -312,7 +312,7 @@ impl SecretsService { ctx: RequestContext, ) -> Result { let t = Instant::now(); - let user_id = Self::user_id_from_ctx(&ctx)?; + let user_id = Self::require_user_id(&ctx)?; tracing::info!( tool = "secrets_search", ?user_id, @@ -334,11 +334,11 @@ impl SecretsService { sort: input.sort.as_deref().unwrap_or("name"), limit: input.limit.unwrap_or(20), offset: input.offset.unwrap_or(0), - user_id, + user_id: Some(user_id), }, ) .await - .map_err(|e| mcp_err_internal_logged("secrets_search", user_id, e))?; + .map_err(|e| mcp_err_internal_logged("secrets_search", Some(user_id), e))?; let summary = input.summary.unwrap_or(false); let entries: Vec = result @@ -849,7 +849,7 @@ impl ServerHandler for SecretsService { "Manage cross-device secrets and configuration securely. \ Data is encrypted with your passphrase-derived key. \ Include your 64-char hex key in the X-Encryption-Key header for all read/write operations. \ - Use secrets_search to discover entries (no key needed), \ + Use secrets_search to discover entries (Bearer token required; encryption key not needed), \ secrets_get to decrypt secret values, \ and secrets_add/secrets_update to write encrypted secrets." .to_string(), diff --git a/scripts/cleanup-orphan-user-ids.sql b/scripts/cleanup-orphan-user-ids.sql new file mode 100644 index 0000000..427b5b2 --- /dev/null +++ b/scripts/cleanup-orphan-user-ids.sql @@ -0,0 +1,22 @@ +-- Run against prod BEFORE deploying secrets-mcp with FK migration. +-- Requires: write access to SECRETS_DATABASE_URL. +-- Example: psql "$SECRETS_DATABASE_URL" -v ON_ERROR_STOP=1 -f scripts/cleanup-orphan-user-ids.sql + +BEGIN; + +UPDATE entries +SET user_id = NULL +WHERE user_id IS NOT NULL + AND NOT EXISTS (SELECT 1 FROM users u WHERE u.id = entries.user_id); + +UPDATE entries_history +SET user_id = NULL +WHERE user_id IS NOT NULL + AND NOT EXISTS (SELECT 1 FROM users u WHERE u.id = entries_history.user_id); + +UPDATE audit_log +SET user_id = NULL +WHERE user_id IS NOT NULL + AND NOT EXISTS (SELECT 1 FROM users u WHERE u.id = audit_log.user_id); + +COMMIT;