Adds `HostFunctions` to `tendermint-light-client-verifier` #1138

seunlanlege · 2022-06-03T05:56:33Z

This encapsulates all crypto (hasing, signature verifications) behind a trait, so downstream crates can delegate these functions to host functions

…rifier

…-host-functions Introduced `HostFunctionsProvider` trait to `tenderrmint-light-client-verifier`

thanethomson · 2022-06-11T15:59:41Z

@seunlanlege what's the context for this PR? We usually insist on having an issue associated with a PR so that there's at least opportunity for discussion, context sharing and planning/coordination before someone submits a PR.

seunlanlege · 2022-06-20T08:19:56Z

hi @thanethomson you can find the discussion thread here: informalsystems/ibc-rs#2110

cc: @adizere

seunlanlege · 2022-06-20T09:19:48Z

One thing to note is that tendermint-light-client depends on tendermint-light-client-verifier so i think it makes sense that someone from informal refactors tendermint-light-client with the new changes as i don't have enough context on this code.

Downgrade `k256` to `v0.10.4`

tony-iqlusion · 2022-06-20T12:11:35Z

Why are you downgrading k256? This will cause incompatibilities with TMKMS.

Also if this trait is supposed to be cryptography-related functionality, shouldn't it have something like Crypto in its name, rather than Host*?

seunlanlege · 2022-06-20T13:49:41Z

Why are you downgrading k256? This will cause incompatibilities with TMKMS.

@Wizdave97

Also if this trait is supposed to be cryptography-related functionality, shouldn't it have something like Crypto in its name, rather than Host*?

yes for now it is related to crypto functionality, but its more semantically correct to call them HostFunctions (a la wasm) because they could potentially be extended for other native-optimized non-crypto operations.

seunlanlege · 2022-06-20T14:52:42Z

@tony-iqlusion @thanethomson please approve CI steps so we can start to move this PR along 🙏🏿

tony-iqlusion · 2022-06-20T15:19:56Z

light-client-verifier/src/host_functions.rs

+    /// Verify an ed25519 signature
+    fn ed25519_verify(sig: &[u8], msg: &[u8], pub_key: &[u8]) -> bool;
+
+    /// verify secp256k1 signatures
+    fn secp256k1_verify(sig: &[u8], message: &[u8], public: &[u8]) -> bool;


Using a bool for signature verification is a dangerous API which has lead to cryptographic vulnerabilities in practice:

https://github.com/libp2p/rust-libp2p/pull/1127/files

The signature crate, already used by tendermint-rs, provides a well thought-out type safe trait-based approach to encapsulating signing and verification providers for various digital signature algorithms.

TMKMS uses this to support hardware signers for signatures, for example (e.g. Ledger, YubiHSM2)

Likewise the digest crate provides an abstraction over hash functions.

.gitignore

tony-iqlusion · 2022-06-20T15:23:31Z

light-client-verifier/Cargo.toml


 [dependencies]
 tendermint = { version = "0.24.0-pre.2", path = "../tendermint", default-features = false }
 derive_more = { version = "0.99.5", default-features = false, features = ["display"] }
 serde = { version = "1.0.106", default-features = false }
 time = { version = "0.3.5", default-features = false }
 flex-error = { version = "0.4.4", default-features = false }
+sp-std = { version = "4.0.0", default-features = false }


Shouldn't this be an optional dependency?

its a drop in replacement for the std lib, so no default features means core, std features means std

tony-iqlusion · 2022-06-20T15:28:53Z

yes for now it is related to crypto functionality, but its more semantically correct to call them HostFunctions (a la wasm)

I think it is semantically quite unclear what the trait's name means.

The particular meaning of "host" in that content relates specifically to WASM, i.e. "outside the VM". But this crate is intended to be used, and indeed is primarily used, in environments other than WASM, where "host" has many other meanings such as hostnames.

because they could potentially be extended for other native-optimized non-crypto operations.

That sounds like YAGNI, and really I would suggest against a trait which has both cryptographic and non-cryptographic responsibilities unless the cryptography is tightly-coupled to the non-cryptographic functionality in some inseparable way.

thanethomson

I also have to agree with @tony-iqlusion in terms of the naming of the Host* entities. If there's even a need in future to broaden their functionality, I'd rather have multiple separate smaller traits, each focusing on one area of functionality.

.gitignore

light-client-js/Cargo.toml

thanethomson · 2022-06-20T16:38:22Z

light-client-verifier/Cargo.toml

+sp-std = { version = "4.0.0", default-features = false }
+sp-core = { version = "6.0.0", features = ["full_crypto"], optional = true }


Are these dependencies really necessary?

yes its used for tests and std end-users

Can we instead use core and alloc crates? We would rather avoid introducing Substrate-specific dependencies into the light client.

xla · 2022-06-20T17:17:18Z

@seunlanlege what's the context for this PR? We usually insist on having an issue associated with a PR so that there's at least opportunity for discussion, context sharing and planning/coordination before someone submits a PR.

If this is the required process, linking to a discussion on another repository is not sufficient. There also seems to be quite a lot of reservation over the general approach, which indicates that a precursor in the form of a problem statement and a suggested design is necessary.

tony-iqlusion · 2022-06-20T17:41:03Z

As a general design, I'd suggest using existing traits to express the required functionality (e.g. Digest, Signer, Verifier), and either generic type parameters or associated types to describe concrete implementations.

seunlanlege · 2022-06-21T07:08:03Z

As a general design, I'd suggest using existing traits to express the required functionality (e.g. Digest, Signer, Verifier), and either generic type parameters or associated types to describe concrete implementations.

I took a look at the crate and it doesn't seem that they can be used for the work that we're doing.

This PR is part of a general ibc-rs refactor Issue: https://github.com/informalsystems/ibc-rs/issues/2110 (Relevant PR: informalsystems/hermes#2284) in order to delegate expensive crypto operations to native-optimized host functions in wasm environments. Which indirectly necessitates that we refactor its dependencies (tendermint-rs and confio/ics23) as well.

I can rename the trait to be CryptoProvider, but a similar pr already went into cosmos/ics23#90 where its called HostFunctions and @ethanfrey didn't seem to mind. I personally don't mind either.

Using a bool for signature verification is a dangerous API which has lead to cryptographic vulnerabilities in practice:

The signature verification host functions that substrate provides, doesn't use a result interface: https://github.com/paritytech/substrate/blob/9461b2de04210c6c193726a745c3ec6552b4ce9f/primitives/io/src/lib.rs#L616-L989

Seeing as we'd be delegating to these host functions, I don't see how this api can be misused.

ethanfrey · 2022-06-21T08:15:44Z

Comment on the API.

If parity returns bool, we can still require a HostFunctions trait that returns Result<(), Error>.

Just wrap the parity one with this. I agree for critical operations, this ensures the return value won't get silently ignored.

The point is not to mirror parity 1:1 but find a good api that we can easily make a transform to from various implementations

tony-iqlusion · 2022-06-21T12:23:03Z

I took a look at the [signature] crate and it doesn't seem that they can be used for the work that we're doing.

You didn't actually explain why?

These traits already have widespread usage for the purposes of abstracting over signature and verification providers. They should work just fine for this application, and are much more carefully thought out than the API you're proposing.

The signature verification host functions that substrate provides, doesn't use a result interface

...and that was a mistake which created a sharp edge which lead to a critical signature verification bypass in one of their other crates.

Let's not repeat it?

seunlanlege · 2022-06-21T12:30:09Z

You didn't actually explain why?

the purpose of the interface is for the crate to describe to downstream users what host functions it needs.
The signature crate only defines interfaces that should be implemented by signature schemes, which is tangent to delegating the actual operations to native.

tony-iqlusion · 2022-06-21T12:42:13Z

That is not the case. As I explained earlier, we already use these traits to delegate signing operations to hardware devices including Ledger Nanos, YubiKeys, and YubiHSM2s, and the same thing can work just as well for verification.

In fact the whole purpose is to be able to plug in different signing or verification providers so the API is type safe and avoids confusion of supported signature algorithms.

seunlanlege · 2022-06-21T12:50:00Z

Maybe you can spec out some pseudo-code as to how you see the verification interface being used in the context of wasm host functions?

tony-iqlusion · 2022-06-21T14:37:23Z

Leveraging existing traits, the CryptoProvider can contain associated types rather than a bag of unrelated methods:

pub trait CryptoProvider {
    type Sha256: Digest + FixedOutput<OutputSize = U32>;

    type EcdsaSecp256k1Signer: Signer<k256::ecdsa::Signature>;
    type EcdsaSecp256k1Verifier: Verifier<k256::ecdsa::Signature>;

    type Ed25519Signer: Signer<ed25519::Signature>;
    type Ed25519Verifier: Verifier<ed25519::Signature>;
}

This allows code to be written generically over traits. For example, when ECDSA/secp256r1 support is added, code can be written generically such that it can support both that and ECDSA/secp256k1 by leveraging the Signer and Verifier traits but keeping the signature type generic.

The ed25519 crate has a code example of what generic code written using these traits looks like:

https://docs.rs/ed25519/latest/ed25519/#using-ed25519-generically-over-algorithm-implementationsproviders

siriustaikun · 2022-07-13T18:50:11Z

This would be instrumental for my project.

I'm implementing tendermint light client validity predicates in CosmWasm as a workaround to trustlessly access validator voting powers from within a contract. At present, this is otherwise impossible, and it constricts the ways we are able to enforce network security measures via liquid staking protocols.

Furthermore, this would serve as a substantial catalyst toward the completion of ICS-8 Wasm Client.

adizere · 2022-08-03T14:14:36Z

It seems this feature is important for at least 2 use-cases (supporting IBC in Substrate that Composable Finance are doing, and CosmWasm validity predicates). I think at the moment Composable is maintaining their own long fork. That works temporarily but it fragments the ecosystem, and there are other orgs who seem to need this feature.

If we consider this feature an ecosystem requirement on the tendermint-rs crates, maybe someone from tendermint-rs team could shepherd the PR and align everyone to get Host* functions (adapted to use CryptoProvider) upstreamed into the light-client-verifier crate?

blasrodri · 2022-10-17T11:51:21Z

It seems this feature is important for at least 2 use-cases (supporting IBC in Substrate that Composable Finance are doing, and CosmWasm validity predicates). I think at the moment Composable is maintaining their own long fork. That works temporarily but it fragments the ecosystem, and there are other orgs who seem to need this feature.

If we consider this feature an ecosystem requirement on the tendermint-rs crates, maybe someone from tendermint-rs team could shepherd the PR and align everyone to get Host* functions (adapted to use CryptoProvider) upstreamed into the light-client-verifier crate?

I'm just wondering if we could re-use this implementation addressing https://github.com/informalsystems/tendermint-rs/pull/1138/files#r937928435

We're happy to work on a better API (following #1138 (comment)). I can try to summarize all comments into one, and then we can see how to draft it?

tony-iqlusion · 2022-10-17T16:24:18Z

@blasrodri I think it would be great to open a planning issue for this and sketch out the API

blasrodri · 2022-10-17T16:46:45Z

@blasrodri I think it would be great to open a planning issue for this and sketch out the API

Will do sir. Thanks

mzabaluev · 2022-10-29T17:04:36Z

#1214 (currently a draft) seems to be a more recent work along the same lines.

blasrodri · 2022-10-29T18:49:06Z

#1214 (currently a draft) seems to be a more recent work along the same lines.

@mzabaluev it's an iteration over this PR based on the gathered feedback. @seunlanlege can we close this one?

seunlanlege added 5 commits June 1, 2022 16:46

Introduced HostFunctionsProvider trait to tenderrmint-light-client-ve…

cc000c8

…rifier

fixes tests

14e3fcf

secpk1256 doesn't work, but hey, its optional

d9cc919

Merge pull request #1 from ComposableFi/seun-tenndermint-light-client…

0422391

…-host-functions Introduced `HostFunctionsProvider` trait to `tenderrmint-light-client-verifier`

Merge branch 'informalsystems:master' into master

2897cc1

switch k256 to 0.10.4

3b021da

Merge pull request #3 from ComposableFi/david/lower-k256

5a74e0f

Downgrade `k256` to `v0.10.4`

seunlanlege added 2 commits June 20, 2022 15:47

refactor tendermint-light-client

c56e6a7

Merge branch 'master' of github.com:ComposableFi/tendermint-rs

cb07a2d

tony-iqlusion reviewed Jun 20, 2022

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

tony-iqlusion reviewed Jun 20, 2022

View reviewed changes

thanethomson reviewed Jun 20, 2022

View reviewed changes

seunlanlege added 4 commits June 21, 2022 12:54

refactor api to use result interface

a0b3f16

rename to CryptoProvider, CryptoManager

337f046

fmt

ed3480b

reset .gitignore

0368aff

fix light client verifier

f160afc

blasrodri mentioned this pull request Oct 17, 2022

Trait-based cryptographic routines API to delegate implementation to downstream crates #1213

Closed

mzabaluev mentioned this pull request Nov 2, 2022

Add crypto provider #1214

Closed

seunlanlege closed this Nov 9, 2022

seunlanlege deleted the master branch November 9, 2022 02:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds `HostFunctions` to `tendermint-light-client-verifier` #1138

Adds `HostFunctions` to `tendermint-light-client-verifier` #1138

seunlanlege commented Jun 3, 2022

thanethomson commented Jun 11, 2022 •

edited

Loading

seunlanlege commented Jun 20, 2022

seunlanlege commented Jun 20, 2022

tony-iqlusion commented Jun 20, 2022

seunlanlege commented Jun 20, 2022

seunlanlege commented Jun 20, 2022

tony-iqlusion Jun 20, 2022 •

edited

Loading

tony-iqlusion Jun 20, 2022

seunlanlege Jun 21, 2022

tony-iqlusion commented Jun 20, 2022 •

edited

Loading

thanethomson left a comment

thanethomson Jun 20, 2022

seunlanlege Jun 21, 2022

romac Aug 4, 2022

xla commented Jun 20, 2022

tony-iqlusion commented Jun 20, 2022

seunlanlege commented Jun 21, 2022

ethanfrey commented Jun 21, 2022

tony-iqlusion commented Jun 21, 2022 •

edited

Loading

seunlanlege commented Jun 21, 2022

tony-iqlusion commented Jun 21, 2022

seunlanlege commented Jun 21, 2022

tony-iqlusion commented Jun 21, 2022 •

edited

Loading

siriustaikun commented Jul 13, 2022

adizere commented Aug 3, 2022

blasrodri commented Oct 17, 2022 •

edited

Loading

tony-iqlusion commented Oct 17, 2022

blasrodri commented Oct 17, 2022

mzabaluev commented Oct 29, 2022

blasrodri commented Oct 29, 2022

		sp-std = { version = "4.0.0", default-features = false }
		sp-core = { version = "6.0.0", features = ["full_crypto"], optional = true }

Adds HostFunctions to tendermint-light-client-verifier #1138

Adds HostFunctions to tendermint-light-client-verifier #1138

Conversation

seunlanlege commented Jun 3, 2022

thanethomson commented Jun 11, 2022 • edited Loading

seunlanlege commented Jun 20, 2022

seunlanlege commented Jun 20, 2022

tony-iqlusion commented Jun 20, 2022

seunlanlege commented Jun 20, 2022

seunlanlege commented Jun 20, 2022

tony-iqlusion Jun 20, 2022 • edited Loading

Choose a reason for hiding this comment

tony-iqlusion Jun 20, 2022

Choose a reason for hiding this comment

seunlanlege Jun 21, 2022

Choose a reason for hiding this comment

tony-iqlusion commented Jun 20, 2022 • edited Loading

thanethomson left a comment

Choose a reason for hiding this comment

thanethomson Jun 20, 2022

Choose a reason for hiding this comment

seunlanlege Jun 21, 2022

Choose a reason for hiding this comment

romac Aug 4, 2022

Choose a reason for hiding this comment

xla commented Jun 20, 2022

tony-iqlusion commented Jun 20, 2022

seunlanlege commented Jun 21, 2022

ethanfrey commented Jun 21, 2022

tony-iqlusion commented Jun 21, 2022 • edited Loading

seunlanlege commented Jun 21, 2022

tony-iqlusion commented Jun 21, 2022

seunlanlege commented Jun 21, 2022

tony-iqlusion commented Jun 21, 2022 • edited Loading

siriustaikun commented Jul 13, 2022

adizere commented Aug 3, 2022

blasrodri commented Oct 17, 2022 • edited Loading

tony-iqlusion commented Oct 17, 2022

blasrodri commented Oct 17, 2022

mzabaluev commented Oct 29, 2022

blasrodri commented Oct 29, 2022

Adds `HostFunctions` to `tendermint-light-client-verifier` #1138

Adds `HostFunctions` to `tendermint-light-client-verifier` #1138

thanethomson commented Jun 11, 2022 •

edited

Loading

tony-iqlusion Jun 20, 2022 •

edited

Loading

tony-iqlusion commented Jun 20, 2022 •

edited

Loading

tony-iqlusion commented Jun 21, 2022 •

edited

Loading

tony-iqlusion commented Jun 21, 2022 •

edited

Loading

blasrodri commented Oct 17, 2022 •

edited

Loading