From 615a7c9f47ccd14fed94a0c2fe3fb75e45bbcddb Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Thu, 8 Oct 2026 15:26:07 -0400 Subject: [PATCH 1/9] feat(dgw): keep provisioner task secrets in the provisioning store The provisioning store gains a third kind of provisioned material, next to the injected credentials and the connection options: the secrets of provisioner tasks, such as the API key of an AI analysis. A provisioner task needs them while it runs, but they must never be written to disk. Task secrets are keyed by Task ID and follow their own policy, as the credential intent asks for each kind of provisioning: a new secret replaces the old one, and a secret goes away when its Task ends or at its expiry, through the existing cleanup task. Like the injected credentials, they are encrypted on the way in with the in-memory master key, and decrypted into a zeroizing value only when read. The store treats a secret as an opaque string, so each task decides what it holds. The session halves are unchanged. --- devolutions-gateway/src/credential/mod.rs | 10 ++ devolutions-gateway/src/provisioning.rs | 166 ++++++++++++++++++++-- 2 files changed, 167 insertions(+), 9 deletions(-) diff --git a/devolutions-gateway/src/credential/mod.rs b/devolutions-gateway/src/credential/mod.rs index 8c8673089..9fd017d90 100644 --- a/devolutions-gateway/src/credential/mod.rs +++ b/devolutions-gateway/src/credential/mod.rs @@ -7,6 +7,16 @@ use secrecy::ExposeSecret as _; use self::crypto::MASTER_KEY; +/// 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: &EncryptedPassword) -> 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/provisioning.rs b/devolutions-gateway/src/provisioning.rs index e581b6719..402c83fc7 100644 --- a/devolutions-gateway/src/provisioning.rs +++ b/devolutions-gateway/src/provisioning.rs @@ -6,11 +6,12 @@ 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, warn}; use uuid::Uuid; -use crate::credential::{AppCredentialMapping, CleartextAppCredentialMapping}; +use crate::credential::{AppCredentialMapping, CleartextAppCredentialMapping, EncryptedPassword}; use crate::target_connection_options::TargetConnectionOptions; /// Error returned when inserting into the credentials half of the provisioning store. @@ -68,19 +69,29 @@ struct ConnectionOptionsEntry { expires_at: time::OffsetDateTime, } -/// Two independent token-keyed stores that together provision a session. +#[derive(Debug, Clone)] +struct TaskSecretEntry { + secret: EncryptedPassword, + expires_at: time::OffsetDateTime, +} + +/// What the provisioner hands Gateway to use later, kept in memory only, each kind with its own policy. /// -/// The credentials store is the encryption boundary: cleartext mappings are encrypted on the way -/// in, so entries only ever hold encrypted material. Token-only rows (`mapping = None`) match the -/// existing `provision-token` behavior on master. The connection-options store holds plaintext -/// routing metadata only and has no crypto dependency. +/// Two token-keyed stores together provision a session. The credentials store is the encryption +/// boundary: cleartext mappings are encrypted on the way in, so entries only ever hold encrypted +/// material. Token-only rows (`mapping = None`) match the existing `provision-token` behavior on +/// master. The connection-options store holds plaintext routing metadata only and has no crypto +/// dependency. Both are keyed by the association-token JTI. The halves are provisioned by separate +/// preflight operations and may arrive, expire, or be replaced independently. /// -/// Both are keyed by the association-token JTI. The halves are provisioned by separate preflight -/// operations and may arrive, expire, or be replaced independently. +/// The task-secrets store holds what a provisioner task needs but must never write to disk, such as +/// the API key of an AI analysis. It is keyed by Task ID, encrypted like the credentials, and its +/// entries go away when their Task ends or at their expiry. #[derive(Debug, Clone)] pub struct ProvisioningStore { credentials: Arc>>, connection_options: Arc>>, + task_secrets: Arc>>, cleanup_notify: Arc, } @@ -95,6 +106,7 @@ impl ProvisioningStore { Self { credentials: Arc::new(Mutex::new(HashMap::new())), connection_options: Arc::new(Mutex::new(HashMap::new())), + task_secrets: Arc::new(Mutex::new(HashMap::new())), cleanup_notify: Arc::new(Notify::new()), } } @@ -302,9 +314,57 @@ impl ProvisioningStore { self.connection_options.lock().get(&jti).map(|entry| entry.expires_at) } + /// Stores the secret of a provisioner task, encrypted, until `expires_at`; it replaces any earlier secret of the Task. + /// + /// The store does not look inside: the task decides what the secret holds, such as a serialized API key and URL. + pub fn insert_task_secret( + &self, + task_id: Uuid, + secret: &SecretString, + expires_at: time::OffsetDateTime, + ) -> anyhow::Result<()> { + let secret = crate::credential::encrypt_secret(secret).context("encrypt the task secret")?; + + self.task_secrets + .lock() + .insert(task_id, TaskSecretEntry { secret, expires_at }); + self.cleanup_notify.notify_one(); + + Ok(()) + } + + /// Returns the decrypted secret of `task_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 task_secret(&self, task_id: Uuid, now: time::OffsetDateTime) -> anyhow::Result> { + let encrypted = { + let mut task_secrets = self.task_secrets.lock(); + + let Some(entry) = task_secrets.get(&task_id) else { + return Ok(None); + }; + if now >= entry.expires_at { + task_secrets.remove(&task_id); + return Ok(None); + } + + entry.secret.clone() + }; + + crate::credential::decrypt_secret(&encrypted) + .context("decrypt the task secret") + .map(Some) + } + + /// Removes the secret of `task_id`, as when its Task ends. + pub fn remove_task_secret(&self, task_id: Uuid) { + self.task_secrets.lock().remove(&task_id); + } + fn remove_expired(&self, now: time::OffsetDateTime) { self.credentials.lock().retain(|_, entry| now < entry.expires_at); self.connection_options.lock().retain(|_, entry| now < entry.expires_at); + self.task_secrets.lock().retain(|_, entry| now < entry.expires_at); } fn next_expiry(&self) -> Option { @@ -315,7 +375,12 @@ impl ProvisioningStore { .values() .map(|entry| entry.expires_at) .min(); - credentials_expiry.into_iter().chain(options_expiry).min() + let task_secrets_expiry = self.task_secrets.lock().values().map(|entry| entry.expires_at).min(); + credentials_expiry + .into_iter() + .chain(options_expiry) + .chain(task_secrets_expiry) + .min() } } @@ -409,6 +474,89 @@ mod tests { serde_json::from_value(serde_json::json!({ "krb_kdc": "tcp://dc.example:88" })).expect("options") } + #[test] + fn task_secret_is_kept_encrypted_and_read_back() { + use secrecy::ExposeSecret as _; + + let store = ProvisioningStore::new(); + let task_id = Uuid::new_v4(); + let now = time::OffsetDateTime::now_utc(); + + store + .insert_task_secret( + task_id, + &SecretString::from("sk-task-secret"), + now + time::Duration::hours(1), + ) + .expect("insert"); + + assert!( + !format!("{store:?}").contains("sk-task-secret"), + "only the encrypted secret is held" + ); + let secret = store.task_secret(task_id, now).expect("decrypt").expect("present"); + assert_eq!(secret.expose_secret(), "sk-task-secret"); + + store + .insert_task_secret( + task_id, + &SecretString::from("sk-replaced"), + now + time::Duration::hours(1), + ) + .expect("replace"); + let secret = store.task_secret(task_id, now).expect("decrypt").expect("present"); + assert_eq!(secret.expose_secret(), "sk-replaced"); + } + + #[test] + fn task_secret_goes_away_when_removed_or_expired() { + let store = ProvisioningStore::new(); + let now = time::OffsetDateTime::now_utc(); + let removed = Uuid::new_v4(); + let expired = Uuid::new_v4(); + let expiring = Uuid::new_v4(); + + store + .insert_task_secret(removed, &SecretString::from("a"), now + time::Duration::hours(1)) + .expect("insert"); + store + .insert_task_secret(expired, &SecretString::from("b"), now) + .expect("insert"); + store + .insert_task_secret(expiring, &SecretString::from("c"), now + time::Duration::minutes(1)) + .expect("insert"); + + store.remove_task_secret(removed); + assert!(store.task_secret(removed, now).expect("read").is_none()); + assert!( + store.task_secret(expired, now).expect("read").is_none(), + "due at its expiry" + ); + + assert_eq!(store.next_expiry(), Some(now + time::Duration::minutes(1))); + store.remove_expired(now + time::Duration::minutes(1)); + assert!(store.task_secrets.lock().is_empty(), "the cleanup task removes it"); + } + + #[test] + fn task_secrets_are_independent_from_session_provisioning() { + let store = ProvisioningStore::new(); + let id = Uuid::new_v4(); + let token = association_token(id); + let now = time::OffsetDateTime::now_utc(); + store + .insert_credentials(token.clone(), Some(mapping()), time::Duration::minutes(5)) + .expect("insert credentials"); + store + .insert_task_secret(id, &SecretString::from("sk"), now + time::Duration::hours(1)) + .expect("insert secret"); + + store.remove_task_secret(id); + + assert_eq!(store.mapping_status(id), MappingStatus::Available); + store.get_mapping(id, &token).expect("credentials stay"); + } + #[test] fn take_returns_token_only_entry() { let store = ProvisioningStore::new(); From 634281fc178f274d14ccebb4a4c25c60e8b2fb1b Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Thu, 8 Oct 2026 16:33:23 -0400 Subject: [PATCH 2/9] fix(dgw): read the clock when a task secret is read task_secret took the current time from its caller, so a caller passing an earlier time could still read an expired secret. It now reads the clock itself. --- devolutions-gateway/src/provisioning.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/devolutions-gateway/src/provisioning.rs b/devolutions-gateway/src/provisioning.rs index 402c83fc7..90b265659 100644 --- a/devolutions-gateway/src/provisioning.rs +++ b/devolutions-gateway/src/provisioning.rs @@ -336,7 +336,11 @@ impl ProvisioningStore { /// Returns the decrypted secret of `task_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 task_secret(&self, task_id: Uuid, now: time::OffsetDateTime) -> anyhow::Result> { + pub fn task_secret(&self, task_id: Uuid) -> anyhow::Result> { + self.task_secret_at(task_id, time::OffsetDateTime::now_utc()) + } + + fn task_secret_at(&self, task_id: Uuid, now: time::OffsetDateTime) -> anyhow::Result> { let encrypted = { let mut task_secrets = self.task_secrets.lock(); @@ -494,7 +498,7 @@ mod tests { !format!("{store:?}").contains("sk-task-secret"), "only the encrypted secret is held" ); - let secret = store.task_secret(task_id, now).expect("decrypt").expect("present"); + let secret = store.task_secret_at(task_id, now).expect("decrypt").expect("present"); assert_eq!(secret.expose_secret(), "sk-task-secret"); store @@ -504,7 +508,7 @@ mod tests { now + time::Duration::hours(1), ) .expect("replace"); - let secret = store.task_secret(task_id, now).expect("decrypt").expect("present"); + let secret = store.task_secret_at(task_id, now).expect("decrypt").expect("present"); assert_eq!(secret.expose_secret(), "sk-replaced"); } @@ -527,9 +531,9 @@ mod tests { .expect("insert"); store.remove_task_secret(removed); - assert!(store.task_secret(removed, now).expect("read").is_none()); + assert!(store.task_secret_at(removed, now).expect("read").is_none()); assert!( - store.task_secret(expired, now).expect("read").is_none(), + store.task_secret_at(expired, now).expect("read").is_none(), "due at its expiry" ); From 3036fc5383618755666141b09f465672aea2120a Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Mon, 12 Oct 2026 20:51:04 -0400 Subject: [PATCH 3/9] docs(dgw): describe the task secret store by its contract Keep the existing `ProvisioningStore` paragraphs as they were and only add the task-secrets paragraph, and describe the task secret methods by what they guarantee rather than by what the AI analysis caller stores. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/provisioning.rs | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/devolutions-gateway/src/provisioning.rs b/devolutions-gateway/src/provisioning.rs index 90b265659..ba67f7c05 100644 --- a/devolutions-gateway/src/provisioning.rs +++ b/devolutions-gateway/src/provisioning.rs @@ -77,12 +77,15 @@ struct TaskSecretEntry { /// What the provisioner hands Gateway to use later, kept in memory only, each kind with its own policy. /// -/// Two token-keyed stores together provision a session. The credentials store is the encryption -/// boundary: cleartext mappings are encrypted on the way in, so entries only ever hold encrypted -/// material. Token-only rows (`mapping = None`) match the existing `provision-token` behavior on -/// master. The connection-options store holds plaintext routing metadata only and has no crypto -/// dependency. Both are keyed by the association-token JTI. The halves are provisioned by separate -/// preflight operations and may arrive, expire, or be replaced independently. +/// Two independent token-keyed stores together provision a session. +/// +/// The credentials store is the encryption boundary: cleartext mappings are encrypted on the way +/// in, so entries only ever hold encrypted material. Token-only rows (`mapping = None`) match the +/// existing `provision-token` behavior on master. The connection-options store holds plaintext +/// routing metadata only and has no crypto dependency. +/// +/// Both are keyed by the association-token JTI. The halves are provisioned by separate preflight +/// operations and may arrive, expire, or be replaced independently. /// /// The task-secrets store holds what a provisioner task needs but must never write to disk, such as /// the API key of an AI analysis. It is keyed by Task ID, encrypted like the credentials, and its @@ -314,9 +317,9 @@ impl ProvisioningStore { self.connection_options.lock().get(&jti).map(|entry| entry.expires_at) } - /// Stores the secret of a provisioner task, encrypted, until `expires_at`; it replaces any earlier secret of the Task. + /// Stores the encrypted secret of a provisioner task until `expires_at`, replacing any earlier secret of the Task. /// - /// The store does not look inside: the task decides what the secret holds, such as a serialized API key and URL. + /// The secret is opaque to the store. pub fn insert_task_secret( &self, task_id: Uuid, @@ -360,7 +363,7 @@ impl ProvisioningStore { .map(Some) } - /// Removes the secret of `task_id`, as when its Task ends. + /// Removes the secret of `task_id`. pub fn remove_task_secret(&self, task_id: Uuid) { self.task_secrets.lock().remove(&task_id); } From 73e1e0cb2a38a86279c0af0585a195e734094671 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Tue, 13 Oct 2026 11:08:42 -0400 Subject: [PATCH 4/9] docs(dgw): say what the task secret store and its tests actually check - The store keeps an entry until it is removed or expires; removing it when the Task ends is the caller's job. - `insert_task_secret` takes the secret in clear and encrypts it. - The tests are named after what they check: the Debug output never shows the secret, and the cleanup pass, not the cleanup task, removes it. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/provisioning.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/devolutions-gateway/src/provisioning.rs b/devolutions-gateway/src/provisioning.rs index ba67f7c05..dcf61f52b 100644 --- a/devolutions-gateway/src/provisioning.rs +++ b/devolutions-gateway/src/provisioning.rs @@ -88,8 +88,8 @@ struct TaskSecretEntry { /// operations and may arrive, expire, or be replaced independently. /// /// The task-secrets store holds what a provisioner task needs but must never write to disk, such as -/// the API key of an AI analysis. It is keyed by Task ID, encrypted like the credentials, and its -/// entries go away when their Task ends or at their expiry. +/// the API key of an AI analysis. It is keyed by Task ID, encrypted like the credentials, and an +/// entry stays until it is removed or reaches its expiry. #[derive(Debug, Clone)] pub struct ProvisioningStore { credentials: Arc>>, @@ -317,7 +317,7 @@ impl ProvisioningStore { self.connection_options.lock().get(&jti).map(|entry| entry.expires_at) } - /// Stores the encrypted secret of a provisioner task until `expires_at`, replacing any earlier secret of the Task. + /// Encrypts and stores the secret of a provisioner task until `expires_at`, replacing any earlier secret of the Task. /// /// The secret is opaque to the store. pub fn insert_task_secret( @@ -482,7 +482,7 @@ mod tests { } #[test] - fn task_secret_is_kept_encrypted_and_read_back() { + fn task_secret_is_read_back_and_replaced() { use secrecy::ExposeSecret as _; let store = ProvisioningStore::new(); @@ -499,7 +499,7 @@ mod tests { assert!( !format!("{store:?}").contains("sk-task-secret"), - "only the encrypted secret is held" + "the store's Debug output never shows the secret" ); let secret = store.task_secret_at(task_id, now).expect("decrypt").expect("present"); assert_eq!(secret.expose_secret(), "sk-task-secret"); @@ -542,7 +542,7 @@ mod tests { assert_eq!(store.next_expiry(), Some(now + time::Duration::minutes(1))); store.remove_expired(now + time::Duration::minutes(1)); - assert!(store.task_secrets.lock().is_empty(), "the cleanup task removes it"); + assert!(store.task_secrets.lock().is_empty(), "the cleanup pass removes it"); } #[test] From 13c53410d110dd9a4a368f3ff16ff4b3e7746bf0 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Mon, 19 Oct 2026 17:08:06 -0400 Subject: [PATCH 5/9] style(dgw): wrap the task secret doc and compare expiry with <= Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/provisioning.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/devolutions-gateway/src/provisioning.rs b/devolutions-gateway/src/provisioning.rs index dcf61f52b..19abb03fb 100644 --- a/devolutions-gateway/src/provisioning.rs +++ b/devolutions-gateway/src/provisioning.rs @@ -317,7 +317,8 @@ impl ProvisioningStore { self.connection_options.lock().get(&jti).map(|entry| entry.expires_at) } - /// Encrypts and stores the secret of a provisioner task until `expires_at`, replacing any earlier secret of the Task. + /// Encrypts and stores the secret of a provisioner Task until `expires_at`, + /// replacing any earlier secret of the Task. /// /// The secret is opaque to the store. pub fn insert_task_secret( @@ -350,7 +351,7 @@ impl ProvisioningStore { let Some(entry) = task_secrets.get(&task_id) else { return Ok(None); }; - if now >= entry.expires_at { + if entry.expires_at <= now { task_secrets.remove(&task_id); return Ok(None); } From 0ea2da3588031bfc92db206a917206094bb82dda Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Wed, 21 Oct 2026 15:38:56 -0400 Subject: [PATCH 6/9] refactor(dgw): move task secrets into a standalone SecretStore The provisioning store is keyed by the association-token JTI and follows the lifecycle of a session. These secrets have neither: a caller picks their ID and decides when they go. A separate store keeps the provisioning store about sessions only. The new store is generic: an opaque secret, encrypted with the in-memory master key, keyed by a `SecretId`, replaced on re-insert, and removed explicitly or at its expiry by its own cleanup task. `EncryptedSecret` names the encrypted value for what it holds. Co-Authored-By: Claude Opus 5.5 --- devolutions-gateway/src/credential/mod.rs | 7 +- devolutions-gateway/src/lib.rs | 1 + devolutions-gateway/src/provisioning.rs | 162 +----------------- devolutions-gateway/src/secret_store.rs | 191 ++++++++++++++++++++++ 4 files changed, 200 insertions(+), 161 deletions(-) create mode 100644 devolutions-gateway/src/secret_store.rs diff --git a/devolutions-gateway/src/credential/mod.rs b/devolutions-gateway/src/credential/mod.rs index 9fd017d90..8e12c0a77 100644 --- a/devolutions-gateway/src/credential/mod.rs +++ b/devolutions-gateway/src/credential/mod.rs @@ -7,13 +7,16 @@ 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 { +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: &EncryptedPassword) -> anyhow::Result { +pub(crate) fn decrypt_secret(encrypted: &EncryptedSecret) -> anyhow::Result { MASTER_KEY.lock().decrypt(encrypted) } 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/provisioning.rs b/devolutions-gateway/src/provisioning.rs index 19abb03fb..e581b6719 100644 --- a/devolutions-gateway/src/provisioning.rs +++ b/devolutions-gateway/src/provisioning.rs @@ -6,12 +6,11 @@ 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, warn}; use uuid::Uuid; -use crate::credential::{AppCredentialMapping, CleartextAppCredentialMapping, EncryptedPassword}; +use crate::credential::{AppCredentialMapping, CleartextAppCredentialMapping}; use crate::target_connection_options::TargetConnectionOptions; /// Error returned when inserting into the credentials half of the provisioning store. @@ -69,15 +68,7 @@ struct ConnectionOptionsEntry { expires_at: time::OffsetDateTime, } -#[derive(Debug, Clone)] -struct TaskSecretEntry { - secret: EncryptedPassword, - expires_at: time::OffsetDateTime, -} - -/// What the provisioner hands Gateway to use later, kept in memory only, each kind with its own policy. -/// -/// Two independent token-keyed stores together provision a session. +/// Two independent token-keyed stores that together provision a session. /// /// The credentials store is the encryption boundary: cleartext mappings are encrypted on the way /// in, so entries only ever hold encrypted material. Token-only rows (`mapping = None`) match the @@ -86,15 +77,10 @@ struct TaskSecretEntry { /// /// Both are keyed by the association-token JTI. The halves are provisioned by separate preflight /// operations and may arrive, expire, or be replaced independently. -/// -/// The task-secrets store holds what a provisioner task needs but must never write to disk, such as -/// the API key of an AI analysis. It is keyed by Task ID, encrypted like the credentials, and an -/// entry stays until it is removed or reaches its expiry. #[derive(Debug, Clone)] pub struct ProvisioningStore { credentials: Arc>>, connection_options: Arc>>, - task_secrets: Arc>>, cleanup_notify: Arc, } @@ -109,7 +95,6 @@ impl ProvisioningStore { Self { credentials: Arc::new(Mutex::new(HashMap::new())), connection_options: Arc::new(Mutex::new(HashMap::new())), - task_secrets: Arc::new(Mutex::new(HashMap::new())), cleanup_notify: Arc::new(Notify::new()), } } @@ -317,62 +302,9 @@ impl ProvisioningStore { self.connection_options.lock().get(&jti).map(|entry| entry.expires_at) } - /// Encrypts and stores the secret of a provisioner Task until `expires_at`, - /// replacing any earlier secret of the Task. - /// - /// The secret is opaque to the store. - pub fn insert_task_secret( - &self, - task_id: Uuid, - secret: &SecretString, - expires_at: time::OffsetDateTime, - ) -> anyhow::Result<()> { - let secret = crate::credential::encrypt_secret(secret).context("encrypt the task secret")?; - - self.task_secrets - .lock() - .insert(task_id, TaskSecretEntry { secret, expires_at }); - self.cleanup_notify.notify_one(); - - Ok(()) - } - - /// Returns the decrypted secret of `task_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 task_secret(&self, task_id: Uuid) -> anyhow::Result> { - self.task_secret_at(task_id, time::OffsetDateTime::now_utc()) - } - - fn task_secret_at(&self, task_id: Uuid, now: time::OffsetDateTime) -> anyhow::Result> { - let encrypted = { - let mut task_secrets = self.task_secrets.lock(); - - let Some(entry) = task_secrets.get(&task_id) else { - return Ok(None); - }; - if entry.expires_at <= now { - task_secrets.remove(&task_id); - return Ok(None); - } - - entry.secret.clone() - }; - - crate::credential::decrypt_secret(&encrypted) - .context("decrypt the task secret") - .map(Some) - } - - /// Removes the secret of `task_id`. - pub fn remove_task_secret(&self, task_id: Uuid) { - self.task_secrets.lock().remove(&task_id); - } - fn remove_expired(&self, now: time::OffsetDateTime) { self.credentials.lock().retain(|_, entry| now < entry.expires_at); self.connection_options.lock().retain(|_, entry| now < entry.expires_at); - self.task_secrets.lock().retain(|_, entry| now < entry.expires_at); } fn next_expiry(&self) -> Option { @@ -383,12 +315,7 @@ impl ProvisioningStore { .values() .map(|entry| entry.expires_at) .min(); - let task_secrets_expiry = self.task_secrets.lock().values().map(|entry| entry.expires_at).min(); - credentials_expiry - .into_iter() - .chain(options_expiry) - .chain(task_secrets_expiry) - .min() + credentials_expiry.into_iter().chain(options_expiry).min() } } @@ -482,89 +409,6 @@ mod tests { serde_json::from_value(serde_json::json!({ "krb_kdc": "tcp://dc.example:88" })).expect("options") } - #[test] - fn task_secret_is_read_back_and_replaced() { - use secrecy::ExposeSecret as _; - - let store = ProvisioningStore::new(); - let task_id = Uuid::new_v4(); - let now = time::OffsetDateTime::now_utc(); - - store - .insert_task_secret( - task_id, - &SecretString::from("sk-task-secret"), - now + time::Duration::hours(1), - ) - .expect("insert"); - - assert!( - !format!("{store:?}").contains("sk-task-secret"), - "the store's Debug output never shows the secret" - ); - let secret = store.task_secret_at(task_id, now).expect("decrypt").expect("present"); - assert_eq!(secret.expose_secret(), "sk-task-secret"); - - store - .insert_task_secret( - task_id, - &SecretString::from("sk-replaced"), - now + time::Duration::hours(1), - ) - .expect("replace"); - let secret = store.task_secret_at(task_id, now).expect("decrypt").expect("present"); - assert_eq!(secret.expose_secret(), "sk-replaced"); - } - - #[test] - fn task_secret_goes_away_when_removed_or_expired() { - let store = ProvisioningStore::new(); - let now = time::OffsetDateTime::now_utc(); - let removed = Uuid::new_v4(); - let expired = Uuid::new_v4(); - let expiring = Uuid::new_v4(); - - store - .insert_task_secret(removed, &SecretString::from("a"), now + time::Duration::hours(1)) - .expect("insert"); - store - .insert_task_secret(expired, &SecretString::from("b"), now) - .expect("insert"); - store - .insert_task_secret(expiring, &SecretString::from("c"), now + time::Duration::minutes(1)) - .expect("insert"); - - store.remove_task_secret(removed); - assert!(store.task_secret_at(removed, now).expect("read").is_none()); - assert!( - store.task_secret_at(expired, now).expect("read").is_none(), - "due at its expiry" - ); - - assert_eq!(store.next_expiry(), Some(now + time::Duration::minutes(1))); - store.remove_expired(now + time::Duration::minutes(1)); - assert!(store.task_secrets.lock().is_empty(), "the cleanup pass removes it"); - } - - #[test] - fn task_secrets_are_independent_from_session_provisioning() { - let store = ProvisioningStore::new(); - let id = Uuid::new_v4(); - let token = association_token(id); - let now = time::OffsetDateTime::now_utc(); - store - .insert_credentials(token.clone(), Some(mapping()), time::Duration::minutes(5)) - .expect("insert credentials"); - store - .insert_task_secret(id, &SecretString::from("sk"), now + time::Duration::hours(1)) - .expect("insert secret"); - - store.remove_task_secret(id); - - assert_eq!(store.mapping_status(id), MappingStatus::Available); - store.get_mapping(id, &token).expect("credentials stay"); - } - #[test] fn take_returns_token_only_entry() { let store = ProvisioningStore::new(); diff --git a/devolutions-gateway/src/secret_store.rs b/devolutions-gateway/src/secret_store.rs new file mode 100644 index 000000000..b53218391 --- /dev/null +++ b/devolutions-gateway/src/secret_store.rs @@ -0,0 +1,191 @@ +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"); + + 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"); + 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"); + assert_eq!(secret.expose_secret(), "secret-b"); + } + + #[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_eq!(store.next_expiry(), Some(now + time::Duration::minutes(1))); + store.remove_expired(now + time::Duration::minutes(1)); + assert!(store.entries.lock().is_empty(), "the cleanup pass removes it"); + } +} From 5a050f4b6fd79da4c3903dfe8eaed2c753596fca Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Thu, 22 Oct 2026 11:25:56 -0400 Subject: [PATCH 7/9] docs(dgw): state the secret store intent A secret is anything that could grant access to a third-party service. It is never persisted, kept out of swap and core dumps by itself or by the key that encrypts it, and zeroed as soon as it is no longer used, all on a best-effort basis. Co-Authored-By: Claude Opus 5.5 --- devolutions-gateway/src/secret_store.intent.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 devolutions-gateway/src/secret_store.intent.md 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. From 46f55978713723bdb65ecbfd8a80e45f96dad9ec Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Thu, 22 Oct 2026 11:25:56 -0400 Subject: [PATCH 8/9] feat(dgw): zero an encrypted secret when it is dropped The secret store intent asks that a secret be zeroed as soon as it is no longer used, encrypted or not. An encrypted password now zeroes its nonce and ciphertext on drop, which covers the secret store and the injected credentials alike. Co-Authored-By: Claude Opus 5.5 --- devolutions-gateway/src/credential/crypto.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) 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 { From 49b37deb5482081cf6564fc9c710ed48b4ffb0fb Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Thu, 22 Oct 2026 11:25:56 -0400 Subject: [PATCH 9/9] test(dgw): cite the secret store intent in its tests Each assertion now names the intent section or law that decides it. Reading an expired secret is checked to drop it from memory, instead of checking the private next expiry. Co-Authored-By: Claude Opus 5.5 --- devolutions-gateway/src/secret_store.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/devolutions-gateway/src/secret_store.rs b/devolutions-gateway/src/secret_store.rs index b53218391..650316046 100644 --- a/devolutions-gateway/src/secret_store.rs +++ b/devolutions-gateway/src/secret_store.rs @@ -147,20 +147,24 @@ mod tests { .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(); @@ -183,8 +187,11 @@ mod tests { 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" + ); - assert_eq!(store.next_expiry(), Some(now + time::Duration::minutes(1))); store.remove_expired(now + time::Duration::minutes(1)); assert!(store.entries.lock().is_empty(), "the cleanup pass removes it"); }