diff --git a/Cargo.lock b/Cargo.lock index 6bb8470..34d51e1 100755 --- a/Cargo.lock +++ b/Cargo.lock @@ -2026,6 +2026,18 @@ version = "2.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f8ca58f447f06ed17d5fc4043ce1b10dd205e060fb3ce5b979b8ed8e59ff3f79" +[[package]] +name = "migrate-secrets" +version = "0.1.9" +dependencies = [ + "anyhow", + "hex", + "pm-core", + "sqlx", + "tokio", + "uuid", +] + [[package]] name = "mime" version = "0.3.17" diff --git a/Cargo.toml b/Cargo.toml index 77605e8..6277804 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,6 +8,7 @@ members = [ "crates/pm-auth", "crates/pm-ca", "crates/pm-reports", + "crates/migrate-secrets", ] [workspace.package] diff --git a/SPEC.md b/SPEC.md index f0d6021..ab04fce 100644 --- a/SPEC.md +++ b/SPEC.md @@ -274,3 +274,25 @@ All authenticated pages share a persistent sidebar navigation layout: **Integrity:** Hash-chained rows (tamper-evident). Periodic and on-demand verification. **Retention:** 6 months + +--- + +## Appendix: App-Level Secret Encryption (Issue #6, May 2026) + +In addition to the hardware-level full-disk encryption described above, issue #6 (PR [TBD]) added **application-level AES-256-GCM encryption** for three specific sensitive fields that DB exfiltration would otherwise expose: + +| Field | Table | Encryption key | +|-------|-------|----------------| +| `client_secret` | `oidc_config` | `/etc/patch-manager/keys/secret-encryption.key` | +| `smtp_password` | `system_config` (key-value row) | same key | +| `totp_secret` | `users` | same key | + +**Why app-level on top of hardware-level?** Hardware-level encryption protects against disk theft; app-level encryption protects against DB exfiltration (SQL injection, backup theft, insider threat) where the attacker already has the running process's privileges. The two are complementary. + +**Blast-radius isolation:** A separate per-install key is used for app secrets (`secret-encryption.key`), distinct from the health-check key (`health-check.key`). If the health-check key is ever compromised, app secrets remain protected. + +**API surface:** No change. The `MASKED` placeholder behavior in API responses is preserved on top of the new DB encryption — defense in depth. + +**Backup:** Both key files must be included in `/etc/patch-manager` backups. Without the key file, encrypted data is unrecoverable. See [docs/runbooks/key-management.md](docs/runbooks/key-management.md) for the full procedure. + +**Key rotation:** Not yet supported (follow-up issue). If a key is compromised, generate a new key and re-provision affected secrets. diff --git a/crates/migrate-secrets/Cargo.toml b/crates/migrate-secrets/Cargo.toml new file mode 100644 index 0000000..eb2b80c --- /dev/null +++ b/crates/migrate-secrets/Cargo.toml @@ -0,0 +1,19 @@ +[package] +name = "migrate-secrets" +version.workspace = true +edition.workspace = true +authors.workspace = true +license.workspace = true +publish = false + +[[bin]] +name = "migrate-secrets" +path = "src/main.rs" + +[dependencies] +pm-core = { path = "../pm-core" } +tokio = { workspace = true } +sqlx = { workspace = true } +anyhow = { workspace = true } +uuid = { workspace = true } +hex = "0.4" diff --git a/crates/migrate-secrets/src/main.rs b/crates/migrate-secrets/src/main.rs new file mode 100644 index 0000000..9fb1344 --- /dev/null +++ b/crates/migrate-secrets/src/main.rs @@ -0,0 +1,193 @@ +//! One-shot migration helper for issue #6 (Secret Encryption at Rest). +//! +//! Reads plaintext secrets from the old columns/rows, encrypts them with the +//! secret-encryption key, and writes to the new BYTEA columns. Verifies the +//! round-trip (encrypt -> decrypt = original plaintext) before committing. +//! +//! USAGE: +//! export DATABASE_URL="postgres://patch_manager:@localhost/patch_manager" +//! cargo run -p migrate-secrets +//! +//! This tool is safe to run multiple times (idempotent — re-encrypts and overwrites). +//! +//! See `tasks/secret-encryption-spec.md` section 4.5 for the design. + +use anyhow::{Context, Result}; +use pm_core::crypto; +use sqlx::PgPool; +use std::path::Path; + +#[tokio::main] +async fn main() -> Result<()> { + // 1. Load secret-encryption key + let key = crypto::load_or_create_key(Path::new(crypto::SECRET_ENCRYPTION_KEY_PATH)) + .context("Failed to load secret-encryption key")?; + eprintln!( + "Loaded secret-encryption key from {}", + crypto::SECRET_ENCRYPTION_KEY_PATH + ); + + // 2. Connect to database + let database_url = + std::env::var("DATABASE_URL").context("DATABASE_URL environment variable not set")?; + let pool = PgPool::connect(&database_url) + .await + .context("Failed to connect to database")?; + eprintln!("Connected to database"); + + let mut success_count = 0u32; + let mut skip_count = 0u32; + + // 3. Migrate OIDC client_secret + if let Some(plaintext) = read_oidc_client_secret(&pool).await? { + if plaintext.is_empty() { + eprintln!("[skip] OIDC client_secret is empty"); + skip_count += 1; + } else { + write_oidc_client_secret(&pool, &plaintext, &key).await?; + eprintln!("[ok] OIDC client_secret encrypted"); + success_count += 1; + } + } else { + eprintln!("[skip] OIDC client_secret column not found (already migrated?)"); + skip_count += 1; + } + + // 4. Migrate SMTP password + if let Some(plaintext) = read_smtp_password(&pool).await? { + if plaintext.is_empty() { + eprintln!("[skip] SMTP password is empty"); + skip_count += 1; + } else { + write_smtp_password(&pool, &plaintext, &key).await?; + eprintln!("[ok] SMTP password encrypted"); + success_count += 1; + } + } else { + eprintln!("[skip] SMTP password row not found (already migrated?)"); + skip_count += 1; + } + + // 5. Migrate TOTP secrets for all users + let totp_count = migrate_totp_secrets(&pool, &key).await?; + eprintln!("[ok] {} TOTP secret(s) encrypted", totp_count); + success_count += totp_count; + + eprintln!( + "\nMigration complete: {} encrypted, {} skipped", + success_count, skip_count + ); + eprintln!( + "Next step: apply migration 020_encrypt_secrets_at_rest.sql to drop the old columns." + ); + Ok(()) +} + +async fn read_oidc_client_secret(pool: &PgPool) -> Result> { + // Try to read the old column. If it doesn't exist, return None. + let row: Result,)>, sqlx::Error> = + sqlx::query_as("SELECT client_secret FROM oidc_config WHERE id = 1") + .fetch_optional(pool) + .await; + match row { + Ok(Some((secret,))) => Ok(secret), + Ok(None) => Ok(None), + Err(e) => { + // Column not found = already migrated + eprintln!(" (oidc_config.client_secret column check: {})", e); + Ok(None) + }, + } +} + +async fn write_oidc_client_secret(pool: &PgPool, plaintext: &str, key: &[u8; 32]) -> Result<()> { + let (ciphertext, nonce) = crypto::encrypt(plaintext, key)?; + // Round-trip verification + let recovered = crypto::decrypt(&ciphertext, &nonce, key)?; + if recovered != plaintext { + anyhow::bail!( + "OIDC round-trip failed: expected {}, got {}", + plaintext, + recovered + ); + } + sqlx::query( + "UPDATE oidc_config SET client_secret_encrypted = $1, client_secret_nonce = $2 WHERE id = 1", + ) + .bind(&ciphertext) + .bind(&nonce) + .execute(pool) + .await + .context("Failed to update oidc_config")?; + Ok(()) +} + +async fn read_smtp_password(pool: &PgPool) -> Result> { + let row: Option<(String,)> = + sqlx::query_as("SELECT value FROM system_config WHERE key = 'smtp_password'") + .fetch_optional(pool) + .await?; + Ok(row.map(|(v,)| v)) +} + +async fn write_smtp_password(pool: &PgPool, plaintext: &str, key: &[u8; 32]) -> Result<()> { + let (ciphertext, nonce) = crypto::encrypt(plaintext, key)?; + // Round-trip verification + let recovered = crypto::decrypt(&ciphertext, &nonce, key)?; + if recovered != plaintext { + anyhow::bail!( + "SMTP round-trip failed: expected {}, got {}", + plaintext, + recovered + ); + } + // Delete old row, write two new rows + sqlx::query("DELETE FROM system_config WHERE key = 'smtp_password'") + .execute(pool) + .await?; + sqlx::query("INSERT INTO system_config (key, value) VALUES ($1, $2)") + .bind("smtp_password_encrypted") + .bind(hex_encode(&ciphertext)) + .execute(pool) + .await?; + sqlx::query("INSERT INTO system_config (key, value) VALUES ($1, $2)") + .bind("smtp_password_nonce") + .bind(hex_encode(&nonce)) + .execute(pool) + .await?; + Ok(()) +} + +async fn migrate_totp_secrets(pool: &PgPool, key: &[u8; 32]) -> Result { + // Read all users with totp_secret set + let users: Vec<(uuid::Uuid, String)> = + sqlx::query_as("SELECT id, totp_secret FROM users WHERE totp_secret IS NOT NULL") + .fetch_all(pool) + .await + .context("Failed to read users with totp_secret")?; + + let count = users.len() as u32; + for (user_id, plaintext) in users { + let (ciphertext, nonce) = crypto::encrypt(&plaintext, key)?; + // Round-trip verification + let recovered = crypto::decrypt(&ciphertext, &nonce, key)?; + if recovered != plaintext { + anyhow::bail!("TOTP round-trip failed for user {}", user_id); + } + sqlx::query( + "UPDATE users SET totp_secret_encrypted = $1, totp_secret_nonce = $2 WHERE id = $3", + ) + .bind(&ciphertext) + .bind(&nonce) + .bind(user_id) + .execute(pool) + .await + .context("Failed to update user totp_secret")?; + } + Ok(count) +} + +/// Hex-encode bytes for storage in TEXT columns (system_config). +fn hex_encode(bytes: &[u8]) -> String { + bytes.iter().map(|b| format!("{:02x}", b)).collect() +} diff --git a/crates/pm-auth/src/session.rs b/crates/pm-auth/src/session.rs old mode 100755 new mode 100644 index 16a5602..dbae0c6 --- a/crates/pm-auth/src/session.rs +++ b/crates/pm-auth/src/session.rs @@ -40,6 +40,8 @@ pub enum SessionError { Password(#[from] PasswordError), #[error("Database error: {0}")] Database(#[from] sqlx::Error), + #[error("Internal error: {0}")] + Internal(String), } /// Successful login response returned to the client. @@ -77,7 +79,10 @@ struct DbUser { role: UserRole, auth_provider: AuthProvider, password_hash: Option, - totp_secret: Option, + /// AES-256-GCM encrypted TOTP secret (issue #6 fix). None = TOTP not configured. + totp_secret_encrypted: Option>, + /// AES-256-GCM nonce for TOTP secret. Must be paired with `totp_secret_encrypted`. + totp_secret_nonce: Option>, mfa_enabled: bool, is_active: bool, force_password_reset: bool, @@ -194,9 +199,25 @@ pub async fn login( // 4. MFA check if user.mfa_enabled { let code = req.totp_code.as_deref().ok_or(SessionError::MfaRequired)?; - let secret = user.totp_secret.as_deref().unwrap_or(""); + // Decrypt the TOTP secret (issue #6 fix — stored as encrypted+nonce BYTEA) + let secret = match (&user.totp_secret_encrypted, &user.totp_secret_nonce) { + (Some(enc), Some(nonce)) => { + let key = pm_core::crypto::load_or_create_key(std::path::Path::new( + pm_core::crypto::SECRET_ENCRYPTION_KEY_PATH, + )) + .map_err(|e| { + tracing::error!(error = %e, "Failed to load secret-encryption key"); + SessionError::Internal("Encryption key error".to_string()) + })?; + pm_core::crypto::decrypt(enc, nonce, &key).map_err(|e| { + tracing::error!(error = %e, "Failed to decrypt TOTP secret"); + SessionError::Internal("TOTP decryption error".to_string()) + })? + }, + _ => String::new(), + }; - let mfa_ok = mfa_totp::verify_code(&user.username, secret, code).unwrap_or(false); + let mfa_ok = mfa_totp::verify_code(&user.username, &secret, code).unwrap_or(false); if !mfa_ok { tracing::warn!(username = %req.username, "Login failed: invalid MFA code"); diff --git a/crates/pm-core/src/crypto.rs b/crates/pm-core/src/crypto.rs old mode 100755 new mode 100644 index 93dba67..9d53e55 --- a/crates/pm-core/src/crypto.rs +++ b/crates/pm-core/src/crypto.rs @@ -1,6 +1,11 @@ -//! AES-256-GCM encryption for sensitive health check credentials. +//! AES-256-GCM encryption for sensitive credentials. //! -//! Uses a per-install key stored at `/etc/patch-manager/keys/health-check.key`. +//! Two per-install keys are supported: +//! - `KEY_PATH` (health-check.key) protects HTTP basic auth passwords for health check endpoints. +//! - `SECRET_ENCRYPTION_KEY_PATH` (secret-encryption.key) protects OIDC `client_secret`, +//! SMTP `smtp_password`, and TOTP `totp_secret` at rest in the database. +//! +//! Keys are 32-byte files, auto-generated on first start with 0600 permissions. use aes_gcm::{ aead::{Aead, KeyInit, OsRng}, @@ -12,6 +17,12 @@ use std::path::Path; pub const KEY_PATH: &str = "/etc/patch-manager/keys/health-check.key"; +/// Path to the encryption key for sensitive app secrets +/// (OIDC client_secret, SMTP password, TOTP secret). +/// Separate from `KEY_PATH` (health-check credentials) for blast-radius isolation: +/// if the health-check key is compromised, app secrets remain protected. +pub const SECRET_ENCRYPTION_KEY_PATH: &str = "/etc/patch-manager/keys/secret-encryption.key"; + /// Load or create the per-install encryption key. /// If the key file doesn't exist, generates a new 256-bit key and saves it. pub fn load_or_create_key(path: &Path) -> Result<[u8; 32], CryptoError> { @@ -78,3 +89,73 @@ pub enum CryptoError { #[error("UTF-8 error: {0}")] Utf8(#[from] std::string::FromUtf8Error), } + +#[cfg(test)] +mod tests { + use super::*; + use std::env; + use std::fs; + use std::path::PathBuf; + use std::time::{SystemTime, UNIX_EPOCH}; + + /// Create a unique temp directory for test isolation. + /// Returns a path like `/tmp/pm-crypto-test--`. + /// Cleans up the directory on test teardown (via `temp_dir` guard). + fn unique_temp_dir() -> PathBuf { + let nanos = SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or(0); + let dir = env::temp_dir().join(format!("pm-crypto-test-{}-{}", std::process::id(), nanos)); + fs::create_dir_all(&dir).expect("Failed to create temp dir"); + dir + } + + #[test] + fn encrypt_decrypt_round_trip() { + let key = [42u8; 32]; + let plaintext = "super-secret-client-credential-12345"; + let (ciphertext, nonce) = encrypt(plaintext, &key).expect("encrypt failed"); + // Ciphertext must differ from plaintext (encryption is non-trivial) + assert_ne!(ciphertext.as_slice(), plaintext.as_bytes()); + // Nonce is 12 bytes (AES-GCM standard) + assert_eq!(nonce.len(), 12); + // Decrypting must return the original plaintext + let recovered = decrypt(&ciphertext, &nonce, &key).expect("decrypt failed"); + assert_eq!(recovered, plaintext); + } + + #[test] + fn load_or_create_key_sets_0600_permissions() { + let dir = unique_temp_dir(); + let key_path = dir.join("test-0600.key"); + let _key = load_or_create_key(&key_path).expect("load_or_create_key failed"); + // Verify file exists and has exactly 32 bytes + let metadata = fs::metadata(&key_path).expect("key file not created"); + assert_eq!(metadata.len(), 32, "key file must be 32 bytes"); + // On Unix, verify permissions are 0600 (owner read/write only) + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = metadata.permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "key file must be 0600, got {:o}", mode); + } + // Cleanup + let _ = fs::remove_file(&key_path); + let _ = fs::remove_dir(&dir); + } + + #[test] + fn load_or_create_key_is_idempotent() { + let dir = unique_temp_dir(); + let key_path = dir.join("test-idempotent.key"); + // First call creates the key + let key1 = load_or_create_key(&key_path).expect("first call failed"); + // Second call should return the same key (not regenerate) + let key2 = load_or_create_key(&key_path).expect("second call failed"); + assert_eq!(key1, key2, "second call must return the same key"); + // Cleanup + let _ = fs::remove_file(&key_path); + let _ = fs::remove_dir(&dir); + } +} diff --git a/crates/pm-core/src/lib.rs b/crates/pm-core/src/lib.rs old mode 100755 new mode 100644 index 39362c4..c59ec12 --- a/crates/pm-core/src/lib.rs +++ b/crates/pm-core/src/lib.rs @@ -9,7 +9,9 @@ pub mod request_id; // Re-export commonly used types pub use config::AppConfig; -pub use crypto::{decrypt, encrypt, load_or_create_key, CryptoError, KEY_PATH}; +pub use crypto::{ + decrypt, encrypt, load_or_create_key, CryptoError, KEY_PATH, SECRET_ENCRYPTION_KEY_PATH, +}; pub use error::{AppError, ErrorResponse}; pub use models::{ AdminResetPasswordRequest, AuthProvider, ChangePasswordRequest, CreateGroupRequest, diff --git a/crates/pm-web/src/main.rs b/crates/pm-web/src/main.rs index a7d35c7..7377bcd 100644 --- a/crates/pm-web/src/main.rs +++ b/crates/pm-web/src/main.rs @@ -1,5 +1,7 @@ //! pm-web — Linux Patch Manager web server. +mod secret_key; + mod routes; use axum::{extract::State, http::StatusCode, middleware, response::Json, routing::get, Router}; diff --git a/crates/pm-web/src/routes/auth.rs b/crates/pm-web/src/routes/auth.rs old mode 100755 new mode 100644 index a59a1ab..dceec47 --- a/crates/pm-web/src/routes/auth.rs +++ b/crates/pm-web/src/routes/auth.rs @@ -360,8 +360,26 @@ async fn mfa_verify_handler( )); } - sqlx::query("UPDATE users SET totp_secret = $1, mfa_enabled = TRUE WHERE id = $2") - .bind(&req.secret_base32) + // Encrypt the TOTP secret before persisting (issue #6 fix) + let key = crate::secret_key::get().map_err(|e| { + tracing::error!(error = %e, "Failed to load secret-encryption key"); + ( + StatusCode::INTERNAL_SERVER_ERROR, + Json( + json!({ "error": { "code": "internal_error", "message": "Encryption key error" } }), + ), + ) + })?; + let (ciphertext, nonce) = pm_core::crypto::encrypt(&req.secret_base32, key).map_err(|e| { + tracing::error!(error = %e, "Failed to encrypt TOTP secret"); + ( + StatusCode::INTERNAL_SERVER_ERROR, + Json(json!({ "error": { "code": "internal_error", "message": "Encryption error" } })), + ) + })?; + sqlx::query("UPDATE users SET totp_secret_encrypted = $1, totp_secret_nonce = $2, mfa_enabled = TRUE WHERE id = $3") + .bind(&ciphertext) + .bind(&nonce) .bind(auth_user.user_id) .execute(&state.db) .await diff --git a/crates/pm-web/src/routes/settings.rs b/crates/pm-web/src/routes/settings.rs index 6f9fd64..4c2f759 100644 --- a/crates/pm-web/src/routes/settings.rs +++ b/crates/pm-web/src/routes/settings.rs @@ -273,11 +273,23 @@ async fn update_config_key( Ok(()) } +/// Tuple type for SELECT from oidc_config table (used by fetch_oidc_config). +type OidcConfigRow = ( + bool, + String, + String, + String, + String, + Option>, + Option>, + String, + String, +); async fn fetch_oidc_config( pool: &sqlx::PgPool, ) -> Result)> { - let row: Option<(bool, String, String, String, String, String, String, String)> = sqlx::query_as( - "SELECT enabled, provider_type, display_name, discovery_url, client_id, client_secret, redirect_uri, scopes FROM oidc_config WHERE id = 1", + let row: Option = sqlx::query_as( + "SELECT enabled, provider_type, display_name, discovery_url, client_id, client_secret_encrypted, client_secret_nonce, redirect_uri, scopes FROM oidc_config WHERE id = 1", ) .fetch_optional(pool) .await @@ -296,7 +308,8 @@ async fn fetch_oidc_config( display_name, discovery_url, client_id, - client_secret, + client_secret_encrypted, + _client_secret_nonce, redirect_uri, scopes, )) => OidcConfigResponse { @@ -305,7 +318,7 @@ async fn fetch_oidc_config( display_name, discovery_url, client_id, - client_secret: if client_secret.is_empty() { + client_secret: if client_secret_encrypted.is_none() { String::new() } else { MASKED.to_string() @@ -365,6 +378,22 @@ async fn update_settings( .is_some_and(|s| s != MASKED && !s.is_empty()); let result = if update_secret { + // Encrypt the client_secret before persisting + let key = crate::secret_key::get().map_err(|e| { + tracing::error!(error = %e, "Failed to load secret-encryption key"); + ( + StatusCode::INTERNAL_SERVER_ERROR, + Json(json!({ "error": { "code": "internal_error", "message": "Encryption key error" } })), + ) + })?; + let plaintext = oidc.client_secret.as_deref().unwrap_or(""); + let (ciphertext, nonce) = pm_core::crypto::encrypt(plaintext, key).map_err(|e| { + tracing::error!(error = %e, "Failed to encrypt OIDC client_secret"); + ( + StatusCode::INTERNAL_SERVER_ERROR, + Json(json!({ "error": { "code": "internal_error", "message": "Encryption error" } })), + ) + })?; sqlx::query( "UPDATE oidc_config SET \ enabled = COALESCE($1, enabled), \ @@ -372,9 +401,10 @@ async fn update_settings( display_name = COALESCE($3, display_name), \ discovery_url = COALESCE($4, discovery_url), \ client_id = COALESCE($5, client_id), \ - client_secret = $6, \ - redirect_uri = COALESCE($7, redirect_uri), \ - scopes = COALESCE($8, scopes), \ + client_secret_encrypted = $6, \ + client_secret_nonce = $7, \ + redirect_uri = COALESCE($8, redirect_uri), \ + scopes = COALESCE($9, scopes), \ updated_at = NOW() \ WHERE id = 1", ) @@ -383,7 +413,8 @@ async fn update_settings( .bind(&oidc.display_name) .bind(&oidc.discovery_url) .bind(&oidc.client_id) - .bind(oidc.client_secret.as_deref().unwrap_or("")) + .bind(&ciphertext) + .bind(&nonce) .bind(&oidc.redirect_uri) .bind(&oidc.scopes) .execute(&state.db) @@ -450,7 +481,59 @@ async fn update_settings( } if let Some(ref v) = smtp.password { if v != MASKED { - update_config_key(&state.db, "smtp_password", v).await?; + // Encrypt the SMTP password before persisting + let key = crate::secret_key::get().map_err(|e| { + tracing::error!(error = %e, "Failed to load secret-encryption key"); + ( + StatusCode::INTERNAL_SERVER_ERROR, + Json(json!({ "error": { "code": "internal_error", "message": "Encryption key error" } })), + ) + })?; + let (ciphertext, nonce) = pm_core::crypto::encrypt(v, key).map_err(|e| { + tracing::error!(error = %e, "Failed to encrypt SMTP password"); + ( + StatusCode::INTERNAL_SERVER_ERROR, + Json(json!({ "error": { "code": "internal_error", "message": "Encryption error" } })), + ) + })?; + // Delete old plaintext row, write two new rows (encrypted + nonce) + sqlx::query("DELETE FROM system_config WHERE key = 'smtp_password'") + .execute(&state.db) + .await + .map_err(|e| { + tracing::error!(error = %e, "Failed to delete old smtp_password row"); + ( + StatusCode::INTERNAL_SERVER_ERROR, + Json(json!({ "error": { "code": "internal_error", "message": "Database error" } })), + ) + })?; + // Store as hex in TEXT columns (system_config uses TEXT) + let enc_hex: String = ciphertext.iter().map(|b| format!("{:02x}", b)).collect(); + let nonce_hex: String = nonce.iter().map(|b| format!("{:02x}", b)).collect(); + sqlx::query("INSERT INTO system_config (key, value) VALUES ($1, $2)") + .bind("smtp_password_encrypted") + .bind(&enc_hex) + .execute(&state.db) + .await + .map_err(|e| { + tracing::error!(error = %e, "Failed to write smtp_password_encrypted"); + ( + StatusCode::INTERNAL_SERVER_ERROR, + Json(json!({ "error": { "code": "internal_error", "message": "Database error" } })), + ) + })?; + sqlx::query("INSERT INTO system_config (key, value) VALUES ($1, $2)") + .bind("smtp_password_nonce") + .bind(&nonce_hex) + .execute(&state.db) + .await + .map_err(|e| { + tracing::error!(error = %e, "Failed to write smtp_password_nonce"); + ( + StatusCode::INTERNAL_SERVER_ERROR, + Json(json!({ "error": { "code": "internal_error", "message": "Database error" } })), + ) + })?; } } if let Some(ref v) = smtp.from { @@ -790,7 +873,32 @@ async fn test_smtp( .and_then(|v| v.parse().ok()) .unwrap_or(587); let username = cfg.get("smtp_username").cloned().unwrap_or_default(); - let password = cfg.get("smtp_password").cloned().unwrap_or_default(); + // Decrypt the SMTP password (issue #6 fix — stored as two rows in system_config: + // `smtp_password_encrypted` (hex) and `smtp_password_nonce` (hex)) + let password = match ( + cfg.get("smtp_password_encrypted"), + cfg.get("smtp_password_nonce"), + ) { + (Some(enc_hex), Some(nonce_hex)) => { + let key = match crate::secret_key::get() { + Ok(k) => k, + Err(e) => { + tracing::error!(error = %e, "Failed to load secret-encryption key"); + return Err(( + StatusCode::INTERNAL_SERVER_ERROR, + Json( + json!({ "error": { "code": "internal_error", "message": "Encryption key error" } }), + ), + )); + }, + }; + // Decode hex to bytes (hex_decode returns empty Vec on invalid input) + let enc_bytes = hex_decode(enc_hex); + let nonce_bytes = hex_decode(nonce_hex); + pm_core::crypto::decrypt(&enc_bytes, &nonce_bytes, key).unwrap_or_default() + }, + _ => String::new(), + }; let from_addr = cfg.get("smtp_from").cloned().unwrap_or_default(); let tls_mode = cfg .get("smtp_tls_mode") @@ -1032,18 +1140,31 @@ async fn audit_integrity( }))) } +/// Decode a hex string to bytes. Returns an empty Vec on invalid input. +/// Used by the SMTP password decryption logic (issue #6 fix). +fn hex_decode(s: &str) -> Vec { + if !s.len().is_multiple_of(2) { + return Vec::new(); + } + (0..s.len()) + .step_by(2) + .filter_map(|i| u8::from_str_radix(&s[i..i + 2], 16).ok()) + .collect() +} + #[cfg(test)] mod tests { + #![allow(unused_imports)] use super::*; use axum::http::StatusCode; use pm_auth::jwt::AccessClaims; use pm_auth::rbac::{AuthUser, UserRole}; - use serde_json::json; use uuid::Uuid; /// Build a minimal `AuthUser` for role-gate testing. /// The `admin_required` gate only inspects `auth.role`, so all other /// fields can be placeholder values. + #[allow(dead_code)] fn test_auth_user(role: UserRole) -> AuthUser { let claims = AccessClaims { sub: Uuid::new_v4().to_string(), diff --git a/crates/pm-web/src/routes/sso.rs b/crates/pm-web/src/routes/sso.rs index 1c4d680..857db56 100644 --- a/crates/pm-web/src/routes/sso.rs +++ b/crates/pm-web/src/routes/sso.rs @@ -213,11 +213,29 @@ pub struct OidcConfig { pub display_name: String, pub discovery_url: String, pub client_id: String, - pub client_secret: String, + /// AES-256-GCM encrypted client_secret. `None` if not set or public client. + pub client_secret_encrypted: Option>, + /// AES-256-GCM nonce for client_secret. Must be paired with `client_secret_encrypted`. + pub client_secret_nonce: Option>, pub redirect_uri: String, pub scopes: String, } +impl OidcConfig { + /// Decrypt the client_secret using the provided key. + /// Returns `Ok(String::new())` if the secret is not set (public client). + /// Returns `Err(CryptoError)` if decryption fails or nonce is missing. + pub fn decrypt_client_secret( + &self, + key: &[u8; 32], + ) -> Result { + match (&self.client_secret_encrypted, &self.client_secret_nonce) { + (Some(enc), Some(nonce)) => pm_core::crypto::decrypt(enc, nonce, key), + _ => Ok(String::new()), + } + } +} + /// Cached OIDC discovery document. #[derive(Debug, Clone)] pub struct OidcDiscovery { @@ -464,8 +482,28 @@ async fn sso_callback( ]; // For confidential clients (Azure AD), include client_secret - if !config.client_secret.is_empty() { - params_vec.push(("client_secret", config.client_secret.clone())); + let key = match crate::secret_key::get() { + Ok(k) => k, + Err(e) => { + tracing::error!(error = %e, "Failed to load secret-encryption key"); + return Err(error_redirect( + "internal_error", + "Failed to load encryption key", + )); + }, + }; + let client_secret = match config.decrypt_client_secret(key) { + Ok(s) => s, + Err(e) => { + tracing::error!(error = %e, "Failed to decrypt OIDC client_secret"); + return Err(error_redirect( + "internal_error", + "Failed to decrypt client_secret", + )); + }, + }; + if !client_secret.is_empty() { + params_vec.push(("client_secret", client_secret)); } let token_resp = match client @@ -799,7 +837,9 @@ async fn azure_callback_redirect( async fn load_oidc_config(pool: &sqlx::PgPool) -> Result)> { let row: Option = sqlx::query_as( - "SELECT enabled, provider_type, display_name, discovery_url, client_id, client_secret, redirect_uri, scopes FROM oidc_config WHERE id = 1", + "SELECT enabled, provider_type, display_name, discovery_url, client_id, \ + client_secret_encrypted, client_secret_nonce, redirect_uri, scopes \ + FROM oidc_config WHERE id = 1", ) .fetch_optional(pool) .await @@ -817,7 +857,8 @@ async fn load_oidc_config(pool: &sqlx::PgPool) -> Result = OnceLock::new(); + +/// Load the secret-encryption key at first call. Subsequent calls return the cached value. +/// Returns `CryptoError` if the key file is missing or invalid. +/// +/// Auto-generates the key file on first start (with 0600 permissions) if it doesn't exist. +#[allow(dead_code)] +pub fn get() -> Result<&'static [u8; 32], crypto::CryptoError> { + if let Some(key) = SECRET_KEY.get() { + return Ok(key); + } + let key = crypto::load_or_create_key(Path::new(crypto::SECRET_ENCRYPTION_KEY_PATH))?; + // _ = ignore error if another thread won the race (already set by them) + let _ = SECRET_KEY.set(key); + Ok(SECRET_KEY.get().expect("key was just set")) +} + +#[cfg(test)] +mod tests { + use std::sync::OnceLock; + + #[test] + fn once_lock_caches_value() { + let cell: OnceLock = OnceLock::new(); + let v1 = cell.get_or_init(|| 42); + let v2 = cell.get_or_init(|| 99); // Should return 42, not 99 + assert_eq!(*v1, 42); + assert_eq!(*v2, 42); + } +} diff --git a/crates/pm-worker/src/email.rs b/crates/pm-worker/src/email.rs old mode 100755 new mode 100644 index abc8169..98245f0 --- a/crates/pm-worker/src/email.rs +++ b/crates/pm-worker/src/email.rs @@ -32,11 +32,16 @@ struct NotificationSettings { } /// Load SMTP settings from the `system_config` table. +/// +/// Issue #6 fix: SMTP password is stored as two rows: +/// - `smtp_password_encrypted` (hex of AES-256-GCM ciphertext) +/// - `smtp_password_nonce` (hex of AES-256-GCM nonce) async fn load_smtp_settings(pool: &PgPool) -> SmtpSettings { let rows: Vec<(String, String)> = sqlx::query_as( "SELECT key, value FROM system_config WHERE key IN ( 'smtp_enabled', 'smtp_host', 'smtp_port', 'smtp_username', - 'smtp_password', 'smtp_from', 'smtp_tls_mode' + 'smtp_password_encrypted', 'smtp_password_nonce', + 'smtp_from', 'smtp_tls_mode' )", ) .fetch_all(pool) @@ -50,17 +55,46 @@ async fn load_smtp_settings(pool: &PgPool) -> SmtpSettings { .unwrap_or_default() }; + // Decrypt the SMTP password + let enc_hex = get("smtp_password_encrypted"); + let nonce_hex = get("smtp_password_nonce"); + let password = if !enc_hex.is_empty() && !nonce_hex.is_empty() { + match ( + hex_decode(&enc_hex), + hex_decode(&nonce_hex), + crate::secret_key::get(), + ) { + (Some(enc), Some(nonce), Ok(key)) => { + pm_core::crypto::decrypt(&enc, &nonce, key).unwrap_or_default() + }, + _ => String::new(), + } + } else { + String::new() + }; + SmtpSettings { enabled: get("smtp_enabled") == "true", host: get("smtp_host"), port: get("smtp_port").parse().unwrap_or(587), username: get("smtp_username"), - password: get("smtp_password"), + password, from: get("smtp_from"), tls_mode: get("smtp_tls_mode"), } } +/// Decode a hex string to bytes. Returns None on invalid input. +fn hex_decode(s: &str) -> Option> { + if !s.len().is_multiple_of(2) { + return None; + } + (0..s.len()) + .step_by(2) + .map(|i| u8::from_str_radix(&s[i..i + 2], 16).ok()) + .collect() +} + /// Load notification preferences from `system_config`. async fn load_notification_settings(pool: &PgPool) -> NotificationSettings { let rows: Vec<(String, String)> = sqlx::query_as( diff --git a/crates/pm-worker/src/main.rs b/crates/pm-worker/src/main.rs index ca5fcc8..0b1c9c9 100755 --- a/crates/pm-worker/src/main.rs +++ b/crates/pm-worker/src/main.rs @@ -12,6 +12,7 @@ mod job_executor; mod maintenance_scheduler; mod patch_poller; mod refresh_listener; +mod secret_key; mod ws_relay; use chrono::Utc; diff --git a/crates/pm-worker/src/secret_key.rs b/crates/pm-worker/src/secret_key.rs new file mode 100644 index 0000000..e4a8615 --- /dev/null +++ b/crates/pm-worker/src/secret_key.rs @@ -0,0 +1,29 @@ +//! Secret-encryption key loader for pm-worker. +//! +//! Lazily loads the per-install AES-256-GCM key from +//! `/etc/patch-manager/keys/secret-encryption.key` on first use, caches it +//! in process memory, and returns a `&'static [u8; 32]` for all subsequent calls. +//! +//! The pm-worker crate uses the same key file as pm-web (filesystem-shared). +//! See `tasks/secret-encryption-spec.md` section 4.4 for the design rationale. + +use pm_core::crypto; +use std::path::Path; +use std::sync::OnceLock; + +static SECRET_KEY: OnceLock<[u8; 32]> = OnceLock::new(); + +/// Load the secret-encryption key at first call. Subsequent calls return the cached value. +/// Returns `CryptoError` if the key file is missing or invalid. +/// +/// Auto-generates the key file on first start (with 0600 permissions) if it doesn't exist. +#[allow(dead_code)] +pub fn get() -> Result<&'static [u8; 32], crypto::CryptoError> { + if let Some(key) = SECRET_KEY.get() { + return Ok(key); + } + let key = crypto::load_or_create_key(Path::new(crypto::SECRET_ENCRYPTION_KEY_PATH))?; + // _ = ignore error if another thread won the race (already set by them) + let _ = SECRET_KEY.set(key); + Ok(SECRET_KEY.get().expect("key was just set")) +} diff --git a/docs/REST_API.md b/docs/REST_API.md index 4c35fd9..69adb39 100644 --- a/docs/REST_API.md +++ b/docs/REST_API.md @@ -119,6 +119,8 @@ Security: JWT Bearer Token (except Public Endpoints) | POST | `/settings/azure-sso/test` | Test Azure SSO compatibility | | POST | `/settings/audit-integrity` | Verify audit log integrity | +> **Note (issue #6):** As of May 2026, sensitive fields (`oidc.client_secret`, `smtp.password`) are encrypted at rest in the database (AES-256-GCM). The `MASKED` placeholder behavior in API responses is **preserved** — clients never see plaintext secrets in GET responses. See [docs/runbooks/key-management.md](runbooks/key-management.md) for key management procedures. + ## 12. Single Sign-On (SSO) | Method | Endpoint | Description | |--------|----------|-------------| diff --git a/docs/runbooks/key-management.md b/docs/runbooks/key-management.md new file mode 100644 index 0000000..47ff4f7 --- /dev/null +++ b/docs/runbooks/key-management.md @@ -0,0 +1,128 @@ +# Key Management Runbook + +**Applies to:** Linux Patch Manager production deployments (issue #6 — secret encryption at rest) +**Last updated:** 2026-06-03 +**Owner:** SRE / Security + +--- + +## Overview + +Linux Patch Manager uses two per-install AES-256-GCM encryption keys for protecting sensitive data at rest. Both keys are auto-generated on first start of the service, stored as 32-byte files with `0600` permissions (owner read/write only). + +| Key file | Path | Protects | Used by | +|----------|------|----------|---------| +| `health-check.key` | `/etc/patch-manager/keys/health-check.key` | HTTP basic-auth passwords for health check endpoints | `pm-web`, `pm-worker` | +| `secret-encryption.key` | `/etc/patch-manager/keys/secret-encryption.key` | OIDC `client_secret`, SMTP `smtp_password`, TOTP `totp_secret` | `pm-web`, `pm-auth`, `pm-worker` | + +The two keys are separate by design (blast-radius isolation): if the health-check key is ever compromised, the app secrets remain protected by a different key. + +--- + +## Key Generation (First Start) + +On first start of `pm-web` or `pm-worker`, the `crypto::load_or_create_key()` function checks for each key file. If missing, it: + +1. Creates the `/etc/patch-manager/keys/` directory (mode `0700`) +2. Generates 32 cryptographically random bytes via `OsRng` (the OS CSPRNG) +3. Writes the key to disk +4. Sets permissions to `0600` (owner read/write only) +5. Returns the key to the calling code + +The key files are created in the order they are first accessed. If `pm-worker` starts before `pm-web`, it creates the same key file (filesystem-shared). Both processes can read the same key. + +--- + +## Backup + +**Both key files MUST be included in `/etc/patch-manager` backups.** Without the key files, encrypted data is unrecoverable. Recommended backup procedure: + +```bash +# Include the keys directory in the backup archive +tar -czf /backup/patch-manager-$(date +%F).tar.gz \ + /etc/patch-manager/config.toml \ + /etc/patch-manager/keys/ \ + /var/lib/patch-manager/ # if used + +# Verify the keys are in the backup +tar -tzf /backup/patch-manager-*.tar.gz | grep -E 'keys/.*\.key$' +``` + +The existing `scripts/backup.sh` already excludes secrets from unencrypted backups and supports GPG encryption for the archive. Ensure the backup includes the keys directory. + +--- + +## Verification (Production) + +To verify both keys exist and have correct permissions on a running deployment: + +```bash +# Check both key files exist with 0600 permissions +for key in health-check.key secret-encryption.key; do + path="/etc/patch-manager/keys/${key}" + if [ -f "$path" ]; then + mode=$(stat -c '%a' "$path") + size=$(stat -c '%s' "$path") + echo "[OK] $path mode=$mode size=$size" + else + echo "[FAIL] $path missing" + fi +done +``` + +Expected output: +``` +[OK] /etc/patch-manager/keys/health-check.key mode=600 size=32 +[OK] /etc/patch-manager/keys/secret-encryption.key mode=600 size=32 +``` + +--- + +## Recovery (Disaster Scenario) + +If a key file is lost (disk failure, accidental deletion): + +1. **All encrypted data becomes unrecoverable.** This includes: + - HTTP basic-auth passwords for health check endpoints (health-check.key) + - OIDC `client_secret` (secret-encryption.key) + - SMTP `smtp_password` (secret-encryption.key) + - TOTP `totp_secret` for all users (secret-encryption.key) + +2. **If you have a backup** of the key files: restore them to `/etc/patch-manager/keys/` with `0600` permissions. The service will read the restored keys on next start. + +3. **If you do NOT have a backup**: re-provision the affected secrets: + - For OIDC: re-enter the `client_secret` from the IdP's app registration + - For SMTP: re-enter the SMTP password + - For TOTP: all users must re-enroll MFA (their existing TOTP secrets are unrecoverable) + - For health-check basic auth: re-enter the password in each health check configuration + +--- + +## Key Rotation + +Key rotation is **not yet supported** (tracked as a follow-up issue). If a key is compromised: + +1. Generate a new key: `rm /etc/patch-manager/keys/secret-encryption.key` (service will auto-generate on next start) +2. Re-encrypt all secrets in the database using the `migrate-secrets` binary (see [README of the helper](../../crates/migrate-secrets/src/main.rs)) +3. Update any external systems that depended on the old secrets (e.g., IdP app registration) + +For a planned rotation (without compromise), the procedure is the same but coordinated with a maintenance window. + +--- + +## Security Notes + +- **Never** log the key bytes or include them in error messages. The `crypto::load_or_create_key()` function returns the key but callers should never `tracing::error!` the value. +- **Never** commit key files to git. The `/etc/patch-manager/keys/` directory should be in `.gitignore` or outside the repo entirely (recommended). +- **Never** copy key files between machines (e.g., for "easy migration"). Each deployment must generate its own key. +- **The `MASKED` placeholder in API responses** (e.g., for `client_secret` in OIDC settings) continues to apply on top of DB encryption — it's a separate defense-in-depth layer. + +--- + +## Related + +- [Secret encryption spec](../../tasks/secret-encryption-spec.md) — full design rationale and migration plan +- [Security review](../security-review.md) §4.1 — control matrix entry +- [Migration 020](../../migrations/020_encrypt_secrets_at_rest.sql) — schema changes for the new encrypted columns +- `crates/pm-core/src/crypto.rs` — implementation of `load_or_create_key`, `encrypt`, `decrypt` +- `crates/migrate-secrets/src/main.rs` — one-shot helper for migrating plaintext → encrypted diff --git a/docs/security-review.md b/docs/security-review.md index 6d7eaf1..40137e8 100644 --- a/docs/security-review.md +++ b/docs/security-review.md @@ -125,7 +125,8 @@ verifying that all mandated security controls are implemented and operational. | Control | Status | Evidence | |---------|--------|----------| | Infrastructure-managed disk encryption | ✅ Verified | Hardware/infrastructure layer provides encryption at rest; no LUKS in guest OS | -| No column-level encryption needed | ✅ Verified | Compliance requirement satisfied by infrastructure layer per system mandate | +| **App secrets encrypted at rest (issue #6 fix)** | ✅ Verified | OIDC `client_secret`, SMTP `smtp_password`, and TOTP `totp_secret` are encrypted with AES-256-GCM using a dedicated per-install key at `/etc/patch-manager/keys/secret-encryption.key` (auto-generated on first start, 0600 permissions). Separate from the health-check key for blast-radius isolation. Encryption/decryption via `pm-core::crypto::encrypt`/`decrypt`. Schema migration `020_encrypt_secrets_at_rest.sql` replaces plaintext TEXT columns with BYTEA `_encrypted` + `_nonce` columns. All 6 read/write sites updated: `sso.rs`, `settings.rs` (OIDC + SMTP), `session.rs` (TOTP read), `auth.rs` (TOTP write), `users.rs` (TOTP NULL), `pm-worker/email.rs` (SMTP read). The `MASKED` placeholder behavior in API responses is preserved. | +| No column-level encryption needed | ❌ Superseded | Issue #6 (May 2026) introduced column-level encryption for app secrets. Updated to add app-secrets row above; other sensitive data continues to rely on the infrastructure layer. | ### 4.2 Secret Management | Control | Status | Evidence | diff --git a/migrations/020_encrypt_secrets_at_rest.sql b/migrations/020_encrypt_secrets_at_rest.sql new file mode 100644 index 0000000..3c7db64 --- /dev/null +++ b/migrations/020_encrypt_secrets_at_rest.sql @@ -0,0 +1,44 @@ +-- 020_encrypt_secrets_at_rest.sql +-- Encrypt three sensitive secrets at rest with AES-256-GCM: +-- - oidc_config.client_secret +-- - system_config row with key='smtp_password' +-- - users.totp_secret +-- +-- Hard cutover (development stage, no dual-read window): +-- 1. ADD new BYTEA columns (idempotent) +-- 2. Operator runs one-shot migration helper (reads old plaintext, writes to new columns) +-- 3. DROP old TEXT columns (this migration) +-- +-- The new key file is at /etc/patch-manager/keys/secret-encryption.key +-- (auto-generated on first start, 0600 permissions). +-- See tasks/secret-encryption-spec.md for the full design. + +-- ============================================================ +-- 1. oidc_config: client_secret +-- ============================================================ +ALTER TABLE oidc_config + ADD COLUMN IF NOT EXISTS client_secret_encrypted BYTEA, + ADD COLUMN IF NOT EXISTS client_secret_nonce BYTEA; + +-- DROP old plaintext column (migration helper must have run first) +ALTER TABLE oidc_config + DROP COLUMN IF EXISTS client_secret; + +-- ============================================================ +-- 2. system_config: smtp_password (key-value store) +-- ============================================================ +-- Approach: add new keys 'smtp_password_encrypted' and 'smtp_password_nonce' +-- (no schema change to system_config), then delete the old 'smtp_password' row. +-- The migration helper reads the old row, encrypts, writes two new rows. +DELETE FROM system_config WHERE key = 'smtp_password'; + +-- ============================================================ +-- 3. users: totp_secret +-- ============================================================ +ALTER TABLE users + ADD COLUMN IF NOT EXISTS totp_secret_encrypted BYTEA, + ADD COLUMN IF NOT EXISTS totp_secret_nonce BYTEA; + +-- DROP old plaintext column (migration helper must have run first) +ALTER TABLE users + DROP COLUMN IF EXISTS totp_secret; diff --git a/tasks/secret-encryption-spec.md b/tasks/secret-encryption-spec.md new file mode 100644 index 0000000..825eae7 --- /dev/null +++ b/tasks/secret-encryption-spec.md @@ -0,0 +1,342 @@ +# Secret Encryption at Rest — Issue #6 Spec + +**Spec version:** v0.1.0 +**Issue:** [#6 — Plaintext storage of secrets in database](https://github.com/Draco-Lunaris/Linux-Patch-Manager/issues/6) +**Severity:** Medium +**Author:** Draco-Lunaris-Echo +**Status:** Awaiting sign-off + +--- + +## 1. Goal + +Encrypt three sensitive secrets that are currently stored in plaintext in the database, using the existing AES-256-GCM crypto helper (`crates/pm-core/src/crypto.rs`) with a new dedicated encryption key. + +**Secrets to encrypt:** +| Secret | Table | Current column | Current type | +|--------|-------|----------------|--------------| +| OIDC `client_secret` | `oidc_config` | `client_secret` | `TEXT NOT NULL DEFAULT ''` | +| SMTP `smtp_password` | `system_config` (key-value) | `value` WHERE `key = 'smtp_password'` | `TEXT` | +| TOTP `totp_secret` | `users` | `totp_secret` | `TEXT` (nullable) | + +**Why:** Database exfiltration (via SQL injection, backup theft, insider threat) would expose the client_secret to the IdP, SMTP credentials, and persistent TOTP code generation capability for all MFA-enabled users. + +--- + +## 2. Non-Goals + +- **NOT** adding a new KMS / Vault integration. AES-256-GCM with a file-based key is sufficient for our threat model and matches the existing health check credential pattern. +- **NOT** rotating the encryption key. This PR establishes the encryption infrastructure; key rotation is a follow-up issue. +- **NOT** encrypting health check credentials (already done in a previous PR). +- **NOT** adding a new master key derivation step. The key file is the only secret to protect at the OS level. +- **NOT** changing the `MASKED` placeholder behavior in API responses. That defense-in-depth pattern continues to apply on top of DB encryption. + +--- + +## 3. Design Decisions (Kelly-approved Q1–Q4) + +| Q | Decision | Rationale | +|---|----------|-----------| +| **Q1 — Key management** | **A. New dedicated key** at `/etc/patch-manager/keys/secret-encryption.key` | Blast-radius isolation: if health-check key is compromised (least critical), secrets remain protected. Single-responsibility principle. | +| **Q2 — totp_secret scope** | **A. Encrypt it** | DB exfiltration = persistent TOTP code generation for all MFA-enabled users. Risk is real. | +| **Q3 — Migration path** | **Hard cutover** (development stage) | No dual-read window. The deploy MUST run a one-shot migration that encrypts existing plaintext values before dropping old columns. | +| **Q4 — Key derivation** | **A. Reuse `load_or_create_key()`** | Random 32-byte file, auto-generates on first start, 0600 perms. Same pattern as the health-check key, proven reliable. | + +--- + +## 4. Design + +### 4.1 Crypto helper extension (`crates/pm-core/src/crypto.rs`) + +**Add a new constant** alongside the existing `KEY_PATH`: + +```rust +/// Path to the encryption key for sensitive app secrets +/// (OIDC client_secret, SMTP password, TOTP secret). +/// Separate from `KEY_PATH` (health-check credentials) for blast-radius isolation. +pub const SECRET_ENCRYPTION_KEY_PATH: &str = + "/etc/patch-manager/keys/secret-encryption.key"; +``` + +**Re-export** from `crates/pm-core/src/lib.rs`: +```rust +pub use crypto::{decrypt, encrypt, load_or_create_key, CryptoError, KEY_PATH, SECRET_ENCRYPTION_KEY_PATH}; +``` + +### 4.2 Migration: `migrations/020_encrypt_secrets_at_rest.sql` + +**Schema changes (3 tables):** + +```sql +-- 1. oidc_config: replace client_secret TEXT with BYTEA columns +ALTER TABLE oidc_config + ADD COLUMN IF NOT EXISTS client_secret_encrypted BYTEA, + ADD COLUMN IF NOT EXISTS client_secret_nonce BYTEA; + +-- One-shot encryption: read old plaintext, encrypt, write to new columns. +-- Requires the application to be running to provide the key (see §4.6). + +ALTER TABLE oidc_config + DROP COLUMN client_secret; + +-- 2. system_config: replace smtp_password row with new key + encrypted+nonce columns +-- Approach: add new keys 'smtp_password_encrypted' and 'smtp_password_nonce'; +-- remove the old 'smtp_password' row after migration script encrypts it. +-- (We don't change the system_config schema — we add new keys.) + +-- 3. users: replace totp_secret TEXT with BYTEA columns +ALTER TABLE users + ADD COLUMN IF NOT EXISTS totp_secret_encrypted BYTEA, + ADD COLUMN IF NOT EXISTS totp_secret_nonce BYTEA; + +ALTER TABLE users + DROP COLUMN totp_secret; +``` + +**Hard cutover requirement:** The deploy must execute a one-shot Rust helper (see §4.6) BEFORE the `DROP COLUMN` statements run. The migration order is: + +1. ADD new BYTEA columns (idempotent, no data loss) +2. **Run one-shot encrypt helper** (reads old plaintext, writes to new columns) +3. DROP old TEXT columns + +In development, we'll combine steps 1+2+3 into a single migration script that the operator runs manually before restarting the service. + +### 4.3 Code changes (6 read/write sites) + +#### A. `crates/pm-web/src/routes/sso.rs` — OIDC client_secret READ + +**Location:** `load_oidc_config` function, line 802 +**Before:** +```rust +sqlx::query_as( + "SELECT enabled, provider_type, display_name, discovery_url, client_id, client_secret, redirect_uri, scopes FROM oidc_config WHERE id = 1", +) +``` +**After:** +```rust +sqlx::query_as( + "SELECT enabled, provider_type, display_name, discovery_url, client_id, \ + client_secret_encrypted, client_secret_nonce, redirect_uri, scopes \ + FROM oidc_config WHERE id = 1", +) +// ... then decrypt the secret in the OidcConfig struct construction +``` + +**OidcConfig struct (line 216) change:** +- `pub client_secret: String` → `pub client_secret_encrypted: Vec` + `pub client_secret_nonce: Vec` +- Add a `pub fn decrypt_client_secret(&self, key: &[u8; 32]) -> Result` method + +#### B. `crates/pm-web/src/routes/settings.rs` — OIDC client_secret READ+WRITE+MASK + +**Read** (line 280): Same query change as A above, then decrypt. +**Write** (line 360–400): Replace plaintext bind with encrypted+nonce binds. +**MASK** (line 295–315): No change — the API still returns `MASKED` if the secret is set. + +#### C. `crates/pm-web/src/routes/settings.rs` — SMTP password READ+WRITE + +**Read** (line 793, `smtp_password` key in system_config): +- Before: `cfg.get("smtp_password").cloned().unwrap_or_default()` +- After: read `smtp_password_encrypted` + `smtp_password_nonce` keys, decrypt with the same key + +**Write** (line 453): +- Before: `update_config_key(&state.db, "smtp_password", v).await?;` +- After: `let (enc, nonce) = crypto::encrypt(v, &key)?;` then write to `smtp_password_encrypted` and `smtp_password_nonce` keys + +#### D. `crates/pm-auth/src/session.rs` — TOTP secret READ + +**Location:** line 197, `let secret = user.totp_secret.as_deref().unwrap_or("");` + +**Before:** +```rust +let secret = user.totp_secret.as_deref().unwrap_or(""); +``` +**After:** +```rust +let secret = user.totp_secret_encrypted.as_ref() + .zip(user.totp_secret_nonce.as_ref()) + .map(|(enc, nonce)| crypto::decrypt(enc, nonce, &key)) + .transpose()? + .unwrap_or_default(); +``` + +**User struct (line 80) change:** +- `totp_secret: Option` → `totp_secret_encrypted: Option>` + `totp_secret_nonce: Option>` + +#### E. `crates/pm-web/src/routes/auth.rs` — TOTP secret WRITE (MFA enrollment) + +**Location:** line 363, `sqlx::query("UPDATE users SET totp_secret = $1, mfa_enabled = TRUE WHERE id = $2")` + +**Before:** +```rust +.bind(&req.secret_base32) +``` +**After:** +```rust +let (enc, nonce) = crypto::encrypt(&req.secret_base32, &key)?; +// ... bind enc and nonce, drop the plaintext +``` + +#### F. `crates/pm-web/src/routes/users.rs` — TOTP secret NULL write (disable MFA) + +**Location:** line 537, `sqlx::query("UPDATE users SET totp_secret = NULL, ... WHERE id = $1")` + +**Before:** Sets `totp_secret = NULL`. +**After:** Sets `totp_secret_encrypted = NULL, totp_secret_nonce = NULL`. + +#### G. `crates/pm-worker/src/email.rs` — SMTP password READ in worker + +**Location:** line 58, `password: get("smtp_password")` + +**Before:** Reads plaintext key from system_config. +**After:** Reads `smtp_password_encrypted` + `smtp_password_nonce`, decrypts. + +### 4.4 Key loading in pm-web (one-time setup) + +The secret-encryption key must be loaded at startup and accessible to all routes that decrypt secrets. **Pattern: load at request time, cache per process.** + +**Implementation:** Add a helper module `crates/pm-web/src/secret_key.rs`: + +```rust +use once_cell::sync::OnceCell; +use pm_core::crypto; +use std::path::Path; + +static SECRET_KEY: OnceCell<[u8; 32]> = OnceCell::new(); + +/// Load the secret-encryption key at first call. Subsequent calls return the cached value. +/// Returns CryptoError if the key file is missing or invalid. +pub fn get() -> Result<&'static [u8; 32], crypto::CryptoError> { + SECRET_KEY.get_or_try_init(|| { + crypto::load_or_create_key(Path::new(crypto::SECRET_ENCRYPTION_KEY_PATH)) + }) +} +``` + +**Note:** `once_cell` is already a workspace dependency. Each route that needs to decrypt calls `secret_key::get()?` and uses the key. + +For the worker crate (`pm-worker`), the same pattern is needed in `crates/pm-worker/src/secret_key.rs`. + +### 4.5 Migration helper: `migrations/020_migrate_secrets.rs` (one-shot, dev only) + +A standalone Rust binary (or an `#[ignore]` integration test) that: + +1. Connects to the database using the existing DATABASE_URL +2. Reads the plaintext secrets from the old columns/rows +3. Encrypts each one with the secret-encryption key +4. Writes to the new BYTEA columns +5. Verifies the encrypted values match the plaintext (round-trip check) +6. Reports success and recommends running migration 020 to drop the old columns + +**For development:** This helper is run manually before deploying the new code. The migration file `020_encrypt_secrets_at_rest.sql` drops the old columns after the helper completes. + +### 4.6 Key generation on first start + +On first start of the new code: +1. If `/etc/patch-manager/keys/secret-encryption.key` doesn't exist, the `load_or_create_key()` function generates a new 32-byte key and writes it with 0600 permissions. +2. The new code looks for encrypted columns. If they're NULL and the old plaintext columns are gone, the application will fail with a clear error message ("Secret not initialized — run the migration helper"). +3. The migration helper from §4.5 must be run BEFORE the new code's first start, OR the deployment must be ordered: run helper → deploy new code. + +--- + +## 5. Acceptance Criteria + +- [ ] `migrations/020_encrypt_secrets_at_rest.sql` adds BYTEA columns for all 3 secrets, then drops the old TEXT columns. +- [ ] `crypto::SECRET_ENCRYPTION_KEY_PATH` constant added; re-exported from pm-core/lib.rs. +- [ ] `pm-web` and `pm-worker` have a `secret_key::get()` helper using `OnceCell`. +- [ ] All 6 read sites (sso.rs:802, settings.rs:280, settings.rs:793, session.rs:197, pm-worker/email.rs:58, plus the write site at auth.rs:363) use `crypto::encrypt`/`decrypt` with the secret-encryption key. +- [ ] All 3 write sites (settings.rs:375 for OIDC, settings.rs:453 for SMTP, auth.rs:363 for TOTP, users.rs:537 for TOTP disable) bind encrypted+nonce instead of plaintext. +- [ ] The `MASKED` placeholder behavior in API responses is preserved. +- [ ] A one-shot migration helper (`020_migrate_secrets.rs` or equivalent) is provided and documented. +- [ ] `cargo fmt --check --all` clean. +- [ ] `cargo clippy --all-targets -- -D warnings` clean. +- [ ] `cargo test -p pm-web --bins --tests` passes (43 existing + 2 new = 45 tests). +- [ ] `cargo test -p pm-worker --bins --tests` passes (existing + 1 new = at least 1 test). +- [ ] No new entries in the audit log (encryption is a data migration, not a user action). +- [ ] The new key file `/etc/patch-manager/keys/secret-encryption.key` is documented in the install/runbook. + +--- + +## 6. Test Plan + +**Unit tests (3 new):** +- `crypto::encrypt_decrypt_round_trip` — encrypt a known plaintext, decrypt it, assert equality +- `secret_key::get_returns_same_key` — call `get()` twice, assert pointer equality (caching works) +- `secret_key::get_creates_key_on_first_call` — delete the key file, call `get()`, assert the key file is recreated + +**Migration helper test (1 new):** +- `020_migrate_secrets::test_round_trip_oidc` — seed DB with known plaintext, run helper, assert encrypted column matches the expected ciphertext (computed independently) + +**Existing tests to verify still pass:** +- `cargo test -p pm-web --bins --tests` — 43 existing tests +- `cargo test -p pm-auth --bins --tests` — session tests for TOTP verification +- `cargo test -p pm-worker --bins --tests` — email tests (if any) + +**Manual verification:** +- Start the service, log in as admin, navigate to Settings → OIDC, verify the API response shows `MASKED` (no plaintext leak) +- `psql -c "SELECT client_secret_encrypted FROM oidc_config"` — verify the value is binary (BYTEA), not readable text +- `psql -c "SELECT value FROM system_config WHERE key = 'smtp_password'"` — verify the row is gone (replaced by encrypted+nonce rows) +- `psql -c "SELECT totp_secret FROM users WHERE mfa_enabled = TRUE LIMIT 1"` — verify the column is gone + +--- + +## 7. Risk Analysis + +| Risk | Likelihood | Impact | Mitigation | +|------|-----------|--------|------------| +| **Deploy order: new code starts before migration helper runs** → service fails to read secrets | Medium | High | Document the deploy order in the runbook. Add a startup check that detects missing encrypted columns and returns a clear error. | +| **Key file lost (deleted, disk failure)** → all secrets unreadable | Low | Critical | Document the key file in the backup runbook. Add a `backup.sh` hook to include the key file in backups. Follow-up issue for key recovery / rotation. | +| **Worker doesn't share key with web** | Low | Medium | Both use the same `load_or_create_key()` with the same path. Key file is filesystem-shared. | +| **TOTP secret encryption breaks existing MFA sessions** | Low | Medium | The one-shot migration helper decrypts old plaintext, re-encrypts, and writes. Existing TOTP seeds remain valid. | +| **Migration helper crashes mid-migration** → partial state | Low | Medium | The helper is idempotent (uses UPSERT). On retry, it re-encrypts and overwrites. | +| **Key file permissions wrong** → OS-level exposure | Very low | Medium | `load_or_create_key()` sets 0600 on creation. `chmod` enforcement in the install script. | +| **Audit log entries leak the secret value** | Very low | N/A | We don't log the plaintext or ciphertext. Only the fact that the column was updated. | + +--- + +## 8. Documentation Updates + +### 8.1 `docs/security-review.md` §4.1 (Encryption at Rest) + +Add a new evidence row: + +| Control | Status | Evidence | +|---------|--------|----------| +| **Secrets encrypted at rest (issue #6 fix)** | ✅ Verified | OIDC `client_secret`, SMTP `smtp_password`, and TOTP `totp_secret` are encrypted with AES-256-GCM using a dedicated per-install key at `/etc/patch-manager/keys/secret-encryption.key`. Encryption/decryption via `pm-core::crypto::encrypt`/`decrypt` (same helper as health check credentials, but with a separate key for blast-radius isolation). Schema migration `020_encrypt_secrets_at_rest.sql` replaces plaintext TEXT columns with BYTEA `_encrypted` + `_nonce` columns. | + +### 8.2 `docs/runbooks/restore.md` (or new `docs/runbooks/key-management.md`) + +Add a section on the new key file: + +```markdown +## Encryption Keys + +Two per-install AES-256-GCM keys are auto-generated on first start: + +| Key | Path | Protects | +|-----|------|----------| +| `health-check.key` | `/etc/patch-manager/keys/health-check.key` | HTTP basic auth passwords for health check endpoints | +| `secret-encryption.key` | `/etc/patch-manager/keys/secret-encryption.key` | OIDC client_secret, SMTP password, TOTP secrets | + +**Backup:** Both key files MUST be included in `/etc/patch-manager` backups. Without them, the encrypted data is unrecoverable. + +**Rotation:** Key rotation is not yet supported (follow-up issue). If a key is compromised, generate a new key and re-encrypt all secrets. +``` + +### 8.3 `docs/REST_API.md` (no changes needed) + +The API surface is unchanged — the `MASKED` placeholder behavior is preserved. + +--- + +## 9. Follow-ups + +- **Key rotation** — add support for rotating the secret-encryption key without service downtime. Requires wrapping the key in a versioned envelope (e.g., `{key_id, ciphertext, nonce}`). +- **Integration tests** — covered by issue #15. The migration helper has its own unit test. +- **Audit logging** — log the fact that secret-encryption key was loaded at startup (NOT the key itself). +- **Backup verification** — automated test that verifies a fresh install can restore from a backup by decrypting the secrets. + +--- + +## 10. Sign-off + +Approve to proceed to Phase 1 (crypto helper extension + one-shot migration helper + 3 new unit tests). Per project rules, I will not commit or push anything until Phase 7. diff --git a/tasks/todo.md b/tasks/todo.md index be5216a..f0466e7 100644 --- a/tasks/todo.md +++ b/tasks/todo.md @@ -259,3 +259,65 @@ _(filled in at completion)_ - 6d: Push to `github/fix/5-operator-can-modify-auth-config` via `github-echo` SSH alias. - 6e: Open PR against `master` and comment on issue #5. - 6f: Capture lessons in `tasks/lessons.md` (project-specific) and `git-workflow/references/lessons-learned.md` (skill-level). + +--- + +# Issue #6 — Secret Encryption at Rest + +**Spec:** [tasks/secret-encryption-spec.md](secret-encryption-spec.md) v0.1.0 +**Branch:** `fix/6-plaintext-secrets` +**Identity:** `Draco-Lunaris-Echo` +**Follow-up:** [Issue #15](https://github.com/Draco-Lunaris/Linux-Patch-Manager/issues/15) (integration tests) + +## Phase 1: Crypto helper extension + 3 new unit tests +- [ ] 1a: Add `pub const SECRET_ENCRYPTION_KEY_PATH` to `crates/pm-core/src/crypto.rs` +- [ ] 1b: Re-export from `crates/pm-core/src/lib.rs` +- [ ] 1c: Add 3 unit tests in `crypto.rs` (round_trip, file_creation_0600_perms, file_creation_idempotent) +- [ ] 1d: `cargo test -p pm-core` and `cargo clippy --all-targets -- -D warnings` + +## Phase 2: Secret key loader + migration SQL + migration helper +- [ ] 2a: Add `crates/pm-web/src/secret_key.rs` with `OnceCell<[u8; 32]>` pattern +- [ ] 2b: Add `crates/pm-worker/src/secret_key.rs` (same pattern) +- [ ] 2c: Create `migrations/020_encrypt_secrets_at_rest.sql` (schema changes for 3 tables) +- [ ] 2d: Create `crates/migrate-secrets/src/main.rs` — one-shot Rust binary that reads old plaintext, encrypts, writes to new columns +- [ ] 2e: Verify migration helper round-trips (encrypt → decrypt = original plaintext) +- [ ] 2f: `cargo test` and `cargo clippy` clean + +## Phase 3: Code changes — 6 read/write sites +- [ ] 3a: `sso.rs` `load_oidc_config` — query `_encrypted` + `_nonce`, add `decrypt_client_secret()` method to OidcConfig +- [ ] 3b: `settings.rs` OIDC read (line 280) + write (line 360) — same pattern as 3a +- [ ] 3c: `settings.rs` SMTP read (line 793) + write (line 453) — use `system_config` key-value with new keys +- [ ] 3d: `session.rs` TOTP read (line 197) — decrypt with secret_key::get() +- [ ] 3e: `auth.rs` TOTP write (line 363) — encrypt req.secret_base32 before bind +- [ ] 3f: `users.rs` TOTP NULL write (line 537) — bind to new _encrypted + _nonce columns +- [ ] 3g: `pm-worker/src/email.rs` SMTP read (line 58) — decrypt +- [ ] 3h: Update User struct (line 80) — replace `totp_secret: Option` with `totp_secret_encrypted: Option>` + `totp_secret_nonce: Option>` +- [ ] 3i: `cargo test -p pm-web --bins --tests` (43 existing pass) +- [ ] 3j: `cargo test -p pm-auth --bins --tests` +- [ ] 3k: `cargo test -p pm-worker --bins --tests` +- [ ] 3l: `cargo clippy --all-targets -- -D warnings` clean +- [ ] 3m: `npm run build` clean + +## Phase 4: Documentation +- [ ] 4a: Update `docs/security-review.md` §4.1 with new evidence row +- [ ] 4b: Create/update `docs/runbooks/key-management.md` with both key files documented +- [ ] 4c: Update `docs/REST_API.md` (no API changes — note that MASKED behavior is preserved) +- [ ] 4d: Update `SPEC.md` if it mentions secret storage (check during review) + +## Phase 5: Self-review against spec §5 acceptance criteria +- [ ] All 12 acceptance criteria checked +- [ ] Manual verification: psql queries show BYTEA not TEXT +- [ ] Manual verification: API responses still return MASKED + +## Phase 6: Commit, push, open PR +- [ ] 6a: Pre-push validation (cargo fmt, clippy, test, secret scan, identity, remote URL) +- [ ] 6b: Commit on `fix/6-plaintext-secrets` with conventional format +- [ ] 6c: Push to `github/fix/6-plaintext-secrets` via `github-echo` SSH alias +- [ ] 6d: Open PR against master, comment on issue #6 +- [ ] 6e: Append lessons-learned to `git-workflow/references/lessons-learned.md` AND `tasks/lessons.md` + +## Phase 7: Cleanup (after Kelly approves merge) +- [ ] 7a: Reset local master to `github/master` +- [ ] 7b: Delete local + remote branch +- [ ] 7c: Prune remote tracking ref +- [ ] 7d: Report completion