diff --git a/devolutions-gateway/src/credential/crypto.rs b/devolutions-gateway/src/credential/crypto.rs index 2f1a84d70..be57969cf 100644 --- a/devolutions-gateway/src/credential/crypto.rs +++ b/devolutions-gateway/src/credential/crypto.rs @@ -7,6 +7,7 @@ //! ## Security properties //! //! - Passwords encrypted at rest in regular heap memory. +//! - Encrypted passwords zeroized on drop. //! - Decryption on-demand into short-lived zeroized buffers. //! - ChaCha20-Poly1305 provides authenticated encryption. //! - Random 96-bit nonces prevent nonce reuse. @@ -23,6 +24,7 @@ use chacha20poly1305::{ChaCha20Poly1305, Nonce}; use parking_lot::Mutex; use secrecy::SecretString; use secure_memory::{ProtectedBytes, ProtectionLevel}; +use zeroize::Zeroize as _; /// Global master key for credential encryption. /// @@ -139,6 +141,7 @@ impl MasterKeyManager { /// /// Contains the nonce and ciphertext (including the Poly1305 authentication /// tag). Safe to store in regular memory because it is encrypted. +/// It is zeroed on drop. #[derive(Clone)] pub struct EncryptedPassword { /// 96-bit nonce (12 bytes) for ChaCha20-Poly1305. @@ -156,6 +159,13 @@ impl fmt::Debug for EncryptedPassword { } } +impl Drop for EncryptedPassword { + fn drop(&mut self) { + self.nonce.as_mut_slice().zeroize(); + self.ciphertext.zeroize(); + } +} + #[cfg(test)] #[expect(clippy::unwrap_used, reason = "test code, panics are expected")] mod tests { diff --git a/devolutions-gateway/src/credential/mod.rs b/devolutions-gateway/src/credential/mod.rs index 8c8673089..8e12c0a77 100644 --- a/devolutions-gateway/src/credential/mod.rs +++ b/devolutions-gateway/src/credential/mod.rs @@ -7,6 +7,19 @@ use secrecy::ExposeSecret as _; use self::crypto::MASTER_KEY; +/// The same thing as [`EncryptedPassword`], named for secrets that aren't passwords, like an API key. +pub type EncryptedSecret = EncryptedPassword; + +/// Encrypts any secret with the in-memory key that protects injected credentials. +pub(crate) fn encrypt_secret(secret: &secrecy::SecretString) -> anyhow::Result { + MASTER_KEY.lock().encrypt(secret.expose_secret()) +} + +/// Decrypts a secret from [`encrypt_secret`] into a short-lived value that zeroizes on drop. +pub(crate) fn decrypt_secret(encrypted: &EncryptedSecret) -> anyhow::Result { + MASTER_KEY.lock().decrypt(encrypted) +} + /// Credential at the application protocol level #[derive(Debug, Clone)] pub enum AppCredential { diff --git a/devolutions-gateway/src/lib.rs b/devolutions-gateway/src/lib.rs index cd53900b2..a163bfa40 100644 --- a/devolutions-gateway/src/lib.rs +++ b/devolutions-gateway/src/lib.rs @@ -36,6 +36,7 @@ pub mod rd_clean_path; pub mod rdp_pcb; pub mod rdp_proxy; pub mod recording; +pub mod secret_store; pub mod session; pub mod streaming; pub mod subscriber; diff --git a/devolutions-gateway/src/secret_store.intent.md b/devolutions-gateway/src/secret_store.intent.md new file mode 100644 index 000000000..d24e08089 --- /dev/null +++ b/devolutions-gateway/src/secret_store.intent.md @@ -0,0 +1,15 @@ +# Secret store + + +## Secrets + +A secret is any data that could grant access to a third-party service, such as an API key and the URL it is used with. +A secret is never persisted. +A secret is not accessible through swap or a core dump: either it is never in swap or a core dump itself, or it is encrypted with a key that never is, as a chain of trust. +On a best-effort basis, a secret, in plain text or encrypted, is zeroed as soon as it is no longer used: when it is removed, or when it reaches its expiry, without waiting for a read. +All of this is best effort, as if someone has root access and we still try our best to keep the secret away from them; a secret is in plain text while it is used, such as when it is sent in an HTTP request. + + +## Replacement + +Storing a secret under an ID that already has one replaces the old one. diff --git a/devolutions-gateway/src/secret_store.rs b/devolutions-gateway/src/secret_store.rs new file mode 100644 index 000000000..650316046 --- /dev/null +++ b/devolutions-gateway/src/secret_store.rs @@ -0,0 +1,198 @@ +use std::collections::HashMap; +use std::sync::Arc; + +use anyhow::Context as _; +use async_trait::async_trait; +use devolutions_gateway_task::{ShutdownSignal, Task}; +use parking_lot::Mutex; +use secrecy::SecretString; +use tokio::sync::Notify; +use tracing::{debug, instrument}; +use uuid::Uuid; + +use crate::credential::EncryptedSecret; + +/// The key a caller picks for its secret. +/// +/// It wraps the caller's own ID, so the caller says which IDs it uses and they can't be mixed up with JTIs. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub struct SecretId(pub Uuid); + +#[derive(Debug)] +struct SecretEntry { + secret: EncryptedSecret, + expires_at: time::OffsetDateTime, +} + +/// Opaque secrets that must stay in memory only and never be written to disk. +/// Each is keyed by a caller-chosen ID, encrypted with the in-memory master key, +/// replaced on re-insert, and kept until it is removed or reaches its expiry. +#[derive(Debug, Clone, Default)] +pub struct SecretStore { + entries: Arc>>, + cleanup_notify: Arc, +} + +impl SecretStore { + /// Encrypts and stores `secret` under `id` until `expires_at`, + /// replacing any earlier secret with the same ID. + pub fn insert(&self, id: SecretId, secret: &SecretString, expires_at: time::OffsetDateTime) -> anyhow::Result<()> { + let secret = crate::credential::encrypt_secret(secret).context("encrypt the secret")?; + + self.entries.lock().insert(id, SecretEntry { secret, expires_at }); + self.cleanup_notify.notify_one(); + + Ok(()) + } + + /// Returns the decrypted secret of `id`, or `None` once it is removed or expired. + /// + /// The returned value zeroizes on drop; keep it only as long as it is needed. + pub fn get(&self, id: SecretId) -> anyhow::Result> { + self.get_at(id, time::OffsetDateTime::now_utc()) + } + + fn get_at(&self, id: SecretId, now: time::OffsetDateTime) -> anyhow::Result> { + let encrypted = { + let mut entries = self.entries.lock(); + + let Some(entry) = entries.get(&id) else { + return Ok(None); + }; + if entry.expires_at <= now { + entries.remove(&id); + return Ok(None); + } + + entry.secret.clone() + }; + + crate::credential::decrypt_secret(&encrypted) + .context("decrypt the secret") + .map(Some) + } + + /// Removes the secret of `id`. + pub fn remove(&self, id: SecretId) { + self.entries.lock().remove(&id); + } + + fn remove_expired(&self, now: time::OffsetDateTime) { + self.entries.lock().retain(|_, entry| now < entry.expires_at); + } + + fn next_expiry(&self) -> Option { + self.entries.lock().values().map(|entry| entry.expires_at).min() + } +} + +pub struct CleanupTask { + pub handle: SecretStore, +} + +#[async_trait] +impl Task for CleanupTask { + type Output = anyhow::Result<()>; + + const NAME: &'static str = "secret store cleanup"; + + async fn run(self, shutdown_signal: ShutdownSignal) -> Self::Output { + cleanup_task(self.handle, shutdown_signal).await; + Ok(()) + } +} + +#[instrument(skip_all)] +async fn cleanup_task(handle: SecretStore, mut shutdown_signal: ShutdownSignal) { + debug!("Task started"); + + loop { + let now = time::OffsetDateTime::now_utc(); + handle.remove_expired(now); + + match handle.next_expiry() { + Some(deadline) => { + let delay = (deadline - now).try_into().unwrap_or_default(); + tokio::select! { + _ = tokio::time::sleep(delay) => {} + _ = handle.cleanup_notify.notified() => {} + _ = shutdown_signal.wait() => break, + } + } + None => { + tokio::select! { + _ = handle.cleanup_notify.notified() => {} + _ = shutdown_signal.wait() => break, + } + } + } + } + + debug!("Task terminated"); +} + +#[cfg(test)] +mod tests { + use secrecy::ExposeSecret as _; + + use super::*; + + #[test] + fn secret_is_read_back_and_replaced() { + let store = SecretStore::default(); + let id = SecretId(Uuid::new_v4()); + let now = time::OffsetDateTime::now_utc(); + + store + .insert(id, &SecretString::from("secret-a"), now + time::Duration::hours(1)) + .expect("insert"); + + // oracle: intent devolutions-gateway/src/secret_store.intent.md §secrets + assert!( + !format!("{store:?}").contains("secret-a"), + "the store's Debug output never shows the secret" + ); + let secret = store.get_at(id, now).expect("decrypt").expect("present"); + // oracle: property round-trip + assert_eq!(secret.expose_secret(), "secret-a"); + + store + .insert(id, &SecretString::from("secret-b"), now + time::Duration::hours(1)) + .expect("replace"); + let secret = store.get_at(id, now).expect("decrypt").expect("present"); + // oracle: intent devolutions-gateway/src/secret_store.intent.md §replacement + assert_eq!(secret.expose_secret(), "secret-b"); + } + + // oracle: intent devolutions-gateway/src/secret_store.intent.md §secrets + #[test] + fn secret_goes_away_when_removed_or_expired() { + let store = SecretStore::default(); + let now = time::OffsetDateTime::now_utc(); + let removed = SecretId(Uuid::new_v4()); + let expired = SecretId(Uuid::new_v4()); + let expiring = SecretId(Uuid::new_v4()); + + store + .insert(removed, &SecretString::from("a"), now + time::Duration::hours(1)) + .expect("insert"); + store.insert(expired, &SecretString::from("b"), now).expect("insert"); + store + .insert(expiring, &SecretString::from("c"), now + time::Duration::minutes(1)) + .expect("insert"); + + store.remove(removed); + assert!(store.get_at(removed, now).expect("read").is_none()); + assert!( + store.get_at(expired, now).expect("read").is_none(), + "a secret is gone once its expiry is reached" + ); + assert!( + !store.entries.lock().contains_key(&expired), + "reading an expired secret drops it" + ); + + store.remove_expired(now + time::Duration::minutes(1)); + assert!(store.entries.lock().is_empty(), "the cleanup pass removes it"); + } +}