Radish alpha
h
rad:z3gqcJUoA1n9HaHKufZs5FCSGazv5
Radicle Heartwood Protocol & Stack
Radicle
Git
cli: Detect key mismatch
Merged lorenz opened 7 months ago

When run with a secret and public key that do not cryptographically match, fn radicle_cli::terminal::io::signer would prompt the user for a password to unlock the secret key, and then carry on with a mismatching key pair.

Fix this by verifying that the keys match in MemorySigner::load and only swallow errors initializing the signer in cases where prompting for a password makes sense. It does not make sense in the case of mismatched keys.

6 files changed +78 -20 ed8b0860 fafb3493
added crates/radicle-cli/examples/rad-key-mismatch.md
@@ -0,0 +1,6 @@
+
This test assumes that one of the two keys in `$RAD_HOME/keys` was swapped so that `$RAD_HOME/keys/radicle{,.pub}` do not match anymore.
+

+
``` (fail)
+
$ rad issue open --title "flux capacitor underpowered" --description "Flux capacitor power requirements exceed current supply" --no-announce
+
✗ Error: secret and public keys in '[..]/.radicle/keys' do not match
+
```

\ No newline at end of file
modified crates/radicle-cli/src/terminal/io.rs
@@ -45,9 +45,15 @@ impl inquire::validator::StringValidator for PassphraseValidator {
/// Get the signer. First we try getting it from ssh-agent, otherwise we prompt the user,
/// if we're connected to a TTY.
pub fn signer(profile: &Profile) -> anyhow::Result<BoxedDevice> {
-
    if let Ok(signer) = profile.signer() {
-
        return Ok(signer);
+
    match profile.signer() {
+
        Ok(signer) => return Ok(signer),
+
        Err(err) if !err.prompt_for_passphrase() => return Err(anyhow!(err)),
+
        Err(_) => {
+
            // The error returned is potentially recoverable by prompting
+
            // the user for the correct passphrase.
+
        }
    }
+

    let validator = PassphraseValidator::new(profile.keystore.clone());
    let passphrase = match passphrase(validator)? {
        Some(p) => p,
modified crates/radicle-cli/tests/commands.rs
@@ -1,6 +1,6 @@
use std::path::Path;
use std::str::FromStr;
-
use std::{net, thread, time};
+
use std::{fs, net, thread, time};

use radicle::cob;
use radicle::git;
@@ -56,6 +56,20 @@ fn rad_auth() {
}

#[test]
+
fn rad_key_mismatch() {
+
    let mut environment = Environment::new();
+
    let alice = environment.profile("alice");
+
    environment.repository(&alice);
+

+
    environment.test("rad-init", &alice).unwrap();
+

+
    // Replace the public key with one that does not match the secret key anymore.
+
    fs::write(alice.home.path().join("keys").join("radicle.pub"), "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIE6Ul/D+P0I/Hl1JVOWGS8Z589us9FqKQXWv8OMOpKCh snakeoil\n").unwrap();
+

+
    environment.test("rad-key-mismatch", &alice).unwrap();
+
}
+

+
#[test]
fn rad_auth_errors() {
    test("examples/rad-auth-errors.md", Path::new("."), None, []).unwrap();
}
modified crates/radicle-crypto/src/ssh/keystore.rs
@@ -183,6 +183,8 @@ pub enum MemorySignerError {
    NotFound(PathBuf),
    #[error("invalid passphrase")]
    InvalidPassphrase,
+
    #[error("secret and public keys in '{path}' do not match")]
+
    KeyMismatch { path: PathBuf },
}

/// An in-memory signer that keeps its secret key internally
@@ -272,6 +274,12 @@ impl MemorySigner {
            })?
            .ok_or_else(|| MemorySignerError::NotFound(keystore.path().to_path_buf()))?;

+
        secret
+
            .validate_public_key(&public)
+
            .map_err(|_| MemorySignerError::KeyMismatch {
+
                path: keystore.path().to_path_buf(),
+
            })?;
+

        Ok(Self { public, secret })
    }

modified crates/radicle-remote-helper/src/push.rs
@@ -74,6 +74,9 @@ pub enum Error {
    /// Profile error.
    #[error(transparent)]
    Profile(#[from] radicle::profile::Error),
+
    /// Signer error.
+
    #[error(transparent)]
+
    Signer(#[from] radicle::profile::SignerError),
    /// Parse error for object IDs.
    #[error(transparent)]
    ParseObjectId(#[from] ParseObjectId),
modified crates/radicle/src/profile.rs
@@ -172,14 +172,8 @@ pub enum Error {
    Routing(#[from] node::routing::Error),
    #[error(transparent)]
    Keystore(#[from] keystore::Error),
-
    #[error(transparent)]
-
    MemorySigner(#[from] keystore::MemorySignerError),
    #[error("no radicle profile found at path '{0}'")]
    NotFound(PathBuf),
-
    #[error("error connecting to ssh-agent: {0}")]
-
    Agent(#[from] crate::crypto::ssh::agent::Error),
-
    #[error("radicle key `{0}` is not registered; run `rad auth` to register it with ssh-agent")]
-
    KeyNotRegistered(PublicKey),
    #[error(transparent)]
    PolicyStore(#[from] node::policy::store::Error),
    #[error(transparent)]
@@ -194,6 +188,37 @@ pub enum Error {
    Storage(#[from] storage::Error),
}

+
#[derive(Debug, Error)]
+
pub enum SignerError {
+
    #[error(transparent)]
+
    MemorySigner(#[from] keystore::MemorySignerError),
+

+
    #[error(transparent)]
+
    Agent(#[from] crate::crypto::ssh::agent::Error),
+

+
    #[error("radicle key `{0}` is not registered; run `rad auth` to register it with ssh-agent")]
+
    KeyNotRegistered(PublicKey),
+

+
    #[error(transparent)]
+
    Keystore(#[from] keystore::Error),
+

+
    #[error("error connecting to ssh-agent: {source}")]
+
    AgentConnection {
+
        source: crate::crypto::ssh::agent::Error,
+
    },
+
}
+

+
impl SignerError {
+
    /// Some signer errors are potentially recoverable by prompting the user
+
    /// for a password.
+
    pub fn prompt_for_passphrase(&self) -> bool {
+
        matches!(
+
            self,
+
            Self::AgentConnection { .. } | Self::KeyNotRegistered(_)
+
        )
+
    }
+
}
+

#[derive(Debug, Clone)]
pub struct Profile {
    pub home: Home,
@@ -296,7 +321,7 @@ impl Profile {
        Did::from(self.public_key)
    }

-
    pub fn signer(&self) -> Result<BoxedDevice, Error> {
+
    pub fn signer(&self) -> Result<BoxedDevice, SignerError> {
        if !self.keystore.is_encrypted()? {
            let signer = keystore::MemorySigner::load(&self.keystore, None)?;
            return Ok(Device::from(signer).boxed());
@@ -307,16 +332,12 @@ impl Profile {
            return Ok(Device::from(signer).boxed());
        }

-
        match Agent::connect() {
-
            Ok(agent) => {
-
                let signer = agent.signer(self.public_key);
-
                if signer.is_ready()? {
-
                    Ok(Device::from(signer).boxed())
-
                } else {
-
                    Err(Error::KeyNotRegistered(self.public_key))
-
                }
-
            }
-
            Err(err) => Err(err.into()),
+
        let agent = Agent::connect().map_err(|source| SignerError::AgentConnection { source })?;
+
        let signer = agent.signer(self.public_key);
+
        if signer.is_ready()? {
+
            Ok(Device::from(signer).boxed())
+
        } else {
+
            Err(SignerError::KeyNotRegistered(self.public_key))
        }
    }