torrust-tracker icon indicating copy to clipboard operation
torrust-tracker copied to clipboard

Overhaul Secrets: consider using `secrecy` to handle secrets

Open josecelano opened this issue 7 months ago • 0 comments

Context

The Torrust Tracker handles these secrets:

  • API tokens
  • Database passwords

You can find those secrets in the configuration in these sections:

# Database secrets
[core.database]
driver = "mysql"
path = "mysql://db_user:db_user_secret_password@mysql:3306/torrust_tracker"

# API token
[http_api.access_tokens]
admin = "MyAccessToken"

When the tracker starts, it prints to the console the current configuration used (after merging all sources with Figment)

To avoid exposing these secrets, the configuration includes methods to mask them.

During the app setup:

pub fn setup() -> (Configuration, AppContainer) {
    #[cfg(not(test))]
    check_seed();

    let configuration = initialize_configuration();

    if let Err(e) = configuration.validate() {
        panic!("Configuration error: {e}");
    }

    initialize_global_services(&configuration);

    tracing::info!("Configuration:\n{}", configuration.clone().mask_secrets().to_json());

    let app_container = AppContainer::initialize(&configuration);

    (configuration, app_container)
}

The configuration is cloned and secrets are masked (mask_secrets). That function calls the other config section to mask all the secrets in the configuration.

pub fn mask_secrets(mut self) -> Self {
    self.core.database.mask_secrets();

    if let Some(ref mut api) = self.http_api {
        api.mask_secrets();
    }

    self
}

Problem

Even with that functionality, secrets may leak, as happened here:

https://github.com/torrust/torrust-tracker/issues/1441

In that case, they were exposed via tracing instrumentation.

Solution

I've been considering using the newtype pattern to create a custom type for "Secret" that implements Debug and Display traits without exposing the secrets.

It would be something like:

use std::fmt;

/// Secret wrapper around a `String`
#[derive(Clone, PartialEq, Eq)]
pub struct Secret(String);

impl Secret {
    pub fn new(value: impl Into<String>) -> Self {
        Secret(value.into())
    }

    /// Access the inner value (use with care!)
    pub fn expose(&self) -> &str {
        &self.0
    }

    /// Consume the secret and extract the value
    pub fn into_inner(self) -> String {
        self.0
    }
}

impl fmt::Debug for Secret {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "\"***\"")
    }
}

impl fmt::Display for Secret {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "***")
    }
}

But after a preliminary research with AI, there are some crates related to this topic:

  • secrecy: https://docs.rs/secrecy/0.10.3/secrecy/index.html
  • zeroize: https://docs.rs/zeroize/latest/zeroize/index.html
  • secrets: https://crates.io/crates/secrets

I have only read a little bit but it seems:

  • secrecyprovided a wrapped type for secrets.
  • zeroize makes sure memory is cleaned after using the secrets.

Since secrecy also ensures secrets are securely wiped from memory when dropped, I would choose it. Another advantage is that you can clearly identify all secrets in the app.

Notes

For the database password we use a URL type.

[core.database]
driver = "mysql"
path = "mysql://db_user:db_user_secret_password@mysql:3306/torrust_tracker"

That means the whole value is not a secret. It's only a substring (db_user_secret_password).

We could need to split that information into multiple configuration values to isolate the secret. In fact, that's something I would do anyway. I have always thought the configuration is too coupled to the underlying database drivers. Using an URL for the DB connection string might not be a common practice. However, if we split it we can end up having config values that only make sense for some DB drivers. For example:

SQLite:

[core.database]
path = "/var/lib/torrust/tracker/database/sqlite3.db"

MySQL:

[core.database]
driver = "mysql"
path = "mysql://db_user:db_user_secret_password@mysql:3306/torrust_tracker"

Proposal:

[core.database]

[core.database.sqlite]
path = "/var/lib/torrust/tracker/database/sqlite3.db"

[core.database.mysql]
user = "db_user"
password = "db_user_secret_password"
host = "mysql"
port = "3306"
database = "torrust_tracker"

It could be implemented as a non-breaking change if we prioritise the old field "path".

cc @da2ce7

josecelano avatar Apr 30 '25 09:04 josecelano