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

fix: remove the "identity" feature #196

Closed
wants to merge 1 commit into from

Conversation

Stebalien
Copy link
Member

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

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 Stebalien requested a review from vmx March 24, 2022 14:48
@Stebalien
Copy link
Member Author

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

@vmx
Copy link
Member

vmx commented Mar 24, 2022

@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.

@mxinden
Copy link
Member

mxinden commented Mar 25, 2022

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?

@vmx
Copy link
Member

vmx commented Mar 25, 2022

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.

@Stebalien
Copy link
Member Author

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)
}

@thomaseizinger
Copy link
Contributor

There is #289 now which is up-to-date with latest master.

@vmx
Copy link
Member

vmx commented May 31, 2023

This feature got merged with #289, hence closing this one.

@vmx vmx closed this May 31, 2023
@vmx vmx deleted the feat/remove-identity-panic branch May 31, 2023 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Code::Identity support
4 participants