From 5571271bad0999c50615cdb00a60af23b10b1a33 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Thu, 23 Sep 2021 02:27:12 +0200 Subject: [PATCH 01/16] Support multiple signers of different types behind a HSM feature flag, and support in principle selecting which signer to use for which purpose. (#539) --- Cargo.toml | 1 + src/commons/crypto/signing.rs | 265 +++++++++++++++++++++++++------- src/commons/util/dummysigner.rs | 48 ++++++ src/commons/util/mod.rs | 2 + 4 files changed, 263 insertions(+), 53 deletions(-) create mode 100644 src/commons/util/dummysigner.rs diff --git a/Cargo.toml b/Cargo.toml index 0a7f38258..50339bafd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -71,6 +71,7 @@ ui-tests = [] extra-debug = [ "rpki/extra-debug" ] static-openssl = [ "openssl/vendored" ] all-except-ui-tests = [ "multi-user", "rta", "static-openssl" ] +hsm = [] # Make sure that Krill crashes on panics, rather than losing threads and # limping on in a bad state. diff --git a/src/commons/crypto/signing.rs b/src/commons/crypto/signing.rs index 70e6b18b8..1ebe1e02d 100644 --- a/src/commons/crypto/signing.rs +++ b/src/commons/crypto/signing.rs @@ -6,6 +6,7 @@ use std::{ {convert::TryFrom, path::Path}, }; +use bcder::Captured; use bytes::Bytes; use rpki::{ @@ -23,6 +24,9 @@ use rpki::{ uri, }; +#[cfg(feature = "hsm")] +use crate::commons::util::dummysigner::DummySigner; + use crate::{ commons::{ api::{IssuedCert, RcvdCert, ReplacedObject, RepoInfo, RequestResourceLimit, ResourceSet}, @@ -36,84 +40,244 @@ use crate::{ //------------ Signer -------------------------------------------------------- -// This is an enum in preparation of other supported signer types +#[derive(Clone, Debug)] +enum SignerProvider { + OpenSsl(OpenSslSigner), + + #[cfg(feature = "hsm")] + #[allow(dead_code)] + Dummy(DummySigner), +} + +impl SignerProvider { + pub fn create_key(&mut self) -> CryptoResult { + match self { + SignerProvider::OpenSsl(signer) => signer.create_key(PublicKeyFormat::Rsa), + #[cfg(feature = "hsm")] + SignerProvider::Dummy(signer) => signer.create_key(PublicKeyFormat::Rsa), + } + .map_err(crypto::Error::signer) + } + + pub fn destroy_key(&mut self, key_id: &KeyIdentifier) -> CryptoResult<()> { + match self { + SignerProvider::OpenSsl(signer) => signer.destroy_key(key_id), + #[cfg(feature = "hsm")] + SignerProvider::Dummy(signer) => signer.destroy_key(key_id), + } + .map_err(crypto::Error::key_error) + } + + pub fn get_key_info(&self, key_id: &KeyIdentifier) -> CryptoResult { + match self { + SignerProvider::OpenSsl(signer) => signer.get_key_info(key_id), + #[cfg(feature = "hsm")] + SignerProvider::Dummy(signer) => signer.get_key_info(key_id), + } + .map_err(crypto::Error::key_error) + } + + pub fn random_serial(&self) -> CryptoResult { + match self { + SignerProvider::OpenSsl(signer) => Serial::random(signer.deref()), + #[cfg(feature = "hsm")] + SignerProvider::Dummy(signer) => Serial::random(signer.deref()), + } + .map_err(crypto::Error::signer) + } + + pub fn sign + ?Sized>( + &self, + key_id: &KeyIdentifier, + sig_alg: SignatureAlgorithm, + data: &D, + ) -> CryptoResult { + match self { + SignerProvider::OpenSsl(signer) => signer.sign(key_id, sig_alg, data), + #[cfg(feature = "hsm")] + SignerProvider::Dummy(signer) => signer.sign(key_id, sig_alg, data), + } + .map_err(crypto::Error::signing) + } + + pub fn sign_one_off + ?Sized>( + &self, + sig_alg: SignatureAlgorithm, + data: &D, + ) -> CryptoResult<(Signature, PublicKey)> { + match self { + SignerProvider::OpenSsl(signer) => signer.sign_one_off(sig_alg, data), + #[cfg(feature = "hsm")] + SignerProvider::Dummy(signer) => signer.sign_one_off(sig_alg, data), + } + .map_err(crypto::Error::signer) + } + + pub fn sign_csr(&self, base_repo: &RepoInfo, name_space: &str, key: &KeyIdentifier) -> CryptoResult { + fn func(signer: &T, base_repo: &RepoInfo, name_space: &str, key: &KeyIdentifier) -> CryptoResult + where + T: Signer, + { + let pub_key = signer.get_key_info(key).map_err(crypto::Error::key_error)?; + Csr::construct( + signer.deref(), + key, + &base_repo.ca_repository(name_space).join(&[]).unwrap(), // force trailing slash + &base_repo.rpki_manifest(name_space, &pub_key.key_identifier()), + Some(&base_repo.rpki_notify()), + ) + .map_err(crypto::Error::signing) + } + + let enc = match self { + SignerProvider::OpenSsl(signer) => func(signer.deref(), base_repo, name_space, key), + #[cfg(feature = "hsm")] + SignerProvider::Dummy(signer) => func(signer.deref(), base_repo, name_space, key), + }?; + + Ok(Csr::decode(enc.as_slice())?) + } + + pub fn sign_cert(&self, tbs: TbsCert, key_id: &KeyIdentifier) -> CryptoResult { + match self { + SignerProvider::OpenSsl(signer) => tbs.into_cert(signer.deref(), key_id), + #[cfg(feature = "hsm")] + SignerProvider::Dummy(signer) => tbs.into_cert(signer.deref(), key_id), + } + .map_err(crypto::Error::signing) + } + + pub fn sign_crl(&self, tbs: TbsCertList>, key_id: &KeyIdentifier) -> CryptoResult { + match self { + SignerProvider::OpenSsl(signer) => tbs.into_crl(signer.deref(), key_id), + #[cfg(feature = "hsm")] + SignerProvider::Dummy(signer) => tbs.into_crl(signer.deref(), key_id), + } + .map_err(crypto::Error::signing) + } + + pub fn sign_manifest( + &self, + content: ManifestContent, + builder: SignedObjectBuilder, + key_id: &KeyIdentifier, + ) -> CryptoResult { + match self { + SignerProvider::OpenSsl(signer) => content.into_manifest(builder, signer.deref(), key_id), + #[cfg(feature = "hsm")] + SignerProvider::Dummy(signer) => content.into_manifest(builder, signer.deref(), key_id), + } + .map_err(crypto::Error::signing) + } + + pub fn sign_roa( + &self, + roa_builder: RoaBuilder, + object_builder: SignedObjectBuilder, + key_id: &KeyIdentifier, + ) -> CryptoResult { + match self { + SignerProvider::OpenSsl(signer) => roa_builder.finalize(object_builder, signer.deref(), key_id), + #[cfg(feature = "hsm")] + SignerProvider::Dummy(signer) => roa_builder.finalize(object_builder, signer.deref(), key_id), + } + .map_err(crypto::Error::signing) + } + + pub fn sign_rta(&self, rta_builder: &mut rta::RtaBuilder, ee: Cert) -> CryptoResult<()> { + let key = ee.subject_key_identifier(); + rta_builder.push_cert(ee); + + match self { + SignerProvider::OpenSsl(signer) => rta_builder.sign(signer.deref(), &key, None, None), + #[cfg(feature = "hsm")] + SignerProvider::Dummy(signer) => rta_builder.sign(signer.deref(), &key, None, None), + } + .map_err(crypto::Error::signing) + } +} + #[derive(Clone, Debug)] pub struct KrillSigner { - // use a blocking lock to avoid having to be async, for signing operations - // this should be fine. - signer: Arc>, + // KrillSigner chooses which signer to use when. The noise of handling the enum based dispatch is handled by the + // SignerProvider type defined above, patterned after the existing AuthProvider enum based approach. + // + // Use Arc references so that we can use refer to the same signer instance more than once if that signer should be + // used for multiple purposes, e.g. as both general_signer and one_off_signer in this case. + // + // Use an RwLock because the Signer trait from the rpki-rs crate uses &mut self for create_key() and destroy_key() + // operations. In future we might move the responsibility for locking into the signer so that it can lock only what + // actually needs to be locked raet + + // The general signer is used for all signing operations except one off signing. + general_signer: Arc>, + + // As the security of a HSM isn't needed for one off keys, and HSMs are slow, by default this should be an instance + // of OpenSslSigner. However, if users think the perceived extra security is warranted let them use a different + // Signer for one off keys if that's what they want. + one_off_signer: Arc>, } impl KrillSigner { pub fn build(work_dir: &Path) -> KrillResult { - let signer = OpenSslSigner::build(work_dir)?; - let signer = Arc::new(RwLock::new(signer)); - Ok(KrillSigner { signer }) + // The types of signer to initialize, the details needed to initialize them and the intended purpose for each + // signer (e.g. signer for past keys, currently used signer, signer to use for a key roll, etc.) should come + // from the configuration file. KrillSigner should combine that input its own rules, e.g. to dispatch a signing + // request to the correct signer we will need to determine which signer possesses the signing key, and the + // signer to use to create a new key depends on whether the key is one-off or not and whether or not it is + // being created for a key roll. For now the capability for different signers for different purposes exists but + // is not yet used. + + let openssl_signer = OpenSslSigner::build(work_dir)?; + let openssl_signer = Arc::new(RwLock::new(SignerProvider::OpenSsl(openssl_signer))); + let general_signer = openssl_signer.clone(); + let one_off_signer = openssl_signer; + Ok(KrillSigner { + general_signer, + one_off_signer, + }) } -} -impl KrillSigner { pub fn create_key(&self) -> CryptoResult { - let mut signer = self.signer.write().unwrap(); - signer.create_key(PublicKeyFormat::Rsa).map_err(crypto::Error::signer) + self.general_signer.write().unwrap().create_key() } pub fn destroy_key(&self, key_id: &KeyIdentifier) -> CryptoResult<()> { - let mut signer = self.signer.write().unwrap(); - signer.destroy_key(key_id).map_err(crypto::Error::key_error) + self.general_signer.write().unwrap().destroy_key(key_id) } pub fn get_key_info(&self, key_id: &KeyIdentifier) -> CryptoResult { - self.signer - .read() - .unwrap() - .get_key_info(key_id) - .map_err(crypto::Error::key_error) + self.general_signer.read().unwrap().get_key_info(key_id) } pub fn random_serial(&self) -> CryptoResult { - let signer = self.signer.read().unwrap(); - Serial::random(signer.deref()).map_err(crypto::Error::signer) + self.general_signer.read().unwrap().random_serial() } pub fn sign + ?Sized>(&self, key_id: &KeyIdentifier, data: &D) -> CryptoResult { - self.signer + self.general_signer .read() .unwrap() .sign(key_id, SignatureAlgorithm::default(), data) - .map_err(crypto::Error::signing) } pub fn sign_one_off + ?Sized>(&self, data: &D) -> CryptoResult<(Signature, PublicKey)> { - self.signer + self.one_off_signer .read() .unwrap() .sign_one_off(SignatureAlgorithm::default(), data) - .map_err(crypto::Error::signer) } pub fn sign_csr(&self, base_repo: &RepoInfo, name_space: &str, key: &KeyIdentifier) -> CryptoResult { - let signer = self.signer.read().unwrap(); - let pub_key = signer.get_key_info(key).map_err(crypto::Error::key_error)?; - let enc = Csr::construct( - signer.deref(), - key, - &base_repo.ca_repository(name_space).join(&[]).unwrap(), // force trailing slash - &base_repo.rpki_manifest(name_space, &pub_key.key_identifier()), - Some(&base_repo.rpki_notify()), - ) - .map_err(crypto::Error::signing)?; - Ok(Csr::decode(enc.as_slice())?) + self.general_signer.read().unwrap().sign_csr(base_repo, name_space, key) } pub fn sign_cert(&self, tbs: TbsCert, key_id: &KeyIdentifier) -> CryptoResult { - let signer = self.signer.read().unwrap(); - tbs.into_cert(signer.deref(), key_id).map_err(crypto::Error::signing) + self.general_signer.read().unwrap().sign_cert(tbs, key_id) } pub fn sign_crl(&self, tbs: TbsCertList>, key_id: &KeyIdentifier) -> CryptoResult { - let signer = self.signer.read().unwrap(); - tbs.into_crl(signer.deref(), key_id).map_err(crypto::Error::signing) + self.general_signer.read().unwrap().sign_crl(tbs, key_id) } pub fn sign_manifest( @@ -122,10 +286,10 @@ impl KrillSigner { builder: SignedObjectBuilder, key_id: &KeyIdentifier, ) -> CryptoResult { - let signer = self.signer.read().unwrap(); - content - .into_manifest(builder, signer.deref(), key_id) - .map_err(crypto::Error::signing) + self.general_signer + .read() + .unwrap() + .sign_manifest(content, builder, key_id) } pub fn sign_roa( @@ -134,19 +298,14 @@ impl KrillSigner { object_builder: SignedObjectBuilder, key_id: &KeyIdentifier, ) -> CryptoResult { - let signer = self.signer.read().unwrap(); - roa_builder - .finalize(object_builder, signer.deref(), key_id) - .map_err(crypto::Error::signing) + self.general_signer + .read() + .unwrap() + .sign_roa(roa_builder, object_builder, key_id) } pub fn sign_rta(&self, rta_builder: &mut rta::RtaBuilder, ee: Cert) -> CryptoResult<()> { - let signer = self.signer.read().unwrap(); - let key = ee.subject_key_identifier(); - rta_builder.push_cert(ee); - rta_builder - .sign(signer.deref(), &key, None, None) - .map_err(crypto::Error::signing) + self.general_signer.read().unwrap().sign_rta(rta_builder, ee) } } diff --git a/src/commons/util/dummysigner.rs b/src/commons/util/dummysigner.rs new file mode 100644 index 000000000..0582de7c9 --- /dev/null +++ b/src/commons/util/dummysigner.rs @@ -0,0 +1,48 @@ +use rpki::repository::crypto::{KeyIdentifier, PublicKey, PublicKeyFormat, Signature, SignatureAlgorithm, Signer}; + +use super::softsigner::SignerError; + +/// A dummy signer to prove that compilation with two different Signer implementations works +#[derive(Clone, Debug)] +pub struct DummySigner; + +impl Signer for DummySigner { + type KeyId = KeyIdentifier; + type Error = SignerError; + + fn create_key(&mut self, _: PublicKeyFormat) -> Result { + unreachable!() + } + + fn get_key_info( + &self, + _: &Self::KeyId, + ) -> Result> { + unreachable!() + } + + fn destroy_key(&mut self, _: &Self::KeyId) -> Result<(), rpki::repository::crypto::signer::KeyError> { + unreachable!() + } + + fn sign + ?Sized>( + &self, + _: &Self::KeyId, + _: SignatureAlgorithm, + _: &D, + ) -> Result> { + unreachable!() + } + + fn sign_one_off + ?Sized>( + &self, + _: SignatureAlgorithm, + _: &D, + ) -> Result<(Signature, PublicKey), Self::Error> { + unreachable!() + } + + fn rand(&self, _: &mut [u8]) -> Result<(), Self::Error> { + unreachable!() + } +} diff --git a/src/commons/util/mod.rs b/src/commons/util/mod.rs index a4a50686e..65138a303 100644 --- a/src/commons/util/mod.rs +++ b/src/commons/util/mod.rs @@ -11,6 +11,8 @@ use rpki::{ use crate::constants::KRILL_VERSION; +#[cfg(feature = "hsm")] +pub mod dummysigner; pub mod ext_serde; pub mod file; pub mod httpclient; From 1f2c9ee70a7344cd6ac902fa09d8e6196cbe139d Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Thu, 23 Sep 2021 10:16:06 +0200 Subject: [PATCH 02/16] Added some developer docs. --- doc/development/hsm/architecture.md | 22 ++++++++++++++++ doc/development/hsm/overview.md | 41 +++++++++++++++++++++++++++++ doc/development/hsm/readme.md | 14 ++++++++++ doc/development/hsm/requirements.md | 7 +++++ doc/development/readme.md | 2 +- 5 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 doc/development/hsm/architecture.md create mode 100644 doc/development/hsm/overview.md create mode 100644 doc/development/hsm/readme.md create mode 100644 doc/development/hsm/requirements.md diff --git a/doc/development/hsm/architecture.md b/doc/development/hsm/architecture.md new file mode 100644 index 000000000..76a8d2d8a --- /dev/null +++ b/doc/development/hsm/architecture.md @@ -0,0 +1,22 @@ +# HSM: Architecture + +## Why create new KMIP crates? + +No well or actively maintained Rust support for KMIP with sufficient functionality for Krill +existed at the time of writing. + +The closest candidate, https://github.com/visa/kmip, was used to explore KMIP support in the +Krill HSM prototype code. However, it was decided to create our own KMIP library because the +visa crate: + + - Was not published on crates.io. + - Had only rudimentary error reporting (as it targets no-std environments). + - Lacks documentation. + - Lacked TCP+TLS client support. + - Lacked support for KMIP operations that Krill requires. + - Did not appear to be actively maintained or intended for use by others. + +## Why not add KMIP code to Krill directly? + +The Krill code base is already large enough and slow enough to compile. KMIP support may also +be of interest to others. It thus seemed a good candidate for separation from Krill itself. \ No newline at end of file diff --git a/doc/development/hsm/overview.md b/doc/development/hsm/overview.md new file mode 100644 index 000000000..2cc05342f --- /dev/null +++ b/doc/development/hsm/overview.md @@ -0,0 +1,41 @@ +# HSM: Overview + +## New or changed functionality + +The feature adds (or will add) the following to Krill: + - Pluggable signers: + - Three "plugins": + - Host file-based OpenSSL key management & signing + - [PKCS#11](https://www.cryptsoft.com/pkcs11doc/) dynamic library support + - [Key Management Interoperability Protocol (KMIP)](https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=kmip) support + - Partial support for KMIP 1.0-1.2 (signing was introduced in 1.2) + - TTLV over TLS only, no XML/JSON over HTTPS + + - Key ownership tracking: + - To sign with a key the request must be delegated to the correct signer. + - Therefore we must keep track of which signer "owns" each key. + + - Random value generation fallback support: + - Not all PKCS#11 or KMIP compatible devices support generating random values. + - Fallback in such cases to the OpenSSL signer (or to a user specified signer?) + + - The concept of signing with specific signers for specific purposes: + - Signer for one-time keys (avoid slow HSMs for keys that don't need the security guarantees an HSM provides) + - Signer for new keys + - Signer for existing keys (based on key tracking as mentioned above) + - Signer for key rollover + - Signer for fallback random value generation + +The feature also adds two new Rust crates for KMIP support: + - https://github.com/NLnetLabs/kmip-protocol (https://crates.io/crates/kmip-protocol) + - High level TCP+TLS client for (de)serializing and reading/writing KMIP 1.0-1.2 responses/requests. + + - https://github.com/NLnetLabs/kmip-ttlv (https://crates.io/crates/kmip-ttlv) + - Low level (de)serialization of the KMIP TTLV binary encoding. + +## Impacted source components + +The feature only lightly touches the core RPKI related code in Krill in order to dispatch signing +related requests to the correct signer. The main source code components impacted by this feature are: + + - ``src/daemon/crypto/signing.rs`` diff --git a/doc/development/hsm/readme.md b/doc/development/hsm/readme.md new file mode 100644 index 000000000..b125fee2c --- /dev/null +++ b/doc/development/hsm/readme.md @@ -0,0 +1,14 @@ +# HSM Feature + +HSM is a feature both in the sense that it adds the functionality to Krill for enabling support for +using external cryptographic tokens such as hardware security modules (HSMs), and in the sense that +the functionality is gated behind a Cargo feature of the same name. + +The feature is currently NOT enabled by default. To enable the feature when building pass the +`--features hsm` argument to the `cargo build` command. + +Further reading: + +- [Overview](./overview.md) +- [Requirements](./requirements.md) +- [Architecture](./architecture.md) \ No newline at end of file diff --git a/doc/development/hsm/requirements.md b/doc/development/hsm/requirements.md new file mode 100644 index 000000000..bf2576c23 --- /dev/null +++ b/doc/development/hsm/requirements.md @@ -0,0 +1,7 @@ +# HSM: Requirements + +The primary initial requirements that influenced the architecture were: + + - Support for key creation, deletion, signing using HSMs via two standard protocols: + - [PKCS#11](https://www.cryptsoft.com/pkcs11doc/) + - [Key Management Interoperability Protocol (KMIP)](https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=kmip) \ No newline at end of file diff --git a/doc/development/readme.md b/doc/development/readme.md index b56584f50..6c65c97d4 100644 --- a/doc/development/readme.md +++ b/doc/development/readme.md @@ -18,7 +18,7 @@ components and/or concepts in Krill: 5. [Repository Manager](./05_repo_manager.md) 6. [Certificate Authority Manager](./06_ca_manager.md) 7. [Multi-User Feature](./multi_user/readme.md) - +8. [HSM Feature](./hsm/readme.md) Release Versions ---------------- From 870a1023fa70d17fd41053404c4590fee14f84e2 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Tue, 5 Oct 2021 13:24:27 +0200 Subject: [PATCH 03/16] - Bump to v0.3.1 of the kmip-protocol crate. - Add a dependency on the backoff crate for retry support. - Add a dependency on the r2d2 crate for connection pooling support. - Uses GitHub versions of the bcder and rpki crates for the DER Unsigned Integer support needed by the KMIP signer. - Refactor signers to crypto::signers and replace the Dummy signer with a KMIP signer. - Added a "hsmtest" job to the GitHub Actions CI workflow that runs all Krill tests using the KMIP signer against PyKMIP. - Added a "hsm-tests" Cargo feature flag for configuring Krill to use ONLY KMIP as a signer, not OpenSSL at all. Currently building without the "hsm-tests" feature flag set will fail if the "hsm" feature flag is set. Krill isn't ready to be used in "hsm" mode yet. - Changes SignerProvider to implement the Signer trait so that it can be passed to builders so that their invocation of a signer also goes via SignerProvider dispatching to the correct signer. --- .github/workflows/ci.yml | 41 + Cargo.lock | 199 ++++- Cargo.toml | 15 +- doc/development/hsm/architecture.md | 72 +- src/commons/api/ca.rs | 3 +- src/commons/crypto/mod.rs | 2 + src/commons/crypto/signers/error.rs | 48 ++ src/commons/crypto/signers/kmip/connpool.rs | 125 +++ src/commons/crypto/signers/kmip/internal.rs | 782 ++++++++++++++++++ src/commons/crypto/signers/kmip/keymap.rs | 13 + src/commons/crypto/signers/kmip/mod.rs | 21 + src/commons/crypto/signers/kmip/signer.rs | 85 ++ src/commons/crypto/signers/mod.rs | 7 + .../{util => crypto/signers}/softsigner.rs | 52 +- src/commons/crypto/signers/util.rs | 17 + src/commons/crypto/signing.rs | 285 ++++--- src/commons/error.rs | 3 +- src/commons/util/dummysigner.rs | 48 -- src/commons/util/mod.rs | 3 - test-resources/pykmip/README.md | 48 ++ test-resources/pykmip/ca.crt | 14 + test-resources/pykmip/run-server.py | 10 + test-resources/pykmip/server.conf | 11 + test-resources/pykmip/server.crt | 41 + test-resources/pykmip/server.key | 5 + 25 files changed, 1718 insertions(+), 232 deletions(-) create mode 100644 src/commons/crypto/signers/error.rs create mode 100644 src/commons/crypto/signers/kmip/connpool.rs create mode 100644 src/commons/crypto/signers/kmip/internal.rs create mode 100644 src/commons/crypto/signers/kmip/keymap.rs create mode 100644 src/commons/crypto/signers/kmip/mod.rs create mode 100644 src/commons/crypto/signers/kmip/signer.rs create mode 100644 src/commons/crypto/signers/mod.rs rename src/commons/{util => crypto/signers}/softsigner.rs (85%) create mode 100644 src/commons/crypto/signers/util.rs delete mode 100644 src/commons/util/dummysigner.rs create mode 100644 test-resources/pykmip/README.md create mode 100644 test-resources/pykmip/ca.crt create mode 100755 test-resources/pykmip/run-server.py create mode 100644 test-resources/pykmip/server.conf create mode 100644 test-resources/pykmip/server.crt create mode 100644 test-resources/pykmip/server.key diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b8b07977b..47c14ec9a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -67,3 +67,44 @@ jobs: name: cypress-ui-test-captures ${{ matrix.os }} ${{ matrix.rust }} path: target/ui/ if-no-files-found: ignore + + hsmtest: + name: hsmtest + runs-on: ubuntu-18.04 + steps: + - name: Checkout repository + uses: actions/checkout@v1 + + - name: Install Rust + uses: hecrj/setup-rust-action@v1 + with: + rust-version: stable + + - uses: actions/setup-python@v2 + with: + python-version: '3.x' + + - name: Install PyKMIP + uses: BSFishy/pip-action@v1 + with: + packages: pykmip + + - name: Compile the tests + run: | + cargo build --tests --no-default-features --features hsm,hsm-tests + + - name: Run the tests against the PyKMIP server + run: | + cd test-resources/pykmip + python run-server.py & + sleep 5s + openssl s_client -connect 127.0.0.1:5696 || true + cd - + cargo test --no-default-features --features hsm,hsm-tests -- --test-threads=1 2>&1 + + - name: Dump PyKMIP log on failure + if: failure() + working-directory: test-resources/pykmip + run: | + ls -la + cat server.log \ No newline at end of file diff --git a/Cargo.lock b/Cargo.lock index 2868bb7a0..f6324c8fa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -82,6 +82,17 @@ version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cdb031dd78e28731d87d56cc8ffef4a8f36ca26c38fe2de700543e627f8a464a" +[[package]] +name = "backoff" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9fe17f59a06fe8b87a6fc8bf53bb70b3aba76d7685f432487a68cd5552853625" +dependencies = [ + "getrandom 0.2.3", + "instant", + "rand 0.8.4", +] + [[package]] name = "backtrace" version = "0.3.61" @@ -122,9 +133,8 @@ dependencies = [ [[package]] name = "bcder" -version = "0.6.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c16eb8ab9a5deef7188129b3ea73775a67fa07cc153ebe8d4b08ea21985d9c5b" +version = "0.6.1-dev" +source = "git+https://github.com/NLnetLabs/bcder#0e0ef26d23afe18603768b7f6757df997b87568b" dependencies = [ "backtrace", "bytes", @@ -364,6 +374,28 @@ dependencies = [ "cfg-if", ] +[[package]] +name = "enum-display-derive" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f16ef37b2a9b242295d61a154ee91ae884afff6b8b933b486b12481cc58310ca" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "enum-flags" +version = "0.1.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3682d2328e61f5529088a02cd20bb0a9aeaeeeb2f26597436dd7d75d1340f8f5" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "error-chain" version = "0.11.0" @@ -708,6 +740,15 @@ dependencies = [ "hashbrown", ] +[[package]] +name = "instant" +version = "0.1.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "716d3d89f35ac6a34fd0eed635395f4c3b76fa889338a4632e5231a8684216bd" +dependencies = [ + "cfg-if", +] + [[package]] name = "intervaltree" version = "0.2.6" @@ -768,10 +809,45 @@ dependencies = [ "wasm-bindgen", ] +[[package]] +name = "kmip-protocol" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ef3e9e144b27f0e12e027f8eae5f174656c5c5fae9a45f3a06c2b866ce6e4f02" +dependencies = [ + "cfg-if", + "enum-display-derive", + "enum-flags", + "kmip-ttlv", + "log", + "maybe-async", + "openssl", + "rustc_version 0.4.0", + "serde", + "serde_bytes", + "serde_derive", + "trait-set", +] + +[[package]] +name = "kmip-ttlv" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4829d1eaf141b0fd93402d9e219a2453984106931de6e0cdbd8ca02bec58f2bc" +dependencies = [ + "cfg-if", + "hex", + "maybe-async", + "rustc_version 0.4.0", + "serde", + "trait-set", +] + [[package]] name = "krill" version = "0.9.2-rc3" dependencies = [ + "backoff", "base64 0.13.0", "basic-cookies", "bcder", @@ -787,17 +863,19 @@ dependencies = [ "hyper", "intervaltree", "jmespatch", + "kmip-protocol", "libc", "libflate", "log", "openidconnect", "openssl", "oso", + "r2d2", "regex", "reqwest", "rpassword", "rpki", - "rustc_version", + "rustc_version 0.2.3", "scrypt", "serde", "serde_json", @@ -876,6 +954,15 @@ dependencies = [ "rle-decode-fast", ] +[[package]] +name = "lock_api" +version = "0.4.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "712a4d093c9976e24e7dbca41db895dabcbac38eb5f4045393d17a95bdfb1109" +dependencies = [ + "scopeguard", +] + [[package]] name = "log" version = "0.4.14" @@ -906,6 +993,17 @@ version = "0.1.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a3e378b66a060d48947b590737b30a1be76706c8dd7b8ba0f2fe3989c68a853f" +[[package]] +name = "maybe-async" +version = "0.2.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6007f9dad048e0a224f27ca599d669fca8cfa0dac804725aab542b2eb032bce6" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "memchr" version = "2.4.1" @@ -1165,6 +1263,31 @@ dependencies = [ "tracing-subscriber", ] +[[package]] +name = "parking_lot" +version = "0.11.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7d17b78036a60663b797adeaee46f5c9dfebb86948d1255007a1d6be0271ff99" +dependencies = [ + "instant", + "lock_api", + "parking_lot_core", +] + +[[package]] +name = "parking_lot_core" +version = "0.8.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d76e8e1493bcac0d2766c42737f34458f1c8c50c0d23bcb24ea953affb273216" +dependencies = [ + "cfg-if", + "instant", + "libc", + "redox_syscall", + "smallvec", + "winapi", +] + [[package]] name = "pbkdf2" version = "0.7.5" @@ -1290,6 +1413,17 @@ dependencies = [ "proc-macro2", ] +[[package]] +name = "r2d2" +version = "0.8.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "545c5bc2b880973c9c10e4067418407a0ccaa3091781d1671d46eb35107cb26f" +dependencies = [ + "log", + "parking_lot", + "scheduled-thread-pool", +] + [[package]] name = "rand" version = "0.7.3" @@ -1493,9 +1627,8 @@ dependencies = [ [[package]] name = "rpki" -version = "0.12.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "718f0d99ad56874728162d2f082e43c2cb378ab62b87202cdf806b9fd13b2d0a" +version = "0.12.3" +source = "git+https://github.com/ximon18/rpki-rs?branch=0.12.3-unsigned-from-slice#643e9e14dcf19de136e280989ae96c00144f5be5" dependencies = [ "base64 0.13.0", "bcder", @@ -1521,7 +1654,16 @@ version = "0.2.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "138e3e0acb6c9fb258b19b67cb8abd63c00679d2851805ea151465464fe9030a" dependencies = [ - "semver", + "semver 0.9.0", +] + +[[package]] +name = "rustc_version" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bfa0f585226d2e68097d4f95d113b15b83a82e819ab25717ec0590d9584ef366" +dependencies = [ + "semver 1.0.4", ] [[package]] @@ -1568,6 +1710,21 @@ dependencies = [ "winapi", ] +[[package]] +name = "scheduled-thread-pool" +version = "0.2.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dc6f74fd1204073fa02d5d5d68bec8021be4c38690b61264b2fdb48083d0e7d7" +dependencies = [ + "parking_lot", +] + +[[package]] +name = "scopeguard" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d29ab0c6d3fc0ee92fe66e2d99f700eab17a8d57d1c1d3b748380fb20baa78cd" + [[package]] name = "scrypt" version = "0.6.5" @@ -1622,6 +1779,12 @@ dependencies = [ "semver-parser", ] +[[package]] +name = "semver" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "568a8e6258aa33c13358f81fd834adb854c6f7c9468520910a9b1e8fac068012" + [[package]] name = "semver-parser" version = "0.7.0" @@ -1647,6 +1810,15 @@ dependencies = [ "serde", ] +[[package]] +name = "serde_bytes" +version = "0.11.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "16ae07dd2f88a366f15bd0632ba725227018c69a1c8550a927324f8eb8368bb9" +dependencies = [ + "serde", +] + [[package]] name = "serde_derive" version = "1.0.130" @@ -2067,6 +2239,17 @@ dependencies = [ "tracing-serde", ] +[[package]] +name = "trait-set" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "875c4c873cc824e362fa9a9419ffa59807244824275a44ad06fec9684fff08f2" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "try-lock" version = "0.2.3" diff --git a/Cargo.toml b/Cargo.toml index da95c3985..5afbff151 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,9 +21,10 @@ exclude = [ build = "build.rs" [dependencies] +backoff = { version = "0.3.0", optional = true } base64 = "^0.13" basic-cookies = { version = "^0.1", optional = true } -bcder = "^0.6" +bcder = "0.6.1-dev" bytes = "1" chrono = { version = "^0.4", features = ["serde"] } clap = "^2.33" @@ -35,16 +36,17 @@ hex = "^0.4" hyper = { version = "^0.14", features = ["server"] } intervaltree = "0.2.6" jmespatch = { version = "^0.3", features = ["sync"], optional = true } +kmip = { version = "0.3.1", package = "kmip-protocol", features = ["tls-with-openssl"], optional = true } libflate = "^1" log = "^0.4" openidconnect = { version = "^2.0.0", optional = true, default_features = false } openssl = { version = "^0.10", features = ["v110"] } oso = { version = "^0.12", optional = true, default_features = false } +r2d2 = { version = "0.8.9", optional = true } regex = { version = "^1.4", optional = true, default_features = false, features = ["std"] } reqwest = { version = "0.11", features = ["json"] } rpassword = { version = "^5.0", optional = true } -# rpki = { version = "0.11.1-dev", features = [ "repository", "rrdp", "serde" ], git = "https://github.com/NLnetLabs/rpki-rs.git" } -rpki = { version = "0.12.2", features = [ "repository", "rrdp", "serde" ] } +rpki = { version = "0.12.3", features = [ "repository", "rrdp", "serde" ] } scrypt = { version = "^0.6", optional = true, default-features = false } serde = { version = "^1.0", features = ["derive"] } serde_json = "^1.0" @@ -71,7 +73,8 @@ ui-tests = [] extra-debug = [ "rpki/extra-debug" ] static-openssl = [ "openssl/vendored" ] all-except-ui-tests = [ "multi-user", "rta", "static-openssl" ] -hsm = [] +hsm = ["backoff", "kmip", "r2d2"] +hsm-tests = [] # Make sure that Krill crashes on panics, rather than losing threads and # limping on in a bad state. @@ -181,3 +184,7 @@ shadow-utils = "*" # END RPM PACKAGING # ------------------------------------------------------------------------------ + +[patch.crates-io] +bcder = { git = 'https://github.com/NLnetLabs/bcder' } +rpki = { git = 'https://github.com/ximon18/rpki-rs', branch = '0.12.3-unsigned-from-slice' } \ No newline at end of file diff --git a/doc/development/hsm/architecture.md b/doc/development/hsm/architecture.md index 76a8d2d8a..548533a77 100644 --- a/doc/development/hsm/architecture.md +++ b/doc/development/hsm/architecture.md @@ -1,22 +1,68 @@ # HSM: Architecture -## Why create new KMIP crates? +## Key data -No well or actively maintained Rust support for KMIP with sufficient functionality for Krill -existed at the time of writing. +- Krill `KeyIdentifier`: SHA-1 of the binary DER encoding of the X.509 ASN.1 SubjectPublicKeyInfo data structure) used + by Krill. Cannot be changed as it is stored in certificates as per standards and then later the certificate is the + only information available to the calling code, i.e. it doesn't have anything else to identify the key involved. -The closest candidate, https://github.com/visa/kmip, was used to explore KMIP support in the -Krill HSM prototype code. However, it was decided to create our own KMIP library because the -visa crate: +## Key architectural decisions + +- Create our own KMIP Rust library as no actively maintained Rust support for KMIP with sufficient functionality for + Krill existed at the time of writing. The closest candidate, https://github.com/visa/kmip, was used to explore KMIP + support in the Krill HSM prototype code. However, it was decided to create our own KMIP library because the visa + crate: - Was not published on crates.io. - Had only rudimentary error reporting (as it targets no-std environments). - - Lacks documentation. - - Lacked TCP+TLS client support. - - Lacked support for KMIP operations that Krill requires. - - Did not appear to be actively maintained or intended for use by others. + - Lacked documentation, support for needed KMIP operations & TCP+TLS client support. + + Don't include the KMIP library in the main Krill code base as it is orthogonal to and independently useful outside of + Krill, and the Krill codebase is already quite large & slow to compile. + +- Support multiple concurrently active signers to support rollover to a new signer (creation of new keys with the new + signer while continued use of keys created by the previous signer), fallback to the OpenSSL signer for random number + generation whne the chosen doesn't support this capability, generation of one-time keys using OpenSSL as doing this + with an HSM would be slow (create, activate, sign, deactivate, destroy, potentially each being a network round trip + plus relatively slow execution of operations compared to local OpenSSL) and because the security benefits of an HSM + are not needed for one-time signing keys. + +- Maintain a mapping of Krill `KeyIdentifier` to signer identifier so that we can dispatch signing requests to the + correct signer. + +- Maintain a mapping of Krill `KeyIdentifier` to signer specific key identifiers. + +- Support generation of the `KeyIdentifier` from component parts (RSA modulus and exponent) for cases where the signer + doesn't (guarantee) support for exposing that for us itself but only provides access to the component parts. + +- Don't fail to start Krill if a signer backend is not reachable or lacks required capabilities. Preventing Krill from + operating won't fix the problem and prevents Krill from doing anything else useful with keys from other signers or + offering its API or UI. + +## Design + +- KrillSigner acts as the central hub aware of all signers and key mappings and dispatches requests to the correct + signer based on the action being performed and/or the `KeyIdentifier` involved. + +- Persist key mappings to: TBD + +- Permit signers to be async: TBD + +- Be robust in case of network delays and errors and problems in external signing services. Retry requests that fail + due to issues potentially caused by transient network degradation. Re-use TCP+TLS sessions to avoid costly TCP+TLS + setup and teardown costs per request to the signer service. + +- The signer can be in one of three statuses: + - Proving - Rather than constantly attempt to "probe" (contact the server and check its capabilities) we track in + this state when we last probed the server so that we can limit how often we try probing the server. + - Unusable - Probing was able to contact the server and found it unusable. + - Usable - Probing was able to contact the server and found it usable. -## Why not add KMIP code to Krill directly? +## Implementation details -The Krill code base is already large enough and slow enough to compile. KMIP support may also -be of interest to others. It thus seemed a good candidate for separation from Krill itself. \ No newline at end of file +- Connection pooling is handled by the `r2d2` crate. +- Retry and backoff is handled by the `backoff` crate. +- TLS is handled by the `openssl` crate as `rustls` may impose stricter limits on outbound connectivity to HSMs than + can be pragmatically expected to work in all customer environments. +- TBD: Switch to `tokio-native-tls` to keep the benefits of and control of local O/S native TLS providers and avoid the + 'modern security' limitations of `rustls` while switching to an `async` model. \ No newline at end of file diff --git a/src/commons/api/ca.rs b/src/commons/api/ca.rs index f2be0d8ed..50c7479c4 100644 --- a/src/commons/api/ca.rs +++ b/src/commons/api/ca.rs @@ -2551,7 +2551,8 @@ mod test { use rpki::repository::crypto::{signer::Signer, PublicKeyFormat}; - use crate::commons::util::softsigner::OpenSslSigner; + use crate::commons::crypto::signers::softsigner::OpenSslSigner; + use crate::test; use super::*; diff --git a/src/commons/crypto/mod.rs b/src/commons/crypto/mod.rs index 53d41d61c..c69fdcea9 100644 --- a/src/commons/crypto/mod.rs +++ b/src/commons/crypto/mod.rs @@ -10,4 +10,6 @@ pub use self::error::*; mod signing; pub use self::signing::*; +pub mod signers; + pub type CryptoResult = std::result::Result; diff --git a/src/commons/crypto/signers/error.rs b/src/commons/crypto/signers/error.rs new file mode 100644 index 000000000..327454913 --- /dev/null +++ b/src/commons/crypto/signers/error.rs @@ -0,0 +1,48 @@ +use std::{fmt, path::PathBuf}; + +use openssl::error::ErrorStack; + +use crate::commons::error::KrillIoError; + +#[derive(Debug)] +pub enum SignerError { + KmipError(String), + OpenSslError(ErrorStack), + JsonError(serde_json::Error), + InvalidWorkDir(PathBuf), + IoError(KrillIoError), + KeyNotFound, + DecodeError, +} + +impl fmt::Display for SignerError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + SignerError::KmipError(e) => write!(f, "KMIP Error: {}", e), + SignerError::OpenSslError(e) => write!(f, "OpenSsl Error: {}", e), + SignerError::JsonError(e) => write!(f, "Could not decode public key info: {}", e), + SignerError::InvalidWorkDir(path) => write!(f, "Invalid base path: {}", path.to_string_lossy()), + SignerError::IoError(e) => e.fmt(f), + SignerError::KeyNotFound => write!(f, "Could not find key"), + SignerError::DecodeError => write!(f, "Could not decode key"), + } + } +} + +impl From for SignerError { + fn from(e: ErrorStack) -> Self { + SignerError::OpenSslError(e) + } +} + +impl From for SignerError { + fn from(e: serde_json::Error) -> Self { + SignerError::JsonError(e) + } +} + +impl From for SignerError { + fn from(e: KrillIoError) -> Self { + SignerError::IoError(e) + } +} diff --git a/src/commons/crypto/signers/kmip/connpool.rs b/src/commons/crypto/signers/kmip/connpool.rs new file mode 100644 index 000000000..629b9bcc9 --- /dev/null +++ b/src/commons/crypto/signers/kmip/connpool.rs @@ -0,0 +1,125 @@ +/// KMIP TLS connection pool +/// +/// Used to: +/// - Avoid repeated TCP connection setup and TLS session establishment for mutiple KMIP requests made close together +/// in time. +/// - Handle loss of connectivity by re-creating the connection when an existing connection is considered to be +/// "broken" at the network level. +use std::time::Duration; + +use kmip::client::ConnectionSettings; + +use crate::commons::crypto::signers::{error::SignerError, kmip::internal::KmipTlsClient}; + +/// Manages KMIP TCP + TLS connection creation. +/// +/// Uses the [r2d2] crate to manage a pool of connections. +/// +/// [r2d2]: https://crates.io/crates/r2d2/ +#[derive(Debug)] +pub struct ConnectionManager { + conn_settings: ConnectionSettings, +} + +impl ConnectionManager { + /// Create a pool of up-to N TCP + TLS connections to the KMIP server. + #[rustfmt::skip] + pub fn create_connection_pool( + conn_settings: ConnectionSettings, + max_size: u32, + ) -> Result, SignerError> { + let pool = r2d2::Pool::builder() + // Don't pre-create idle connections to the KMIP server + .min_idle(Some(0)) + + // Create at most this many concurrent connections to the KMIP server + .max_size(max_size) + + // Don't verify that a connection is usable when fetching it from the pool (as doing so requires sending a + // request to the server and we might as well just try the actual request that we want the connection for) + .test_on_check_out(false) + + // Don't use the default logging behaviour as `[ERROR] [r2d2] Server error: ...` is a bit confusing for end + // users who shouldn't know or care that we use the r2d2 crate. + .error_handler(Box::new(ErrorLoggingHandler)) + + // Don't keep using the same connection for longer than around 30 minutes (unless in use in which case it + // will wait until the connection is returned to the pool before closing it) - maybe long held connections + // would run into problems with some firewalls. + .max_lifetime(Some(Duration::from_secs(60*30))) + + // Don't keep connections open that were not used in the last 10 minutes. + .idle_timeout(Some(Duration::from_secs(60*10))) + + // Don't wait longer than 30 seconds for a new connection to be established, instead try again to connect. + .connection_timeout(Duration::from_secs(30)) + + // Use our connection manager to create connections in the pool and to verify their health + .build(ConnectionManager { conn_settings })?; + + Ok(pool) + } + + /// Connect using the given connection settings to a KMIP server. + /// + /// This function creates a new connection to the server. The connection is NOT taken from the connection pool. + pub fn connect_one_off(settings: &ConnectionSettings) -> Result { + let conn = kmip::client::tls::openssl::connect(settings)?; + Ok(conn) + } +} + +impl r2d2::ManageConnection for ConnectionManager { + type Connection = KmipTlsClient; + + type Error = kmip::client::Error; + + /// Establishes a KMIP server connection which will be added to the connection pool. + fn connect(&self) -> Result { + Self::connect_one_off(&self.conn_settings) + } + + /// This function is never used because the [r2d2] `test_on_check_out` flag is set to false when the connection + /// pool is created. + /// + /// [r2d2]: https://crates.io/crates/r2d2/ + fn is_valid(&self, _conn: &mut Self::Connection) -> Result<(), Self::Error> { + unreachable!() + } + + /// Quickly verify if an existing connection is broken. + /// + /// Used to discard and re-create connections that encounter multiple connection related errors. + fn has_broken(&self, conn: &mut Self::Connection) -> bool { + conn.connection_error_count() > 1 + } +} + +/// A Krill specific [r2d2] error logging handler. +/// +/// Logs connection pool related connection error messages using the format `"[] Pool error: ..."` instead of +/// the default [r2d2] `"[ERROR] [r2d2] Server error: ..."` format. Assumes that the logging framework will include the +/// logging module context in the logged message, i.e. `xxx::kmip::xxx` and thus we don't need to mention KMIP in the +/// logged message content. +/// +/// Rationale: +/// - The use of the [r2d2] crate is an internal detail which of no use to end users consulting the logs and which we +/// may change at any time. +/// - Krill should be the one to determine the appropriate level to log a connection issue at, not [r2d2]. +#[derive(Debug)] +struct ErrorLoggingHandler; + +impl r2d2::HandleError for ErrorLoggingHandler +where + E: std::fmt::Display, +{ + fn handle_error(&self, err: E) { + warn!("Pool error: {}", err) + } +} + +impl From for SignerError { + fn from(err: r2d2::Error) -> Self { + SignerError::KmipError(format!("{}", err)) + } +} diff --git a/src/commons/crypto/signers/kmip/internal.rs b/src/commons/crypto/signers/kmip/internal.rs new file mode 100644 index 000000000..944e89ee9 --- /dev/null +++ b/src/commons/crypto/signers/kmip/internal.rs @@ -0,0 +1,782 @@ +use std::{ + net::TcpStream, + ops::Deref, + sync::{Arc, RwLock, RwLockReadGuard}, + time::{Duration, Instant}, +}; + +use backoff::ExponentialBackoff; +use bcder::encode::{PrimitiveContent, Values}; +use bytes::Bytes; +use kmip::{ + client::{Client, ClientCertificate, ConnectionSettings}, + types::{ + common::{KeyMaterial, ObjectType, Operation}, + response::{ManagedObject, RNGRetrieveResponsePayload}, + }, +}; +use openssl::ssl::SslStream; +use r2d2::PooledConnection; +use rpki::repository::crypto::{ + signer::KeyError, KeyIdentifier, PublicKey, PublicKeyFormat, Signature, SignatureAlgorithm, Signer, +}; + +use crate::commons::{ + api::Timestamp, + crypto::signers::{ + error::SignerError, + kmip::{ + connpool::ConnectionManager, + keymap::{KeyMap, KmipKeyPairIds}, + }, + util, + }, +}; + +//------------ Types and constants ------------------------------------------------------------------------------------ + +/// The time to wait between attempts to initially connect to the KMIP server to verify our connection settings and the +/// server capabilities. +const RETRY_INIT_EVERY: Duration = Duration::from_secs(30); + +/// The time to wait between an initial and subsequent attempt at sending a request to the KMIP server. +const RETRY_REQ_AFTER: Duration = Duration::from_secs(2); + +/// How much longer should we wait from one request attempt to the next compared to the previous wait? +const RETRY_REQ_AFTER_MULTIPLIER: f64 = 1.5; + +/// The maximum amount of time to keep retrying a failed request. +const RETRY_REQ_UNTIL_MAX: Duration = Duration::from_secs(30); + +/// The maximum number of concurrent connections to the KMIP server to pool. +const MAX_CONCURRENT_SERVER_CONNECTIONS: u32 = 5; + +/// TODO: Make this a configuration setting. For now set it to false because PyKMIP 0.10.0 says it doesn't support the +/// `ModifyAttribute` but sending a modify attribute request succeeds. +const IGNORE_MISSING_CAPABILITIES: bool = false; + +/// A KMIP client that uses a specific TLS and TCP stream implementation. Currently set to [SslStream] from the +/// [openssl] crate. This will be a different type if we switch to different TCP and/or TLS implementations or to an +/// async implementation, but the client interface will remain the same. +pub type KmipTlsClient = Client>; + +//------------ The KMIP signer management interface ------------------------------------------------------------------- + +#[derive(Clone, Debug)] +pub struct KmipSigner { + /// A probe dependent interface to the KMIP server. + server: Arc>, +} + +impl KmipSigner { + /// Creates a new instance of KmipSigner. + pub fn build() -> Result { + let conn_settings = Self::get_test_connection_settings(); + let server = Arc::new(RwLock::new(ProbingServerConnector::new(conn_settings))); + Ok(KmipSigner { server }) + } + + /// Returns true if the KMIP server supports generation of random numbers, false otherwise. + pub fn supports_random(&self) -> bool { + match self.server() { + Ok(status) => status.state().supports_rng_retrieve, + Err(_) => false, + } + } + + // TODO: Remove me once we support passing configuration in to `fn build()`. + fn get_test_connection_settings() -> ConnectionSettings { + let client_cert = ClientCertificate::SeparatePem { + cert_bytes: include_bytes!("../../../../../test-resources/pykmip/server.crt").to_vec(), + key_bytes: Some(include_bytes!("../../../../../test-resources/pykmip/server.key").to_vec()), + }; + let server_cert = include_bytes!("../../../../../test-resources/pykmip/server.crt").to_vec(); + let ca_cert = include_bytes!("../../../../../test-resources/pykmip/ca.crt").to_vec(); + + ConnectionSettings { + host: "127.0.0.1".to_string(), + port: 5696, + username: None, + password: None, + insecure: true, + client_cert: Some(client_cert), + server_cert: Some(server_cert), + ca_cert: Some(ca_cert), + connect_timeout: Some(Duration::from_secs(5)), + read_timeout: Some(Duration::from_secs(5)), + write_timeout: Some(Duration::from_secs(5)), + max_response_bytes: Some(64 * 1024), + } + } +} + +//------------ Probe based server access ------------------------------------------------------------------------------ + +/// Probe status based access to the KMIP server. +/// +/// To avoid blocking Krill startup due to HSM connection timeout or failure we start in a `Pending` status which +/// signifies that we haven't yet verified that we can connect to the HSM or that it supports the capabilities that we +/// require. +/// +/// At some point later once an initial connection has been established the KMIP signer changes status to either +/// `Usable` or `Unusable` based on what was discovered about the KMIP server. +#[derive(Clone, Debug)] +enum ProbingServerConnector { + /// We haven't yet been able to connect to the HSM using the TCP+TLS+KMIP protocol. If there was already a failed + /// attempt to connect the timestamp of the attempt is remembered so that we can choose to space out connection + /// attempts rather than attempt to connect every time Krill tries to use the signer. + Probing { + // The connection settings are not optional but are stored in an Option so that we can "take" them out when + // moving from the Probing status for use in the Usable status. + conn_settings: Option, + last_probe_time: Option, + }, + + /// The HSM was successfully probed but found to be lacking required capabilities and is thus unusable by Krill. + Unusable, + + /// The HSM was successfully probed and confirmed to have the required capabilities. + /// + /// Note that this does not mean that the HSM is currently contactable, only that we were able to contact it at + /// least once since Krill was started. If the domain name/IP address used to connect to Krill now point to a + /// different HSM instance the previously determined conclusion that the HSM is usable may no longer be valid. + /// + /// In this status we keep state concerning our relationship with the HSM. + Usable(UsableServerState), +} + +impl ProbingServerConnector { + /// Create a new connector to a KMIP server that hasn't been probed yet. + pub fn new(conn_settings: ConnectionSettings) -> Self { + ProbingServerConnector::Probing { + conn_settings: Some(conn_settings), + last_probe_time: None, + } + } + + /// Marks now as the last probe attempt timestamp. + /// + /// Calling this function while not in the Probing state will result in a panic. + pub fn mark(&self) { + match self { + ProbingServerConnector::Probing { + mut last_probe_time, .. + } => { + last_probe_time.replace(Instant::now()); + } + _ => unreachable!(), + } + } + + pub fn conn_settings(&self) -> &ConnectionSettings { + match self { + ProbingServerConnector::Probing { conn_settings, .. } => conn_settings.as_ref().unwrap(), + _ => unreachable!(), + } + } + + pub fn take_conn_settings(&mut self) -> ConnectionSettings { + match self { + ProbingServerConnector::Probing { conn_settings, .. } => conn_settings.take().unwrap(), + _ => unreachable!(), + } + } + + /// Helper function to retrieve the state associated with status Usable. Only called when in status `Usable`. + /// Calling this function while in another state will result in a panic. + pub fn state(&self) -> &UsableServerState { + match self { + ProbingServerConnector::Usable(state) => state, + _ => unreachable!(), + } + } +} + +/// The result of a successful attempt to probe if a KMIP server is usable or not. +#[derive(Clone, Debug)] +struct SuccessfulProbeResult { + identification: String, + supported_operations: Vec, +} + +/// The details needed to interact with a usable KMIP server. +#[derive(Clone, Debug)] +struct UsableServerState { + /// A pool of TCP + TLS clients for connecting to the KMIP server + pool: r2d2::Pool, + + /// TODO: Replace me by a persistent mechanism shared by all signers, not just the KMIP signer. + key_map: Arc>, + + /// Does the KMIP server support the RNG Retrieve operation (for generating random values)? + supports_rng_retrieve: bool, +} + +impl UsableServerState { + pub fn new(pool: r2d2::Pool, supports_rng_retrieve: bool) -> UsableServerState { + let key_map = Arc::new(RwLock::new(KeyMap::new())); + + UsableServerState { + pool, + key_map, + supports_rng_retrieve, + } + } +} + +impl KmipSigner { + /// Get a read lock on the Usable server status, if the server is usable. + /// + /// Returns `Ok` with the status read lock if the KMIP server is usable, otherwise returns an `Err` because the + /// server is unusable or we haven't yet been able to establish if it is usable or not. + /// + /// Will try probing again if we didn't already manage to connect to the server and the delay period between probes + /// has elapsed. + fn server(&self) -> Result, SignerError> { + fn get_server_if_usable( + status: RwLockReadGuard, + ) -> Option, SignerError>> { + // Check the status through the unlocked read lock + match &*status { + ProbingServerConnector::Usable(_) => { + // KMIP server has been confirmed as usable, return the read-lock granting access to the current + // status and via it the current state of our relationship with the KMIP server. + Some(Ok(status)) + } + + ProbingServerConnector::Unusable => { + // KMIP server has been confirmed as unusable, fail. + Some(Err(SignerError::KmipError("KMIP server is unusable".into()))) + } + + ProbingServerConnector::Probing { last_probe_time, .. } => { + // We haven't yet established whether the KMIP server is usable or not. If we haven't yet checked or we + // haven't tried checking again for a while, then try contacting it again. If we can't establish + // whether or not the server is usable, return an error. + if !is_time_to_check(RETRY_INIT_EVERY, *last_probe_time) { + Some(Err(SignerError::KmipError("KMIP server is not yet available".into()))) + } else { + None + } + } + } + } + + // Return the current status or attempt to set it by probing the server + let status = self.server.read().expect("KMIP status lock is poisoned"); + get_server_if_usable(status).unwrap_or_else(|| { + self.probe_server() + .and_then(|probe_result| self.use_server(probe_result)) + .and_then(|_| Ok(self.server.read().expect("KMIP status lock is poisoned"))) + .map_err(|err| SignerError::KmipError(format!("KMIP server is not yet available: {}", err))) + }) + } + + /// Verify if the configured KMIP server is contactable and supports the required capabilities. + fn probe_server(&self) -> Result { + // Hold a write lock for the duration of our attempt to verify the KMIP server so that no other attempt occurs + // at the same time. + let mut status = self.server.write().expect("KMIP status lock is poisoned"); + + // Update the timestamp of our last attempt to contact the KMIP server. This is used above to know when we have + // waited long enough before attempting to contact the server again. + status.mark(); + + let conn_settings = status.conn_settings(); + debug!("Probing server at {}:{}", conn_settings.host, conn_settings.port); + + // Attempt a one-off connection to check if we should abort due to a configuration error (e.g. unusable + // certificate) that will never work, and to determine the capabilities of the server (which may affect our + // behaviour). + let conn = ConnectionManager::connect_one_off(conn_settings).map_err(|err| { + let reason = match err { + // Fatal error + kmip::client::Error::ConfigurationError(err) => { + format!("Failed to connect KMIP server: Configuration error: {}", err) + } + + // Impossible errors: we didn't yet try to send a request or receive a response + kmip::client::Error::SerializeError(err) + | kmip::client::Error::RequestWriteError(err) + | kmip::client::Error::ResponseReadError(err) + | kmip::client::Error::DeserializeError(err) + | kmip::client::Error::InternalError(err) + | kmip::client::Error::Unknown(err) => { + format!("Failed to connect KMIP server: Unexpected error: {}", err) + } + + // I/O error attempting to contact the server or a problem on an internal problem at the server, not + // necessarily fatal or a reason to abort creating the pool. + kmip::client::Error::ServerError(err) => { + format!("Failed to connect KMIP server: Server error: {}", err) + } + }; + + SignerError::KmipError(reason) + })?; + + // We managed to establish a TCP+TLS connection to the KMIP server. Send it a Query request to discover how + // it calls itself and which KMIP operations it supports. + let server_properties = conn.query().map_err(|err| SignerError::KmipError(err.to_string()))?; + let supported_operations = server_properties.operations.unwrap_or_default(); + + // Check whether or not the KMIP operations that we require are supported by the server + let mut unsupported_operations = Vec::new(); + for required_op in &[ + Operation::CreateKeyPair, + Operation::Activate, + Operation::Sign, + Operation::Revoke, + Operation::Destroy, + Operation::Get, + Operation::ModifyAttribute, + ] { + if !supported_operations.contains(required_op) { + unsupported_operations.push(required_op.to_string()); + } + } + + // Warn about and (optionally) fail due to the lack of any unsupported operations. + if !unsupported_operations.is_empty() { + warn!( + "KMIP server lacks support for one or more required operations: {}", + unsupported_operations.join(",") + ); + + // Hard fail due to unsupported operations, unless our configuration tells us to try using this server + // anyway. For example, PyKMIP 0.10.0 does not include the ModifyAttribute operation in the set of + // supported operations even though it does support it. Without this flag we would not be able to use + // PyKMIP with Krill! + if IGNORE_MISSING_CAPABILITIES { + *status = ProbingServerConnector::Unusable; + return Err(SignerError::KmipError("KMIP server is unusable".to_string())); + } + } + + Ok(SuccessfulProbeResult { + identification: server_properties.vendor_identification.unwrap_or("Unknown".into()), + supported_operations, + }) + } + + /// Switch from probing the server to using it. + fn use_server(&self, probe_result: SuccessfulProbeResult) -> Result<(), SignerError> { + let mut status = self.server.write().expect("KMIP status lock is poisoned"); + + // Take the ConnectionSettings out of the Probing status so that we can move it to the Usable status. (we + // could clone it but it potentially contains a lot of certificate and key byte data and is about to get + // dropped when we change status which is silly when we still need it, instead take it with us to the new + // status) + let conn_settings = status.take_conn_settings(); + + // Success! We can use this server. Announce it and switch our status to KmipSignerStatus::Usable. + info!( + "Using KMIP server '{}' at {}:{}", + probe_result.identification, conn_settings.host, conn_settings.port + ); + + let supports_rng_retrieve = probe_result.supported_operations.contains(&Operation::RNGRetrieve); + let pool = ConnectionManager::create_connection_pool(conn_settings, MAX_CONCURRENT_SERVER_CONNECTIONS)?; + let state = UsableServerState::new(pool, supports_rng_retrieve); + + *status = ProbingServerConnector::Usable(state); + Ok(()) + } +} + +//------------ Connection related functions --------------------------------------------------------------------------- + +impl KmipSigner { + /// Get a connection to the KMIP server from the pool, if the server is usable. + fn connect(&self) -> Result, SignerError> { + let conn = self.server()?.state().pool.get()?; + Ok(conn) + } + + /// Perform some operation using a KMIP server pool connection. + /// + /// Fails if the KMIP server is not [KmipSignerStatus::Usable]. If the operation fails due to a transient + /// connection error, retry with backoff upto a defined retry limit. + fn with_conn(&self, desc: &str, do_something_with_conn: F) -> Result + where + F: FnOnce(&KmipTlsClient) -> Result + Copy, + { + // Define the backoff policy to use + let backoff_policy = ExponentialBackoff { + initial_interval: RETRY_REQ_AFTER, + multiplier: RETRY_REQ_AFTER_MULTIPLIER, + max_elapsed_time: Some(RETRY_REQ_UNTIL_MAX), + ..Default::default() + }; + + // Define a notify callback to customize messages written to the logger + let notify = |err, next: Duration| { + warn!("{} failed, retrying in {} seconds: {}", desc, next.as_secs(), err); + }; + + // Define an operation to (re)try + let op = || { + // First get a (possibly already existing) connection from the pool + let conn = self.connect()?; + + // Next, try to execute the callers operation using the connection. If it fails, examine the cause of + // failure to determine if it should be a hard-fail (no more retries) or if we should try again. + Ok((do_something_with_conn)(conn.deref()).map_err(retry_on_connection_error)?) + }; + + // Don't even bother going round the retry loop if we haven't yet successfully connected to the KMIP server + // and verified its capabilities: + let _ = self.server()?; + + // Try (and retry if needed) the requested operation. + Ok(backoff::retry_notify(backoff_policy, op, notify)?) + } +} + +/// The status of a key. +/// +/// KMIP servers require that a key be activated before it can be used for signing and be inactive (revoked) before it +/// can be deleted. +#[derive(Debug, PartialEq)] +pub enum KeyStatus { + /// The key is inactive. + Inactive, + + /// The key was activated. + Active, +} + +// High level helper functions for use by the public Signer interface implementation +impl KmipSigner { + /// Remember that the given KMIP public and private key pair IDs correspond to the given KeyIdentifier. + pub fn remember_kmip_key_ids( + &self, + key_id: &KeyIdentifier, + kmip_key_ids: KmipKeyPairIds, + ) -> Result<(), SignerError> { + let _ = self + .server()? + .state() + .key_map + .write() + .expect("KMIP key map lock is poisoned") + .insert(key_id.clone(), kmip_key_ids); + Ok(()) + } + + /// Given a KeyIdentifier lookup the corresponding KMIP public and private key pair IDs. + pub fn lookup_kmip_key_ids( + &self, + key_id: &KeyIdentifier, + ) -> Result::Error>> { + Ok(self + .server()? + .state() + .key_map + .read() + .expect("KMIP key map lock is poisoned") + .get(key_id) + .ok_or(KeyError::KeyNotFound)? + .clone()) + } + + /// Create a key pair in the KMIP server in the requested format and make it ready for use by Krill. + pub fn build_key(&self, algorithm: PublicKeyFormat) -> Result<(PublicKey, KmipKeyPairIds), SignerError> { + if !matches!(algorithm, PublicKeyFormat::Rsa) { + return Err(SignerError::KmipError(format!( + "Algorithm {:?} not supported while creating key", + &algorithm + ))); + } + + // Give keys a Krill specific but random name initially. Once we have created them we can determine the SHA-1 + // of their X.509 SubjectPublicKeyInfo aka the Krill KeyIdentifier and use that in the name instead of the + // random component. + + // The name given to a key is purely for our own use, the KMIP server doesn't care about it. We give keys a + // name that clearly indicates they relate to Krill as this may be helpful to the KMIP server operator. Once + // the key is created we rename it to include its Krill KeyIdentifier (aka the SHA-1 of the X.509 + // SubjectPublicKeyInfo) so that we can relate the key back to its usage in Krill. We include the Unix seconds + // since 1970-01-01 timestamp in the name initially just as some rough at-a-glance indication of when it was + // created and to differentiate it from other keys with the same name (of which there should be none as they + // key should either be renamed after creation or should have been deleted at some point). + let prefix = format!("krill_new_key_{}", Timestamp::now()); + let private_key_name = format!("{}_priv", prefix); + let public_key_name = format!("{}_pub", prefix); + + // Create the RSA key pair + let kmip_key_pair_ids = self.create_rsa_key_pair(private_key_name, public_key_name)?; + + // Prepare the new keys for use, and attempt to destroy them if anything goes wrong + let public_key = self + .prepare_keypair_for_use(&kmip_key_pair_ids.private_key_id, &kmip_key_pair_ids.public_key_id) + .or_else(|err| { + let _ = self.destroy_key_pair(&kmip_key_pair_ids, KeyStatus::Inactive); + Err(SignerError::KmipError(err.to_string())) + })?; + + Ok((public_key, kmip_key_pair_ids)) + } + + /// Create an RSA key pair in the KMIP server. + fn create_rsa_key_pair( + &self, + private_key_name: String, + public_key_name: String, + ) -> Result { + let (private_key_id, public_key_id) = self + .with_conn("create key pair", |conn| { + conn.create_rsa_key_pair(2048, private_key_name.clone(), public_key_name.clone()) + }) + .map_err(|err| SignerError::KmipError(format!("Failed to create RSA key pair: {}", err)))?; + + let kmip_key_ids = KmipKeyPairIds { + public_key_id, + private_key_id, + }; + + Ok(kmip_key_ids) + } + + /// Make the given KMIP private and public key pair ready for use by Krill. + fn prepare_keypair_for_use(&self, private_key_id: &str, public_key_id: &str) -> Result { + // Create a public key object for the public key + let public_key = self.get_public_key_from_id(&public_key_id)?; + + // Determine names for the public and private key that allow them to be related back to their usage in Krill + // TODO: Give even more helpful names to the keys such as the name of the CA they were created for? + let hex_key_id = hex::encode(public_key.key_identifier()); + let new_public_key_name = format!("krill-public-key-{}", hex_key_id); + let new_private_key_name = format!("krill-private-key-{}", hex_key_id); + + // Rename the keys to their new names + self.with_conn("rename key", |conn| { + conn.rename_key(public_key_id, new_public_key_name.clone()) + }) + .map_err(|err| { + SignerError::KmipError(format!( + "Failed to set name on new public key '{}': {}", + public_key_id, err + )) + })?; + + self.with_conn("rename key", |conn| { + conn.rename_key(private_key_id, new_private_key_name.clone()) + }) + .map_err(|err| { + SignerError::KmipError(format!( + "Failed to set name on new private key '{}': {}", + private_key_id, err + )) + })?; + + // Activate the private key so that it can be used for signing. Do this last otherwise if there is a problem + // with preparing the key pair for use we have to deactivate the private key before we can destroy it. + self.with_conn("activate key", |conn| conn.activate_key(&private_key_id)) + .map_err(|err| { + SignerError::KmipError(format!( + "Failed to activate new private key '{}': {}", + private_key_id, err + )) + })?; + + Ok(public_key) + } + + /// Get the RSA public bytes for the given KMIP server public key. + fn get_rsa_public_key_bytes(&self, public_key_id: &str) -> Result { + let response_payload = self + .with_conn("get key", |conn| conn.get_key(public_key_id)) + .map_err(|err| { + SignerError::KmipError(format!( + "Failed to get key material for public key with ID '{}': {:?}", + public_key_id, err + )) + })?; + + if response_payload.object_type != ObjectType::PublicKey { + return Err(SignerError::KmipError(format!( + "Failed to get key material: unsupported object type '{:?}' returned by KMIP Get operation for public key with ID '{}'", + response_payload.object_type, public_key_id))); + } + + let key_material = match response_payload.cryptographic_object { + ManagedObject::PublicKey(public_key) => public_key.key_block.key_value.key_material, + _ => { + return Err(SignerError::KmipError(format!( + "Failed to get key material: unsupported cryptographic object type returned by KMIP Get operation for public key with ID '{}'", + public_key_id))); + } + }; + + let rsa_public_key_bytes = match key_material { + KeyMaterial::Bytes(bytes) => bytes::Bytes::from(bytes), + KeyMaterial::TransparentRSAPublicKey(pub_key) => { + util::rsa_public_key_bytes_from_parts(&pub_key.modulus, &pub_key.public_exponent)? + } + KeyMaterial::TransparentRSAPrivateKey(priv_key) => { + if let Some(public_exponent) = priv_key.public_exponent { + util::rsa_public_key_bytes_from_parts(&priv_key.modulus, &public_exponent)? + } else { + return Err(SignerError::KmipError(format!( + "Failed to get key material: missing exponent in transparent RSA private key returned by KMIP Get operation for public key with ID '{}'", + public_key_id))); + } + } + _ => { + return Err(SignerError::KmipError(format!( + "Failed to get key material: unsupported key material type {:?} returned by KMIP Get operation for public key with ID '{}'", + key_material, public_key_id))); + } + }; + + Ok(rsa_public_key_bytes) + } + + pub fn get_public_key_from_id(&self, public_key_id: &str) -> Result { + let rsa_public_key_bytes = self.get_rsa_public_key_bytes(public_key_id)?; + + let subject_public_key = bcder::BitString::new(0, rsa_public_key_bytes); + + let subject_public_key_info = + bcder::encode::sequence((PublicKeyFormat::Rsa.encode(), subject_public_key.encode())); + + let mut subject_public_key_info_source: Vec = Vec::new(); + subject_public_key_info + .write_encoded(bcder::Mode::Der, &mut subject_public_key_info_source) + .map_err(|err| { + SignerError::KmipError(format!( + "Failed to create DER encoded SubjectPublicKeyInfo from constituent parts: {}", + err + )) + })?; + + let public_key = PublicKey::decode(subject_public_key_info_source.as_slice()).map_err(|err| { + SignerError::KmipError(format!( + "Failed to create public key from the DER encoded SubjectPublicKeyInfo: {}", + err + )) + })?; + + Ok(public_key) + } + + pub fn sign_with_key( + &self, + private_key_id: &str, + algorithm: SignatureAlgorithm, + data: &[u8], + ) -> Result { + if algorithm.public_key_format() != PublicKeyFormat::Rsa { + return Err(SignerError::KmipError(format!( + "Algorithm '{:?}' not supported", + algorithm.public_key_format() + ))); + } + + let signed = self + .with_conn("sign", |conn| conn.sign(&private_key_id, data)) + .map_err(|err| SignerError::KmipError(format!("Signing failed: {}", err)))?; + + let sig = Signature::new(SignatureAlgorithm::default(), Bytes::from(signed.signature_data)); + + Ok(sig) + } + + pub fn destroy_key_pair(&self, kmip_key_pair_ids: &KmipKeyPairIds, mode: KeyStatus) -> Result { + let mut success = true; + + if let Err(err) = self.with_conn("destroy key", |conn| conn.destroy_key(&kmip_key_pair_ids.public_key_id)) { + success = false; + warn!( + "Failed to destroy KMIP public key '{}': {}", + &kmip_key_pair_ids.public_key_id, err + ); + } + + let mut deactivated = true; + if mode == KeyStatus::Active { + // TODO: it's unclear from the KMIP 1.2 specification if this can fail because the key is already revoked. + // If that is a possible failure scenario we should not abort here but instead continue to delete the key. + if let Err(err) = self.with_conn("revoke key", |conn| conn.revoke_key(&kmip_key_pair_ids.private_key_id)) { + success = false; + deactivated = false; + warn!( + "Failed to revoke KMIP private key '{}': {}", + &kmip_key_pair_ids.private_key_id, err + ); + } + } + + if deactivated { + // TODO: This can fail if the key is not in the correct state, e.g. one cause can is if the key is not + // revoked. We don't expect this because we assume we know whether we activated or revoked the key or not + // but if for some reason the key exists, we think it does not require revocation but actually it does, + // then we would fail here. In such a case we could attempt to revoke and retry, but that assumes we can + // detect that specific failure scenario. + if let Err(err) = self.with_conn("destroy key", |conn| { + conn.destroy_key(&kmip_key_pair_ids.private_key_id) + }) { + success = false; + warn!( + "Failed to destroy KMIP private key '{}': {}", + &kmip_key_pair_ids.private_key_id, err + ); + } + } + + Ok(success) + } + + pub fn get_random_bytes(&self, num_bytes_wanted: usize) -> Result, ::Error> { + if !self.supports_random() { + return Err(SignerError::KmipError( + "The KMIP server does not support random number generation".to_string(), + )); + } + let res: RNGRetrieveResponsePayload = self + .with_conn("rng retrieve", |conn: &KmipTlsClient| { + conn.rng_retrieve(num_bytes_wanted as i32) + }) + .map_err(|err| SignerError::KmipError(format!("Failed to retrieve random bytes: {:?}", err)))?; + + Ok(res.data) + } +} + +fn is_time_to_check(time_between_checks: Duration, possible_lack_check_time: Option) -> bool { + match possible_lack_check_time { + None => true, + Some(instant) => Instant::now().saturating_duration_since(instant) > time_between_checks, + } +} + +// -------------------------------------------------------------------------------------------------------------------- +// Retry with backoff related helper impls/fns: +// -------------------------------------------------------------------------------------------------------------------- + +impl From for SignerError { + fn from(err: kmip::client::Error) -> Self { + SignerError::KmipError(format!("Client error: {}", err)) + } +} + +impl From> for SignerError { + fn from(err: backoff::Error) -> Self { + match err { + backoff::Error::Permanent(err) => err, + backoff::Error::Transient(err) => err, + } + } +} + +fn retry_on_connection_error(err: kmip::client::Error) -> backoff::Error +where + E: From, +{ + if err.is_connection_error() { + backoff::Error::Transient(err.into()) + } else { + backoff::Error::Permanent(err.into()) + } +} diff --git a/src/commons/crypto/signers/kmip/keymap.rs b/src/commons/crypto/signers/kmip/keymap.rs new file mode 100644 index 000000000..abf58554e --- /dev/null +++ b/src/commons/crypto/signers/kmip/keymap.rs @@ -0,0 +1,13 @@ +use std::collections::HashMap; + +use rpki::repository::crypto::KeyIdentifier; + +/// KMIP key identifiers needed to use keys stored in the HSM. +#[derive(Clone, Debug)] +pub struct KmipKeyPairIds { + pub public_key_id: String, + pub private_key_id: String, +} + +/// An in-memory mapping of Krill [KeyIdentifier] to KMIP server internal key pair identifiers. +pub type KeyMap = HashMap; diff --git a/src/commons/crypto/signers/kmip/mod.rs b/src/commons/crypto/signers/kmip/mod.rs new file mode 100644 index 000000000..754f952dc --- /dev/null +++ b/src/commons/crypto/signers/kmip/mod.rs @@ -0,0 +1,21 @@ +//! Support for signing things using an external KMIP compliant cryptographic token. +//! +//! Currently only intended for sanity checking the use of KMIP with Krill by running as the only signer in place of +//! the usual [OpenSslSigner]. Assumes that the KMIP server is a [PyKMIP] instance that is created for and destroyed +//! after the Krill tests have run. Uses hard-coded connection details and in-memory storage of key identifiers issued +//! by the KMIP server. +//! +//! The current implementation splits the KMIP signer into four Rust modules: +//! - `connpool`: Connection pooling related functionality. +//! - `internal`: KMIP server interaction, including probing and retry/backoff logic. +//! - `keymap`: In-memory mapping of `KeyIdentifier` to KMIP key identifiers. +//! - `signer`: The public signer trait implementation. Delegates to `internal`. +//! +//! [OpenSslSigner]: crate::commons::util::softsigner::OpenSslSigner +//! [PyKMIP]: https://github.com/OpenKMIP/PyKMIP +pub mod connpool; +pub mod internal; +pub mod keymap; +pub mod signer; + +pub use internal::KmipSigner; diff --git a/src/commons/crypto/signers/kmip/signer.rs b/src/commons/crypto/signers/kmip/signer.rs new file mode 100644 index 000000000..9db0ef419 --- /dev/null +++ b/src/commons/crypto/signers/kmip/signer.rs @@ -0,0 +1,85 @@ +use rpki::repository::crypto::{ + signer::KeyError, KeyIdentifier, PublicKey, PublicKeyFormat, Signature, SignatureAlgorithm, Signer, SigningError, +}; + +use crate::commons::crypto::signers::{ + error::SignerError, + kmip::{internal::KeyStatus, KmipSigner}, +}; + +impl Signer for KmipSigner { + type KeyId = KeyIdentifier; + type Error = SignerError; + + fn create_key(&mut self, algorithm: PublicKeyFormat) -> Result { + let (key, kmip_key_pair_ids) = self.build_key(algorithm)?; + let key_id = key.key_identifier(); + self.remember_kmip_key_ids(&key_id, kmip_key_pair_ids)?; + Ok(key_id) + } + + fn get_key_info(&self, key_id: &Self::KeyId) -> Result> { + let kmip_key_pair_ids = self.lookup_kmip_key_ids(key_id)?; + self.get_public_key_from_id(&kmip_key_pair_ids.public_key_id) + .map_err(|err| KeyError::Signer(err)) + } + + fn destroy_key(&mut self, key_id: &Self::KeyId) -> Result<(), KeyError> { + let kmip_key_pair_ids = self.lookup_kmip_key_ids(key_id)?; + match self.destroy_key_pair(&kmip_key_pair_ids, KeyStatus::Active)? { + true => Ok(()), + false => Err( + SignerError::KmipError( + format!("Failed to completely destroy KMIP key pair for Krill KeyIdentifier '{:?}', KMIP public key '{}' and KMIP private key '{}'", + key_id, kmip_key_pair_ids.public_key_id, kmip_key_pair_ids.private_key_id)))?, + } + } + + fn sign + ?Sized>( + &self, + key_id: &Self::KeyId, + algorithm: SignatureAlgorithm, + data: &D, + ) -> Result> { + let kmip_key_pair_ids = self.lookup_kmip_key_ids(key_id)?; + + let signature = self + .sign_with_key(&kmip_key_pair_ids.private_key_id, algorithm, data.as_ref()) + .map_err(|err| { + SigningError::Signer(SignerError::KmipError(format!( + "Signing data failed for Krill KeyIdentifier '{}' and KMIP private key id '{}': {}", + key_id, kmip_key_pair_ids.private_key_id, err + ))) + })?; + + Ok(signature) + } + + fn sign_one_off + ?Sized>( + &self, + algorithm: SignatureAlgorithm, + data: &D, + ) -> Result<(Signature, PublicKey), Self::Error> { + // TODO: Is it possible to use a KMIP batch request to implement the create, activate, sign, deactivate, delete + // in one round-trip to the server? + let (key, kmip_key_pair_ids) = self.build_key(PublicKeyFormat::Rsa)?; + + let signature_res = self + .sign_with_key(&kmip_key_pair_ids.private_key_id, algorithm, data.as_ref()) + .map_err(|err| SignerError::KmipError(format!("One-off signing of data failed: {}", err))); + + let _ = self.destroy_key_pair(&kmip_key_pair_ids, KeyStatus::Active); + + let signature = signature_res?; + + Ok((signature, key)) + } + + fn rand(&self, data: &mut [u8]) -> Result<(), Self::Error> { + let random_bytes = self.get_random_bytes(data.len())?; + + data.copy_from_slice(&random_bytes); + + Ok(()) + } +} diff --git a/src/commons/crypto/signers/mod.rs b/src/commons/crypto/signers/mod.rs new file mode 100644 index 000000000..29ba11639 --- /dev/null +++ b/src/commons/crypto/signers/mod.rs @@ -0,0 +1,7 @@ +pub mod error; + +#[cfg(feature = "hsm")] +pub mod kmip; + +pub mod softsigner; +pub mod util; diff --git a/src/commons/util/softsigner.rs b/src/commons/crypto/signers/softsigner.rs similarity index 85% rename from src/commons/util/softsigner.rs rename to src/commons/crypto/signers/softsigner.rs index ae7c31d16..4d25a23bb 100644 --- a/src/commons/util/softsigner.rs +++ b/src/commons/crypto/signers/softsigner.rs @@ -1,7 +1,7 @@ //! Support for signing things using software keys (through openssl) and //! storing them unencrypted on disk. use std::{ - fmt, fs, + fs, fs::File, io::Write, path::{Path, PathBuf}, @@ -12,7 +12,6 @@ use bytes::Bytes; use serde::{de, ser, Deserialize, Deserializer, Serialize, Serializer}; use openssl::{ - error::ErrorStack, hash::MessageDigest, pkey::{PKey, PKeyRef, Private}, rsa::Rsa, @@ -22,7 +21,7 @@ use rpki::repository::crypto::{ signer::KeyError, KeyIdentifier, PublicKey, PublicKeyFormat, Signature, SignatureAlgorithm, Signer, SigningError, }; -use crate::commons::error::KrillIoError; +use crate::commons::{crypto::signers::error::SignerError, error::KrillIoError}; //------------ OpenSslSigner ------------------------------------------------- @@ -62,6 +61,10 @@ impl OpenSslSigner { Err(SignerError::InvalidWorkDir(work_dir.to_path_buf())) } } + + pub fn supports_random(&self) -> bool { + true + } } impl OpenSslSigner { @@ -214,49 +217,6 @@ impl OpenSslKeyPair { } } -//------------ OpenSslKeyError ----------------------------------------------- - -#[derive(Debug)] -pub enum SignerError { - OpenSslError(ErrorStack), - JsonError(serde_json::Error), - InvalidWorkDir(PathBuf), - IoError(KrillIoError), - KeyNotFound, - DecodeError, -} - -impl fmt::Display for SignerError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match self { - SignerError::OpenSslError(e) => write!(f, "OpenSsl Error: {}", e), - SignerError::JsonError(e) => write!(f, "Could not decode public key info: {}", e), - SignerError::InvalidWorkDir(path) => write!(f, "Invalid base path: {}", path.to_string_lossy()), - SignerError::IoError(e) => e.fmt(f), - SignerError::KeyNotFound => write!(f, "Could not find key"), - SignerError::DecodeError => write!(f, "Could not decode key"), - } - } -} - -impl From for SignerError { - fn from(e: ErrorStack) -> Self { - SignerError::OpenSslError(e) - } -} - -impl From for SignerError { - fn from(e: serde_json::Error) -> Self { - SignerError::JsonError(e) - } -} - -impl From for SignerError { - fn from(e: KrillIoError) -> Self { - SignerError::IoError(e) - } -} - //------------ Tests --------------------------------------------------------- #[cfg(test)] diff --git a/src/commons/crypto/signers/util.rs b/src/commons/crypto/signers/util.rs new file mode 100644 index 000000000..56534cdea --- /dev/null +++ b/src/commons/crypto/signers/util.rs @@ -0,0 +1,17 @@ +use bcder::encode::{PrimitiveContent, Values}; + +use crate::commons::crypto::signers::error::SignerError; + +/// Helper function to create X.509 RSA Public Key bytes from a given RSA modulus and exponent. +pub fn rsa_public_key_bytes_from_parts(modulus: &[u8], public_exponent: &[u8]) -> Result { + let modulus = bcder::Unsigned::from_slice(modulus).map_err(|_| SignerError::DecodeError)?; + let public_exp = bcder::Unsigned::from_slice(public_exponent).map_err(|_| SignerError::DecodeError)?; + let rsa_public_key = bcder::encode::sequence((modulus.encode(), public_exp.encode())); + + let mut bytes: Vec = Vec::new(); + rsa_public_key + .write_encoded(bcder::Mode::Der, &mut bytes) + .map_err(|_| SignerError::DecodeError)?; + + Ok(bytes::Bytes::from(bytes)) +} diff --git a/src/commons/crypto/signing.rs b/src/commons/crypto/signing.rs index 1ebe1e02d..8eacbe034 100644 --- a/src/commons/crypto/signing.rs +++ b/src/commons/crypto/signing.rs @@ -1,12 +1,10 @@ //! Support for signing mft, crl, certificates, roas.. //! Common objects for TAs and CAs use std::{ - ops::Deref, sync::{Arc, RwLock}, {convert::TryFrom, path::Path}, }; -use bcder::Captured; use bytes::Bytes; use rpki::{ @@ -24,136 +22,142 @@ use rpki::{ uri, }; -#[cfg(feature = "hsm")] -use crate::commons::util::dummysigner::DummySigner; - use crate::{ commons::{ api::{IssuedCert, RcvdCert, ReplacedObject, RepoInfo, RequestResourceLimit, ResourceSet}, - crypto::{self, CryptoResult}, + crypto::{ + self, + signers::{error::SignerError, softsigner::OpenSslSigner}, + CryptoResult, + }, error::Error, - util::{softsigner::OpenSslSigner, AllowedUri}, + util::AllowedUri, KrillResult, }, daemon::ca::CertifiedKey, }; +#[cfg(feature = "hsm")] +use crate::commons::crypto::signers::kmip::KmipSigner; + //------------ Signer -------------------------------------------------------- +#[allow(dead_code)] // Needed as we currently only ever construct one variant #[derive(Clone, Debug)] enum SignerProvider { OpenSsl(OpenSslSigner), #[cfg(feature = "hsm")] - #[allow(dead_code)] - Dummy(DummySigner), + Kmip(KmipSigner), } -impl SignerProvider { - pub fn create_key(&mut self) -> CryptoResult { +impl Signer for SignerProvider { + type KeyId = KeyIdentifier; + + type Error = SignerError; + + fn create_key(&mut self, algorithm: PublicKeyFormat) -> Result { match self { - SignerProvider::OpenSsl(signer) => signer.create_key(PublicKeyFormat::Rsa), + SignerProvider::OpenSsl(signer) => signer.create_key(algorithm), #[cfg(feature = "hsm")] - SignerProvider::Dummy(signer) => signer.create_key(PublicKeyFormat::Rsa), + SignerProvider::Kmip(signer) => signer.create_key(algorithm), } - .map_err(crypto::Error::signer) } - pub fn destroy_key(&mut self, key_id: &KeyIdentifier) -> CryptoResult<()> { + fn get_key_info( + &self, + key: &Self::KeyId, + ) -> Result> { match self { - SignerProvider::OpenSsl(signer) => signer.destroy_key(key_id), + SignerProvider::OpenSsl(signer) => signer.get_key_info(key), #[cfg(feature = "hsm")] - SignerProvider::Dummy(signer) => signer.destroy_key(key_id), + SignerProvider::Kmip(signer) => signer.get_key_info(key), } - .map_err(crypto::Error::key_error) } - pub fn get_key_info(&self, key_id: &KeyIdentifier) -> CryptoResult { + fn destroy_key( + &mut self, + key: &Self::KeyId, + ) -> Result<(), rpki::repository::crypto::signer::KeyError> { match self { - SignerProvider::OpenSsl(signer) => signer.get_key_info(key_id), + SignerProvider::OpenSsl(signer) => signer.destroy_key(key), #[cfg(feature = "hsm")] - SignerProvider::Dummy(signer) => signer.get_key_info(key_id), + SignerProvider::Kmip(signer) => signer.destroy_key(key), } - .map_err(crypto::Error::key_error) } - pub fn random_serial(&self) -> CryptoResult { + fn sign + ?Sized>( + &self, + key: &Self::KeyId, + algorithm: SignatureAlgorithm, + data: &D, + ) -> Result> { match self { - SignerProvider::OpenSsl(signer) => Serial::random(signer.deref()), + SignerProvider::OpenSsl(signer) => signer.sign(key, algorithm, data), #[cfg(feature = "hsm")] - SignerProvider::Dummy(signer) => Serial::random(signer.deref()), + SignerProvider::Kmip(signer) => signer.sign(key, algorithm, data), } - .map_err(crypto::Error::signer) } - pub fn sign + ?Sized>( + fn sign_one_off + ?Sized>( &self, - key_id: &KeyIdentifier, - sig_alg: SignatureAlgorithm, + algorithm: SignatureAlgorithm, data: &D, - ) -> CryptoResult { + ) -> Result<(Signature, PublicKey), Self::Error> { match self { - SignerProvider::OpenSsl(signer) => signer.sign(key_id, sig_alg, data), + SignerProvider::OpenSsl(signer) => signer.sign_one_off(algorithm, data), #[cfg(feature = "hsm")] - SignerProvider::Dummy(signer) => signer.sign(key_id, sig_alg, data), + SignerProvider::Kmip(signer) => signer.sign_one_off(algorithm, data), } - .map_err(crypto::Error::signing) } - pub fn sign_one_off + ?Sized>( - &self, - sig_alg: SignatureAlgorithm, - data: &D, - ) -> CryptoResult<(Signature, PublicKey)> { + fn rand(&self, target: &mut [u8]) -> Result<(), Self::Error> { match self { - SignerProvider::OpenSsl(signer) => signer.sign_one_off(sig_alg, data), + SignerProvider::OpenSsl(signer) => signer.rand(target), #[cfg(feature = "hsm")] - SignerProvider::Dummy(signer) => signer.sign_one_off(sig_alg, data), + SignerProvider::Kmip(signer) => signer.rand(target), } - .map_err(crypto::Error::signer) } +} - pub fn sign_csr(&self, base_repo: &RepoInfo, name_space: &str, key: &KeyIdentifier) -> CryptoResult { - fn func(signer: &T, base_repo: &RepoInfo, name_space: &str, key: &KeyIdentifier) -> CryptoResult - where - T: Signer, - { - let pub_key = signer.get_key_info(key).map_err(crypto::Error::key_error)?; - Csr::construct( - signer.deref(), - key, - &base_repo.ca_repository(name_space).join(&[]).unwrap(), // force trailing slash - &base_repo.rpki_manifest(name_space, &pub_key.key_identifier()), - Some(&base_repo.rpki_notify()), - ) - .map_err(crypto::Error::signing) +impl SignerProvider { + pub fn supports_random(&self) -> bool { + match self { + SignerProvider::OpenSsl(signer) => signer.supports_random(), + #[cfg(feature = "hsm")] + SignerProvider::Kmip(signer) => signer.supports_random(), } + } - let enc = match self { - SignerProvider::OpenSsl(signer) => func(signer.deref(), base_repo, name_space, key), + pub fn random_serial(&self) -> CryptoResult { + Serial::random(self).map_err(crypto::Error::signer) + } + + pub fn sign_csr(&self, base_repo: &RepoInfo, name_space: &str, key: &KeyIdentifier) -> CryptoResult { + let pub_key = match self { + SignerProvider::OpenSsl(signer) => signer.get_key_info(key).map_err(crypto::Error::key_error)?, #[cfg(feature = "hsm")] - SignerProvider::Dummy(signer) => func(signer.deref(), base_repo, name_space, key), - }?; + SignerProvider::Kmip(signer) => signer.get_key_info(key).map_err(crypto::Error::key_error)?, + }; + + let enc = Csr::construct( + self, + key, + &base_repo.ca_repository(name_space).join(&[]).unwrap(), // force trailing slash + &base_repo.rpki_manifest(name_space, &pub_key.key_identifier()), + Some(&base_repo.rpki_notify()), + ) + .map_err(crypto::Error::signing)?; Ok(Csr::decode(enc.as_slice())?) } pub fn sign_cert(&self, tbs: TbsCert, key_id: &KeyIdentifier) -> CryptoResult { - match self { - SignerProvider::OpenSsl(signer) => tbs.into_cert(signer.deref(), key_id), - #[cfg(feature = "hsm")] - SignerProvider::Dummy(signer) => tbs.into_cert(signer.deref(), key_id), - } - .map_err(crypto::Error::signing) + tbs.into_cert(self, key_id).map_err(crypto::Error::signing) } pub fn sign_crl(&self, tbs: TbsCertList>, key_id: &KeyIdentifier) -> CryptoResult { - match self { - SignerProvider::OpenSsl(signer) => tbs.into_crl(signer.deref(), key_id), - #[cfg(feature = "hsm")] - SignerProvider::Dummy(signer) => tbs.into_crl(signer.deref(), key_id), - } - .map_err(crypto::Error::signing) + tbs.into_crl(self, key_id).map_err(crypto::Error::signing) } pub fn sign_manifest( @@ -162,12 +166,9 @@ impl SignerProvider { builder: SignedObjectBuilder, key_id: &KeyIdentifier, ) -> CryptoResult { - match self { - SignerProvider::OpenSsl(signer) => content.into_manifest(builder, signer.deref(), key_id), - #[cfg(feature = "hsm")] - SignerProvider::Dummy(signer) => content.into_manifest(builder, signer.deref(), key_id), - } - .map_err(crypto::Error::signing) + content + .into_manifest(builder, self, key_id) + .map_err(crypto::Error::signing) } pub fn sign_roa( @@ -176,24 +177,15 @@ impl SignerProvider { object_builder: SignedObjectBuilder, key_id: &KeyIdentifier, ) -> CryptoResult { - match self { - SignerProvider::OpenSsl(signer) => roa_builder.finalize(object_builder, signer.deref(), key_id), - #[cfg(feature = "hsm")] - SignerProvider::Dummy(signer) => roa_builder.finalize(object_builder, signer.deref(), key_id), - } - .map_err(crypto::Error::signing) + roa_builder + .finalize(object_builder, self, key_id) + .map_err(crypto::Error::signing) } pub fn sign_rta(&self, rta_builder: &mut rta::RtaBuilder, ee: Cert) -> CryptoResult<()> { let key = ee.subject_key_identifier(); rta_builder.push_cert(ee); - - match self { - SignerProvider::OpenSsl(signer) => rta_builder.sign(signer.deref(), &key, None, None), - #[cfg(feature = "hsm")] - SignerProvider::Dummy(signer) => rta_builder.sign(signer.deref(), &key, None, None), - } - .map_err(crypto::Error::signing) + rta_builder.sign(self, &key, None, None).map_err(crypto::Error::signing) } } @@ -209,13 +201,16 @@ pub struct KrillSigner { // operations. In future we might move the responsibility for locking into the signer so that it can lock only what // actually needs to be locked raet - // The general signer is used for all signing operations except one off signing. + // The general signer is used for all signing operations except one-off signing. general_signer: Arc>, - // As the security of a HSM isn't needed for one off keys, and HSMs are slow, by default this should be an instance + // As the security of a HSM isn't needed for one-off keys, and HSMs are slow, by default this should be an instance // of OpenSslSigner. However, if users think the perceived extra security is warranted let them use a different - // Signer for one off keys if that's what they want. + // Signer for one-off keys if that's what they want. one_off_signer: Arc>, + + // The signer to use when a configured signer doesn't support generation of random numbers. + rand_fallback_signer: Arc>, } impl KrillSigner { @@ -228,30 +223,102 @@ impl KrillSigner { // being created for a key roll. For now the capability for different signers for different purposes exists but // is not yet used. - let openssl_signer = OpenSslSigner::build(work_dir)?; - let openssl_signer = Arc::new(RwLock::new(SignerProvider::OpenSsl(openssl_signer))); - let general_signer = openssl_signer.clone(); - let one_off_signer = openssl_signer; - Ok(KrillSigner { - general_signer, - one_off_signer, - }) + // TODO: Once it becomes possible to configure how an HSM is used by Krill we need to decide what the + // defaults should be and what should be configurable or not concerning HSM usage, and to document why, if + // permitted, it is acceptable to use local keys, signing & random number genration instead of the more + // secure HSM based alternatives (if available). + + // We always need an OpenSSL signer, either for keys created by a previous instance of Krill, or as a fallback + // random number generator for HSMs that don't support random number generation, or for creating one-off short + // lived signing keys. + let openssl_signer = Arc::new(RwLock::new(SignerProvider::OpenSsl(OpenSslSigner::build(work_dir)?))); + + #[cfg(not(feature = "hsm"))] + { + // For backward compatibility with currently released Krill, use OpenSSL for everything. + Ok(KrillSigner { + general_signer: openssl_signer.clone(), + one_off_signer: openssl_signer.clone(), + rand_fallback_signer: openssl_signer, + }) + } + + #[cfg(all(feature = "hsm", not(feature = "hsm-tests")))] + { + // Currently the behaviour is the same with the hsm-tests feature enabled or disabled. This is because with + // it disabled the Krill 'functional' test fails with error 'Krill failed to start: Signing issue: Could + // not find key'. + // + // For some reason a long lived signing key is created with the KMIP signer and then that key is used with + // one of the KrillSigner functions that takes a `SignedObjectBuilder` argument. These functions dispatch + // to the configured one-off signer (because ultimately their code paths invoke `sign_one_off` on the + // Signer) which in the commented out configuration below is NOT the KMIP signer but instead is the OpenSSL + // signer. The OpenSSL signer didn't create the key and so cannot find a key with the given KeyIdentifier. + // + // Either this logic is wrong, or this will be solved once we have a mapping of KeyIdentifier to Signer so + // that we ensure that we invoke the signer that created the key. Implementing that mapping is beyond the + // scope of the current task and so this has for now been commented out. + compile_error!("The 'hsm' feature can only be used in combination with the 'hsm-tests' feature at present"); + unreachable!(); + // // When the HSM feature is activated but we are not in test mode: + // // - Use the HSM for key creation, signing, deletion, except for one-off keys. + // // - Use the HSM for random number generation, if supported, else use the OpenSSL signer. + // // - Use the OpenSSL signer for one-off keys. + // let kmip_signer = Arc::new(RwLock::new(SignerProvider::Kmip(KmipSigner::build()?))); + + // Ok(KrillSigner { + // general_signer: kmip_signer.clone(), + // one_off_signer: openssl_signer.clone(), + // rand_fallback_signer: openssl_signer, + // }) + } + + #[cfg(all(feature = "hsm", feature = "hsm-tests"))] + { + // When the HSM feature is activated AND test mode is activated: + // - Use the HSM for as much as possible to depend on it as broadly as possible in the Krill test suite.. + // - Fallback to OpenSSL for random number generation if the HSM doesn't support it. + let kmip_signer = Arc::new(RwLock::new(SignerProvider::Kmip(KmipSigner::build()?))); + + Ok(KrillSigner { + general_signer: kmip_signer.clone(), + one_off_signer: kmip_signer.clone(), + rand_fallback_signer: openssl_signer, + }) + } } pub fn create_key(&self) -> CryptoResult { - self.general_signer.write().unwrap().create_key() + self.general_signer + .write() + .unwrap() + .create_key(PublicKeyFormat::Rsa) + .map_err(crate::commons::crypto::error::Error::signer) } pub fn destroy_key(&self, key_id: &KeyIdentifier) -> CryptoResult<()> { - self.general_signer.write().unwrap().destroy_key(key_id) + self.general_signer + .write() + .unwrap() + .destroy_key(key_id) + .map_err(crate::commons::crypto::error::Error::signer) } pub fn get_key_info(&self, key_id: &KeyIdentifier) -> CryptoResult { - self.general_signer.read().unwrap().get_key_info(key_id) + self.general_signer + .read() + .unwrap() + .get_key_info(key_id) + .map_err(crate::commons::crypto::error::Error::signer) } pub fn random_serial(&self) -> CryptoResult { - self.general_signer.read().unwrap().random_serial() + let signer = self.general_signer.read().unwrap(); + if signer.supports_random() { + signer.random_serial() + } else { + self.rand_fallback_signer.read().unwrap().random_serial() + } } pub fn sign + ?Sized>(&self, key_id: &KeyIdentifier, data: &D) -> CryptoResult { @@ -259,6 +326,7 @@ impl KrillSigner { .read() .unwrap() .sign(key_id, SignatureAlgorithm::default(), data) + .map_err(crate::commons::crypto::error::Error::signer) } pub fn sign_one_off + ?Sized>(&self, data: &D) -> CryptoResult<(Signature, PublicKey)> { @@ -266,6 +334,7 @@ impl KrillSigner { .read() .unwrap() .sign_one_off(SignatureAlgorithm::default(), data) + .map_err(crate::commons::crypto::error::Error::signer) } pub fn sign_csr(&self, base_repo: &RepoInfo, name_space: &str, key: &KeyIdentifier) -> CryptoResult { @@ -286,7 +355,7 @@ impl KrillSigner { builder: SignedObjectBuilder, key_id: &KeyIdentifier, ) -> CryptoResult { - self.general_signer + self.one_off_signer .read() .unwrap() .sign_manifest(content, builder, key_id) @@ -298,14 +367,14 @@ impl KrillSigner { object_builder: SignedObjectBuilder, key_id: &KeyIdentifier, ) -> CryptoResult { - self.general_signer + self.one_off_signer .read() .unwrap() .sign_roa(roa_builder, object_builder, key_id) } pub fn sign_rta(&self, rta_builder: &mut rta::RtaBuilder, ee: Cert) -> CryptoResult<()> { - self.general_signer.read().unwrap().sign_rta(rta_builder, ee) + self.one_off_signer.read().unwrap().sign_rta(rta_builder, ee) } } diff --git a/src/commons/error.rs b/src/commons/error.rs index 29af3f3e5..40a340073 100644 --- a/src/commons/error.rs +++ b/src/commons/error.rs @@ -15,12 +15,13 @@ use crate::{ rrdp::PublicationDeltaError, ChildHandle, ErrorResponse, Handle, ParentHandle, PublisherHandle, ResourceClassName, ResourceSetError, RoaDefinition, }, + crypto::signers::error::SignerError, eventsourcing::{AggregateStoreError, KeyValueError}, remote::{ rfc6492::{self, NotPerformedResponse}, rfc8181::{self, ReportErrorCode}, }, - util::{httpclient, softsigner::SignerError}, + util::httpclient, }, daemon::{ca::RouteAuthorization, http::tls_keys}, }; diff --git a/src/commons/util/dummysigner.rs b/src/commons/util/dummysigner.rs deleted file mode 100644 index 0582de7c9..000000000 --- a/src/commons/util/dummysigner.rs +++ /dev/null @@ -1,48 +0,0 @@ -use rpki::repository::crypto::{KeyIdentifier, PublicKey, PublicKeyFormat, Signature, SignatureAlgorithm, Signer}; - -use super::softsigner::SignerError; - -/// A dummy signer to prove that compilation with two different Signer implementations works -#[derive(Clone, Debug)] -pub struct DummySigner; - -impl Signer for DummySigner { - type KeyId = KeyIdentifier; - type Error = SignerError; - - fn create_key(&mut self, _: PublicKeyFormat) -> Result { - unreachable!() - } - - fn get_key_info( - &self, - _: &Self::KeyId, - ) -> Result> { - unreachable!() - } - - fn destroy_key(&mut self, _: &Self::KeyId) -> Result<(), rpki::repository::crypto::signer::KeyError> { - unreachable!() - } - - fn sign + ?Sized>( - &self, - _: &Self::KeyId, - _: SignatureAlgorithm, - _: &D, - ) -> Result> { - unreachable!() - } - - fn sign_one_off + ?Sized>( - &self, - _: SignatureAlgorithm, - _: &D, - ) -> Result<(Signature, PublicKey), Self::Error> { - unreachable!() - } - - fn rand(&self, _: &mut [u8]) -> Result<(), Self::Error> { - unreachable!() - } -} diff --git a/src/commons/util/mod.rs b/src/commons/util/mod.rs index 65138a303..cacf3d876 100644 --- a/src/commons/util/mod.rs +++ b/src/commons/util/mod.rs @@ -11,12 +11,9 @@ use rpki::{ use crate::constants::KRILL_VERSION; -#[cfg(feature = "hsm")] -pub mod dummysigner; pub mod ext_serde; pub mod file; pub mod httpclient; -pub mod softsigner; pub mod xml; //------------ KrillVersion -------------------------------------------------- diff --git a/test-resources/pykmip/README.md b/test-resources/pykmip/README.md new file mode 100644 index 000000000..f028bb526 --- /dev/null +++ b/test-resources/pykmip/README.md @@ -0,0 +1,48 @@ +The files created in this directory were created and can be used like so: + +Tested on Ubuntu 18:04 + +``` +apt update +apt install -y python3-pip +pip3 install pykmip + +mkdir /etc/pykmip +cd /etc/pykmip +cat <san.cnf +[ext] +subjectAltName = DNS:localhost +EOF + +mkdir demoCA +touch demoCA/index.txt +echo 01 > demoCA/serial +openssl ecparam -out ca.key -name secp256r1 -genkey +openssl req -x509 -new -key ca.key -out ca.crt -outform PEM -days 3650 -subj "/C=NL/ST=Noord Holland/L=Amsterdam/O=NLnet Labs/CN=localhost" + +openssl ecparam -out server.key -name secp256r1 -genkey + +openssl req -new -nodes -key server.key -outform pem -out server.csr -subj "/C=NL/ST=Noord Holland/L=Amsterdam/O=NLnet Labs/CN=localhost" + +openssl ca -keyfile ca.key -cert ca.crt -in server.csr -out server.crt -outdir . -batch -noemailDN -extfile san.cnf -extensions ext + +openssl pkcs8 -topk8 -nocrypt -in server.key -out server.pkcs8.key + +mv server.pkcs8.key server.key + +cat <server.conf +[server] +hostname=localhost +port=5696 +certificate_path=/etc/pykmip/server.crt +key_path=/etc/pykmip/server.key +ca_path=/etc/pykmip/ca.crt +auth_suite=TLS1.2 +enable_tls_client_auth=False +tls_cipher_suites=TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 +logging_level=DEBUG +database_path=/tmp/pykmip.db +EOF + +pykmip-server +``` diff --git a/test-resources/pykmip/ca.crt b/test-resources/pykmip/ca.crt new file mode 100644 index 000000000..3c23e8f95 --- /dev/null +++ b/test-resources/pykmip/ca.crt @@ -0,0 +1,14 @@ +-----BEGIN CERTIFICATE----- +MIICGTCCAb+gAwIBAgIUf8251O2yXLWfeanKzrtm1TonKpYwCgYIKoZIzj0EAwIw +YjELMAkGA1UEBhMCTkwxFjAUBgNVBAgMDU5vb3JkIEhvbGxhbmQxEjAQBgNVBAcM +CUFtc3RlcmRhbTETMBEGA1UECgwKTkxuZXQgTGFiczESMBAGA1UEAwwJbG9jYWxo +b3N0MB4XDTIxMDkyODE5NTkyNloXDTMxMDkyNjE5NTkyNlowYjELMAkGA1UEBhMC +TkwxFjAUBgNVBAgMDU5vb3JkIEhvbGxhbmQxEjAQBgNVBAcMCUFtc3RlcmRhbTET +MBEGA1UECgwKTkxuZXQgTGFiczESMBAGA1UEAwwJbG9jYWxob3N0MFkwEwYHKoZI +zj0CAQYIKoZIzj0DAQcDQgAEqe0CtnUnyNNcoVGZkNBSsB1eboeMy2469zzh8PJz +vnH6M1o7eEEnX2q9jW2ATzwCy48Zc3V5yMq7+1Ppc5FFEqNTMFEwHQYDVR0OBBYE +FPONAMHT4HJ+QDHWJaGdS37/WcpiMB8GA1UdIwQYMBaAFPONAMHT4HJ+QDHWJaGd +S37/WcpiMA8GA1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0EAwIDSAAwRQIhAKldVHNF +yLWv1AvTy7mTnDuu2oBgpRJfx94Roig36OsEAiBIlHuX4b9YDXQpvn/W8ypZudcZ +GkhXkkl8izmT6ymsOA== +-----END CERTIFICATE----- diff --git a/test-resources/pykmip/run-server.py b/test-resources/pykmip/run-server.py new file mode 100755 index 000000000..295e07789 --- /dev/null +++ b/test-resources/pykmip/run-server.py @@ -0,0 +1,10 @@ +#!/usr/bin/python3 +from kmip.services.server import KmipServer + +server = KmipServer( + config_path='./server.conf', + log_path='./server.log' +) + +with server: + server.serve() diff --git a/test-resources/pykmip/server.conf b/test-resources/pykmip/server.conf new file mode 100644 index 000000000..0bd216394 --- /dev/null +++ b/test-resources/pykmip/server.conf @@ -0,0 +1,11 @@ +[server] +hostname=127.0.0.1 +port=5696 +certificate_path=./server.crt +key_path=./server.key +ca_path=./ca.crt +auth_suite=TLS1.2 +enable_tls_client_auth=False +tls_cipher_suites=TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 +logging_level=DEBUG +database_path=/tmp/pykmip.db diff --git a/test-resources/pykmip/server.crt b/test-resources/pykmip/server.crt new file mode 100644 index 000000000..e1136f9fb --- /dev/null +++ b/test-resources/pykmip/server.crt @@ -0,0 +1,41 @@ +Certificate: + Data: + Version: 3 (0x2) + Serial Number: 1 (0x1) + Signature Algorithm: ecdsa-with-SHA256 + Issuer: C=NL, ST=Noord Holland, L=Amsterdam, O=NLnet Labs, CN=localhost + Validity + Not Before: Sep 28 19:59:26 2021 GMT + Not After : Sep 28 19:59:26 2022 GMT + Subject: C=NL, ST=Noord Holland, O=NLnet Labs, CN=localhost + Subject Public Key Info: + Public Key Algorithm: id-ecPublicKey + Public-Key: (256 bit) + pub: + 04:74:94:a6:97:4b:18:33:71:bb:29:40:75:b0:8a: + 66:ee:75:09:a8:fb:b0:83:71:a7:82:b5:4b:15:76: + d8:bd:2e:16:5c:b3:26:d8:bc:73:9b:fe:10:8b:95: + 4f:41:06:4a:ee:ac:f6:d7:bc:96:a7:ff:ef:39:52: + 0b:ea:25:c7:1d + ASN1 OID: prime256v1 + NIST CURVE: P-256 + X509v3 extensions: + X509v3 Subject Alternative Name: + DNS:localhost + Signature Algorithm: ecdsa-with-SHA256 + 30:45:02:21:00:d8:e6:2e:5b:40:77:dd:3f:1f:9f:5f:79:69: + 63:6c:93:db:c7:84:04:83:21:ed:68:b2:9d:eb:96:00:3e:d9: + db:02:20:50:3d:9a:e1:e3:37:db:94:d5:8d:e6:9f:ba:1f:15: + 18:f3:04:84:96:8e:c4:79:f4:15:90:3e:e5:e5:f0:b6:f9 +-----BEGIN CERTIFICATE----- +MIIBtzCCAV2gAwIBAgIBATAKBggqhkjOPQQDAjBiMQswCQYDVQQGEwJOTDEWMBQG +A1UECAwNTm9vcmQgSG9sbGFuZDESMBAGA1UEBwwJQW1zdGVyZGFtMRMwEQYDVQQK +DApOTG5ldCBMYWJzMRIwEAYDVQQDDAlsb2NhbGhvc3QwHhcNMjEwOTI4MTk1OTI2 +WhcNMjIwOTI4MTk1OTI2WjBOMQswCQYDVQQGEwJOTDEWMBQGA1UECAwNTm9vcmQg +SG9sbGFuZDETMBEGA1UECgwKTkxuZXQgTGFiczESMBAGA1UEAwwJbG9jYWxob3N0 +MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEdJSml0sYM3G7KUB1sIpm7nUJqPuw +g3GngrVLFXbYvS4WXLMm2Lxzm/4Qi5VPQQZK7qz217yWp//vOVIL6iXHHaMYMBYw +FAYDVR0RBA0wC4IJbG9jYWxob3N0MAoGCCqGSM49BAMCA0gAMEUCIQDY5i5bQHfd +Px+fX3lpY2yT28eEBIMh7WiyneuWAD7Z2wIgUD2a4eM325TVjeafuh8VGPMEhJaO +xHn0FZA+5eXwtvk= +-----END CERTIFICATE----- diff --git a/test-resources/pykmip/server.key b/test-resources/pykmip/server.key new file mode 100644 index 000000000..3a9c140cd --- /dev/null +++ b/test-resources/pykmip/server.key @@ -0,0 +1,5 @@ +-----BEGIN PRIVATE KEY----- +MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgt8aDv687Trr5Dsfm +bpJIPM5B9VCGCKyv8XiVIUHZGeehRANCAAR0lKaXSxgzcbspQHWwimbudQmo+7CD +caeCtUsVdti9LhZcsybYvHOb/hCLlU9BBkrurPbXvJan/+85UgvqJccd +-----END PRIVATE KEY----- From b849f662fdbfc93391a431c0dd198ad803e72131 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Tue, 5 Oct 2021 14:48:38 +0200 Subject: [PATCH 04/16] Avoid potential race conditions: Check for expected state, and retain the write lock while switching to using the server as part of finishing a successful probe. --- src/commons/crypto/signers/kmip/internal.rs | 50 +++++++++------------ 1 file changed, 22 insertions(+), 28 deletions(-) diff --git a/src/commons/crypto/signers/kmip/internal.rs b/src/commons/crypto/signers/kmip/internal.rs index 944e89ee9..2d44c103e 100644 --- a/src/commons/crypto/signers/kmip/internal.rs +++ b/src/commons/crypto/signers/kmip/internal.rs @@ -157,14 +157,16 @@ impl ProbingServerConnector { /// Marks now as the last probe attempt timestamp. /// /// Calling this function while not in the Probing state will result in a panic. - pub fn mark(&self) { + pub fn mark(&self) -> Result<(), SignerError> { match self { - ProbingServerConnector::Probing { - mut last_probe_time, .. - } => { + #[rustfmt::skip] + ProbingServerConnector::Probing { mut last_probe_time, .. } => { last_probe_time.replace(Instant::now()); + Ok(()) } - _ => unreachable!(), + _ => Err(SignerError::KmipError( + "Internal error: cannot mark last probe time as probing has already finished.".into(), + )), } } @@ -192,13 +194,6 @@ impl ProbingServerConnector { } } -/// The result of a successful attempt to probe if a KMIP server is usable or not. -#[derive(Clone, Debug)] -struct SuccessfulProbeResult { - identification: String, - supported_operations: Vec, -} - /// The details needed to interact with a usable KMIP server. #[derive(Clone, Debug)] struct UsableServerState { @@ -266,21 +261,25 @@ impl KmipSigner { let status = self.server.read().expect("KMIP status lock is poisoned"); get_server_if_usable(status).unwrap_or_else(|| { self.probe_server() - .and_then(|probe_result| self.use_server(probe_result)) .and_then(|_| Ok(self.server.read().expect("KMIP status lock is poisoned"))) .map_err(|err| SignerError::KmipError(format!("KMIP server is not yet available: {}", err))) }) } /// Verify if the configured KMIP server is contactable and supports the required capabilities. - fn probe_server(&self) -> Result { + fn probe_server(&self) -> Result<(), SignerError> { // Hold a write lock for the duration of our attempt to verify the KMIP server so that no other attempt occurs - // at the same time. - let mut status = self.server.write().expect("KMIP status lock is poisoned"); + // at the same time. Bail out if another thread is performing a probe and has the lock. This is the same result + // as when attempting to use the KMIP server between probe retries. + let mut status = self + .server + .try_write() + .map_err(|_| SignerError::KmipError("KMIP server is not yet available".into()))?; // Update the timestamp of our last attempt to contact the KMIP server. This is used above to know when we have - // waited long enough before attempting to contact the server again. - status.mark(); + // waited long enough before attempting to contact the server again. This also guards against attempts to probe + // when probing has already finished as mark() will fail in that case. + status.mark()?; let conn_settings = status.conn_settings(); debug!("Probing server at {}:{}", conn_settings.host, conn_settings.port); @@ -353,15 +352,10 @@ impl KmipSigner { } } - Ok(SuccessfulProbeResult { - identification: server_properties.vendor_identification.unwrap_or("Unknown".into()), - supported_operations, - }) - } + // Switch from probing the server to using it. + // ------------------------------------------- - /// Switch from probing the server to using it. - fn use_server(&self, probe_result: SuccessfulProbeResult) -> Result<(), SignerError> { - let mut status = self.server.write().expect("KMIP status lock is poisoned"); + let server_identification = server_properties.vendor_identification.unwrap_or("Unknown".into()); // Take the ConnectionSettings out of the Probing status so that we can move it to the Usable status. (we // could clone it but it potentially contains a lot of certificate and key byte data and is about to get @@ -372,10 +366,10 @@ impl KmipSigner { // Success! We can use this server. Announce it and switch our status to KmipSignerStatus::Usable. info!( "Using KMIP server '{}' at {}:{}", - probe_result.identification, conn_settings.host, conn_settings.port + server_identification, conn_settings.host, conn_settings.port ); - let supports_rng_retrieve = probe_result.supported_operations.contains(&Operation::RNGRetrieve); + let supports_rng_retrieve = supported_operations.contains(&Operation::RNGRetrieve); let pool = ConnectionManager::create_connection_pool(conn_settings, MAX_CONCURRENT_SERVER_CONNECTIONS)?; let state = UsableServerState::new(pool, supports_rng_retrieve); From 9b788aed5f074e88333309851546e300047e3224 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Tue, 5 Oct 2021 17:01:38 +0200 Subject: [PATCH 05/16] Always dump the PyKMIP log so that we can see evidence of Krill using PyKMIP. --- .github/workflows/ci.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 47c14ec9a..ca95acd50 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -102,8 +102,7 @@ jobs: cd - cargo test --no-default-features --features hsm,hsm-tests -- --test-threads=1 2>&1 - - name: Dump PyKMIP log on failure - if: failure() + - name: Dump PyKMIP log working-directory: test-resources/pykmip run: | ls -la From 5322233a334d913e46657ecbdea336685504da40 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Tue, 5 Oct 2021 22:55:50 +0200 Subject: [PATCH 06/16] FIX: Signer dispatching must always be routed to the appropriate signer. Previously some Krill logic when invoked was given the same signer as handling the current purpose to invoke later even if for a different purpose. If the initial purpose required the KMIP signer as the key owning signer but the later purpose was one-off signing then that should be able to be routed if desired to the OpenSslSigner, for example. Introduces another layer of indirection: RouterSigner. --- src/commons/crypto/signers/softsigner.rs | 4 +- src/commons/crypto/signing.rs | 324 +++++++++++++---------- 2 files changed, 181 insertions(+), 147 deletions(-) diff --git a/src/commons/crypto/signers/softsigner.rs b/src/commons/crypto/signers/softsigner.rs index 4d25a23bb..de7370707 100644 --- a/src/commons/crypto/signers/softsigner.rs +++ b/src/commons/crypto/signers/softsigner.rs @@ -149,7 +149,7 @@ impl Signer for OpenSslSigner { &self, _algorithm: SignatureAlgorithm, data: &D, - ) -> Result<(Signature, PublicKey), SignerError> { + ) -> Result<(Signature, PublicKey), Self::Error> { let kp = OpenSslKeyPair::build()?; let signature = Self::sign_with_key(kp.pkey.as_ref(), data)?; @@ -159,7 +159,7 @@ impl Signer for OpenSslSigner { Ok((signature, key)) } - fn rand(&self, target: &mut [u8]) -> Result<(), SignerError> { + fn rand(&self, target: &mut [u8]) -> Result<(), Self::Error> { openssl::rand::rand_bytes(target).map_err(SignerError::OpenSslError) } } diff --git a/src/commons/crypto/signing.rs b/src/commons/crypto/signing.rs index 8eacbe034..970e3970e 100644 --- a/src/commons/crypto/signing.rs +++ b/src/commons/crypto/signing.rs @@ -1,6 +1,7 @@ //! Support for signing mft, crl, certificates, roas.. //! Common objects for TAs and CAs use std::{ + ops::Deref, sync::{Arc, RwLock}, {convert::TryFrom, path::Path}, }; @@ -11,7 +12,10 @@ use rpki::{ repository::{ cert::{Cert, KeyUsage, Overclaim, TbsCert}, crl::{Crl, CrlEntry, TbsCertList}, - crypto::{DigestAlgorithm, KeyIdentifier, PublicKey, PublicKeyFormat, Signature, SignatureAlgorithm, Signer}, + crypto::{ + signer::KeyError, DigestAlgorithm, KeyIdentifier, PublicKey, PublicKeyFormat, Signature, + SignatureAlgorithm, Signer, SigningError, + }, csr::Csr, manifest::{FileAndHash, Manifest, ManifestContent}, roa::{Roa, RoaBuilder}, @@ -40,8 +44,11 @@ use crate::{ #[cfg(feature = "hsm")] use crate::commons::crypto::signers::kmip::KmipSigner; -//------------ Signer -------------------------------------------------------- +//------------ SignerProvider ------------------------------------------------ +/// Dispatchers Signer requests to a particular implementation of the Signer trait. +/// +/// Named and modelled after the similar AuthProvider concept that already exists in Krill. #[allow(dead_code)] // Needed as we currently only ever construct one variant #[derive(Clone, Debug)] enum SignerProvider { @@ -51,6 +58,16 @@ enum SignerProvider { Kmip(KmipSigner), } +impl SignerProvider { + pub fn supports_random(&self) -> bool { + match self { + SignerProvider::OpenSsl(signer) => signer.supports_random(), + #[cfg(feature = "hsm")] + SignerProvider::Kmip(signer) => signer.supports_random(), + } + } +} + impl Signer for SignerProvider { type KeyId = KeyIdentifier; @@ -64,10 +81,7 @@ impl Signer for SignerProvider { } } - fn get_key_info( - &self, - key: &Self::KeyId, - ) -> Result> { + fn get_key_info(&self, key: &Self::KeyId) -> Result> { match self { SignerProvider::OpenSsl(signer) => signer.get_key_info(key), #[cfg(feature = "hsm")] @@ -75,10 +89,7 @@ impl Signer for SignerProvider { } } - fn destroy_key( - &mut self, - key: &Self::KeyId, - ) -> Result<(), rpki::repository::crypto::signer::KeyError> { + fn destroy_key(&mut self, key: &Self::KeyId) -> Result<(), KeyError> { match self { SignerProvider::OpenSsl(signer) => signer.destroy_key(key), #[cfg(feature = "hsm")] @@ -91,7 +102,7 @@ impl Signer for SignerProvider { key: &Self::KeyId, algorithm: SignatureAlgorithm, data: &D, - ) -> Result> { + ) -> Result> { match self { SignerProvider::OpenSsl(signer) => signer.sign(key, algorithm, data), #[cfg(feature = "hsm")] @@ -120,87 +131,11 @@ impl Signer for SignerProvider { } } -impl SignerProvider { - pub fn supports_random(&self) -> bool { - match self { - SignerProvider::OpenSsl(signer) => signer.supports_random(), - #[cfg(feature = "hsm")] - SignerProvider::Kmip(signer) => signer.supports_random(), - } - } - - pub fn random_serial(&self) -> CryptoResult { - Serial::random(self).map_err(crypto::Error::signer) - } - - pub fn sign_csr(&self, base_repo: &RepoInfo, name_space: &str, key: &KeyIdentifier) -> CryptoResult { - let pub_key = match self { - SignerProvider::OpenSsl(signer) => signer.get_key_info(key).map_err(crypto::Error::key_error)?, - #[cfg(feature = "hsm")] - SignerProvider::Kmip(signer) => signer.get_key_info(key).map_err(crypto::Error::key_error)?, - }; - - let enc = Csr::construct( - self, - key, - &base_repo.ca_repository(name_space).join(&[]).unwrap(), // force trailing slash - &base_repo.rpki_manifest(name_space, &pub_key.key_identifier()), - Some(&base_repo.rpki_notify()), - ) - .map_err(crypto::Error::signing)?; - - Ok(Csr::decode(enc.as_slice())?) - } - - pub fn sign_cert(&self, tbs: TbsCert, key_id: &KeyIdentifier) -> CryptoResult { - tbs.into_cert(self, key_id).map_err(crypto::Error::signing) - } - - pub fn sign_crl(&self, tbs: TbsCertList>, key_id: &KeyIdentifier) -> CryptoResult { - tbs.into_crl(self, key_id).map_err(crypto::Error::signing) - } - - pub fn sign_manifest( - &self, - content: ManifestContent, - builder: SignedObjectBuilder, - key_id: &KeyIdentifier, - ) -> CryptoResult { - content - .into_manifest(builder, self, key_id) - .map_err(crypto::Error::signing) - } - - pub fn sign_roa( - &self, - roa_builder: RoaBuilder, - object_builder: SignedObjectBuilder, - key_id: &KeyIdentifier, - ) -> CryptoResult { - roa_builder - .finalize(object_builder, self, key_id) - .map_err(crypto::Error::signing) - } - - pub fn sign_rta(&self, rta_builder: &mut rta::RtaBuilder, ee: Cert) -> CryptoResult<()> { - let key = ee.subject_key_identifier(); - rta_builder.push_cert(ee); - rta_builder.sign(self, &key, None, None).map_err(crypto::Error::signing) - } -} +//------------ SignerRouter -------------------------------------------------- +/// Manages multiple Signers and decides which Signer should handle which request. #[derive(Clone, Debug)] -pub struct KrillSigner { - // KrillSigner chooses which signer to use when. The noise of handling the enum based dispatch is handled by the - // SignerProvider type defined above, patterned after the existing AuthProvider enum based approach. - // - // Use Arc references so that we can use refer to the same signer instance more than once if that signer should be - // used for multiple purposes, e.g. as both general_signer and one_off_signer in this case. - // - // Use an RwLock because the Signer trait from the rpki-rs crate uses &mut self for create_key() and destroy_key() - // operations. In future we might move the responsibility for locking into the signer so that it can lock only what - // actually needs to be locked raet - +struct SignerRouter { // The general signer is used for all signing operations except one-off signing. general_signer: Arc>, @@ -213,11 +148,11 @@ pub struct KrillSigner { rand_fallback_signer: Arc>, } -impl KrillSigner { +impl SignerRouter { pub fn build(work_dir: &Path) -> KrillResult { // The types of signer to initialize, the details needed to initialize them and the intended purpose for each // signer (e.g. signer for past keys, currently used signer, signer to use for a key roll, etc.) should come - // from the configuration file. KrillSigner should combine that input its own rules, e.g. to dispatch a signing + // from the configuration file. SignerRouter combines that input with its own rules, e.g. to dispatch a signing // request to the correct signer we will need to determine which signer possesses the signing key, and the // signer to use to create a new key depends on whether the key is one-off or not and whether or not it is // being created for a key roll. For now the capability for different signers for different purposes exists but @@ -236,7 +171,7 @@ impl KrillSigner { #[cfg(not(feature = "hsm"))] { // For backward compatibility with currently released Krill, use OpenSSL for everything. - Ok(KrillSigner { + Ok(SignerRouter { general_signer: openssl_signer.clone(), one_off_signer: openssl_signer.clone(), rand_fallback_signer: openssl_signer, @@ -247,17 +182,8 @@ impl KrillSigner { { // Currently the behaviour is the same with the hsm-tests feature enabled or disabled. This is because with // it disabled the Krill 'functional' test fails with error 'Krill failed to start: Signing issue: Could - // not find key'. - // - // For some reason a long lived signing key is created with the KMIP signer and then that key is used with - // one of the KrillSigner functions that takes a `SignedObjectBuilder` argument. These functions dispatch - // to the configured one-off signer (because ultimately their code paths invoke `sign_one_off` on the - // Signer) which in the commented out configuration below is NOT the KMIP signer but instead is the OpenSSL - // signer. The OpenSSL signer didn't create the key and so cannot find a key with the given KeyIdentifier. - // - // Either this logic is wrong, or this will be solved once we have a mapping of KeyIdentifier to Signer so - // that we ensure that we invoke the signer that created the key. Implementing that mapping is beyond the - // scope of the current task and so this has for now been commented out. + // not find key'. Implementing the mapping of keys to signers is beyond the scope of the current task and + // so this has for now been commented out. compile_error!("The 'hsm' feature can only be used in combination with the 'hsm-tests' feature at present"); unreachable!(); // // When the HSM feature is activated but we are not in test mode: @@ -266,7 +192,7 @@ impl KrillSigner { // // - Use the OpenSSL signer for one-off keys. // let kmip_signer = Arc::new(RwLock::new(SignerProvider::Kmip(KmipSigner::build()?))); - // Ok(KrillSigner { + // Ok(SignerRouter { // general_signer: kmip_signer.clone(), // one_off_signer: openssl_signer.clone(), // rand_fallback_signer: openssl_signer, @@ -280,73 +206,141 @@ impl KrillSigner { // - Fallback to OpenSSL for random number generation if the HSM doesn't support it. let kmip_signer = Arc::new(RwLock::new(SignerProvider::Kmip(KmipSigner::build()?))); - Ok(KrillSigner { + Ok(SignerRouter { general_signer: kmip_signer.clone(), one_off_signer: kmip_signer.clone(), rand_fallback_signer: openssl_signer, }) } } +} + +impl Signer for SignerRouter { + type KeyId = KeyIdentifier; + type Error = SignerError; + + fn create_key(&mut self, algorithm: PublicKeyFormat) -> Result { + self.general_signer.write().unwrap().create_key(algorithm) + } + + fn get_key_info(&self, key_id: &KeyIdentifier) -> Result> { + self.general_signer.read().unwrap().get_key_info(key_id) + } + + fn destroy_key(&mut self, key_id: &KeyIdentifier) -> Result<(), KeyError> { + self.general_signer.write().unwrap().destroy_key(key_id) + } + + fn sign + ?Sized>( + &self, + key_id: &KeyIdentifier, + algorithm: SignatureAlgorithm, + data: &D, + ) -> Result> { + self.general_signer.read().unwrap().sign(key_id, algorithm, data) + } + + fn sign_one_off + ?Sized>( + &self, + algorithm: SignatureAlgorithm, + data: &D, + ) -> Result<(Signature, PublicKey), Self::Error> { + self.one_off_signer.read().unwrap().sign_one_off(algorithm, data) + } + + fn rand(&self, target: &mut [u8]) -> Result<(), Self::Error> { + let signer = self.general_signer.read().unwrap(); + if signer.supports_random() { + signer.rand(target) + } else { + self.rand_fallback_signer.read().unwrap().rand(target) + } + } +} + +//------------ KrillSigner --------------------------------------------------- + +/// High level signing interface between Krill and the Signer backends. +/// +/// KrillSigner: +/// - Is configured via the Krill configuration file. +/// - Maps Result to KrillResult. +/// - Directs signers to use the RPKI standard key format (RSA). +/// - Directs signers to use the RPKI standard signature algorithm (RSA PKCS #1 v1.5 with SHA-256). +/// - Offers a higher level interface than the Signer trait. +#[derive(Clone, Debug)] +pub struct KrillSigner { + dispatcher: Arc>, +} + +impl KrillSigner { + pub fn build(work_dir: &Path) -> KrillResult { + Ok(KrillSigner { + dispatcher: Arc::new(RwLock::new(SignerRouter::build(work_dir)?)), + }) + } pub fn create_key(&self) -> CryptoResult { - self.general_signer - .write() - .unwrap() - .create_key(PublicKeyFormat::Rsa) - .map_err(crate::commons::crypto::error::Error::signer) + let mut signer = self.dispatcher.write().unwrap(); + signer.create_key(PublicKeyFormat::Rsa).map_err(crypto::Error::signer) } pub fn destroy_key(&self, key_id: &KeyIdentifier) -> CryptoResult<()> { - self.general_signer - .write() - .unwrap() - .destroy_key(key_id) - .map_err(crate::commons::crypto::error::Error::signer) + let mut signer = self.dispatcher.write().unwrap(); + signer.destroy_key(key_id).map_err(crypto::Error::key_error) } pub fn get_key_info(&self, key_id: &KeyIdentifier) -> CryptoResult { - self.general_signer + self.dispatcher .read() .unwrap() .get_key_info(key_id) - .map_err(crate::commons::crypto::error::Error::signer) + .map_err(crypto::Error::key_error) } pub fn random_serial(&self) -> CryptoResult { - let signer = self.general_signer.read().unwrap(); - if signer.supports_random() { - signer.random_serial() - } else { - self.rand_fallback_signer.read().unwrap().random_serial() - } + let signer = self.dispatcher.read().unwrap(); + Serial::random(signer.deref()).map_err(crypto::Error::signer) } pub fn sign + ?Sized>(&self, key_id: &KeyIdentifier, data: &D) -> CryptoResult { - self.general_signer + self.dispatcher .read() .unwrap() .sign(key_id, SignatureAlgorithm::default(), data) - .map_err(crate::commons::crypto::error::Error::signer) + .map_err(crypto::Error::signing) } pub fn sign_one_off + ?Sized>(&self, data: &D) -> CryptoResult<(Signature, PublicKey)> { - self.one_off_signer + self.dispatcher .read() .unwrap() .sign_one_off(SignatureAlgorithm::default(), data) - .map_err(crate::commons::crypto::error::Error::signer) + .map_err(crypto::Error::signer) } pub fn sign_csr(&self, base_repo: &RepoInfo, name_space: &str, key: &KeyIdentifier) -> CryptoResult { - self.general_signer.read().unwrap().sign_csr(base_repo, name_space, key) + let signer = self.dispatcher.read().unwrap(); + let pub_key = signer.get_key_info(key).map_err(crypto::Error::key_error)?; + let enc = Csr::construct( + signer.deref(), + key, + &base_repo.ca_repository(name_space).join(&[]).unwrap(), // force trailing slash + &base_repo.rpki_manifest(name_space, &pub_key.key_identifier()), + Some(&base_repo.rpki_notify()), + ) + .map_err(crypto::Error::signing)?; + Ok(Csr::decode(enc.as_slice())?) } pub fn sign_cert(&self, tbs: TbsCert, key_id: &KeyIdentifier) -> CryptoResult { - self.general_signer.read().unwrap().sign_cert(tbs, key_id) + let signer = self.dispatcher.read().unwrap(); + tbs.into_cert(signer.deref(), key_id).map_err(crypto::Error::signing) } pub fn sign_crl(&self, tbs: TbsCertList>, key_id: &KeyIdentifier) -> CryptoResult { - self.general_signer.read().unwrap().sign_crl(tbs, key_id) + let signer = self.dispatcher.read().unwrap(); + tbs.into_crl(signer.deref(), key_id).map_err(crypto::Error::signing) } pub fn sign_manifest( @@ -355,10 +349,10 @@ impl KrillSigner { builder: SignedObjectBuilder, key_id: &KeyIdentifier, ) -> CryptoResult { - self.one_off_signer - .read() - .unwrap() - .sign_manifest(content, builder, key_id) + let signer = self.dispatcher.read().unwrap(); + content + .into_manifest(builder, signer.deref(), key_id) + .map_err(crypto::Error::signing) } pub fn sign_roa( @@ -367,22 +361,22 @@ impl KrillSigner { object_builder: SignedObjectBuilder, key_id: &KeyIdentifier, ) -> CryptoResult { - self.one_off_signer - .read() - .unwrap() - .sign_roa(roa_builder, object_builder, key_id) + let signer = self.dispatcher.read().unwrap(); + roa_builder + .finalize(object_builder, signer.deref(), key_id) + .map_err(crypto::Error::signing) } pub fn sign_rta(&self, rta_builder: &mut rta::RtaBuilder, ee: Cert) -> CryptoResult<()> { - self.one_off_signer.read().unwrap().sign_rta(rta_builder, ee) + let signer = self.dispatcher.read().unwrap(); + let key = ee.subject_key_identifier(); + rta_builder.push_cert(ee); + rta_builder + .sign(signer.deref(), &key, None, None) + .map_err(crypto::Error::signing) } } -// //------------ Signer -------------------------------------------------------- -// -// pub trait Signer: crypto::Signer + Clone + Sized + Sync + Send + 'static {} -// impl + Clone + Sized + Sync + Send + 'static> Signer for T {} - //------------ CsrInfo ------------------------------------------------------- pub type CaRepository = uri::Rsync; @@ -622,3 +616,43 @@ impl ManifestEntry for Crl { self.to_captured().into_bytes() } } + +// // ---- + +// impl From for crypto::error::Error { +// fn from(err: SignerError) -> Self { +// crypto::error::Error::from(err) +// } +// } + +// fn transform_key_error(err: KeyError) -> KeyError { +// match err { +// KeyError::KeyNotFound => crypto::error::Error::KeyNotFound, +// KeyError::Signer(err) => crypto::error::Error::from(err), +// } +// .into() +// } + +// fn transform_signing_error(err: SigningError) -> SigningError { +// match err { +// SigningError::KeyNotFound => crypto::error::Error::KeyNotFound, +// SigningError::IncompatibleKey => crypto::error::Error::KeyError("Incompatible key".into()), +// SigningError::Signer(err) => crypto::error::Error::from(err), +// } +// .into() +// } + +// pub fn unwrap_key_error(err: KeyError) -> crypto::error::Error { +// match err { +// KeyError::KeyNotFound => crypto::error::Error::KeyNotFound, +// KeyError::Signer(err) => crypto::error::Error::from(err), +// } +// } + +// pub fn unwrap_signing_error(err: SigningError) -> crypto::error::Error { +// match err { +// SigningError::KeyNotFound => crypto::error::Error::KeyNotFound, +// SigningError::IncompatibleKey => crypto::error::Error::KeyError("Incompatible key".into()), +// SigningError::Signer(err) => crypto::error::Error::from(err), +// } +// } From 5e359b3235e6cbd0c492d43276cb94e408178047 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Tue, 5 Oct 2021 22:56:12 +0200 Subject: [PATCH 07/16] Additional architecture docs regarding signing. --- doc/development/hsm/architecture.md | 31 ++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/doc/development/hsm/architecture.md b/doc/development/hsm/architecture.md index 548533a77..ef392cabf 100644 --- a/doc/development/hsm/architecture.md +++ b/doc/development/hsm/architecture.md @@ -65,4 +65,33 @@ - TLS is handled by the `openssl` crate as `rustls` may impose stricter limits on outbound connectivity to HSMs than can be pragmatically expected to work in all customer environments. - TBD: Switch to `tokio-native-tls` to keep the benefits of and control of local O/S native TLS providers and avoid the - 'modern security' limitations of `rustls` while switching to an `async` model. \ No newline at end of file + 'modern security' limitations of `rustls` while switching to an `async` model. + +## Control flow + +Prior to the addition of HSM support there was only ever a single Signer and control flow looked like this: + +``` +Krill calling code -> KrillSigner -> OpenSslSigner +``` + +With the addition of HSM support there may be multiple concurrently active Signers and the control flow becomes this: + +``` +Krill calling code -> KrillSigner -> SignerRouter -> SignerProvider -> One of: OpenSslSigner, KmipSigner or Pkcs11Signer. + | | + +--------> KeyMapper <---------+ +``` + +_(note: in the current version of the code there is only an in-memory KeyMap used only by the KmipSigner and only ever + one active Signer impl)_ + +The SignerRouter either: +- Uses the KeyMapper to determine which Signer owns the KeyIdentifier being used, OR +- Uses internal rules to decide which Signer to invoke (e.g. always generate random values and do one-off signing using + the OpenSslSigner for example) + +The Signers: +- Update the KeyMapper when new keys are created to indicate that this Signer owns the key. +- Store KeyIdentifier to HSM internal key identifier mappings in the KeyMapper. +- Retrieve HSM internal key identifiers from the KeyMapper based on the KeyIdentifier being used. \ No newline at end of file From 369d00a16f3cf374dd16d813edeaa68501cb8f5f Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Tue, 5 Oct 2021 22:59:13 +0200 Subject: [PATCH 08/16] Remove commented out code. --- src/commons/crypto/signing.rs | 40 ----------------------------------- 1 file changed, 40 deletions(-) diff --git a/src/commons/crypto/signing.rs b/src/commons/crypto/signing.rs index 970e3970e..7741cbc35 100644 --- a/src/commons/crypto/signing.rs +++ b/src/commons/crypto/signing.rs @@ -616,43 +616,3 @@ impl ManifestEntry for Crl { self.to_captured().into_bytes() } } - -// // ---- - -// impl From for crypto::error::Error { -// fn from(err: SignerError) -> Self { -// crypto::error::Error::from(err) -// } -// } - -// fn transform_key_error(err: KeyError) -> KeyError { -// match err { -// KeyError::KeyNotFound => crypto::error::Error::KeyNotFound, -// KeyError::Signer(err) => crypto::error::Error::from(err), -// } -// .into() -// } - -// fn transform_signing_error(err: SigningError) -> SigningError { -// match err { -// SigningError::KeyNotFound => crypto::error::Error::KeyNotFound, -// SigningError::IncompatibleKey => crypto::error::Error::KeyError("Incompatible key".into()), -// SigningError::Signer(err) => crypto::error::Error::from(err), -// } -// .into() -// } - -// pub fn unwrap_key_error(err: KeyError) -> crypto::error::Error { -// match err { -// KeyError::KeyNotFound => crypto::error::Error::KeyNotFound, -// KeyError::Signer(err) => crypto::error::Error::from(err), -// } -// } - -// pub fn unwrap_signing_error(err: SigningError) -> crypto::error::Error { -// match err { -// SigningError::KeyNotFound => crypto::error::Error::KeyNotFound, -// SigningError::IncompatibleKey => crypto::error::Error::KeyError("Incompatible key".into()), -// SigningError::Signer(err) => crypto::error::Error::from(err), -// } -// } From 280ccc40a000199f0907b3180a9142302e37b981 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Wed, 6 Oct 2021 13:35:43 +0200 Subject: [PATCH 09/16] Remove unnecessary Arc> and naming cleanup. "router", not "dispatcher", and don't lock the entire SignerRouter for create/delete key operations. --- src/commons/crypto/signing.rs | 64 +++++++++++++++++------------------ 1 file changed, 31 insertions(+), 33 deletions(-) diff --git a/src/commons/crypto/signing.rs b/src/commons/crypto/signing.rs index 7741cbc35..395d63c65 100644 --- a/src/commons/crypto/signing.rs +++ b/src/commons/crypto/signing.rs @@ -1,7 +1,6 @@ //! Support for signing mft, crl, certificates, roas.. //! Common objects for TAs and CAs use std::{ - ops::Deref, sync::{Arc, RwLock}, {convert::TryFrom, path::Path}, }; @@ -215,6 +214,20 @@ impl SignerRouter { } } +// Variants of the `Signer` trait functions that take `&mut` arguments and so must be locked to use them, but for which +// we don't want the caller to have to lock the entire `SignerRouter`, only the single `Signer` being used. Ideally the +// `Signer` trait wouldn't use `&mut` at all and rather require the implementation to use the interior mutability +// pattern with as much or as little locking internally at the finest level of granularity possible. +impl SignerRouter { + fn create_key_minimally_locking(&self, algorithm: PublicKeyFormat) -> Result { + self.general_signer.write().unwrap().create_key(algorithm) + } + + fn destroy_key_minimally_locking(&self, key_id: &KeyIdentifier) -> Result<(), KeyError> { + self.general_signer.write().unwrap().destroy_key(key_id) + } +} + impl Signer for SignerRouter { type KeyId = KeyIdentifier; type Error = SignerError; @@ -270,60 +283,50 @@ impl Signer for SignerRouter { /// - Offers a higher level interface than the Signer trait. #[derive(Clone, Debug)] pub struct KrillSigner { - dispatcher: Arc>, + router: SignerRouter, } impl KrillSigner { pub fn build(work_dir: &Path) -> KrillResult { Ok(KrillSigner { - dispatcher: Arc::new(RwLock::new(SignerRouter::build(work_dir)?)), + router: SignerRouter::build(work_dir)?, }) } pub fn create_key(&self) -> CryptoResult { - let mut signer = self.dispatcher.write().unwrap(); - signer.create_key(PublicKeyFormat::Rsa).map_err(crypto::Error::signer) + self.router + .create_key_minimally_locking(PublicKeyFormat::Rsa) + .map_err(crypto::Error::signer) } pub fn destroy_key(&self, key_id: &KeyIdentifier) -> CryptoResult<()> { - let mut signer = self.dispatcher.write().unwrap(); - signer.destroy_key(key_id).map_err(crypto::Error::key_error) + self.router.destroy_key_minimally_locking(key_id).map_err(crypto::Error::key_error) } pub fn get_key_info(&self, key_id: &KeyIdentifier) -> CryptoResult { - self.dispatcher - .read() - .unwrap() - .get_key_info(key_id) - .map_err(crypto::Error::key_error) + self.router.get_key_info(key_id).map_err(crypto::Error::key_error) } pub fn random_serial(&self) -> CryptoResult { - let signer = self.dispatcher.read().unwrap(); - Serial::random(signer.deref()).map_err(crypto::Error::signer) + Serial::random(&self.router).map_err(crypto::Error::signer) } pub fn sign + ?Sized>(&self, key_id: &KeyIdentifier, data: &D) -> CryptoResult { - self.dispatcher - .read() - .unwrap() + self.router .sign(key_id, SignatureAlgorithm::default(), data) .map_err(crypto::Error::signing) } pub fn sign_one_off + ?Sized>(&self, data: &D) -> CryptoResult<(Signature, PublicKey)> { - self.dispatcher - .read() - .unwrap() + self.router .sign_one_off(SignatureAlgorithm::default(), data) .map_err(crypto::Error::signer) } pub fn sign_csr(&self, base_repo: &RepoInfo, name_space: &str, key: &KeyIdentifier) -> CryptoResult { - let signer = self.dispatcher.read().unwrap(); - let pub_key = signer.get_key_info(key).map_err(crypto::Error::key_error)?; + let pub_key = self.router.get_key_info(key).map_err(crypto::Error::key_error)?; let enc = Csr::construct( - signer.deref(), + &self.router, key, &base_repo.ca_repository(name_space).join(&[]).unwrap(), // force trailing slash &base_repo.rpki_manifest(name_space, &pub_key.key_identifier()), @@ -334,13 +337,11 @@ impl KrillSigner { } pub fn sign_cert(&self, tbs: TbsCert, key_id: &KeyIdentifier) -> CryptoResult { - let signer = self.dispatcher.read().unwrap(); - tbs.into_cert(signer.deref(), key_id).map_err(crypto::Error::signing) + tbs.into_cert(&self.router, key_id).map_err(crypto::Error::signing) } pub fn sign_crl(&self, tbs: TbsCertList>, key_id: &KeyIdentifier) -> CryptoResult { - let signer = self.dispatcher.read().unwrap(); - tbs.into_crl(signer.deref(), key_id).map_err(crypto::Error::signing) + tbs.into_crl(&self.router, key_id).map_err(crypto::Error::signing) } pub fn sign_manifest( @@ -349,9 +350,8 @@ impl KrillSigner { builder: SignedObjectBuilder, key_id: &KeyIdentifier, ) -> CryptoResult { - let signer = self.dispatcher.read().unwrap(); content - .into_manifest(builder, signer.deref(), key_id) + .into_manifest(builder, &self.router, key_id) .map_err(crypto::Error::signing) } @@ -361,18 +361,16 @@ impl KrillSigner { object_builder: SignedObjectBuilder, key_id: &KeyIdentifier, ) -> CryptoResult { - let signer = self.dispatcher.read().unwrap(); roa_builder - .finalize(object_builder, signer.deref(), key_id) + .finalize(object_builder, &self.router, key_id) .map_err(crypto::Error::signing) } pub fn sign_rta(&self, rta_builder: &mut rta::RtaBuilder, ee: Cert) -> CryptoResult<()> { - let signer = self.dispatcher.read().unwrap(); let key = ee.subject_key_identifier(); rta_builder.push_cert(ee); rta_builder - .sign(signer.deref(), &key, None, None) + .sign(&self.router, &key, None, None) .map_err(crypto::Error::signing) } } From 3e6c0cb35090b5631c22dce438671b8bb2938457 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Wed, 3 Nov 2021 11:28:27 +0100 Subject: [PATCH 10/16] Apply crate upgrade and signer 'de-mut' changes that were made in successor PR #688 which is now redundant because those changes are now present in the 'dev' branch which this PR targets. --- Cargo.lock | 10 +++++---- Cargo.toml | 8 ++----- src/commons/api/ca.rs | 2 +- src/commons/crypto/signers/kmip/signer.rs | 4 ++-- src/commons/crypto/signers/softsigner.rs | 6 +++--- src/commons/crypto/signing.rs | 26 ++++++----------------- 6 files changed, 20 insertions(+), 36 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 07d2a4151..e722ad608 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -133,8 +133,9 @@ dependencies = [ [[package]] name = "bcder" -version = "0.6.1-dev" -source = "git+https://github.com/NLnetLabs/bcder#0e0ef26d23afe18603768b7f6757df997b87568b" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "52e7c3bae57b01ec5b1b028af1f77b7d41dd2bd97a41bab7968739b5329c0e39" dependencies = [ "backtrace", "bytes", @@ -1628,8 +1629,9 @@ dependencies = [ [[package]] name = "rpki" -version = "0.12.3" -source = "git+https://github.com/ximon18/rpki-rs?branch=0.12.3-unsigned-from-slice#643e9e14dcf19de136e280989ae96c00144f5be5" +version = "0.13.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3ee4c0626a16c808bb6b557298def266e8cd1078515b5ea07b382f2a0b75375c" dependencies = [ "base64 0.13.0", "bcder", diff --git a/Cargo.toml b/Cargo.toml index eff93bc47..0f2a58bad 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -24,7 +24,7 @@ build = "build.rs" backoff = { version = "0.3.0", optional = true } base64 = "^0.13" basic-cookies = { version = "^0.1", optional = true } -bcder = "0.6.1-dev" +bcder = "0.6.1" bytes = "1" chrono = { version = "^0.4", features = ["serde"] } clap = "^2.33" @@ -47,7 +47,7 @@ rand = "^0.8" regex = { version = "^1.4", optional = true, default_features = false, features = ["std"] } reqwest = { version = "0.11", features = ["json"] } rpassword = { version = "^5.0", optional = true } -rpki = { version = "0.12.3", features = [ "repository", "rrdp", "serde" ] } +rpki = { version = "0.13.0", features = [ "repository", "rrdp", "serde" ] } scrypt = { version = "^0.6", optional = true, default-features = false } serde = { version = "^1.0", features = ["derive"] } serde_json = "^1.0" @@ -185,7 +185,3 @@ shadow-utils = "*" # END RPM PACKAGING # ------------------------------------------------------------------------------ - -[patch.crates-io] -bcder = { git = 'https://github.com/NLnetLabs/bcder' } -rpki = { git = 'https://github.com/ximon18/rpki-rs', branch = '0.12.3-unsigned-from-slice' } \ No newline at end of file diff --git a/src/commons/api/ca.rs b/src/commons/api/ca.rs index d563ddb82..8b7a34fa4 100644 --- a/src/commons/api/ca.rs +++ b/src/commons/api/ca.rs @@ -2587,7 +2587,7 @@ mod test { #[test] fn mft_uri() { test::test_under_tmp(|d| { - let mut signer = OpenSslSigner::build(&d).unwrap(); + let signer = OpenSslSigner::build(&d).unwrap(); let key_id = signer.create_key(PublicKeyFormat::Rsa).unwrap(); let pub_key = signer.get_key_info(&key_id).unwrap(); diff --git a/src/commons/crypto/signers/kmip/signer.rs b/src/commons/crypto/signers/kmip/signer.rs index 9db0ef419..3ae87072a 100644 --- a/src/commons/crypto/signers/kmip/signer.rs +++ b/src/commons/crypto/signers/kmip/signer.rs @@ -11,7 +11,7 @@ impl Signer for KmipSigner { type KeyId = KeyIdentifier; type Error = SignerError; - fn create_key(&mut self, algorithm: PublicKeyFormat) -> Result { + fn create_key(&self, algorithm: PublicKeyFormat) -> Result { let (key, kmip_key_pair_ids) = self.build_key(algorithm)?; let key_id = key.key_identifier(); self.remember_kmip_key_ids(&key_id, kmip_key_pair_ids)?; @@ -24,7 +24,7 @@ impl Signer for KmipSigner { .map_err(|err| KeyError::Signer(err)) } - fn destroy_key(&mut self, key_id: &Self::KeyId) -> Result<(), KeyError> { + fn destroy_key(&self, key_id: &Self::KeyId) -> Result<(), KeyError> { let kmip_key_pair_ids = self.lookup_kmip_key_ids(key_id)?; match self.destroy_key_pair(&kmip_key_pair_ids, KeyStatus::Active)? { true => Ok(()), diff --git a/src/commons/crypto/signers/softsigner.rs b/src/commons/crypto/signers/softsigner.rs index de7370707..36a7ff439 100644 --- a/src/commons/crypto/signers/softsigner.rs +++ b/src/commons/crypto/signers/softsigner.rs @@ -100,7 +100,7 @@ impl Signer for OpenSslSigner { type KeyId = KeyIdentifier; type Error = SignerError; - fn create_key(&mut self, _algorithm: PublicKeyFormat) -> Result { + fn create_key(&self, _algorithm: PublicKeyFormat) -> Result { let kp = OpenSslKeyPair::build()?; let pk = &kp.subject_public_key_info()?; @@ -122,7 +122,7 @@ impl Signer for OpenSslSigner { Ok(key_pair.subject_public_key_info()?) } - fn destroy_key(&mut self, key_id: &Self::KeyId) -> Result<(), KeyError> { + fn destroy_key(&self, key_id: &Self::KeyId) -> Result<(), KeyError> { let path = self.key_path(key_id); if path.exists() { fs::remove_file(&path).map_err(|e| { @@ -228,7 +228,7 @@ pub mod tests { #[test] fn should_return_subject_public_key_info() { test::test_under_tmp(|d| { - let mut s = OpenSslSigner::build(&d).unwrap(); + let s = OpenSslSigner::build(&d).unwrap(); let ki = s.create_key(PublicKeyFormat::Rsa).unwrap(); s.get_key_info(&ki).unwrap(); s.destroy_key(&ki).unwrap(); diff --git a/src/commons/crypto/signing.rs b/src/commons/crypto/signing.rs index 395d63c65..e6c41d766 100644 --- a/src/commons/crypto/signing.rs +++ b/src/commons/crypto/signing.rs @@ -72,7 +72,7 @@ impl Signer for SignerProvider { type Error = SignerError; - fn create_key(&mut self, algorithm: PublicKeyFormat) -> Result { + fn create_key(&self, algorithm: PublicKeyFormat) -> Result { match self { SignerProvider::OpenSsl(signer) => signer.create_key(algorithm), #[cfg(feature = "hsm")] @@ -88,7 +88,7 @@ impl Signer for SignerProvider { } } - fn destroy_key(&mut self, key: &Self::KeyId) -> Result<(), KeyError> { + fn destroy_key(&self, key: &Self::KeyId) -> Result<(), KeyError> { match self { SignerProvider::OpenSsl(signer) => signer.destroy_key(key), #[cfg(feature = "hsm")] @@ -214,25 +214,11 @@ impl SignerRouter { } } -// Variants of the `Signer` trait functions that take `&mut` arguments and so must be locked to use them, but for which -// we don't want the caller to have to lock the entire `SignerRouter`, only the single `Signer` being used. Ideally the -// `Signer` trait wouldn't use `&mut` at all and rather require the implementation to use the interior mutability -// pattern with as much or as little locking internally at the finest level of granularity possible. -impl SignerRouter { - fn create_key_minimally_locking(&self, algorithm: PublicKeyFormat) -> Result { - self.general_signer.write().unwrap().create_key(algorithm) - } - - fn destroy_key_minimally_locking(&self, key_id: &KeyIdentifier) -> Result<(), KeyError> { - self.general_signer.write().unwrap().destroy_key(key_id) - } -} - impl Signer for SignerRouter { type KeyId = KeyIdentifier; type Error = SignerError; - fn create_key(&mut self, algorithm: PublicKeyFormat) -> Result { + fn create_key(&self, algorithm: PublicKeyFormat) -> Result { self.general_signer.write().unwrap().create_key(algorithm) } @@ -240,7 +226,7 @@ impl Signer for SignerRouter { self.general_signer.read().unwrap().get_key_info(key_id) } - fn destroy_key(&mut self, key_id: &KeyIdentifier) -> Result<(), KeyError> { + fn destroy_key(&self, key_id: &KeyIdentifier) -> Result<(), KeyError> { self.general_signer.write().unwrap().destroy_key(key_id) } @@ -295,12 +281,12 @@ impl KrillSigner { pub fn create_key(&self) -> CryptoResult { self.router - .create_key_minimally_locking(PublicKeyFormat::Rsa) + .create_key(PublicKeyFormat::Rsa) .map_err(crypto::Error::signer) } pub fn destroy_key(&self, key_id: &KeyIdentifier) -> CryptoResult<()> { - self.router.destroy_key_minimally_locking(key_id).map_err(crypto::Error::key_error) + self.router.destroy_key(key_id).map_err(crypto::Error::key_error) } pub fn get_key_info(&self, key_id: &KeyIdentifier) -> CryptoResult { From 00e93ce7db6b5ed9fa075d534af7e1b96c4dd732 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Wed, 3 Nov 2021 11:32:46 +0100 Subject: [PATCH 11/16] Merge fix. --- src/commons/crypto/signing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/commons/crypto/signing.rs b/src/commons/crypto/signing.rs index 889f0e4c4..4a9030469 100644 --- a/src/commons/crypto/signing.rs +++ b/src/commons/crypto/signing.rs @@ -359,7 +359,7 @@ impl KrillSigner { object_builder: SignedObjectBuilder, key_id: &KeyIdentifier, ) -> CryptoResult { - self.aspa_builder + aspa_builder .finalize(object_builder, &self.router, key_id) .map_err(crypto::Error::signing) } From e2177174d2297fb5f5a48dafb409071fac533aa3 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Tue, 9 Nov 2021 13:00:47 +0100 Subject: [PATCH 12/16] Wait longer to "fix" brittle test when using a slower signer (PyKMIP in this case). --- tests/suspend.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suspend.rs b/tests/suspend.rs index 102eeb4f2..4d126e974 100644 --- a/tests/suspend.rs +++ b/tests/suspend.rs @@ -71,7 +71,7 @@ async fn test_suspension() { // Wait a bit, and then refresh testbed only, it should find that // the child 'CA' has not been updating, and will suspend it. { - sleep_seconds(5).await; + sleep_seconds(15).await; cas_refresh_single(&testbed).await; expect_suspended(&testbed, &ca).await; From 7feffe1471e4b3374ec939401755f5dc34a2d6a4 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Sun, 14 Nov 2021 23:19:26 +0100 Subject: [PATCH 13/16] Review feedback: KmipSigner doesn't needt o implement the Signer trait. --- src/commons/crypto/signers/kmip/internal.rs | 9 +++----- src/commons/crypto/signers/kmip/signer.rs | 25 +++++++++------------ 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/src/commons/crypto/signers/kmip/internal.rs b/src/commons/crypto/signers/kmip/internal.rs index 2d44c103e..35a99936a 100644 --- a/src/commons/crypto/signers/kmip/internal.rs +++ b/src/commons/crypto/signers/kmip/internal.rs @@ -18,7 +18,7 @@ use kmip::{ use openssl::ssl::SslStream; use r2d2::PooledConnection; use rpki::repository::crypto::{ - signer::KeyError, KeyIdentifier, PublicKey, PublicKeyFormat, Signature, SignatureAlgorithm, Signer, + signer::KeyError, KeyIdentifier, PublicKey, PublicKeyFormat, Signature, SignatureAlgorithm, }; use crate::commons::{ @@ -459,10 +459,7 @@ impl KmipSigner { } /// Given a KeyIdentifier lookup the corresponding KMIP public and private key pair IDs. - pub fn lookup_kmip_key_ids( - &self, - key_id: &KeyIdentifier, - ) -> Result::Error>> { + pub fn lookup_kmip_key_ids(&self, key_id: &KeyIdentifier) -> Result> { Ok(self .server()? .state() @@ -722,7 +719,7 @@ impl KmipSigner { Ok(success) } - pub fn get_random_bytes(&self, num_bytes_wanted: usize) -> Result, ::Error> { + pub fn get_random_bytes(&self, num_bytes_wanted: usize) -> Result, SignerError> { if !self.supports_random() { return Err(SignerError::KmipError( "The KMIP server does not support random number generation".to_string(), diff --git a/src/commons/crypto/signers/kmip/signer.rs b/src/commons/crypto/signers/kmip/signer.rs index 3ae87072a..653879aad 100644 --- a/src/commons/crypto/signers/kmip/signer.rs +++ b/src/commons/crypto/signers/kmip/signer.rs @@ -1,5 +1,5 @@ use rpki::repository::crypto::{ - signer::KeyError, KeyIdentifier, PublicKey, PublicKeyFormat, Signature, SignatureAlgorithm, Signer, SigningError, + signer::KeyError, KeyIdentifier, PublicKey, PublicKeyFormat, Signature, SignatureAlgorithm, SigningError, }; use crate::commons::crypto::signers::{ @@ -7,24 +7,21 @@ use crate::commons::crypto::signers::{ kmip::{internal::KeyStatus, KmipSigner}, }; -impl Signer for KmipSigner { - type KeyId = KeyIdentifier; - type Error = SignerError; - - fn create_key(&self, algorithm: PublicKeyFormat) -> Result { +impl KmipSigner { + pub fn create_key(&self, algorithm: PublicKeyFormat) -> Result { let (key, kmip_key_pair_ids) = self.build_key(algorithm)?; let key_id = key.key_identifier(); self.remember_kmip_key_ids(&key_id, kmip_key_pair_ids)?; Ok(key_id) } - fn get_key_info(&self, key_id: &Self::KeyId) -> Result> { + pub fn get_key_info(&self, key_id: &KeyIdentifier) -> Result> { let kmip_key_pair_ids = self.lookup_kmip_key_ids(key_id)?; self.get_public_key_from_id(&kmip_key_pair_ids.public_key_id) .map_err(|err| KeyError::Signer(err)) } - fn destroy_key(&self, key_id: &Self::KeyId) -> Result<(), KeyError> { + pub fn destroy_key(&self, key_id: &KeyIdentifier) -> Result<(), KeyError> { let kmip_key_pair_ids = self.lookup_kmip_key_ids(key_id)?; match self.destroy_key_pair(&kmip_key_pair_ids, KeyStatus::Active)? { true => Ok(()), @@ -35,12 +32,12 @@ impl Signer for KmipSigner { } } - fn sign + ?Sized>( + pub fn sign + ?Sized>( &self, - key_id: &Self::KeyId, + key_id: &KeyIdentifier, algorithm: SignatureAlgorithm, data: &D, - ) -> Result> { + ) -> Result> { let kmip_key_pair_ids = self.lookup_kmip_key_ids(key_id)?; let signature = self @@ -55,11 +52,11 @@ impl Signer for KmipSigner { Ok(signature) } - fn sign_one_off + ?Sized>( + pub fn sign_one_off + ?Sized>( &self, algorithm: SignatureAlgorithm, data: &D, - ) -> Result<(Signature, PublicKey), Self::Error> { + ) -> Result<(Signature, PublicKey), SignerError> { // TODO: Is it possible to use a KMIP batch request to implement the create, activate, sign, deactivate, delete // in one round-trip to the server? let (key, kmip_key_pair_ids) = self.build_key(PublicKeyFormat::Rsa)?; @@ -75,7 +72,7 @@ impl Signer for KmipSigner { Ok((signature, key)) } - fn rand(&self, data: &mut [u8]) -> Result<(), Self::Error> { + pub fn rand(&self, data: &mut [u8]) -> Result<(), SignerError> { let random_bytes = self.get_random_bytes(data.len())?; data.copy_from_slice(&random_bytes); From 40bafc312259df2fdce9f61a2fa6769afe33853a Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Mon, 15 Nov 2021 10:44:53 +0100 Subject: [PATCH 14/16] Review feedback: SignerProvider doesn't need to implement the Signer trait. --- src/commons/crypto/signers/kmip/signer.rs | 2 ++ src/commons/crypto/signing.rs | 34 +++++++++++------------ 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/commons/crypto/signers/kmip/signer.rs b/src/commons/crypto/signers/kmip/signer.rs index 653879aad..4f3570280 100644 --- a/src/commons/crypto/signers/kmip/signer.rs +++ b/src/commons/crypto/signers/kmip/signer.rs @@ -7,6 +7,8 @@ use crate::commons::crypto::signers::{ kmip::{internal::KeyStatus, KmipSigner}, }; +// Implement the functions defined by the `Signer` trait because `SignerProvider` expects to invoke them, but as the +// dispatching is not trait based we don't actually have to implement the `Signer` trait. impl KmipSigner { pub fn create_key(&self, algorithm: PublicKeyFormat) -> Result { let (key, kmip_key_pair_ids) = self.build_key(algorithm)?; diff --git a/src/commons/crypto/signing.rs b/src/commons/crypto/signing.rs index 4a9030469..0a3ad020d 100644 --- a/src/commons/crypto/signing.rs +++ b/src/commons/crypto/signing.rs @@ -68,12 +68,10 @@ impl SignerProvider { } } -impl Signer for SignerProvider { - type KeyId = KeyIdentifier; - - type Error = SignerError; - - fn create_key(&self, algorithm: PublicKeyFormat) -> Result { +// Implement the functions defined by the `Signer` trait because `SignerRouter` expects to invoke them, but as the +// dispatching is not trait based we don't actually have to implement the `Signer` trait. +impl SignerProvider { + fn create_key(&self, algorithm: PublicKeyFormat) -> Result { match self { SignerProvider::OpenSsl(signer) => signer.create_key(algorithm), #[cfg(feature = "hsm")] @@ -81,7 +79,7 @@ impl Signer for SignerProvider { } } - fn get_key_info(&self, key: &Self::KeyId) -> Result> { + fn get_key_info(&self, key: &KeyIdentifier) -> Result> { match self { SignerProvider::OpenSsl(signer) => signer.get_key_info(key), #[cfg(feature = "hsm")] @@ -89,7 +87,7 @@ impl Signer for SignerProvider { } } - fn destroy_key(&self, key: &Self::KeyId) -> Result<(), KeyError> { + fn destroy_key(&self, key: &KeyIdentifier) -> Result<(), KeyError> { match self { SignerProvider::OpenSsl(signer) => signer.destroy_key(key), #[cfg(feature = "hsm")] @@ -99,10 +97,10 @@ impl Signer for SignerProvider { fn sign + ?Sized>( &self, - key: &Self::KeyId, + key: &KeyIdentifier, algorithm: SignatureAlgorithm, data: &D, - ) -> Result> { + ) -> Result> { match self { SignerProvider::OpenSsl(signer) => signer.sign(key, algorithm, data), #[cfg(feature = "hsm")] @@ -114,7 +112,7 @@ impl Signer for SignerProvider { &self, algorithm: SignatureAlgorithm, data: &D, - ) -> Result<(Signature, PublicKey), Self::Error> { + ) -> Result<(Signature, PublicKey), SignerError> { match self { SignerProvider::OpenSsl(signer) => signer.sign_one_off(algorithm, data), #[cfg(feature = "hsm")] @@ -122,7 +120,7 @@ impl Signer for SignerProvider { } } - fn rand(&self, target: &mut [u8]) -> Result<(), Self::Error> { + fn rand(&self, target: &mut [u8]) -> Result<(), SignerError> { match self { SignerProvider::OpenSsl(signer) => signer.rand(target), #[cfg(feature = "hsm")] @@ -219,15 +217,15 @@ impl Signer for SignerRouter { type KeyId = KeyIdentifier; type Error = SignerError; - fn create_key(&self, algorithm: PublicKeyFormat) -> Result { + fn create_key(&self, algorithm: PublicKeyFormat) -> Result { self.general_signer.write().unwrap().create_key(algorithm) } - fn get_key_info(&self, key_id: &KeyIdentifier) -> Result> { + fn get_key_info(&self, key_id: &KeyIdentifier) -> Result> { self.general_signer.read().unwrap().get_key_info(key_id) } - fn destroy_key(&self, key_id: &KeyIdentifier) -> Result<(), KeyError> { + fn destroy_key(&self, key_id: &KeyIdentifier) -> Result<(), KeyError> { self.general_signer.write().unwrap().destroy_key(key_id) } @@ -236,7 +234,7 @@ impl Signer for SignerRouter { key_id: &KeyIdentifier, algorithm: SignatureAlgorithm, data: &D, - ) -> Result> { + ) -> Result> { self.general_signer.read().unwrap().sign(key_id, algorithm, data) } @@ -244,11 +242,11 @@ impl Signer for SignerRouter { &self, algorithm: SignatureAlgorithm, data: &D, - ) -> Result<(Signature, PublicKey), Self::Error> { + ) -> Result<(Signature, PublicKey), SignerError> { self.one_off_signer.read().unwrap().sign_one_off(algorithm, data) } - fn rand(&self, target: &mut [u8]) -> Result<(), Self::Error> { + fn rand(&self, target: &mut [u8]) -> Result<(), SignerError> { let signer = self.general_signer.read().unwrap(); if signer.supports_random() { signer.rand(target) From 8e9cda8df766aaa5192fcfa927e4beefa0d94146 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Mon, 15 Nov 2021 11:18:35 +0100 Subject: [PATCH 15/16] OpenSslSigner no longer needs to implement the Signer trait. --- src/commons/api/ca.rs | 2 +- src/commons/crypto/signers/softsigner.rs | 27 ++++++++++++------------ 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/commons/api/ca.rs b/src/commons/api/ca.rs index e91d96097..c962afd8b 100644 --- a/src/commons/api/ca.rs +++ b/src/commons/api/ca.rs @@ -2665,7 +2665,7 @@ impl ResourceSetError { mod test { use bytes::Bytes; - use rpki::repository::crypto::{signer::Signer, PublicKeyFormat}; + use rpki::repository::crypto::PublicKeyFormat; use crate::commons::crypto::signers::softsigner::OpenSslSigner; diff --git a/src/commons/crypto/signers/softsigner.rs b/src/commons/crypto/signers/softsigner.rs index 36a7ff439..64c505207 100644 --- a/src/commons/crypto/signers/softsigner.rs +++ b/src/commons/crypto/signers/softsigner.rs @@ -18,7 +18,7 @@ use openssl::{ }; use rpki::repository::crypto::{ - signer::KeyError, KeyIdentifier, PublicKey, PublicKeyFormat, Signature, SignatureAlgorithm, Signer, SigningError, + signer::KeyError, KeyIdentifier, PublicKey, PublicKeyFormat, Signature, SignatureAlgorithm, SigningError, }; use crate::commons::{crypto::signers::error::SignerError, error::KrillIoError}; @@ -96,11 +96,10 @@ impl OpenSslSigner { } } -impl Signer for OpenSslSigner { - type KeyId = KeyIdentifier; - type Error = SignerError; - - fn create_key(&self, _algorithm: PublicKeyFormat) -> Result { +// Implement the functions defined by the `Signer` trait because `SignerProvider` expects to invoke them, but as the +// dispatching is not trait based we don't actually have to implement the `Signer` trait. +impl OpenSslSigner { + pub fn create_key(&self, _algorithm: PublicKeyFormat) -> Result { let kp = OpenSslKeyPair::build()?; let pk = &kp.subject_public_key_info()?; @@ -117,12 +116,12 @@ impl Signer for OpenSslSigner { Ok(key_id) } - fn get_key_info(&self, key_id: &Self::KeyId) -> Result> { + pub fn get_key_info(&self, key_id: &KeyIdentifier) -> Result> { let key_pair = self.load_key(key_id)?; Ok(key_pair.subject_public_key_info()?) } - fn destroy_key(&self, key_id: &Self::KeyId) -> Result<(), KeyError> { + pub fn destroy_key(&self, key_id: &KeyIdentifier) -> Result<(), KeyError> { let path = self.key_path(key_id); if path.exists() { fs::remove_file(&path).map_err(|e| { @@ -135,21 +134,21 @@ impl Signer for OpenSslSigner { Ok(()) } - fn sign + ?Sized>( + pub fn sign + ?Sized>( &self, - key_id: &Self::KeyId, + key_id: &KeyIdentifier, _algorithm: SignatureAlgorithm, data: &D, - ) -> Result> { + ) -> Result> { let key_pair = self.load_key(key_id)?; Self::sign_with_key(key_pair.pkey.as_ref(), data).map_err(SigningError::Signer) } - fn sign_one_off + ?Sized>( + pub fn sign_one_off + ?Sized>( &self, _algorithm: SignatureAlgorithm, data: &D, - ) -> Result<(Signature, PublicKey), Self::Error> { + ) -> Result<(Signature, PublicKey), SignerError> { let kp = OpenSslKeyPair::build()?; let signature = Self::sign_with_key(kp.pkey.as_ref(), data)?; @@ -159,7 +158,7 @@ impl Signer for OpenSslSigner { Ok((signature, key)) } - fn rand(&self, target: &mut [u8]) -> Result<(), Self::Error> { + pub fn rand(&self, target: &mut [u8]) -> Result<(), SignerError> { openssl::rand::rand_bytes(target).map_err(SignerError::OpenSslError) } } From 0782791a13a50769f0aca6430f4bc66b76aa6b17 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Fri, 19 Nov 2021 23:17:04 +0100 Subject: [PATCH 16/16] Review feedback: Spend more words on why KMIP keys are renamed after creation. --- src/commons/crypto/signers/kmip/internal.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/commons/crypto/signers/kmip/internal.rs b/src/commons/crypto/signers/kmip/internal.rs index 35a99936a..25a5785e7 100644 --- a/src/commons/crypto/signers/kmip/internal.rs +++ b/src/commons/crypto/signers/kmip/internal.rs @@ -530,6 +530,14 @@ impl KmipSigner { } /// Make the given KMIP private and public key pair ready for use by Krill. + /// + /// Note that this function renames the created keys but this is not needed for correct functioning of Krill, it + /// is rather done to aid the KMIP server operator when administering the HSM. + /// + /// It also activates the private key. Without this the key cannot be used for signing. An alternate approach could + /// be to set the activation date of the key when creating it thereby avoiding the extra activation step, or to + /// perform the activation operation as part of a bulk request also containing the create key operation, thereby + /// reducing the number of round trips to the server. fn prepare_keypair_for_use(&self, private_key_id: &str, public_key_id: &str) -> Result { // Create a public key object for the public key let public_key = self.get_public_key_from_id(&public_key_id)?;