Radish alpha
r
rad:zwTxygwuz5LDGBq255RA2CbNGrz8
Radicle CI broker
Radicle
Git
feat: store sensitive environment variables in a new type
Merged liw opened 1 year ago

Add a new type for sensitive values.

The CI broker can define sensitive environment variables for each adapter. The idea is the this can be used to configure, say, API keys that the CI run needs. To avoid them leaking accidentally, the code tries to prevent the sensitive value from being printed. However, there have been case where this has failed to be done correctly.

The new type provides the protection in a more concentrated location. The value can be created via serde de-serialization, but can’t be printed using normal formatting, debug formatting, or serde serialization.

None of this makes it a good idea to use environment variables to convey sensitive values. They’re just a bad idea and you should probably find a better way to achieve the same thing. However, as long as the CI broker provides the sensitive_envs configuration variable the least it can do is try to make it hard to accidentally leak the values.

This is not a breaking change.

Signed-off-by: Lars Wirzenius liw@liw.fi

6 files changed +149 -6 26d55f06 5aea92df
modified ci-broker.md
@@ -50,7 +50,8 @@ adapters:
    command: ./adapter.sh
    env:
      RADICLE_NATIVE_CI: native-ci.yaml
-
    sensitive_env: {}
+
    sensitive_env:
+
      API_KEY: xyzzy
filters:
  - !Branch "main"
~~~
modified src/adapter.rs
@@ -21,6 +21,7 @@ use crate::{
    msg::{MessageError, Request, Response},
    notif::NotificationSender,
    run::{Run, RunState},
+
    sensitive::Sensitive,
    timeoutcmd::{TimeoutCommand, TimeoutError},
};

@@ -48,6 +49,13 @@ impl Adapter {
        self
    }

+
    pub fn with_sensitive_environment(mut self, env: &HashMap<String, Sensitive>) -> Self {
+
        for (key, value) in env.iter() {
+
            self.env.insert(key.into(), value.as_str().into());
+
        }
+
        self
+
    }
+

    fn envs(&self) -> impl Iterator<Item = (&OsStr, &OsStr)> {
        self.env.iter().map(|(k, v)| (k.as_ref(), v.as_ref()))
    }
modified src/bin/cib.rs
@@ -158,7 +158,7 @@ impl QueuedCmd {
                ))?;
        let adapter = Adapter::new(&spec.command)
            .with_environment(spec.envs())
-
            .with_environment(spec.sensitive_envs());
+
            .with_sensitive_environment(spec.sensitive_envs());
        logger::adapter_config(config);
        broker.set_default_adapter(&adapter);

@@ -246,7 +246,7 @@ impl ProcessEventsCmd {
                ))?;
        let adapter = Adapter::new(&spec.command)
            .with_environment(spec.envs())
-
            .with_environment(spec.sensitive_envs());
+
            .with_sensitive_environment(spec.sensitive_envs());
        logger::adapter_config(config);
        broker.set_default_adapter(&adapter);

modified src/config.rs
@@ -10,7 +10,7 @@ use std::{
use duration_str::deserialize_duration;
use serde::{Deserialize, Serialize};

-
use crate::filter::EventFilter;
+
use crate::{filter::EventFilter, sensitive::Sensitive};

const DEFAULT_MAX_RUN_TIME: Duration = Duration::from_secs(3600);
const DEFAULT_STATUS_PAGE_UPDATE_INTERVAL: u64 = 10;
@@ -80,14 +80,14 @@ impl fmt::Debug for Adapter {
pub struct Adapter {
    pub command: PathBuf,
    pub env: HashMap<String, String>,
-
    pub sensitive_env: HashMap<String, String>,
+
    pub sensitive_env: HashMap<String, Sensitive>,
}

impl Adapter {
    pub fn envs(&self) -> &HashMap<String, String> {
        &self.env
    }
-
    pub fn sensitive_envs(&self) -> &HashMap<String, String> {
+
    pub fn sensitive_envs(&self) -> &HashMap<String, Sensitive> {
        &self.sensitive_env
    }
}
modified src/lib.rs
@@ -22,6 +22,7 @@ pub mod pages;
pub mod queueadd;
pub mod queueproc;
pub mod run;
+
pub mod sensitive;
#[cfg(test)]
pub mod test;
pub mod timeoutcmd;
added src/sensitive.rs
@@ -0,0 +1,133 @@
+
//! A data type for holding sensitive data to avoid accidentally
+
//! leaking it.
+
//!
+
//! Once a `Sensitive` value has been created, it contains a string
+
//! value. It can be created by de-serializing using `serde`.
+
//!
+
//! The sensitive data can be accessed with [`Sensitive::as_str`]. The
+
//! caller needs to be careful to not leak that.
+
//!
+
//! `Sensitive` value itself can't be printed (via the `Display`
+
//! trait), even in debug mode (`Debug` trait), or serialized with
+
//! `serde`. Instead, the value is replaced with the string
+
//! `<REDACTED>`.
+
//!
+
//! Note that this does not prevent the value from ending up in a core
+
//! dump.
+

+
use std::fmt;
+

+
use serde::{de, Deserialize, Deserializer, Serialize, Serializer};
+

+
const PLACEHOLDER: &str = "<REDACTED>";
+

+
/// Hold a sensitive string, such as a password or an API token. The
+
/// value can be accessed ([`Sensitive::as_str`]), but won't be
+
/// printed, even in debug output, and won't be serialized by `serde`.
+
#[derive()]
+
pub struct Sensitive {
+
    #[allow(dead_code)]
+
    //#[serde(serialize_with = "serialize")]
+
    data: String,
+
}
+

+
impl fmt::Display for Sensitive {
+
    /// Serialize for normal output.
+
    fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
+
        write!(f, "{}", PLACEHOLDER)
+
    }
+
}
+

+
impl fmt::Debug for Sensitive {
+
    /// Serialize for debug output.
+
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
+
        write!(f, "{}", PLACEHOLDER)
+
    }
+
}
+

+
impl Serialize for Sensitive {
+
    /// Serialize for `serde`.
+
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+
    where
+
        S: Serializer,
+
    {
+
        serializer.serialize_str(PLACEHOLDER)
+
    }
+
}
+

+
impl<'de> Deserialize<'de> for Sensitive {
+
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+
    where
+
        D: Deserializer<'de>,
+
    {
+
        deserializer.deserialize_str(SensitiveVisitor)
+
    }
+
}
+

+
struct SensitiveVisitor;
+

+
impl<'de> de::Visitor<'de> for SensitiveVisitor {
+
    type Value = Sensitive;
+

+
    fn expecting(&self, _formatter: &mut fmt::Formatter) -> fmt::Result {
+
        Ok(())
+
    }
+

+
    fn visit_str<E>(self, s: &str) -> Result<Self::Value, E>
+
    where
+
        E: de::Error,
+
    {
+
        Ok(Sensitive { data: s.into() })
+
    }
+
}
+

+
impl Sensitive {
+
    #[cfg(test)]
+
    fn new(data: &str) -> Self {
+
        Self { data: data.into() }
+
    }
+

+
    /// Return the contained string in cleartext. Do not leak this string.
+
    pub fn as_str(&self) -> &str {
+
        &self.data
+
    }
+
}
+

+
#[cfg(test)]
+
mod test_sensitive {
+
    use super::*;
+

+
    #[test]
+
    fn displayed() {
+
        let s = Sensitive::new("foo");
+
        let output = format!("{s}");
+
        assert!(!output.contains("foo"));
+
    }
+

+
    #[test]
+
    fn debugged() {
+
        let s = Sensitive::new("foo");
+
        let output = format!("{s:?}");
+
        assert!(!output.contains("foo"));
+
    }
+

+
    #[test]
+
    fn ser() -> Result<(), Box<dyn std::error::Error>> {
+
        let s = Sensitive::new("foo");
+
        let output = serde_yml::to_string(&s)?;
+
        println!("{output:#?}");
+
        assert!(!output.contains("foo"));
+
        Ok(())
+
    }
+

+
    #[test]
+
    fn deser() -> Result<(), Box<dyn std::error::Error>> {
+
        #[derive(Deserialize)]
+
        struct Foo {
+
            secret: Sensitive,
+
        }
+
        let s: Foo = serde_yml::from_str("secret: foo")?;
+
        assert_eq!(s.secret.data, "foo");
+
        Ok(())
+
    }
+
}