Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: use secrecy SerectString to hold secrets option #3804

Merged
merged 16 commits into from
Apr 29, 2024
16 changes: 2 additions & 14 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions licenserc.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ includes = [
excludes = [
# copied sources
"src/common/base/src/readable_size.rs",
"src/common/base/src/secrets.rs",
"src/servers/src/repeated_field.rs",
"src/servers/src/http/test_helpers.rs",
]
Expand Down
2 changes: 1 addition & 1 deletion src/auth/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@ workspace = true
[dependencies]
api.workspace = true
async-trait.workspace = true
common-base.workspace = true
common-error.workspace = true
common-macro.workspace = true
common-telemetry.workspace = true
digest = "0.10"
notify.workspace = true
secrecy = { version = "0.8", features = ["serde", "alloc"] }
sha1 = "0.10"
snafu.workspace = true
sql.workspace = true
Expand Down
2 changes: 1 addition & 1 deletion src/auth/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@

use std::sync::Arc;

use common_base::secrets::SecretString;
use digest::Digest;
use secrecy::SecretString;
use sha1::Sha1;
use snafu::{ensure, OptionExt};

Expand Down
2 changes: 1 addition & 1 deletion src/auth/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use secrecy::ExposeSecret;
use common_base::secrets::ExposeSecret;

use crate::error::{
AccessDeniedSnafu, Result, UnsupportedPasswordTypeSnafu, UserNotFoundSnafu,
Expand Down
2 changes: 1 addition & 1 deletion src/auth/src/user_provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use std::io;
use std::io::BufRead;
use std::path::Path;

use secrecy::ExposeSecret;
use common_base::secrets::ExposeSecret;
use snafu::{ensure, OptionExt, ResultExt};

use crate::common::{Identity, Password};
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/src/standalone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -635,7 +635,7 @@ mod tests {
match &dn_opts.storage.providers[1] {
datanode::config::ObjectStoreConfig::S3(s3_config) => {
assert_eq!(
"Secret([REDACTED alloc::string::String])".to_string(),
"SecretBox<alloc::string::String>([REDACTED])".to_string(),
format!("{:?}", s3_config.access_key_id)
);
}
Expand Down
1 change: 1 addition & 0 deletions src/common/base/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ common-macro.workspace = true
paste = "1.0"
serde = { version = "1.0", features = ["derive"] }
snafu.workspace = true
zeroize = { version = "1.6", default-features = false, features = ["alloc"] }

[dev-dependencies]
toml.workspace = true
1 change: 1 addition & 0 deletions src/common/base/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ pub mod buffer;
pub mod bytes;
#[allow(clippy::all)]
pub mod readable_size;
pub mod secrets;

use core::any::Any;
use std::sync::{Arc, Mutex, MutexGuard};
Expand Down
218 changes: 218 additions & 0 deletions src/common/base/src/secrets.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,218 @@
// This file is copied from: https://github.com/iqlusioninc/crates/blob/f98d4ccf/secrecy/src/lib.rs.

//! [`SecretBox`] wrapper type for more carefully handling secret values
//! (e.g. passwords, cryptographic keys, access tokens or other credentials)
//!
//! # Goals
//!
//! - Make secret access explicit and easy-to-audit via the
//! [`ExposeSecret`] and [`ExposeSecretMut`] traits.
//! - Prevent accidental leakage of secrets via channels like debug logging
//! - Ensure secrets are wiped from memory on drop securely
//! (using the [`zeroize`] crate)
//!
//! Presently this crate favors a simple, `no_std`-friendly, safe i.e.
//! `forbid(unsafe_code)`-based implementation and does not provide more advanced
//! memory protection mechanisms e.g. ones based on `mlock(2)`/`mprotect(2)`.
//! We may explore more advanced protection mechanisms in the future.
//! Those who don't mind `std` and `libc` dependencies should consider using
//! the [`secrets`](https://crates.io/crates/secrets) crate.
//!
//! # `serde` support
//!
//! When the `serde` feature of this crate is enabled, the [`SecretBox`] type will
//! receive a [`Deserialize`] impl for all `SecretBox<T>` types where
//! `T: DeserializeOwned`. This allows *loading* secret values from data
//! deserialized from `serde` (be careful to clean up any intermediate secrets
//! when doing this, e.g. the unparsed input!)
//!
//! To prevent exfiltration of secret values via `serde`, by default `SecretBox<T>`
//! does *not* receive a corresponding [`Serialize`] impl. If you would like
//! types of `SecretBox<T>` to be serializable with `serde`, you will need to impl
//! the [`SerializableSecret`] marker trait on `T`

use std::fmt::Debug;
use std::{any, fmt};

use serde::{de, ser, Deserialize, Serialize};
use zeroize::{Zeroize, ZeroizeOnDrop};

/// Wrapper type for strings that contains secrets. See also [SecretBox].
pub type SecretString = SecretBox<String>;

impl From<String> for SecretString {
fn from(value: String) -> Self {
SecretString::new(Box::new(value))
}
}

/// Wrapper type for values that contains secrets, which attempts to limit
/// accidental exposure and ensure secrets are wiped from memory when dropped.
/// (e.g. passwords, cryptographic keys, access tokens or other credentials)
///
/// Access to the secret inner value occurs through the [`ExposeSecret`]
/// or [`ExposeSecretMut`] traits, which provide methods for accessing the inner secret value.
pub struct SecretBox<S: Zeroize> {
inner_secret: Box<S>,
}

impl<S: Zeroize> Zeroize for SecretBox<S> {
fn zeroize(&mut self) {
self.inner_secret.as_mut().zeroize()
}
}

impl<S: Zeroize> Drop for SecretBox<S> {
fn drop(&mut self) {
self.zeroize()
}
}

impl<S: Zeroize> ZeroizeOnDrop for SecretBox<S> {}

impl<S: Zeroize> From<Box<S>> for SecretBox<S> {
fn from(source: Box<S>) -> Self {
Self::new(source)
}
}

impl<S: Zeroize> SecretBox<S> {
/// Create a secret value using a pre-boxed value.
pub fn new(boxed_secret: Box<S>) -> Self {
Self {
inner_secret: boxed_secret,
}
}
}

impl<S: Zeroize + Default> SecretBox<S> {
/// Create a secret value using a function that can initialize the vale in-place.
pub fn new_with_mut(ctr: impl FnOnce(&mut S)) -> Self {
let mut secret = Self::default();
ctr(secret.expose_secret_mut());
secret
}
}

impl<S: Zeroize + Clone> SecretBox<S> {
/// Create a secret value using the provided function as a constructor.
///
/// The implementation makes an effort to zeroize the locally constructed value
/// before it is copied to the heap, and constructing it inside the closure minimizes
/// the possibility of it being accidentally copied by other code.
///
/// **Note:** using [`Self::new`] or [`Self::new_with_mut`] is preferable when possible,
/// since this method's safety relies on empyric evidence and may be violated on some targets.
pub fn new_with_ctr(ctr: impl FnOnce() -> S) -> Self {
let mut data = ctr();
let secret = Self {
inner_secret: Box::new(data.clone()),
};
data.zeroize();
secret
}

/// Same as [`Self::new_with_ctr`], but the constructor can be fallible.
///
///
/// **Note:** using [`Self::new`] or [`Self::new_with_mut`] is preferable when possible,
/// since this method's safety relies on empyric evidence and may be violated on some targets.
pub fn try_new_with_ctr<E>(ctr: impl FnOnce() -> Result<S, E>) -> Result<Self, E> {
let mut data = ctr()?;
let secret = Self {
inner_secret: Box::new(data.clone()),
};
data.zeroize();
Ok(secret)
}
}

impl<S: Zeroize + Default> Default for SecretBox<S> {
fn default() -> Self {
Self {
inner_secret: Box::<S>::default(),
}
}
}

impl<S: Zeroize> Debug for SecretBox<S> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "SecretBox<{}>([REDACTED])", any::type_name::<S>())
}
}

impl<S> Clone for SecretBox<S>
where
S: Clone + Zeroize,
{
fn clone(&self) -> Self {
SecretBox {
inner_secret: self.inner_secret.clone(),
}
}
}

impl<S: Zeroize> ExposeSecret<S> for SecretBox<S> {
fn expose_secret(&self) -> &S {
self.inner_secret.as_ref()
}
}

impl<S: Zeroize> ExposeSecretMut<S> for SecretBox<S> {
fn expose_secret_mut(&mut self) -> &mut S {
self.inner_secret.as_mut()
}
}

/// Expose a reference to an inner secret
pub trait ExposeSecret<S> {
/// Expose secret: this is the only method providing access to a secret.
fn expose_secret(&self) -> &S;
}

/// Expose a mutable reference to an inner secret
pub trait ExposeSecretMut<S> {
/// Expose secret: this is the only method providing access to a secret.
fn expose_secret_mut(&mut self) -> &mut S;
}

/// Marker trait for secret types which can be [`Serialize`]-d by [`serde`].
///
/// When the `serde` feature of this crate is enabled and types are marked with
/// this trait, they receive a [`Serialize` impl][1] for `SecretBox<T>`.
/// (NOTE: all types which impl `DeserializeOwned` receive a [`Deserialize`]
/// impl)
///
/// This is done deliberately to prevent accidental exfiltration of secrets
/// via `serde` serialization.
///
/// If you really want to have `serde` serialize those types, use the
/// [`serialize_with`][2] attribute to specify a serializer that exposes the secret.
///
/// [1]: https://docs.rs/secrecy/latest/secrecy/struct.Secret.html#implementations
/// [2]: https://serde.rs/field-attrs.html#serialize_with
pub trait SerializableSecret: Serialize {}

impl<'de, T> Deserialize<'de> for SecretBox<T>
where
T: Zeroize + Clone + de::DeserializeOwned + Sized,
{
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: de::Deserializer<'de>,
{
Self::try_new_with_ctr(|| T::deserialize(deserializer))
}
}

impl<T> Serialize for SecretBox<T>
where
T: Zeroize + SerializableSecret + Serialize + Sized,
{
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: ser::Serializer,
{
self.expose_secret().serialize(serializer)
}
}
1 change: 0 additions & 1 deletion src/datanode/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ prometheus.workspace = true
prost.workspace = true
query.workspace = true
reqwest.workspace = true
secrecy = { version = "0.8", features = ["serde", "alloc"] }
serde.workspace = true
servers.workspace = true
session.workspace = true
Expand Down
6 changes: 3 additions & 3 deletions src/datanode/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
//! Datanode configurations

use common_base::readable_size::ReadableSize;
use common_base::secrets::SecretString;
use common_grpc::channel_manager::{
DEFAULT_MAX_GRPC_RECV_MESSAGE_SIZE, DEFAULT_MAX_GRPC_SEND_MESSAGE_SIZE,
};
Expand All @@ -24,7 +25,6 @@ use common_wal::config::DatanodeWalConfig;
use file_engine::config::EngineConfig as FileEngineConfig;
use meta_client::MetaClientOptions;
use mito2::config::MitoConfig;
use secrecy::SecretString;
use serde::{Deserialize, Serialize};
use servers::export_metrics::ExportMetricsOption;
use servers::heartbeat_options::HeartbeatOptions;
Expand Down Expand Up @@ -285,7 +285,7 @@ pub enum RegionEngineConfig {

#[cfg(test)]
mod tests {
use secrecy::ExposeSecret;
use common_base::secrets::ExposeSecret;

use super::*;

Expand All @@ -308,7 +308,7 @@ mod tests {
match &opts.storage.store {
ObjectStoreConfig::S3(cfg) => {
assert_eq!(
"Secret([REDACTED alloc::string::String])".to_string(),
"SecretBox<alloc::string::String>([REDACTED])".to_string(),
format!("{:?}", cfg.access_key_id)
);
assert_eq!("access_key_id", cfg.access_key_id.expose_secret());
Expand Down
Loading
Loading