Allow specifying OIDC Client Secrets in external config file

Signed-off-by: Olivier 'reivilibre <olivier@librepush.net>

config.toml

@@ -11,7 +11,6 @@ rsa_keypair = "keypair.pem"
 name = "some service"
 redirect_uris = ["http://localhost:9876/callback"]
 allow_user_classes = ["user"]
-secret = "lol"
 
 [postgres]
 connect = "postgres:"

secrets.toml

@@ -0,0 +1,3 @@
+[oidc_secrets]
+x = "abc"
+

src/bin/idcoop.rs

@@ -7,7 +7,7 @@ use comfy_table::presets::UTF8_FULL;
 use comfy_table::{Attribute, Cell, Color, ContentArrangement, Row, Table};
 use confique::{Config, Partial};
 use eyre::{bail, Context, ContextCompat};
-use idcoop::config::SecretConfig;
+use idcoop::config::{SecretConfig, SeparateSecretConfiguration};
 use idcoop::passwords::create_password_hash;
 use idcoop::store::{CreateUser, IdCoopStore};
 use idcoop::{config::Configuration, web};
@@ -22,6 +22,9 @@ struct Options {
     #[clap(short = 'c', long = "config")]
     config_files: Vec<PathBuf>,
 
+    #[clap(short = 'S', long = "secrets")]
+    secret_files: Vec<PathBuf>,
+
     #[clap(subcommand)]
     subcommand: Subcommand,
 }
@@ -42,6 +45,23 @@ enum Subcommand {
     },
 }
 
+fn load_config_files<C: Config>(files: &[PathBuf]) -> eyre::Result<C> {
+    let mut partial: C::Partial = Partial::from_env().context("failed to load env vars")?;
+
+    for file in files {
+        let cfg_file = confique::File::new(file)
+            .with_context(|| format!("bad file path: {file:?}"))?
+            .required()
+            .load()
+            .with_context(|| format!("failed to load file: {file:?}"))?;
+        partial = partial.with_fallback(cfg_file);
+    }
+
+    partial = partial.with_fallback(Partial::default_values());
+
+    C::from_partial(partial).context("failed to load config")
+}
+
 #[tokio::main(flavor = "current_thread")]
 async fn main() -> eyre::Result<()> {
     tracing_subscriber::registry()
@@ -54,23 +74,29 @@ async fn main() -> eyre::Result<()> {
 
     let options = Options::parse();
 
-    let mut partial: <Configuration as Config>::Partial =
+    let mut config = load_config_files::<Configuration>(&options.config_files)
-        Partial::from_env().context("failed to load env vars")?;
+        .context("failed to load main config file")?;
+    let mut sep_secrets = load_config_files::<SeparateSecretConfiguration>(&options.secret_files)
+        .context("failed to load separated secrets file")?;
 
-    for file in &options.config_files {
+    // Load secrets into the OIDC Client Configurations
-        let cfg_file = confique::File::new(file)
+    for (oidc_client_id, oidc_client) in &mut config.oidc.clients {
-            .with_context(|| format!("bad file path: {file:?}"))?
+        let specified_in_main = oidc_client.secret.is_some();
-            .required()
+        match sep_secrets.oidc_secrets.remove(oidc_client_id) {
-            .load()
+            Some(secret) => {
-            .with_context(|| format!("failed to load file: {file:?}"))?;
+                if specified_in_main {
-        partial = partial.with_fallback(cfg_file);
+                    bail!("Client secret for OIDC Client {oidc_client_id:?} is specified both in main config file and in separated secret file!");
+                }
+                oidc_client.secret = Some(secret);
+            }
+            None => {
+                if !specified_in_main {
+                    bail!("Client secret for OIDC Client {oidc_client_id:?} is not specified!");
+                }
+            }
+        }
     }
 
-    partial = partial.with_fallback(Partial::default_values());
-
-    let config: Configuration =
-        Configuration::from_partial(partial).context("failed to load config")?;
-
     match options.subcommand {
         Subcommand::Serve { bind } => {
             let Some(bind) = bind.or(config.listen.bind) else {

src/config.rs

@@ -81,13 +81,22 @@ pub struct OidcClientConfiguration {
     /// Must be explicit because it is security sensitive and we don't want a typo to fail open.
     /// User classes are defined by the admin but at the very least includes 'active' and 'not active' (implied if no 'active' class set).
     /// (Design subject to change)
+    /// TODO not sure this design is current
     pub allow_user_classes: Vec<String>,
 
     /// The shared secret for the client.
+    /// Must be populated (this is checked on startup).
+    /// Can be loaded from a separated secret configuration file.
     /// TODO
     /// - We should consider supporting other auth methods in the future.
-    /// - We should allow specifying this out of the main configuration somehow...?
+    pub secret: Option<String>,
-    pub secret: String,
+}
+
+#[derive(Config)]
+pub struct SeparateSecretConfiguration {
+    /// Client secrets for the OIDC clients.
+    #[config(default = {})]
+    pub oidc_secrets: BTreeMap<String, String>,
 }
 
 pub struct SecretConfig {
@@ -101,6 +110,7 @@ impl SecretConfig {
             .context("failed to load RSA private key")?;
         let rsa_key_pair =
             RsaKeyPair::from_pem(&rsa_keypair_bytes).context("Failed to decode RSA key pair")?;
+
         Ok(Self { rsa_key_pair })
     }
 }

src/web/oauth_openid/token.rs

@@ -108,10 +108,13 @@ pub async fn oidc_token(
     };
 
     if !bool::from(
-        basic_auth
+        basic_auth.password().as_bytes().ct_eq(
-            .password()
+            unverified_client_config
-            .as_bytes()
+                .secret
-            .ct_eq(unverified_client_config.secret.as_bytes()),
+                .as_ref()
+                .expect("client secret missing; should have been checked at startup")
+                .as_bytes(),
+        ),
     ) {
         return (
             StatusCode::BAD_REQUEST,