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

serde_derive will be found within precompiled/bin when using git #2526

Closed
Astavie opened this issue Jul 23, 2023 · 4 comments · Fixed by #2590
Closed

serde_derive will be found within precompiled/bin when using git #2526

Astavie opened this issue Jul 23, 2023 · 4 comments · Fixed by #2590

Comments

@Astavie
Copy link

Astavie commented Jul 23, 2023

I'm unsure if this is intentional or not, but I'm currently unable to use serde as a dependency like this:

[dependencies]
serde = { git = "https://github.com/serde-rs/serde.git", features = ["derive"] }

When doing that, I get these errors:

error: proc-macro derive panicked
   --> discord/src/gateway.rs:131:10
    |
131 | #[derive(Deserialize)]
    |          ^^^^^^^^^^^
    |
    = help: message: failed to spawn process: Os { code: 2, kind: NotFound, message: "No such file or directory" }

This is because it tries to run serde_derive/serde_derive-x86_64-unknown-linux-gnu, a binary that is not included in the git repo and is not built automatically. It seems that it finds serde_derive within precompiled/bin/ before the it finds it in serde_derive/

This is does not happen when simply using the crates.io package, so this might seem inconsequential. However, this means it's harder to use serde forks as dependencies for testing purposes. I found this issue when trying to use my own fork:

[dependencies]
serde = { git = "https://github.com/Astavie/serde.git", branch = "integer-tags-for-enums", features = ["derive"] }

For now I have made another branch on my fork where I removed the precompiled folder, but it would be nice if this wasn't necessary.

@Astavie Astavie changed the title when adding serde as a git dependency, serde_derive will be found within precompiles/ serde_derive will be found within precompiled/bin when using git Jul 23, 2023
@dtolnay
Copy link
Member

dtolnay commented Jul 23, 2023

Would you be willing to pursue any of the following fixes in Cargo?

  1. Support an additional argument in serde_derive = { git = "https://github.com/serde-rs/serde", ... } that specifies the subdirectory within the repo where the requested crate is located, in this case subdirectory = "serde_derive"?

  2. Something we can put into the manifest at precompiled/serde_derive/Cargo.toml to specify that it is not supposed to be selected when Cargo crawls the git repo looking for crates?

  3. Increased precedence for top-level workspace members? The top-level Cargo.toml in the git repo is a virtual manifest that lists members = ["serde_derive", ...] but not "precompiled/serde_derive"; that is kept in a different workspace (in part because a workspace can only have one member per crate name).

  4. Paying attention to path of git dependencies? When you have serde = { git = "https://github.com/serde-rs/serde" }, the serde crate manifest at serde/Cargo.toml specifies serde_derive = { ..., path = "../serde_derive" }, so it's pretty clear that that one is the right serde_derive to use in the build.

@Astavie
Copy link
Author

Astavie commented Jul 23, 2023

Those could be viable solutions.

  1. This is probably the simplest but then the library user is expected to know what the issue is. IMO using a git dependency should be as close to a drop in replacement as possible to using the crates.io version
  2. This would solve this right now, yes, but would also prevent using the precompiled version if you actually intended to, no?
  3. This also makes sense but would a breaking change. It also would only solve this specific issue, not the general issue where a git repo has multiple definitions for the same crate
  4. This makes the most sense to me and I'm actually surprised it isn't already default behavior. I'll look into this some more later

kayabaNerve added a commit to serai-dex/serai that referenced this issue Aug 19, 2023
Achieves three notable updates.

1) Resolves RUSTSEC-2022-0093 by updating libp2p-identity.
2) Removes 3 old rand crates via updating ed25519-dalek (a dependency of
libp2p-identity).
3) Sets serde_derive to 1.0.171 via updating to time 0.3.26 which pins at up to
1.0.171.

The last one is the most important. The former two are niceties.

serde_derive, since 1.0.171, ships a non-reproducible binary blob in what's a
complete compromise of supply chain security. This is done in order to reduce
compile times, yet also for the maintainer of serde (dtolnay) to leverage
serde's position as the 8th most downloaded crate to attempt to force changes
to the Rust build pipeline.

While dtolnay's contributions to Rust are respectable, being behind syn, quote,
and proc-macro2 (the top three crates by downloads), along with thiserror,
anyhow, async-trait, and more (I believe also being part of the Rust project),
they have unfortunately decided to refuse to listen to the community on this
issue (or even engage with counter-commentary). Given their political agenda
they seem to try to be accomplishing with force, I'd go as far as to call their
actions terroristic (as they're using the threat of the binary blob as
justification for cargo to ship 'proper' support for binary blobs).

This is arguably representative of dtolnay's past work on watt. watt was a wasm
interpreter to execute a pre-compiled proc macro. This would save the compile
time of proc macros, yet sandbox it so a full binary did not have to be run.

Unfortunately, watt (while decreasing compile times) fails to be a valid
solution to supply chain security (without massive ecosystem changes). It never
implemented reproducible builds for its wasm blobs, and a malicious wasm blob
could still fundamentally compromise a project. The only solution for an end
user to achieve a secure pipeline would be to locally build the project,
verifying the blob aligns, yet doing so would negate all advantages of the
blob.

dtolnay also seems to be giving up their role as a FOSS maintainer given that
serde no longer works in several environments. While FOSS maintainers are not
required to never implement breaking changes, the version number is still 1.0.
While FOSS maintainers are not required to follow semver, releasing a very
notable breaking change *without a new version number* in an ecosystem which
*does follow semver*, then refusing to acknowledge bugs as bugs with their work
does meet my personal definition of "not actively maintaining their existing
work". Maintenance would be to fix bugs, not introduce and ignore.

For now, serde > 1.0.171 has been banned. In the future, we may host a fork
without the blobs (yet with the patches). It may be necessary to ban all of
dtolnay's maintained crates, if they continue to force their agenda as such,
yet I hope this may be resolved within the next week or so.

Sources:

serde-rs/serde#2538 - Binary blob discussion

This includes several reports of various workflows being broken.

serde-rs/serde#2538 (comment)

dtolnay commenting that security should be resolved via Rust toolchain edits,
not via their own work being secure. This is why I say they're trying to
leverage serde in a political game.

serde-rs/serde#2526 - Usage via git broken

dtolnay explicitly asks the submitting user if they'd be willing to advocate
for changes to Rust rather than actually fix the issue they created. This is
further political arm wrestling.

serde-rs/serde#2530 - Usage via Bazel broken

serde-rs/serde#2575 - Unverifiable binary blob

https://github.com/dtolnay/watt - dtolnay's prior work on precompilation
@jayvdb
Copy link

jayvdb commented Aug 21, 2023

This issue may no longer be relevant since https://github.com/serde-rs/serde/releases/tag/v1.0.184 removed the pre-compiled binaries.

@dtolnay dtolnay closed this as completed Aug 21, 2023
@dtolnay dtolnay linked a pull request Aug 21, 2023 that will close this issue
@dtolnay
Copy link
Member

dtolnay commented Aug 21, 2023

Thanks @jayvdb. Closing, but I would still love to see any of #2526 (comment) implemented in Cargo.

kayabaNerve added a commit to serai-dex/serai that referenced this issue Aug 21, 2023
* dalek 4.0

* cargo update

Moves to a version of Substrate which uses curve25519-dalek 4.0 (not a rc).
Doesn't yet update the repo to curve25519-dalek 4.0 (as a branch does) due
to the official schnorrkel using a conflicting curve25519-dalek. This would
prevent installation of frost-schnorrkel without a patch.

* use half-aggregation for tm messages

* fmt

* fix pr comments

* cargo update

Achieves three notable updates.

1) Resolves RUSTSEC-2022-0093 by updating libp2p-identity.
2) Removes 3 old rand crates via updating ed25519-dalek (a dependency of
libp2p-identity).
3) Sets serde_derive to 1.0.171 via updating to time 0.3.26 which pins at up to
1.0.171.

The last one is the most important. The former two are niceties.

serde_derive, since 1.0.171, ships a non-reproducible binary blob in what's a
complete compromise of supply chain security. This is done in order to reduce
compile times, yet also for the maintainer of serde (dtolnay) to leverage
serde's position as the 8th most downloaded crate to attempt to force changes
to the Rust build pipeline.

While dtolnay's contributions to Rust are respectable, being behind syn, quote,
and proc-macro2 (the top three crates by downloads), along with thiserror,
anyhow, async-trait, and more (I believe also being part of the Rust project),
they have unfortunately decided to refuse to listen to the community on this
issue (or even engage with counter-commentary). Given their political agenda
they seem to try to be accomplishing with force, I'd go as far as to call their
actions terroristic (as they're using the threat of the binary blob as
justification for cargo to ship 'proper' support for binary blobs).

This is arguably representative of dtolnay's past work on watt. watt was a wasm
interpreter to execute a pre-compiled proc macro. This would save the compile
time of proc macros, yet sandbox it so a full binary did not have to be run.

Unfortunately, watt (while decreasing compile times) fails to be a valid
solution to supply chain security (without massive ecosystem changes). It never
implemented reproducible builds for its wasm blobs, and a malicious wasm blob
could still fundamentally compromise a project. The only solution for an end
user to achieve a secure pipeline would be to locally build the project,
verifying the blob aligns, yet doing so would negate all advantages of the
blob.

dtolnay also seems to be giving up their role as a FOSS maintainer given that
serde no longer works in several environments. While FOSS maintainers are not
required to never implement breaking changes, the version number is still 1.0.
While FOSS maintainers are not required to follow semver, releasing a very
notable breaking change *without a new version number* in an ecosystem which
*does follow semver*, then refusing to acknowledge bugs as bugs with their work
does meet my personal definition of "not actively maintaining their existing
work". Maintenance would be to fix bugs, not introduce and ignore.

For now, serde > 1.0.171 has been banned. In the future, we may host a fork
without the blobs (yet with the patches). It may be necessary to ban all of
dtolnay's maintained crates, if they continue to force their agenda as such,
yet I hope this may be resolved within the next week or so.

Sources:

serde-rs/serde#2538 - Binary blob discussion

This includes several reports of various workflows being broken.

serde-rs/serde#2538 (comment)

dtolnay commenting that security should be resolved via Rust toolchain edits,
not via their own work being secure. This is why I say they're trying to
leverage serde in a political game.

serde-rs/serde#2526 - Usage via git broken

dtolnay explicitly asks the submitting user if they'd be willing to advocate
for changes to Rust rather than actually fix the issue they created. This is
further political arm wrestling.

serde-rs/serde#2530 - Usage via Bazel broken

serde-rs/serde#2575 - Unverifiable binary blob

https://github.com/dtolnay/watt - dtolnay's prior work on precompilation

* add Rs() api to  SchnorrAggregate

* Correct serai-processor-tests to dalek 4

* fmt + deny

* Slash malevolent validators  (#294)

* add slash tx

* ignore unsigned tx replays

* verify that provided evidence is valid

* fix clippy + fmt

* move application tx handling to another module

* partially handle the tendermint txs

* fix pr comments

* support unsigned app txs

* add slash target to the votes

* enforce provided, unsigned, signed tx ordering within a block

* bug fixes

* add unit test for tendermint txs

* bug fixes

* update tests for tendermint txs

* add tx ordering test

* tidy up tx ordering test

* cargo +nightly fmt

* Misc fixes from rebasing

* Finish resolving clippy

* Remove sha3 from tendermint-machine

* Resolve a DoS in SlashEvidence's read

Also moves Evidence from Vec<Message> to (Message, Option<Message>). That
should meet all requirements while being a bit safer.

* Make lazy_static a dev-depend for tributary

* Various small tweaks

One use of sort was inefficient, sorting unsigned || signed when unsigned was
already properly sorted. Given how the unsigned TXs were given a nonce of 0, an
unstable sort may swap places with an unsigned TX and a signed TX with a nonce
of 0 (leading to a faulty block).

The extra protection added here sorts signed, then concats.

* Fix Tributary tests I broke, start review on tendermint/tx.rs

* Finish reviewing everything outside tests and empty_signature

* Remove empty_signature

empty_signature led to corrupted local state histories. Unfortunately, the API
is only sane with a signature.

We now use the actual signature, which risks creating a signature over a
malicious message if we have ever have an invariant producing malicious
messages. Prior, we only signed the message after the local machine confirmed
it was okay per the local view of consensus.

This is tolerated/preferred over a corrupt state history since production of
such messages is already an invariant. TODOs are added to make handling of this
theoretical invariant further robust.

* Remove async_sequential for tokio::test

There was no competition for resources forcing them to be run sequentially.

* Modify block order test to be statistically significant without multiple runs

* Clean tests

---------

Co-authored-by: Luke Parker <lukeparker5132@gmail.com>

* Add DSTs to Tributary TX sig_hash functions

Prevents conflicts with other systems/other parts of the Tributary.

---------

Co-authored-by: Luke Parker <lukeparker5132@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants