Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Aptos HSM Signer Testing #974

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/checks-all.yml
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,8 @@ jobs:
-p memseq \
-p move-rocks \
-p movement-types \
-p movement-config
-p movement-config \
-p movement-signer-test
EOF

movement-full-node-local:
Expand Down
20 changes: 19 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
resolver = "2"

members = [
# FIXME: do we still need the demo?
"demo/hsm",
"protocol-units/bridge/config",
"protocol-units/bridge/setup",
"protocol-units/execution/maptos/dof",
Expand Down Expand Up @@ -45,7 +47,7 @@ members = [
"util/signing/integrations/aptos",
"util/signing/providers/aws-kms",
"util/signing/providers/hashicorp-vault",
"demo/hsm"
"util/signing/testing",
]

[workspace.package]
Expand Down Expand Up @@ -122,6 +124,7 @@ aptos-account-whitelist = { path = "protocol-units/access-control/aptos/account-
movement-signer = { path = "util/signing/interface" }
movement-signer-aws-kms = { path = "util/signing/providers/aws-kms" }
movement-signer-hashicorp-vault = { path = "util/signing/providers/hashicorp-vault" }
movement-signing-aptos = { path = "util/signing/integrations/aptos" }

## vault
vaultrs = { version = "0.7.3" }
Expand Down Expand Up @@ -267,7 +270,6 @@ proptest = { version = "1.3.1", default-features = false, features = ["alloc"] }
proptest-derive = "0.4"
quote = "1.0"
rand = "0.7.3"
rand_core = "0.5.1"
rayon = "1.10.0"
reqwest = "0.12.4"
risc0-build = "0.20"
Expand Down
1 change: 0 additions & 1 deletion protocol-units/execution/maptos/opt-executor/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ lazy_static = "1.4.0"
tokio = { workspace = true }
tracing = { workspace = true }
rand = { workspace = true }
rand_core = { workspace = true }
bcs = { workspace = true }
futures = { workspace = true }

Expand Down
36 changes: 35 additions & 1 deletion util/signing/interface/src/cryptography/ed25519.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::cryptography::Curve;
use crate::cryptography::{CryptoMaterialError, Curve};
use crate::{Verify, VerifyError};
use anyhow::Context;
use ed25519_dalek::Verifier as _;
Expand Down Expand Up @@ -32,3 +32,37 @@ impl Verify<Ed25519> for Ed25519 {
Ok(verifying_key.verify(message, &signature).is_ok())
}
}

impl TryFrom<[u8; 32]> for PublicKey {
type Error = CryptoMaterialError;

#[inline]
fn try_from(bytes: [u8; 32]) -> Result<Self, Self::Error> {
PublicKey::try_from(&bytes)
}
}

impl TryFrom<&[u8; 32]> for PublicKey {
type Error = CryptoMaterialError;

fn try_from(bytes: &[u8; 32]) -> Result<Self, Self::Error> {
let key = ed25519_dalek::VerifyingKey::from_bytes(bytes)
.map_err(|e| CryptoMaterialError(e.into()))?;
Ok(PublicKey(key.to_bytes()))
}
}

// Following the example given by ed25519, we allow any 64 bytes to construct a
// (possibly invalid) signature.
impl From<[u8; 64]> for Signature {
#[inline]
fn from(bytes: [u8; 64]) -> Self {
Signature(bytes)
}
}
impl From<&[u8; 64]> for Signature {
#[inline]
fn from(bytes: &[u8; 64]) -> Self {
Signature(*bytes)
}
}
21 changes: 14 additions & 7 deletions util/signing/interface/src/cryptography/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,14 @@ macro_rules! fixed_size {
}
}

impl crate::cryptography::TryFromBytes for $Name {
fn try_from_bytes(bytes: &[u8]) -> Result<Self, anyhow::Error> {
impl TryFrom<&[u8]> for $Name {
type Error = crate::cryptography::CryptoMaterialError;

fn try_from(bytes: &[u8]) -> Result<Self, Self::Error> {
use crate::cryptography::CryptoMaterialError;

if bytes.len() != Self::BYTES_LEN {
Err(anyhow::anyhow!("invalid length"))?;
Err(CryptoMaterialError("invalid length".into()))?;
}

let mut inner = [0u8; Self::BYTES_LEN];
Expand All @@ -29,15 +33,18 @@ macro_rules! fixed_size {
pub mod ed25519;
pub mod secp256k1;

pub trait TryFromBytes: Sized {
fn try_from_bytes(bytes: &[u8]) -> Result<Self, anyhow::Error>;
}
use std::error::Error;

/// A designator for an elliptic curve.
///
/// This trait has no methods, but it binds the types of the public key and
/// the signature used by the EC digital signing algorithm.
pub trait Curve {
type PublicKey: TryFromBytes;
type PublicKey;
type Signature;
}

/// Errors that occur when parsing signature or key material from byte sequences.
#[derive(Debug, thiserror::Error)]
#[error(transparent)]
pub struct CryptoMaterialError(Box<dyn Error + Send + Sync>);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will be easier to have one error type for the crate. It avoids adding map_err every time when you need to extract the publickey from an u8 array, for example, and it eases the error management for external crate.
There's a reason you didn't add a new variant to the SignerError. Perhaps we need to rename it in a more generic name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like using the "fat" main error type for the much slimmer use case of TryFrom conversions.

71 changes: 12 additions & 59 deletions util/signing/providers/hashicorp-vault/src/hsm/mod.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
use crate::cryptography::HashiCorpVaultCryptography;
use anyhow::Context;
use movement_signer::cryptography::TryFromBytes;
use movement_signer::{
cryptography::{ed25519::Ed25519, Curve},
cryptography::{
ed25519::{self, Ed25519},
Curve,
},
SignerError, Signing,
};
use vaultrs::api::transit::{requests::CreateKeyRequest, responses::ReadKeyData};
Expand All @@ -14,7 +16,6 @@ pub struct HashiCorpVault<C: Curve + HashiCorpVaultCryptography> {
client: VaultClient,
key_name: String,
mount_name: String,
pub public_key: <C as Curve>::PublicKey,
_cryptography_marker: std::marker::PhantomData<C>,
}

Expand All @@ -23,19 +24,8 @@ where
C: Curve + HashiCorpVaultCryptography,
{
/// Creates a new HashiCorp Vault HSM
pub fn new(
client: VaultClient,
key_name: String,
mount_name: String,
public_key: C::PublicKey,
) -> Self {
Self {
client,
key_name,
mount_name,
public_key,
_cryptography_marker: std::marker::PhantomData,
}
pub fn new(client: VaultClient, key_name: String, mount_name: String) -> Self {
Self { client, key_name, mount_name, _cryptography_marker: std::marker::PhantomData }
}

/// Tries to create a new HashiCorp Vault HSM from the environment
Expand All @@ -53,14 +43,8 @@ where

let key_name = std::env::var("VAULT_KEY_NAME").context("VAULT_KEY_NAME not set")?;
let mount_name = std::env::var("VAULT_MOUNT_NAME").context("VAULT_MOUNT_NAME not set")?;
let public_key = std::env::var("VAULT_PUBLIC_KEY").unwrap_or_default();

Ok(Self::new(
client,
key_name,
mount_name,
C::PublicKey::try_from_bytes(public_key.as_bytes())?,
))
Ok(Self::new(client, key_name, mount_name))
}

/// Creates a new key in the transit backend
Expand All @@ -73,35 +57,8 @@ where
)
.await
.context("Failed to create key")?;

Ok(self)
}

/// Fills with a public key fetched from vault.
pub async fn fill_with_public_key(self) -> Result<Self, anyhow::Error> {
let res = key::read(&self.client, self.mount_name.as_str(), self.key_name.as_str())
.await
.context("Failed to read key")?;
println!("Read key: {:?}", res);

let public_key = match res.keys {
ReadKeyData::Symmetric(_) => {
return Err(anyhow::anyhow!("Symmetric keys are not supported"));
}
ReadKeyData::Asymmetric(keys) => {
let key = keys.values().next().context("No key found")?;
base64::decode(key.public_key.as_str()).context("Failed to decode public key")?
}
};

println!("Public key: {:?}", public_key);
Ok(Self::new(
self.client,
self.key_name,
self.mount_name,
C::PublicKey::try_from_bytes(public_key.as_slice())?,
))
}
}

impl Signing<Ed25519> for HashiCorpVault<Ed25519> {
Expand All @@ -121,18 +78,14 @@ impl Signing<Ed25519> for HashiCorpVault<Ed25519> {
return Err(SignerError::Internal("Symmetric keys are not supported".to_string()));
}
ReadKeyData::Asymmetric(keys) => {
let key = keys
.values()
.next()
.context("No key found")
.map_err(|e| SignerError::KeyNotFound)?;
let key = keys.values().next().ok_or_else(|| SignerError::KeyNotFound)?;
base64::decode(key.public_key.as_str())
.context("Failed to decode public key")
.map_err(|e| SignerError::Internal(e.to_string()))?
.context("failed to decode public key")
.map_err(|e| SignerError::Decode(e.into()))?
}
};

Ok(<Ed25519 as Curve>::PublicKey::try_from_bytes(public_key.as_slice())
.map_err(|e| SignerError::Internal(e.to_string()))?)
Ok(ed25519::PublicKey::try_from(public_key.as_slice())
.map_err(|e| SignerError::Decode(e.into()))?)
}
}
32 changes: 32 additions & 0 deletions util/signing/testing/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
[package]
name = "movement-signer-test"
description = "Test support for Movement signing API"
version.workspace = true
edition.workspace = true
license.workspace = true
authors.workspace = true
repository.workspace = true
homepage.workspace = true
publish.workspace = true
rust-version.workspace = true

[dependencies]
movement-signer = { workspace = true }
ed25519-dalek = { workspace = true }

[dev-dependencies]
maptos-dof-execution = { workspace = true }
maptos-execution-util = { workspace = true }
movement-signing-aptos = { workspace = true }
aptos-crypto = { workspace = true }
aptos-types = { workspace = true }
anyhow = { workspace = true }
chrono = { workspace = true }
ed25519-dalek = { workspace = true, features = ["rand_core"] }
# Workspace is on rand 0.7 due largely to aptos-core
rand = "0.8"
tempfile = { workspace = true }
tokio = { workspace = true, features = ["macros"] }

[lints]
workspace = true
34 changes: 34 additions & 0 deletions util/signing/testing/src/ed25519.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
use movement_signer::cryptography::ed25519::{self, Ed25519};
use movement_signer::{SignerError, Signing};

use ed25519_dalek::{Signer as _, SigningKey};

/// In-process signer used for testing the signing API.
///
/// This signer wraps an Ed25519 private key to provide a signing service with
/// the Ed25519 elliptic curve. Because the private key is kept in process
/// memory, this signer implementation should not be used in production.
pub struct TestSigner {
signing_key: SigningKey,
}

impl TestSigner {
pub fn new(signing_key: SigningKey) -> Self {
Self { signing_key }
}
}

impl Signing<Ed25519> for TestSigner {
async fn sign(&self, message: &[u8]) -> Result<ed25519::Signature, SignerError> {
let signature =
self.signing_key.try_sign(message).map_err(|e| SignerError::Sign(e.into()))?;
Ok(signature.to_bytes().into())
}

async fn public_key(&self) -> Result<ed25519::PublicKey, SignerError> {
let key = self.signing_key.verifying_key();
// The conversion should never fail because it's round-tripping
// a valid key.
Ok(key.to_bytes().try_into().unwrap())
}
}
1 change: 1 addition & 0 deletions util/signing/testing/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
pub mod ed25519;
Loading
Loading