Support for "secrecy" crate

[jhpratt]

Having support for secrecy::Secret behind a flag would be awesome. I think the simplest way would be to implicitly use Secret::new when decoding. When encoding, it's probably best to still require the user explicitly call .expose_secret()

[abonander]

The secret value will still be exposed in the connection buffer, at least until it is overwritten. I'm concerned about providing a false sense of security.

[jhpratt]

Is there any way that a user (or sqlx) could forcibly zero out the connection buffer? As it stands currently, I'm just using a second struct and calling .map(Into::into) on the query result. It's obviously not ideal, but it's what's necessary for my use case.

NB: I, personally, don't need the super strict security of zeroing memory. I'm just using the secrecy crate to mask secrets in the debug representation without having to manually implement it. Obviously use cases vary, however.

[abonander]

You can pretty easily implement a newtype wrapper and manually implement Debug for it:

#[derive(sqlx::Type)]
#[sqlx(transparent)]
pub struct SecretString(String);

impl Debug for SecretString {
    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
        f.pad_str("(secret)")
    }
}

It's really a damn shame we don't have attributes to tell #[derive(Debug)] how to print certain fields (or not at all). If I had the time I'd love to draft an RFC for that. I'm sure there's a crate for that but it'd be nice if it was part of the standard derive.

We could probably implement zeroing for the connection buffer, although the way it's implemented in 0.4.0-beta.1 the buffer is actually a BytesMut that's read into and then split off from, so instead it'd have to be a drop impl on Row that zeroes the data buffer.

However, keep in mind that zeroing memory won't magically make the application more secure; it's just a mitigation in case the application is already an attack target.

I'm no security expert, but assuming we (or you as the user, or another crate in your dependency graph; are you auditing them all?) don't introduce an unsafe bug that allows easy arbitrary memory access, it would take the machine running the application to suffer a combined remote-code-execution and privilege escalation attack in order for someone to be able to read memory of a running application, which means you have worse problems than someone being able to read data in some temporary buffers.

An attacker could just pull the credentials for your DB, which stay resident in memory unless you store them securely and only access them on-demand, and read all your users' juicy secrets at their leisure. This would also happen if they had arbitrary memory access.

Should we also provide a mechanism for secure storage of DB credentials? Maybe. It's certainly an interesting idea.

However, the attack surface for database clients and servers is so large that I don't think zeroing a single temporary buffer, which would likely be overwritten in short order anyways, would significantly hinder an attacker. It would take a comprehensive security-hardening effort that I don't think we have the time, energy or expertise to execute competently.

Instead, I think we should focus on auditing for, and not introducing new, vulnerabilities rather than adding mitigations which will negatively affect performance and lure naive users into a false sense of security. Sure, we could make it opt-in which reduces the impact of the former, but then makes the latter worse as it'd be a visible feature to turn on and think "okay, everything should be good now".

[jhpratt]

Yeah, I'm certainly no security expert — just doing what I think might help.

Have you (or anyone else) considered the possibility of attributes, similar to serde? That allows renaming fields, automatically performing Into/TryInto, etc. Being able to perform Into automatically on a single field would be useful for more than just the secrecy crate, too. I'm not sure how this would work with something like query_as!, as I don't have any derives or similar on existing code.

[abonander]

@jhpratt triage update: adding extra attributes is an orthogonal discussion, IMO. We have an old issue for that, feel free to propose there: #156

And making query_as! work with FromRow is being tracked here: #514

Otherwise, I'm going to close this.

[ohaddahan]

@jhpratt I was thinking of opening a PR suggesting to add something like what I use in my repos (can be extended further, probably need to implement the encode trait too).
Obviously we can still see the secrets if we log the queries but this is the earliest (I'm aware of) we can suppress them.

@abonander you think that adding native sqlx support will give false sense of security with something like this?

#[derive(sqlx::FromRow, Deserialize, Serialize, Debug)]
pub struct User {
    pub id: Uuid,
    pub username: String
    #[serde(skip)]
    pub password: DBHidden<String>,
}

use secrecy::zeroize::DefaultIsZeroes;
use secrecy::Secret;
use sqlx::database::HasValueRef;
use sqlx::{Decode, Postgres};
use std::error::Error;
use std::fmt::{Debug, Display};

pub struct DBHidden<T>(Secret<T>)
where
    T: Default + DefaultIsZeroes + Clone;

impl<T> Default for DBHidden<T>
where
    T: Default + DefaultIsZeroes,
{
    fn default() -> Self {
        Self(Secret::new(T::default()))
    }
}

impl<T> Display for DBHidden<T>
where
    T: Default + DefaultIsZeroes + Display,
{
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "[hidden]")
    }
}

impl<T> Debug for DBHidden<T>
where
    T: Default + DefaultIsZeroes + Debug,
{
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "[hidden]")
    }
}

impl<T> sqlx::Type<Postgres> for DBHidden<T>
where
    T: Default + DefaultIsZeroes + sqlx::Type<Postgres>,
{
    fn type_info() -> sqlx::postgres::PgTypeInfo {
        <T as sqlx::Type<Postgres>>::type_info()
    }
}

impl<T> sqlx::Decode<'_, Postgres> for DBHidden<T>
where
    for<'a> T: DefaultIsZeroes + sqlx::Type<Postgres> + sqlx::Decode<'a, Postgres>,
{
    fn decode(
        value: <Postgres as HasValueRef<'_>>::ValueRef,
    ) -> Result<Self, Box<dyn Error + 'static + Send + Sync>> {
        let value = <T as Decode<Postgres>>::decode(value)?;
        Ok(DBHidden::from(value))
    }
}

impl<T> AsRef<Secret<T>> for DBHidden<T>
where
    T: DefaultIsZeroes,
{
    fn as_ref(&self) -> &Secret<T> {
        &self.0
    }
}

impl<T> From<T> for DBHidden<T>
where
    T: DefaultIsZeroes,
{
    fn from(s: T) -> Self {
        Self(Secret::new(s))
    }
}