Closed Fuzzy-Math closed 1 year ago
Sorry I'm just seeing this but setting update to false, or discarding the update param, generates the warning: package uses cargo_vendor, but is missing '<param name="update">true</param>' to apply security updates
. That explains its high usage, but the rest of the arguments still hold.
In addition to #21, I still have some issues with the update param that I think warrant some more discussion. At best I feel it is a misrepresentation of functionality and at worst I can't see what utility it even brings.
Starting with the worst case: what are we gaining by running
cargo update
on our dependencies? At the build system level, shouldn't we want the reproducible builds thatcargo vendor --locked
provides? Effectively we are just asserting semvar compliance during a downstream build when this largely should be an upstream concern.
Build reproducibility is not something I am willing to discuss here. I have no interest in it, and will not pursue it as a feature in any capacity as part of this obs service or rust in obs.
https://blog.cmpxchg8b.com/2020/07/you-dont-need-reproducible-builds.html
Next, I have a hunch that most packages aren't aware of what the update param is actually doing. By my count, 80 out of the 125 packages that use cargo_vendor are setting update=true and of those 80 I would split into:
- Packages that just copied the service definition from another package
Which is good, because defaults matter, and if people are copying defaults that let us and them apply security updates then that's excellent.
- Packages that didn't read the documentation(or don't know the specifics of
cargo update
) and assumed that update meant to pull any version bumps based on the Cargo.toml- Packages that truly want to break the lockfile and pull in the latest semvar compatible dependencies
I just can't see 80 people falling into category 3 which is why I feel the param is misunderstood. Potentially this could be solved by changing the name to something more conspicuous than "update" or by adjusting documentation.
It's called update because it runs "cargo update". It's pretty clear what it does from the label.
Regardless, I can't see what your "arguments" are. They seem to fall into:
IMO there seems to be no issue here - if you don't like the update flag, then don't set it. The warning you see in the source service check is just a warning so you can feel free to ignore it if you wish.
Build reproducibility is not something I am willing to discuss here. I have no interest in it, and will not pursue it as a feature in any capacity as part of this obs service or rust in obs. https://blog.cmpxchg8b.com/2020/07/you-dont-need-reproducible-builds.html
Miscommunication here, this isn't the same concept I was trying to describe. The issue that's present and I am trying to avoid is for example:
We should let upstream do security updates and be continually pushing out new lock files -> this is not how the rust community works
As per the rust cargo book:
If you’re building a non-end product, such as a rust library that other rust packages will depend on, put Cargo.lock in your .gitignore. If you’re building an end product, which are executable like command-line tool or an application, or a system library with crate-type of staticlib or cdylib, check Cargo.lock into git.
So it would seem the community consenus is to pass around lock files for binary packages for the reasons I mentioned above.
The rust community is able to trust semver and relies on people doing semver updates to tackle security issues like this.
I am a strong supporter of semver and love the fact that I can update my dependencies in a uniform way and almost never have to worry about breakage due to semver non-compliance. But in reality, there is no guarantee here; at some point, someone will make a mistake and you will end up with brokenness. This is fine if you have write access to the repo to fix it and move on, but for a package maintainer you now have a patch to maintain. Until tooling is more mature, practical semver guarantees will remain fairly loose.
While it's great for maintainers to be aware of and able to do security updates, we also need to respond en-masse and apply updates to crates throughout OBS
Valid use case for us as a vendor, so instead add functionality to do something like cargo update -p <package_with_vuln> --precise <fixed_version>
and only (potentially) break builds for known vulnerabilities. Then integrate with cargo_audit and take all the user decision out of it.
Hopefully we can continue discussing and continue to improve Rust-OBS integration. I will help out however I can!
Build reproducibility is not something I am willing to discuss here. I have no interest in it, and will not pursue it as a feature in any capacity as part of this obs service or rust in obs. https://blog.cmpxchg8b.com/2020/07/you-dont-need-reproducible-builds.html
Miscommunication here, this isn't the same concept I was trying to describe. The issue that's present and I am trying to avoid is for example:
- Developer A clones the upstream repo and builds the current main branch
- Developer B sets up an obs service to target the same commit hash that was used in A's build and sets up the cargo_vendor service with update=false
- Developer C does exactly what B does but sets update=true
- A and B are guaranteed to have the same build result whereas C has no such guarantee
If they are cloning the OBS repo they have no need to change the update flag unless they know why they are changing it? I'm not sure I'm understanding what you think the problem is.
We should let upstream do security updates and be continually pushing out new lock files -> this is not how the rust community works
As per the rust cargo book:
If you’re building a non-end product, such as a rust library that other rust packages will depend on, put Cargo.lock in your .gitignore. If you’re building an end product, which are executable like command-line tool or an application, or a system library with crate-type of staticlib or cdylib, check Cargo.lock into git.
- https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries
So it would seem the community consenus is to pass around lock files for binary packages for the reasons I mentioned above.
And that's why we stick the updated Cargo.lock into the vendor tar. Regardless, what repos do in git is not the same as what we have to do in a distro.
The rust community is able to trust semver and relies on people doing semver updates to tackle security issues like this.
I am a strong supporter of semver and love the fact that I can update my dependencies in a uniform way and almost never have to worry about breakage due to semver non-compliance. But in reality, there is no guarantee here; at some point, someone will make a mistake and you will end up with brokenness. This is fine if you have write access to the repo to fix it and move on, but for a package maintainer you now have a patch to maintain. Until tooling is more mature, practical semver guarantees will remain fairly loose.
While it's great for maintainers to be aware of and able to do security updates, we also need to respond en-masse and apply updates to crates throughout OBS
Valid use case for us as a vendor, so instead add functionality to do something like
cargo update -p <package_with_vuln> --precise <fixed_version>
and only (potentially) break builds for known vulnerabilities. Then integrate with cargo_audit and take all the user decision out of it.Hopefully we can continue discussing and continue to improve Rust-OBS integration. I will help out however I can!
In order to do that, we'd basicly need a way to list out semver deps in the obs server on top of what is already there in the Cargo.toml, so that we can specifically target things. You may as well just set "update=false" and then manually change the Cargo.toml with a patch or something else then because that's what you're in effect doing.
If that's how you want to manage your applications, then great!
But going through and manually changing/patching/updating Cargo.toml's isn't my idea of fun when we already have something that works.
Put a different way - I'm not seeing the benefit to your suggestion, I'm only seeing a ton more work, to solve a problem that might maybe eventually happen.
What we have today may be a sledge-hammer, updating everything, but it's a sledge hammer that works and works reliably so far.
- Developer C does exactly what B does but sets update=true
- A and B are guaranteed to have the same build result whereas C has no such guarantee
If they are cloning the OBS repo they have no need to change the update flag unless they know why they are changing it? I'm not sure I'm understanding what you think the problem is.
The problem is that Dev C is the "average maintainer" who is using cargo_vendor with its default settings. His/her goal is probably to update the package to some newer upstream release. So he/she does
osc service ra
andosc build
and ends up with a broken build. So then he or she does a local clone/build of the same upstream release and it builds perfectly. Now he or she goes off trying to fix the build environment (missingBuildRequires
, etc.), but nope, we are broken becausecargo update
was run automatically. And even if I'm expecting this to break at some point and know generally how to fix it, I have to create source patches that don't fix any customer bugs or add any new features.
In regards to a verifiability of the problem, it isn't hard to find breakage:
sevctl
to 0.3.2
and it broke (it's now fixed upstream)virtiofsd 1.5.0
, and it was broken1.1.2
and 1.1.3
were broken and 1.1.4
was fineValid use case for us as a vendor, so instead add functionality to do something like
cargo update -p <package_with_vuln> --precise <fixed_version>
and only (potentially) break builds for known vulnerabilities. Then integrate with cargo_audit and take all the user decision out of it.Put a different way - I'm not seeing the benefit to your suggestion, I'm only seeing a ton more work, to solve a problem that might maybe eventually happen.
The simplest solution is to do exactly what the
go_modules
service does, do nothing and let the language tooling handle it. Of coursego mod
will resolve dependencies to the minimum version andcargo
will resolve it to the maximum version so we really aren't losing much over runningcargo update
. This blog on cargo's version selection does a great job of explaining what I said from the offset: if we are packaging a binary it's imperative for the lockfile to always be preserved. It is the only way that we can achieve "reproducibility" and "compatibility" resulting from using the combination of dependencies that have been tested and are known to work upstream. Note that we don't need to runcargo update
for any library packages sincecargo
will be using the latest deps (with no lockfile) anyways.
See another good read on fearless cargo update and why the ecosystem still has a way to go.
Going back to We also need to respond en-masse and apply updates to crates throughout OBS
, there is no mechanism to execute something like for all OBS packages that use cargo_vendor and have a dependency on [this-package] <= [this_version], update to [that-version]
so inevitably there will have to be some work done by every maintainer that touches the affected package(s).
In the context of update=true
, the maintainer has the easy task of using osc service ra
to update all dependencies, but with the potential (not including interdependencies) to introduce n
unique breakages where n
is the number of dependencies updated by cargo update
. Now how do you upstream this? Commit the lockfile and leave Cargo.toml unchanged? Copy versions from the new Cargo.lock to Cargo.toml and commit one or both? In any case, you potentially end up with changes completely unrelated to the security fix you were actually trying to do.
When update=false
or removing update altogether, you start by patching Cargo.toml but only for the dependency that matters. And you end up with exactly 1 potential breakage. You also now have a trivial patch to maintain and something very easy to upstream.
I'm still not seeing why Rust packages in particular have this notional requirement of tracking the very latest for everything. From what I've seen, no other languages with OBS integration have this requirement. Where is this requirement coming from?
In addition to #21, I still have some issues with the update param that I think warrant some more discussion. At best I feel it is a misrepresentation of functionality and at worst I can't see what utility it even brings.
Starting with the worst case: what are we gaining by running
cargo update
on our dependencies? At the build system level, shouldn't we want the reproducible builds thatcargo vendor --locked
provides? Effectively we are just asserting semvar compliance during a downstream build when this largely should be an upstream concern.Next, I have a hunch that most packages aren't aware of what the update param is actually doing. By my count, 80 out of the 125 packages that use cargo_vendor are setting update=true and of those 80 I would split into:
cargo update
) and assumed that update meant to pull any version bumps based on the Cargo.tomlI just can't see 80 people falling into category 3 which is why I feel the param is misunderstood. Potentially this could be solved by changing the name to something more conspicuous than "update" or by adjusting documentation.