rust-multihash icon indicating copy to clipboard operation
rust-multihash copied to clipboard

fix: remove the "identity" feature

Open Stebalien opened this issue 4 years ago • 5 comments

There are always safer ways to compute an identity hash, and this feature can cause otherwise "safe" code to crash. With this feature enabled, Code::try_from(codec)?.digest(input) would crash if codec was 0 and input was more than 64 bytes.

Unfortunately, this was a cargo "feature" so a single dependency enabling it means it's enabled for all other crates in the build.

Users of this code migrate to Multihash::wrap(0, digest), which returns an error if the digest is too large instead of panicing.

fixes #194

Stebalien avatar Mar 24 '22 14:03 Stebalien

Here's an example of a hack I've needed to prevent code that should "just work" from potentially crashing. I understand that downstream crates should generally use custom Code implementations, but that's not a reason to attach a foot-cannon to the default implementation:

https://github.com/filecoin-project/filecoin-ffi/blob/e81d302831f5dcd0c5b7b9415c353f8b43291b8c/rust/src/fvm/blockstore/fake.rs#L40-L43

Stebalien avatar Mar 24 '22 14:03 Stebalien

@mxinden one of the reasons this feature was originally introduces was rust-libp2p. Could you please check if it would be OK for the project if we would merge this change? I really want to prevent that projects stay on old versions/need to fork.

vmx avatar Mar 24 '22 15:03 vmx

Thanks for the ping @vmx!

As far as I can tell our only usage of Code::Identity is in rust-libp2p's peer id handling:

https://github.com/libp2p/rust-libp2p/blob/6cc3b4ec52c922bfcf562a29b5805c3150e37c75/core/src/peer_id.rs#L55-L112

Do I understand correctly that we would either have to define our own Code enum, or use the magic constant (0) via Multihash::wrap(0, digest) and multihash.code() == 0?

mxinden avatar Mar 25 '22 12:03 mxinden

As far as I can tell our only usage of Code::Identity is in rust-libp2p's peer id handling:

That's also the only place that I've found.

Do I understand correctly that we would either have to define our own Code enum, or use the magic constant (0) via Multihash::wrap(0, digest) and multihash.code() == 0?

It's not a magic constant, 0x00 is the identity hash as specified in the multicodec table. We don't bundle the full table with the library anymore, as it depends on your applications with codes to use (that's true for all multiformats implementations). Usually this constant is specified together with the codec/hash/etc that implements it. I just saw that currently it's not the case with the hashers in this library (they should have a field called code or something like that. Anyway, easiest is to specify the constants in your application for the codes of the multicodec table you're using.

vmx avatar Mar 25 '22 13:03 vmx

Do I understand correctly that we would either have to define our own Code enum, or use the magic constant (0) via Multihash::wrap(0, digest) and multihash.code() == 0?

IMO, it's also reasonable to include some form of helper function here:

pub const IDENTITY_CODE: u64 = 0x0;
pub fn identity_hash<S>(data: &[0]) -> Multihash<S> {
    Multihash::wrap(IDENTITY_CODE, data)
}

Stebalien avatar Mar 25 '22 15:03 Stebalien

There is https://github.com/multiformats/rust-multihash/pull/289 now which is up-to-date with latest master.

thomaseizinger avatar Apr 11 '23 18:04 thomaseizinger

This feature got merged with https://github.com/multiformats/rust-multihash/pull/289, hence closing this one.

vmx avatar May 31 '23 08:05 vmx