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

bug: buffrs should not use protoc-bin-vendored #174

Closed
j-baker opened this issue Nov 30, 2023 · 1 comment · Fixed by #179
Closed

bug: buffrs should not use protoc-bin-vendored #174

j-baker opened this issue Nov 30, 2023 · 1 comment · Fixed by #179
Assignees
Labels
bug Reports or fixes associated with bugs in this project component::cli Everything related to the buffrs cli priority::high Urgent change or idea, please review quickly type::fix
Milestone

Comments

@j-baker
Copy link
Contributor

j-baker commented Nov 30, 2023

buffrs uses protoc-bin-vendored. There are two reasons why it should not do this.

Reason 1 is for supply chain reasons. It's some random outdated version of protoc published by some internet dude. It'd be way too easy for a single internet dude to add some viruses and compromise our infra. We shouldn't use crates which contain binaries as these cannot be easily audited.

Reason 2 is for deployability reasons. The way that protoc-bin-vendored works is that it contains protoc installations for each platform in its crate, checked in. At build time, it gives you links to these binaries, in this crate. The intended usage is that in your cargo build, you can then compile some protos.

What this means in our case is that our compile binary contains a compiled-in path of buffrs' own target/ dir.

What that'd mean is that, if you run:

cargo install --bin buffrs
cargo clean

the compiled version of buffrs will not work, because the binary has been deleted. If you run cargo install buffrs you are presently safe, but when rust-lang/cargo#12633 is stable you will likely see your buffrs break if you haven't updated it in a while.

This is the same with the protobuf-src crate - it's designed to be a build-time and not a runtime crate, and so the binaries generated are vulnerable to a cargo clean.

IMO buffrs needs to rely on the user having gotten their own protoc.

@j-baker j-baker changed the title buffrs should not use protoc-bin-vendored bug: buffrs should not use protoc-bin-vendored Nov 30, 2023
@tomkarw tomkarw self-assigned this Dec 4, 2023
@mara-schulke
Copy link
Contributor

Will do!

@mara-schulke mara-schulke added bug Reports or fixes associated with bugs in this project component::cli Everything related to the buffrs cli priority::high Urgent change or idea, please review quickly type::fix labels Dec 5, 2023
@mara-schulke mara-schulke added this to the Stabilization milestone Dec 5, 2023
@github-project-automation github-project-automation bot moved this to Done in Buffrs Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Reports or fixes associated with bugs in this project component::cli Everything related to the buffrs cli priority::high Urgent change or idea, please review quickly type::fix
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants