Merge branch 'keymgr-refactor-fs-ops' into 'main'
keymgr: Move FS operations out of KeyType impl See merge request tpo/core/arti!1263
This commit is contained in:
commit
db7f46265e
|
@ -4479,6 +4479,7 @@ dependencies = [
|
|||
"tor-error",
|
||||
"tor-hscrypto",
|
||||
"tor-llcrypto",
|
||||
"zeroize",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
|
|
|
@ -19,3 +19,4 @@ thiserror = "1"
|
|||
tor-error = { path = "../tor-error", version = "0.5.0" }
|
||||
tor-hscrypto = { path = "../tor-hscrypto", version = "0.2.0" }
|
||||
tor-llcrypto = { path = "../tor-llcrypto", version = "0.5.0" }
|
||||
zeroize = "1"
|
||||
|
|
|
@ -21,8 +21,6 @@ use std::sync::Arc;
|
|||
// right now: "not found in any of the key stores" when returned by KeyMgr,
|
||||
// and "not found in this key store" when
|
||||
// returned by a KeyStore)
|
||||
// * Create a KeystoreCorruption variant (UnexpectedSshKeyType would be one of the potential
|
||||
// causes of this error)
|
||||
#[derive(Error, Debug, Clone)]
|
||||
#[non_exhaustive]
|
||||
pub enum Error {
|
||||
|
@ -40,35 +38,14 @@ pub enum Error {
|
|||
err: FsErrorSource,
|
||||
},
|
||||
|
||||
/// Encountered a malformed key.
|
||||
#[error("Malformed key: {0}")]
|
||||
MalformedKey(#[from] MalformedKeyErrorSource),
|
||||
|
||||
/// The requested key was not found.
|
||||
#[error("Key not found")]
|
||||
NotFound {/* TODO hs: add context */},
|
||||
|
||||
/// Failed to read an OpenSSH key
|
||||
#[error("Failed to read OpenSSH from {path} with type {key_type:?}")]
|
||||
SshKeyRead {
|
||||
/// The path of the key we were trying to fetch.
|
||||
path: PathBuf,
|
||||
/// The type of key we were trying to fetch.
|
||||
key_type: KeyType,
|
||||
/// The underlying error.
|
||||
#[source]
|
||||
err: Arc<ssh_key::Error>,
|
||||
},
|
||||
|
||||
/// The OpenSSH key we retrieved is of the wrong type.
|
||||
#[error(
|
||||
"Unexpected OpenSSH key type at {path:?}: wanted {wanted_key_algo}, found {found_key_algo}"
|
||||
)]
|
||||
UnexpectedSshKeyType {
|
||||
/// The path of the key we were trying to fetch.
|
||||
path: PathBuf,
|
||||
/// The algorithm we expected the key to use.
|
||||
wanted_key_algo: SshKeyAlgorithm,
|
||||
/// The algorithm of the key we got.
|
||||
found_key_algo: SshKeyAlgorithm,
|
||||
},
|
||||
|
||||
/// An internal error.
|
||||
#[error("Internal error")]
|
||||
Bug(#[from] tor_error::Bug),
|
||||
|
@ -107,14 +84,45 @@ impl From<fs_mistrust::Error> for FsErrorSource {
|
|||
}
|
||||
}
|
||||
|
||||
/// The underlying cause of an [`Error::MalformedKey`] error.
|
||||
//
|
||||
// TODO hs (#901): this introduces multiple levels of error `#[source]` nesting.
|
||||
//
|
||||
#[derive(thiserror::Error, Debug, Clone)]
|
||||
#[non_exhaustive]
|
||||
pub enum MalformedKeyErrorSource {
|
||||
/// Failed to parse an OpenSSH key
|
||||
#[error("Failed to parse OpenSSH with type {key_type:?}")]
|
||||
SshKeyParse {
|
||||
/// The type of key we were trying to fetch.
|
||||
key_type: KeyType,
|
||||
/// The underlying error.
|
||||
#[source]
|
||||
err: Arc<ssh_key::Error>,
|
||||
},
|
||||
|
||||
/// The OpenSSH key we retrieved is of the wrong type.
|
||||
#[error("Unexpected OpenSSH key type: wanted {wanted_key_algo}, found {found_key_algo}")]
|
||||
UnexpectedSshKeyType {
|
||||
/// The algorithm we expected the key to use.
|
||||
wanted_key_algo: SshKeyAlgorithm,
|
||||
/// The algorithm of the key we got.
|
||||
found_key_algo: SshKeyAlgorithm,
|
||||
},
|
||||
|
||||
// TODO hs: remove
|
||||
/// Unsupported key type.
|
||||
#[error("Found a key type we don't support yet: {0:?}")]
|
||||
Unsupported(KeyType),
|
||||
}
|
||||
|
||||
impl HasKind for Error {
|
||||
fn kind(&self) -> ErrorKind {
|
||||
// TODO hs: create `ErrorKind` variants for `tor_keymgr::Error`s.
|
||||
match self {
|
||||
Error::Filesystem { .. } => todo!(),
|
||||
Error::NotFound { .. } => todo!(),
|
||||
Error::SshKeyRead { .. } => todo!(),
|
||||
Error::UnexpectedSshKeyType { .. } => todo!(),
|
||||
Error::MalformedKey { .. } => todo!(),
|
||||
Error::Bug(e) => e.kind(),
|
||||
}
|
||||
}
|
||||
|
|
|
@ -7,29 +7,46 @@ use ssh_key::private::KeypairData;
|
|||
pub(crate) use ssh_key::Algorithm as SshKeyAlgorithm;
|
||||
|
||||
use std::io::ErrorKind;
|
||||
use std::path::Path;
|
||||
|
||||
use crate::err::MalformedKeyErrorSource;
|
||||
use crate::{EncodableKey, ErasedKey, Error, KeyType, Result};
|
||||
|
||||
use tor_llcrypto::pk::ed25519;
|
||||
use zeroize::Zeroizing;
|
||||
|
||||
/// An unparsed OpenSSH key.
|
||||
///
|
||||
/// Note: This is a wrapper around the contents of a file we think is an OpenSSH key. The inner
|
||||
/// value is unchecked/unvalidated, and might not actually be a valid OpenSSH key.
|
||||
///
|
||||
/// The inner value is zeroed on drop.
|
||||
pub(crate) struct UnparsedOpenSshKey(Zeroizing<Vec<u8>>);
|
||||
|
||||
impl UnparsedOpenSshKey {
|
||||
/// Create a new [`UnparsedOpenSshKey`].
|
||||
///
|
||||
/// The contents of `inner` are erased on drop.
|
||||
pub(crate) fn new(inner: Vec<u8>) -> Self {
|
||||
Self(Zeroizing::new(inner))
|
||||
}
|
||||
}
|
||||
|
||||
/// A helper for reading Ed25519 OpenSSH private keys from disk.
|
||||
fn read_ed25519_keypair(key_type: KeyType, path: &Path) -> Result<ErasedKey> {
|
||||
let key = ssh_key::PrivateKey::read_openssh_file(path).map_err(|e| {
|
||||
fn read_ed25519_keypair(key_type: KeyType, key: &UnparsedOpenSshKey) -> Result<ErasedKey> {
|
||||
let sk = ssh_key::PrivateKey::from_openssh(&*key.0).map_err(|e| {
|
||||
if matches!(e, ssh_key::Error::Io(ErrorKind::NotFound)) {
|
||||
Error::NotFound { /* TODO hs */ }
|
||||
} else {
|
||||
Error::SshKeyRead {
|
||||
path: path.into(),
|
||||
Error::MalformedKey(MalformedKeyErrorSource::SshKeyParse {
|
||||
key_type,
|
||||
err: e.into(),
|
||||
}
|
||||
})
|
||||
}
|
||||
})?;
|
||||
|
||||
// Build the expected key type (i.e. convert ssh_key key types to the key types
|
||||
// we're using internally).
|
||||
let key = match key.key_data() {
|
||||
let key = match sk.key_data() {
|
||||
KeypairData::Ed25519(key) => {
|
||||
ed25519::Keypair::from_bytes(&key.to_bytes()).map_err(|_| {
|
||||
Error::Bug(tor_error::internal!(
|
||||
|
@ -38,11 +55,12 @@ fn read_ed25519_keypair(key_type: KeyType, path: &Path) -> Result<ErasedKey> {
|
|||
})?;
|
||||
}
|
||||
_ => {
|
||||
return Err(Error::UnexpectedSshKeyType {
|
||||
path: path.into(),
|
||||
wanted_key_algo: key_type.ssh_algorithm(),
|
||||
found_key_algo: key.algorithm(),
|
||||
});
|
||||
return Err(Error::MalformedKey(
|
||||
MalformedKeyErrorSource::UnexpectedSshKeyType {
|
||||
wanted_key_algo: key_type.ssh_algorithm(),
|
||||
found_key_algo: sk.algorithm(),
|
||||
},
|
||||
));
|
||||
}
|
||||
};
|
||||
|
||||
|
@ -69,23 +87,28 @@ impl KeyType {
|
|||
}
|
||||
}
|
||||
|
||||
/// Read an OpenSSH key, parse the key material into a known key type, returning the
|
||||
/// Parse an OpenSSH key, convert the key material into a known key type, and return the
|
||||
/// type-erased value.
|
||||
///
|
||||
/// The caller is expected to downcast the value returned to a concrete type.
|
||||
pub(crate) fn read_ssh_format_erased(&self, path: &Path) -> Result<ErasedKey> {
|
||||
pub(crate) fn parse_ssh_format_erased(&self, key: &UnparsedOpenSshKey) -> Result<ErasedKey> {
|
||||
// TODO hs: perhaps this needs to be a method on EncodableKey instead?
|
||||
match self {
|
||||
KeyType::Ed25519Keypair => read_ed25519_keypair(*self, path),
|
||||
KeyType::Ed25519Keypair => read_ed25519_keypair(*self, key),
|
||||
KeyType::X25519StaticSecret => {
|
||||
// TODO hs: implement
|
||||
Err(Error::NotFound {})
|
||||
Err(Error::MalformedKey(MalformedKeyErrorSource::Unsupported(
|
||||
*self,
|
||||
)))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Encode an OpenSSH-formatted key and write it to the specified file.
|
||||
pub(crate) fn write_ssh_format(&self, _key: &dyn EncodableKey, _path: &Path) -> Result<()> {
|
||||
/// Encode an OpenSSH-formatted key.
|
||||
//
|
||||
// TODO hs: remove "allow" and choose a better name for this function
|
||||
#[allow(clippy::wrong_self_convention)]
|
||||
pub(crate) fn to_ssh_format(&self, _key: &dyn EncodableKey) -> Result<String> {
|
||||
todo!() // TODO hs
|
||||
}
|
||||
}
|
||||
|
|
|
@ -6,6 +6,7 @@ use std::fs;
|
|||
use std::io::ErrorKind;
|
||||
use std::path::{Path, PathBuf};
|
||||
|
||||
use crate::key_type::ssh::UnparsedOpenSshKey;
|
||||
use crate::keystore::{EncodableKey, ErasedKey, KeySpecifier, KeyStore};
|
||||
use crate::{Error, KeyType, Result};
|
||||
|
||||
|
@ -13,7 +14,7 @@ use fs_mistrust::{CheckedDir, Mistrust};
|
|||
|
||||
/// The Arti key store.
|
||||
///
|
||||
// TODO hs: `ArtiNativeKeyStore` (and `KeyType`) should not be dealing with filesystem operations.
|
||||
// TODO hs: `ArtiNativeKeyStore` should not be dealing with filesystem operations.
|
||||
//
|
||||
// Instead, we need to:
|
||||
// * introduce a `FsMgr` trait that represents the filesystem operations we're interested in
|
||||
|
@ -66,11 +67,18 @@ impl ArtiNativeKeyStore {
|
|||
|
||||
impl KeyStore for ArtiNativeKeyStore {
|
||||
fn get(&self, key_spec: &dyn KeySpecifier, key_type: KeyType) -> Result<ErasedKey> {
|
||||
let key_path = self.key_path(key_spec, key_type)?;
|
||||
let path = self.key_path(key_spec, key_type)?;
|
||||
|
||||
// TODO: use CheckedDir::read_to_string to perform permission checks on the key file too.
|
||||
// This will require some changes to the KeyType impl.
|
||||
key_type.read_ssh_format_erased(&key_path)
|
||||
let inner = self
|
||||
.keystore_dir
|
||||
.read(&path)
|
||||
.map_err(|err| Error::Filesystem {
|
||||
action: "read",
|
||||
path: path.clone(),
|
||||
err: err.into(),
|
||||
})?;
|
||||
|
||||
key_type.parse_ssh_format_erased(&UnparsedOpenSshKey::new(inner))
|
||||
}
|
||||
|
||||
fn insert(
|
||||
|
@ -79,11 +87,16 @@ impl KeyStore for ArtiNativeKeyStore {
|
|||
key_spec: &dyn KeySpecifier,
|
||||
key_type: KeyType,
|
||||
) -> Result<()> {
|
||||
let key_path = self.key_path(key_spec, key_type)?;
|
||||
let path = self.key_path(key_spec, key_type)?;
|
||||
let openssh_key = key_type.to_ssh_format(key)?;
|
||||
|
||||
// TODO: use CheckedDir::write_and_replace to perform permission checks on the key file too.
|
||||
// This will require some changes to the KeyType impl.
|
||||
key_type.write_ssh_format(key, &key_path)
|
||||
self.keystore_dir
|
||||
.write_and_replace(&path, openssh_key)
|
||||
.map_err(|err| Error::Filesystem {
|
||||
action: "write",
|
||||
path,
|
||||
err: err.into(),
|
||||
})
|
||||
}
|
||||
|
||||
fn remove(&self, key_spec: &dyn KeySpecifier, key_type: KeyType) -> Result<()> {
|
||||
|
|
Loading…
Reference in New Issue