feat: Signing with Yubihsm2 #5625
Conversation
| /// This function does not use a cache. Care should be taken to not exhaust the number of sessions on the yubihsm when calling this function. | ||
| fn connect(http_config: HttpConfig, credentials: Credentials) -> Client { | ||
| let connector = ethers::signers::yubihsm::Connector::http(&http_config); | ||
| Client::open(connector.clone(), credentials.clone(), true).unwrap() |
There was a problem hiding this comment.
I wonder if we want to return a
Resulthere and let user of the method to decide what to do in the case of an error?
We could make this change but I'm not sure what we would do to handle it. If connection has failed, something has gone quite wrong.
The hyperlane validator agent generally fatally crashes if it can't get some mission critical resource at startup (DB access is locked, S3 creds misconfigured) and I imagine the scraper/relayer do the same though I don't have experience with them. Crashing here feels in line with the existing behavior.
Let me know if you'd like me to change this though. If you think we should, is there a specific place in the call stack you'd want to propagate it up to before crashing?
| /// A thread safe cache for signers against a yubihsm. | ||
| /// Yubihsms have a limited number of sessions (16), which remain open for at least 30 seconds. If a number of connections is requested | ||
| /// too quickly, the devices resources will become exhausted leading to failures. | ||
| static YUBIHSM_CLIENT_CACHE: LazyLock<Mutex<HashMap<YubihsmClientCacheKey, WrappedSigner>>> = |
There was a problem hiding this comment.
I wonder if it would make sense to get a shared access for readers and exclusive access to writers. I suppose that majority of the times we need only read a value from this hash map. Probably, RwLock is more appropriate here?
Done
|
| let client = YubiHsmSigner::connect(http_config, credentials); | ||
| let signer = Arc::new(YubiSigner::create(client, id).unwrap()); | ||
| { | ||
| let mut cache = YUBIHSM_CLIENT_CACHE.write().unwrap(); |
There was a problem hiding this comment.
I would prefer to use
expectwith clear message if app is crushed.
Done. Also updated the connect function below from unwrap -> expect.
| @@ -1,17 +1,17 @@ | |||
| use super::aws_credentials::AwsChainCredentialsProvider; | |||
There was a problem hiding this comment.
We don't follow the following convention everywhere, but let's do it in new changes. > Could you please order imports in the following order:
stdimports sorted alphabetically followed by empty line- external imports sorted alphabetically followed by empty line
- imports from
hyperlane_*sorted alphabetically followed by empty line- imports from
crate::*sorted alphabetically followed by empty line- imports from
super::*sorted alphabetically followed by empty line
Done. Modified imports of other files I touched as well.
I'm not sure about the correct ordering for the pub statements in src/signer/mod.rs, so feel free to bug me if it's still wrong.
| @@ -30,16 +30,20 @@ tracing-futures.workspace = true | |||
| tracing.workspace = true | |||
| url.workspace = true | |||
|
|
|||
| ecdsa = { version = "0.14", default-features = false } | |||
There was a problem hiding this comment.
These dependencies should be moved to workspace
Cargo.tomlfile and referred as dependencies above. These dependencies are external, so, they should mentioned in the previous list.
Done.
It appears (I am new to rust) that in order to get the traits I need across the libraries to be recognized properly, that yubihsm, ethers-rs and this workspace must have the same dependencies of elliptic-curve, ecdsa and k256.
Since they hyperlane fork of ethers-res has k256 version 0.11 and elliptic-curve version 0.12.3 we need to downgrade the dependencies here.
The alternative (I think) is upgrading ethers-rs. I briefly banged on that for a while, and it seems like a non-trivial amount of work. WDYT?
Description
YubiHSMs are a physical security module utilized by many validators. This PR adds support for signing Hyperlane validator messages on ethereum with a yubihsm.
YubiHSMs are accessible via an HTTP(s) connector server, or directly via USB. This implementation opts to use a connector server because the underlying
yubihsm.rscrate will monopolize the USB connection to the device, which means it can't be used to sign for other purposes.Yubihsm's also support other signature algorithms (such as ed25519), which may be useful for other Hyperlane signing configurations outside of ethereum chains. If desired we could extend this PR to add support.
Testing
Tessellated has been rolled out this change and is signing exclusively with YubiHSMs in our production setups for Hyperlane for 3+ weeks.