STR-482 improved master xpriv resolution (#705)

bridge-client: improved master xpriv resolution

Author: delbonis

bin/bridge-client/src/modes/operator/bootstrap.rs

@@ -1,6 +1,6 @@
 //! Module to bootstrap the operator node by hooking up all the required services.
 
-use std::{path::PathBuf, sync::Arc, time::Duration};
+use std::{env, path::PathBuf, sync::Arc, time::Duration};
 
 use bitcoin::{
     key::{Keypair, Parity},
@@ -36,7 +36,7 @@ use crate::{
     },
     db::open_rocksdb_database,
     rpc_server::{self, BridgeRpc},
-    xpriv::resolve_xpriv,
+    xpriv::{resolve_xpriv, OPXPRIV_ENVVAR},
 };
 
 // TODO: move this to some common util and make this usable outside tokio
@@ -112,13 +112,23 @@ pub(crate) async fn bootstrap(args: Cli) -> anyhow::Result<()> {
         .expect("cannot get RPC client from pool");
 
     // Get the keypair after deriving the wallet xpriv.
-    let operator_keys = resolve_xpriv(args.master_xpriv, args.master_xpriv_path)?;
+    let env_key = match env::var(OPXPRIV_ENVVAR) {
+        Ok(k) => Some(k),
+        Err(env::VarError::NotPresent) => None,
+        Err(env::VarError::NotUnicode(_)) => {
+            error!("operator master xpriv envvar not unicode, ignoring");
+            None
+        }
+    };
+
+    let operator_keys = resolve_xpriv(args.master_xpriv, args.master_xpriv_path, env_key)?;
     let wallet_xpriv = operator_keys.wallet_xpriv();
 
     let mut keypair = wallet_xpriv.to_keypair(SECP256K1);
     let mut sk = SecretKey::from_keypair(&keypair);
 
     // adjust for parity, which should always be even
+    // FIXME bake this into the key derivation fn!
     let (_, parity) = XOnlyPublicKey::from_keypair(&keypair);
     if matches!(parity, Parity::Odd) {
         sk = sk.negate();

bin/bridge-client/src/xpriv.rs

@@ -1,73 +1,68 @@
 //! Parses the operator's master xpriv from a file.
 
-use std::{
-    env,
-    fs::read_to_string,
-    path::{Path, PathBuf},
-};
+use std::fs::read_to_string;
 
 use bitcoin::bip32::Xpriv;
 use strata_key_derivation::operator::OperatorKeys;
-use strata_primitives::keys::ZeroizableXpriv;
+use tracing::*;
 use zeroize::Zeroize;
 
 /// The environment variable that contains the operator's master [`Xpriv`].
-const OPXPRIV_ENVVAR: &str = "STRATA_OP_MASTER_XPRIV";
+pub const OPXPRIV_ENVVAR: &str = "STRATA_OP_MASTER_XPRIV";
 
-/// Parses the master [`Xpriv`] from a file.
-pub(crate) fn parse_master_xpriv(path: &Path) -> anyhow::Result<OperatorKeys> {
-    let mut xpriv_str = read_to_string(path)?;
-    match xpriv_str.parse::<Xpriv>() {
-        Ok(mut xpriv) => {
-            // Zeroize the xpriv string after parsing it.
-            xpriv_str.zeroize();
-
-            // Parse into ZeroizableXpriv
-            let zeroizable_xpriv: ZeroizableXpriv = xpriv.into();
-
-            // Zeroize the xpriv after parsing it.
-            xpriv.private_key.non_secure_erase();
-
-            // Finally return the operator keys
-            //
-            // NOTE: `zeroizable_xpriv` is zeroized on drop.
-            Ok(OperatorKeys::new(&zeroizable_xpriv)
-                .map_err(|_| anyhow::anyhow!("invalid master xpriv"))?)
-        }
-        Err(e) => anyhow::bail!("invalid master xpriv: {}", e),
-    }
-}
-
-/// Resolves the master [`Xpriv`] from CLI arguments or environment variables.
+/// Resolves the master [`Xpriv`] from the various sources.
 ///
-/// Precedence order for resolving the master xpriv:
+/// Rules:
 ///
-/// 1. If a key is supplied via the `--master-xpriv` CLI argument, it is used.
-/// 2. Otherwise, if a file path is supplied via CLI, the key is read from that file.
-/// 3. Otherwise, if the `STRATA_OP_MASTER_XPRIV` environment variable is set, its value is used.
-/// 4. Otherwise, returns an error.
+/// 1. If none are set, error out.
+/// 2. If multiple are set, error out.
+/// 3. If we have the verbatim key provided, parse it.
+/// 4. If we have a path provided, load it and parse that instead.
 ///
 /// # Errors
 ///
-/// Returns an error if the master xpriv is invalid or not found.
+/// Returns an error if the master xpriv is invalid or not found, or if
+/// conflicting options are set.
 pub(crate) fn resolve_xpriv(
     cli_arg: Option<String>,
     cli_path: Option<String>,
+    env_val: Option<String>,
 ) -> anyhow::Result<OperatorKeys> {
-    match (cli_arg, cli_path) {
-        (Some(xpriv), _) => OperatorKeys::new(&xpriv.parse::<Xpriv>()?)
-            .map_err(|_| anyhow::anyhow!("invalid master xpriv from CLI")),
+    if cli_arg.is_some() {
+        error!("FOUND CLI ARG KEY, THIS IS INSECURE!");
+    }
 
-        (_, Some(path)) => parse_master_xpriv(&PathBuf::from(path)),
+    let mut xpriv_str: String = match (cli_arg, cli_path, env_val) {
+        // If there's none set then we error out.
+        (None, None, None) => {
+            anyhow::bail!(
+                "must provide root xpriv with either `--master-xpriv-path` or {OPXPRIV_ENVVAR}"
+            )
+        }
 
-        (None, None) => match env::var(OPXPRIV_ENVVAR) {
-            Ok(xpriv_env_str) => OperatorKeys::new(&xpriv_env_str.parse::<Xpriv>()?)
-                .map_err(|_| anyhow::anyhow!("invalid master xpriv from envvar")),
-            Err(_) => {
-                anyhow::bail!(
-                    "must either set {OPXPRIV_ENVVAR} envvar or pass with `--master-xpriv`"
-                )
-            }
-        },
-    }
+        // If multiple are set then we error out.
+        (_, Some(_), Some(_)) | (Some(_), Some(_), _) | (Some(_), _, Some(_)) => {
+            anyhow::bail!("multiple root xpriv options specified, don't know what to do, aborting");
+        }
+
+        // In these cases we have the string explicitly.
+        (Some(xpriv_str), _, _) | (_, _, Some(xpriv_str)) => xpriv_str.to_owned(),
+
+        // In this case we fetch it from file.
+        (_, Some(path), _) => read_to_string(path)?,
+    };
+
+    // Some fancy dance to securely erase things.
+    let Ok(raw) = xpriv_str.parse::<Xpriv>() else {
+        xpriv_str.zeroize();
+        anyhow::bail!("invalid master xpriv");
+    };
+
+    let Ok(keys) = OperatorKeys::new(&raw) else {
+        xpriv_str.zeroize();
+        // TODO how to secure erase raw?
+        anyhow::bail!("unable to generate leaf keys");
+    };
+
+    Ok(keys)
 }