-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Security advisories in crates.io #1752
Conversation
Discourse's markdown parser doesn't like the current ones
Also unify some terminology: "report" / "vulnerability report" => "advisory" / "security advisory"
|
||
Here's the workflow: | ||
|
||
1. The user invokes `cargo advisory` without the `--upload` option. Cargo will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may be --generate here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea behind that awkward phrasing is that both cargo advisory
and cargo advisory --generate
are valid for this step. "without --upload" is a phrasing that captures both.
(damn "start a review" button)
When compared to other ecosystems such as Python's, Rust's broader community | ||
prefers many single-purpose crates over larger monoliths. This situation, | ||
together with the strongly encouraged practice of pinning MINOR versions of | ||
dependencies, slows down the propagation of critical security fixes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not aware of pinning minor versions in Cargo.toml
being encouraged (except the 0.x.y
case but then x
is the major version for cargo-semver purposes). Can you elaborate on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right about that, I only thought about the 0.x.y
-case, but changing MINOR to MAJOR in that sentence is also wrong. What about this:
...with the strongly encouraged practice of pinning version ranges (sometimes specific versions)
Specific versions are pinned if a semver-compatible version of a dependency introduces a bug that breaks the dependent crate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this bit of motivation doesn't work because
- pinning to specific versions is not the norm,
- cargo makes the ultimate consumer track precise versions via lockfile, giving them more power and responsibility.
So applying the fix should normally not be a problem. Only if the intermediate crates actively prevent this by strict version pinning do you need to act differently (e.g. prevent publishing a crate which makes it impossible to avoid vulnerable dependency versions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the pre-pre-RFC thread I gave a different motivation. Money quote:
Unfortunately I'm depending on open source libraries where nobody gets paid for their work. This means that the maintainer of Z is less likely to backport the patch to other MINOR versions, only the latest version gets a PATCH release. The maintainers of X and Y are less likely to update their dependencies either, because they do all of this in their freetime as well. This is a social problem though, and people are just getting started12 writing about this issue.
I agree with the motivation being a bit weak. Do you think the scenario described in that thread above is more realistic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rewrote the entire motivation. Better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, as opposed to "*"
as version specifier, which Crates.io rejects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the text to concretely mention why pinning is encouraged. Note that I wrote "version ranges" (e.g. ^1.0.0
) as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as opposed to
"*"
as version specifier, which Crates.io rejects
I see, it's the "pinning" part because I wouldn't consider "^1.0.0" pinning, it implies something like "=1.0.0" to me. But is it right to treat them the same way? I don't think "^1.0.0" poses any propagation problems. Dealing with more exotic cases is important but probably won't be the primary focus of this feature.
I don't understand what you mean. Dealing with upgrades is not concern of this RFC at all.
It is because you're opening with slow propagation of security fixes. It's probably stronger to first discuss only the communication part: all users need to be made aware of the advisory. (BTW I wouldn't mind cargo
outright failing the build by default if any dependency is found to be vulnerable.) The propagation discussion can build on that but the feature is valuable even without it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I'm not taking seriously enough the scenario of a "1.x" branch being unmaintained and vulnerable (i.e. cargo update
definitely won't work).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new example I wrote in 06259c1 doesn't imply that.
Assume a crate `W`, which depends on `X`, which depends on `Y`, which depends | ||
on `Z`. If `Z` releases a new MINOR version including a security fix, it | ||
requires the attention of `Y`'s and `X`'s maintainers to propagate that | ||
security fix to `W`. What makes this situation worse is that the author of `W` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If X
and Y
use the (default) caret requirement, performing cargo update
on W
will update Z
(once again, for 0.a.b
MINOR version is b
as far as caret requirement is concerned).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about "semver-incompatible version"? Also I probably should clarify what the version pinning for all crates looks like. I also made the assumption that each crate exposes its dependency's API and therefore needs to do a new semver-incompatible release when their dependencies do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if the fix requires a major version bump, you get the same scenario as with pinned versions.
Well it seems like you're solving a pretty specific problem (a discontinued |
No, that's not the problem I'm trying to solve, that's a pretty common example that exhibits that problem, where the entire issue is not just solved by running |
Enterprise users might only want to do |
Is this something I should write into the Motivation? I feel like it's a bit On Thu, Sep 22, 2016 at 01:56:48PM -0700, arielb1 wrote:
|
@untitaker thanks, it looks great now! |
This looks good to me on my first pass through |
Next week it'll be a month this PR has been open. I understand that this is hard to decide on but I suspect that it is also slipping through the cracks. |
Shall there be advisories for Rust system components (like libstd or libcore)? Shall definition of a security vulnerability included in RFC? For some things (especially in libraries) it can be tricky to discriminate just bugs and vulnerabilities. For example, is memory corruption by a data structure with a safe interface always an advisory-worthy vulnerability? |
@vi the ruby-advisory-db tracks similar vulnerabilities in the Ruby standard library. I think it would be nice to have functionality-wise, but seems a bit orthogonal to advisories for specific crates, and might be something to punt on initially in the hopes of getting anything out the door. |
@tarcieri, @vi: In addition, the push towards stdlib-aware Cargo (and the possible future of much of the stdlib being on crates.io natively) may make it a moot point. |
What does Cargo do with yanked packages? |
What about private Rust projects that are never published to crates.io? Shouldn't they also benefit from advisories? That's why advisories also make sence for crates without reverse dependencies. Also |
My inclination is no, or rather, if they'd like to use this feature, they should run an internal Cargo repository with something that speaks the same API as crates.io (similar to Gemstash) and can thereby manage advisories for internal crates. |
Network access during compilation is a dealbreaker for many bigger Linux distributions, which is why I'm trying to avoid it. For the dev workflow it would be better of course. |
There may be |
Reading over this, I unfortunately don't have a lot of time to follow the conversation and help push this over the finish line, but some thoughts I had were:
I'm sorry that this is taking awhile, but please remember that we're all very busy with lots of projects in flight, so we don't always have a lot of time to allocate to all RFCs coming down the pike. |
The assertion @tarcieri and I have made in this and other threads is that overloading yank with a multitude of "yank reasons" compromises the security aspect of the proposed feature. Tony said it better than I could. My personal viewpoint is that cargo should provide a panic button for security incidents where the user is guided through proper disclosure, not a swiss-army knife where the answer to "What do I do next?" is "it depends, there are endless possibilities". Whether this RFC achieves that is left to discuss. EDIT: Note that internally you could model the API such that it uses yank and
I'm very confused about this reasoning. If a dependency has a vulnerability, and I can disable the corresponding warning with a whitelist, how does this generate warning ridden builds? The purpose of the whitelist is to turn off security warnings that affect featuresets I don't use.
That's totally fair criticism. The bool value is a leftover from older drafts and the preference for underscores is my own bias as a Python developer, and because there are no keys with dashes except for package names in the TOML. In either case it's not a conceptual problem. |
There exists |
Fair enough, fixed. |
@alexcrichton I'm not necessarily opposed to However, the goal of a feature like this is to build a security advisory database which security-related tooling can consume. One like this (except as an integrated part of https://crates.io as opposed to a git repo of YAML files on GitHub): https://github.com/rubysec/ruby-advisory-db More than just eliminating security vulnerabilities during development, such a system can aid security teams in monitoring deployed versions of apps containing vulnerable crates with the assistance of endpoint agents. If this can be accomplished with adding a set of security-related flags to Security advisories by nature span multiple versions (while still being associated with a single logical CVE/DWF-like ID), but Having a generic description associated with a yank event is much less useful than a dedicated security advisory database. |
I don't like fusing of yanking and advisories.
|
Although this seems like a reasonable thing to want, I'm wary to devote a lot of infrastructure to this use case without having more evidence that it's needed now and the design is right. There are a number of specifics here and it seems like there's a fair opportunity to overdesign something that ends up being the wrong design in practice, or just underutilized for whatever reason. Text in the RFC indicating precedent in other package managers, their success, and how this design relates to them, would bolster the argument. A more general, and more incremental approach that doesn't preclude adding a full security advisory feature as specified here, might be "yank with message". Another incremental approach would be to develop something out-of-tree and prove it out, before trying to add the feature to cargo. As an example, I'm not sure that there are even enough crates with known security vulnerabilities to warrant the infrastructure necessary to add a feature to cargo to let one publish advisories from the command line. We could start by just setting up a repo on github containing a blacklist, which people can submit PRs to when security problems become known. Then either cargo itself or a |
I would agree it might be best to start out by developing this out-of-tree. Maybe just an advisory database on GitHub you contribute to with pull requests storing the advisory data format, and a CLI tool you can |
Since so many core members of Rust are against this only form that I consider viable I'm going to close this pull request. I agree that developing this feature in a separate repo outside of crates.io is good as PoC, but I don't think that a separate project would attract the amount of users necessary to make it actually useful. If anybody wants to work with me on a PoC feel free to email me and I'll be happy to help wherever I can, but my motivation to e.g. implement this feature all by myself (which I initially had) has faded away. |
If anyone is interested in continuing this work, I've created a project to continue pursuing it: https://github.com/rustsec/ I've imported the latest version of this RFC into a repo here if anyone would like to continue discussion: https://github.com/rustsec/rfcs/pull/1 At my job we're looking at using Rust in a "high assurance" capacity, so vulnerability tracking is definitely something I'd love to see tackled. |
Audit Cargo.lock for crates with security vulnerabilities reported to the RustSec Advisory Database. This is a PoC implementation of the closed RFC 1752: rust-lang/rfcs#1752 WWW: https://rustsec.org/ git-svn-id: svn+ssh://svn.freebsd.org/ports/head@481321 35697150-7ecd-e111-bb59-0022644237b5
Audit Cargo.lock for crates with security vulnerabilities reported to the RustSec Advisory Database. This is a PoC implementation of the closed RFC 1752: rust-lang/rfcs#1752 WWW: https://rustsec.org/
Audit Cargo.lock for crates with security vulnerabilities reported to the RustSec Advisory Database. This is a PoC implementation of the closed RFC 1752: rust-lang/rfcs#1752 WWW: https://rustsec.org/ git-svn-id: svn+ssh://svn.freebsd.org/ports/head@481321 35697150-7ecd-e111-bb59-0022644237b5
Audit Cargo.lock for crates with security vulnerabilities reported to the RustSec Advisory Database. This is a PoC implementation of the closed RFC 1752: rust-lang/rfcs#1752 WWW: https://rustsec.org/ git-svn-id: svn+ssh://svn.freebsd.org/ports/head@481321 35697150-7ecd-e111-bb59-0022644237b5
Audit Cargo.lock for crates with security vulnerabilities reported to the RustSec Advisory Database. This is a PoC implementation of the closed RFC 1752: rust-lang/rfcs#1752 WWW: https://rustsec.org/
pre-RFC here: https://internals.rust-lang.org/t/pre-rfc-security-advisories-as-part-of-crates-io-metadata/4045