From 6a78cec75aa4734bec04e801e6814e269d3171c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Wed, 21 May 2025 17:47:41 +0200 Subject: [PATCH 1/6] Added comments based on my current understanding of the code. --- rust/krb5-provision-keytab/src/lib.rs | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/rust/krb5-provision-keytab/src/lib.rs b/rust/krb5-provision-keytab/src/lib.rs index 41181b88..e3fcdb2f 100644 --- a/rust/krb5-provision-keytab/src/lib.rs +++ b/rust/krb5-provision-keytab/src/lib.rs @@ -71,24 +71,44 @@ pub async fn provision_keytab(krb5_config_path: &Path, req: &Request) -> Result< let req_str = serde_json::to_vec(&req).context(SerializeRequestSnafu)?; let mut child = Command::new("stackable-krb5-provision-keytab") + // make sure the process is killed if we error out of this fn somewhere due to + // an error when writing to stdin or getting stdout .kill_on_drop(true) .env("KRB5_CONFIG", krb5_config_path) // ldap3 uses the default client keytab to authenticate to the LDAP server .env("KRB5_CLIENT_KTNAME", &req.admin_keytab_path) - // avoid leaking credentials between secret volumes/secretclasses + // avoid leaking credentials between secret volumes/secretclasses by only storing the + // TGT that is obtained for the operation in the memory of the short lives process + // spawned by `Command::new` above - this way it'll be wiped from memory once this exits + // With any shared or persistent ticket cache this might stick around and potentially be + // reused by later runs .env("KRB5CCNAME", "MEMORY:") .stdin(Stdio::piped()) .stdout(Stdio::piped()) .spawn() .context(SpawnProvisionerSnafu)?; + + // Get a `ChildStdin` object for the spawned process and write the serialized request + // for a Principal into it in order for the child process to deserialize it and + // process the request let mut stdin = child.stdin.take().unwrap(); stdin.write_all(&req_str).await.context(WriteRequestSnafu)?; stdin.flush().await.context(WriteRequestSnafu)?; drop(stdin); + + // Wait for the process to finish and capture output + // This will always return Ok(...) regardless of exit code or output of the child process + // Failure here means that something went wrong with connecting to the process or obtaining + // exit code or output let output = child .wait_with_output() .await .context(WaitProvisionerSnafu)?; + + // Check for success of the operation by deserializing stdout of the process to a `Response` + // struct - since `Response` is an empty struct with no fields this effectively means that + // any output will fail to deserialize and cause an `Error::RunProvisioner` to be propagated + // with the output of the child process serde_json::from_slice::>(&output.stdout) .context(DeserializeResponseSnafu)? .map_err(|msg| Error::RunProvisioner { msg }) From 734cd52683a41be026647866635bcfb18a2ee44d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Wed, 21 May 2025 17:51:01 +0200 Subject: [PATCH 2/6] rustfmt --- rust/krb5-provision-keytab/src/lib.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/rust/krb5-provision-keytab/src/lib.rs b/rust/krb5-provision-keytab/src/lib.rs index e3fcdb2f..b2b1fffe 100644 --- a/rust/krb5-provision-keytab/src/lib.rs +++ b/rust/krb5-provision-keytab/src/lib.rs @@ -71,34 +71,34 @@ pub async fn provision_keytab(krb5_config_path: &Path, req: &Request) -> Result< let req_str = serde_json::to_vec(&req).context(SerializeRequestSnafu)?; let mut child = Command::new("stackable-krb5-provision-keytab") - // make sure the process is killed if we error out of this fn somewhere due to + // make sure the process is killed if we error out of this fn somewhere due to // an error when writing to stdin or getting stdout .kill_on_drop(true) .env("KRB5_CONFIG", krb5_config_path) // ldap3 uses the default client keytab to authenticate to the LDAP server .env("KRB5_CLIENT_KTNAME", &req.admin_keytab_path) - // avoid leaking credentials between secret volumes/secretclasses by only storing the - // TGT that is obtained for the operation in the memory of the short lives process + // avoid leaking credentials between secret volumes/secretclasses by only storing the + // TGT that is obtained for the operation in the memory of the short lives process // spawned by `Command::new` above - this way it'll be wiped from memory once this exits - // With any shared or persistent ticket cache this might stick around and potentially be + // With any shared or persistent ticket cache this might stick around and potentially be // reused by later runs .env("KRB5CCNAME", "MEMORY:") .stdin(Stdio::piped()) .stdout(Stdio::piped()) .spawn() .context(SpawnProvisionerSnafu)?; - + // Get a `ChildStdin` object for the spawned process and write the serialized request - // for a Principal into it in order for the child process to deserialize it and + // for a Principal into it in order for the child process to deserialize it and // process the request let mut stdin = child.stdin.take().unwrap(); stdin.write_all(&req_str).await.context(WriteRequestSnafu)?; stdin.flush().await.context(WriteRequestSnafu)?; drop(stdin); - + // Wait for the process to finish and capture output // This will always return Ok(...) regardless of exit code or output of the child process - // Failure here means that something went wrong with connecting to the process or obtaining + // Failure here means that something went wrong with connecting to the process or obtaining // exit code or output let output = child .wait_with_output() @@ -106,7 +106,7 @@ pub async fn provision_keytab(krb5_config_path: &Path, req: &Request) -> Result< .context(WaitProvisionerSnafu)?; // Check for success of the operation by deserializing stdout of the process to a `Response` - // struct - since `Response` is an empty struct with no fields this effectively means that + // struct - since `Response` is an empty struct with no fields this effectively means that // any output will fail to deserialize and cause an `Error::RunProvisioner` to be propagated // with the output of the child process serde_json::from_slice::>(&output.stdout) From 2c059262f09b94dd3eb2f035619df0cc5c6577a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Wed, 21 May 2025 17:52:27 +0200 Subject: [PATCH 3/6] added little detail --- rust/krb5-provision-keytab/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/rust/krb5-provision-keytab/src/lib.rs b/rust/krb5-provision-keytab/src/lib.rs index b2b1fffe..af05fe96 100644 --- a/rust/krb5-provision-keytab/src/lib.rs +++ b/rust/krb5-provision-keytab/src/lib.rs @@ -73,6 +73,8 @@ pub async fn provision_keytab(krb5_config_path: &Path, req: &Request) -> Result< let mut child = Command::new("stackable-krb5-provision-keytab") // make sure the process is killed if we error out of this fn somewhere due to // an error when writing to stdin or getting stdout + // Usually we'd expect the process to terminate on its own, this is a fail safe to ensure + // it gets killed in case it hangs for some reason. .kill_on_drop(true) .env("KRB5_CONFIG", krb5_config_path) // ldap3 uses the default client keytab to authenticate to the LDAP server From 31f1feaa03806d9f6ebaff7232fbb8b2702c6a5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Wed, 21 May 2025 18:33:45 +0200 Subject: [PATCH 4/6] Replace unwrap with Snafu error type --- rust/krb5-provision-keytab/src/lib.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/rust/krb5-provision-keytab/src/lib.rs b/rust/krb5-provision-keytab/src/lib.rs index af05fe96..3a86e7c0 100644 --- a/rust/krb5-provision-keytab/src/lib.rs +++ b/rust/krb5-provision-keytab/src/lib.rs @@ -6,7 +6,7 @@ use std::{ }; use serde::{Deserialize, Serialize}; -use snafu::{ResultExt, Snafu}; +use snafu::{OptionExt, ResultExt, Snafu}; use stackable_secret_operator_crd_utils::SecretReference; use tokio::{io::AsyncWriteExt, process::Command}; @@ -62,6 +62,9 @@ pub enum Error { #[snafu(display("failed to write request"))] WriteRequest { source: std::io::Error }, + + #[snafu(display("failed to obtain stdin for child process"))] + ChildStdin, } /// Provisions a Kerberos Keytab based on the [`Request`]. @@ -93,7 +96,7 @@ pub async fn provision_keytab(krb5_config_path: &Path, req: &Request) -> Result< // Get a `ChildStdin` object for the spawned process and write the serialized request // for a Principal into it in order for the child process to deserialize it and // process the request - let mut stdin = child.stdin.take().unwrap(); + let mut stdin = child.stdin.take().context(ChildStdinSnafu)?; stdin.write_all(&req_str).await.context(WriteRequestSnafu)?; stdin.flush().await.context(WriteRequestSnafu)?; drop(stdin); From 4bf69338da5531595cc7ed7811c64a898909a13a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Fri, 11 Jul 2025 09:51:56 +0200 Subject: [PATCH 5/6] Addressed comments --- rust/krb5-provision-keytab/src/lib.rs | 34 +++++++++++++++++---------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/rust/krb5-provision-keytab/src/lib.rs b/rust/krb5-provision-keytab/src/lib.rs index 3a86e7c0..e1e2716b 100644 --- a/rust/krb5-provision-keytab/src/lib.rs +++ b/rust/krb5-provision-keytab/src/lib.rs @@ -62,9 +62,6 @@ pub enum Error { #[snafu(display("failed to write request"))] WriteRequest { source: std::io::Error }, - - #[snafu(display("failed to obtain stdin for child process"))] - ChildStdin, } /// Provisions a Kerberos Keytab based on the [`Request`]. @@ -86,7 +83,7 @@ pub async fn provision_keytab(krb5_config_path: &Path, req: &Request) -> Result< // TGT that is obtained for the operation in the memory of the short lives process // spawned by `Command::new` above - this way it'll be wiped from memory once this exits // With any shared or persistent ticket cache this might stick around and potentially be - // reused by later runs + // reused by other volumes (which could cause privilege escalations and similar fun issues) .env("KRB5CCNAME", "MEMORY:") .stdin(Stdio::piped()) .stdout(Stdio::piped()) @@ -96,7 +93,10 @@ pub async fn provision_keytab(krb5_config_path: &Path, req: &Request) -> Result< // Get a `ChildStdin` object for the spawned process and write the serialized request // for a Principal into it in order for the child process to deserialize it and // process the request - let mut stdin = child.stdin.take().context(ChildStdinSnafu)?; + let mut stdin = child + .stdin + .take() + .expect("Failed to read from stdin of stackable-krb5-provision-keytab script! "); stdin.write_all(&req_str).await.context(WriteRequestSnafu)?; stdin.flush().await.context(WriteRequestSnafu)?; drop(stdin); @@ -110,11 +110,21 @@ pub async fn provision_keytab(krb5_config_path: &Path, req: &Request) -> Result< .await .context(WaitProvisionerSnafu)?; - // Check for success of the operation by deserializing stdout of the process to a `Response` - // struct - since `Response` is an empty struct with no fields this effectively means that - // any output will fail to deserialize and cause an `Error::RunProvisioner` to be propagated - // with the output of the child process - serde_json::from_slice::>(&output.stdout) - .context(DeserializeResponseSnafu)? - .map_err(|msg| Error::RunProvisioner { msg }) + // Check if the spawned command returned 0 as return code + if output.status.success() { + // Check for success of the operation by deserializing stdout of the process to a `Response` + // struct - since `Response` is an empty struct with no fields this effectively means that + // any output will fail to deserialize and cause an `Error::RunProvisioner` to be propagated + // with the output of the child process + serde_json::from_slice::>(&output.stdout) + .context(DeserializeResponseSnafu)? + .map_err(|msg| Error::RunProvisioner { msg }) + } else { + Err(Error::RunProvisioner { + msg: format!( + "Got non zero return code from stackable-krb5-provision-keytab: [{:?}]", + output.status.code() + ), + }) + } } From 89a46efe8893201c1fdc0929eecba26d72b49d59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Fri, 11 Jul 2025 10:00:44 +0200 Subject: [PATCH 6/6] Removed unused import --- rust/krb5-provision-keytab/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/krb5-provision-keytab/src/lib.rs b/rust/krb5-provision-keytab/src/lib.rs index e1e2716b..543c02bf 100644 --- a/rust/krb5-provision-keytab/src/lib.rs +++ b/rust/krb5-provision-keytab/src/lib.rs @@ -6,7 +6,7 @@ use std::{ }; use serde::{Deserialize, Serialize}; -use snafu::{OptionExt, ResultExt, Snafu}; +use snafu::{ResultExt, Snafu}; use stackable_secret_operator_crd_utils::SecretReference; use tokio::{io::AsyncWriteExt, process::Command};