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

Publish lockfile for binary crates #5654

Closed
matklad opened this issue Jun 26, 2018 · 33 comments · Fixed by #7026
Closed

Publish lockfile for binary crates #5654

matklad opened this issue Jun 26, 2018 · 33 comments · Fixed by #7026
Labels
C-tracking-issue Category: A tracking issue for something unstable. disposition-merge FCP with intent to merge finished-final-comment-period FCP complete T-cargo Team: Cargo

Comments

@matklad
Copy link
Member

matklad commented Jun 26, 2018

Implementation PR: #5093

Summary:

For binary crate, add

cargo-features = ["publish-lockfile"]`
[project]
publish-lockfile = true

to Cargo.toml. cargo install will then use the lockfile of binary crates.

Steps:

Feature ready for stabilization

Stabilization TODO:

@polyzen
Copy link

polyzen commented Nov 8, 2018

Any ETA on this, or anything I could do to help?

@dwijnand
Copy link
Member

Continuing from #6556 (comment):

I think it would be good to start looking at fixing any known issues and moving towards stabilizing that. Although if it is optional, I'm not sure if most people will use it, so it may not help here. I wonder if it was considered if it should always be published, and let the installer decide whether or not to use it?

Why shouldn't it be published? Why would an installer not want to use it?

@Nemo157
Copy link
Member

Nemo157 commented Jan 17, 2019

One reason would be if there were security issues with the any of the versions pinned in the lock file, even if those crates were yanked and semver compatible releases made the lock file would still cause the old versions to be used until the binary crate released a new patch version with an updated lock file.

@ehuss
Copy link
Contributor

ehuss commented Jan 17, 2019

Why would an installer not want to use it?

My only thought is that using the latest versions will bring in the latest and greatest fixes. But maybe the frequency of "cargo install fails" outweighs that? Maybe it could be the default to always use Cargo.lock if available, and have an option to ignore it? 🤔

This bit me recently where mdbook would fail to install (rust-lang/mdBook#852).

@dwijnand
Copy link
Member

I guess we could dual cargo build --locked and have cargo install --unlocked 😁

@dwijnand
Copy link
Member

Btw, there's more detail in the chat in #5093.

@Nemo157
Copy link
Member

Nemo157 commented Jan 17, 2019

I should say I'm for having this feature working, I have encountered the same failure in a binary crate I manage as well.

I think having cargo install --unlocked along with a prominent warning when cargo install with a lockfile brings in a yanked package could be enough to mitigate the security issue. If that's not enough, maybe even require cargo install --allow-yanked or something so that the user must affirmatively allow it.

@polyzen
Copy link

polyzen commented Jan 17, 2019 via email

@Nemo157
Copy link
Member

Nemo157 commented Jan 17, 2019

@polyzen that's related to libraries, this issue is only about installing binaries (a crate can contain both a library and a binary, but my expectation is that even if it contains a lockfile that will be ignored when used as a library).

@polyzen
Copy link

polyzen commented Jan 17, 2019

I was responding to @dwijnand's original question, but (on FastHub) it doesn't seem like his message got quoted.

@dekellum
Copy link
Contributor

For some reason before finding this issue, I thought including the lock file was now default (and therefore stabilized) for all bin crates publishing. Its awkward that this is nightly only, as enabling it would normally interfere with CI (using rust versions other than nightly). However I'm already publishing some crates via a dedicated, publishing-specific git branch, so I can disable CI (or only CI on nightly) in that branch and still use it. Yah!

@matklad's top Summary has a minor typo:

cargo-features = ["publish-lockfile"]`
- [project]
+ [package]
publish-lockfile = true

I do agree with @Nemo157, that minimally, cargo install --allow-yanked should be implemented, and ideally also the cargo install --unlocked flag, including an initial warning that the lock file was downloaded/found and used in the first place. Only the former would seem necessary for the security concern and stabilizing this.

Thanks!

@dwijnand
Copy link
Member

I was responding to @dwijnand's original question, but (on FastHub) it doesn't seem like his message got quoted.

Thanks, but that link's about version control, though, while this issue is about publishing crates.

@dwijnand
Copy link
Member

Its awkward that this is nightly only, as enabling it would normally interfere with CI (using rust versions other than nightly).

Agreed, it's a shame that one cannot opt-into cargo unstable features on stable.

@polyzen
Copy link

polyzen commented Jan 17, 2019

Thanks, but that link's about version control, though, while this issue is about publishing crates.

Crates used to publish all files checked into version control, including lockfiles. Due to the issue with libraries, people would exclude their libraries' lockfiles from their repos.

Eventually lockfiles were filtered out from crates so people wouldn't have to do that. Now we have publish-lockfiles so people can use --locked for binary crates.

@dekellum
Copy link
Contributor

Actually, if crates.io security advisories were implemented, along the lines of:

https://internals.rust-lang.org/t/pre-rfc-reviving-security-advisories-in-crates-io-rfc-pr-1752/9017

..then this would probably cover the security concern as well, and in a more general and complete way.

cc: @tarcieri

@tarcieri
Copy link

tarcieri commented Jan 17, 2019

This seems related/relevant: including Cargo.lock in binary artifacts (cc @Shnatsel)

rust-secure-code/wg#14

One of the goals of the above is definitely to allow RustSec audibility of compiled Rust artifacts, and it seems like that might be useful for solving this particular problem as well.

dekellum added a commit to dekellum/body-image that referenced this issue Jan 17, 2019
As per tracking issue rust-lang/cargo#5654, this is currently a
nightly-only feature and would fail CI or user which is not using a
nightly-channel rust+cargo.
@dekellum
Copy link
Contributor

In setting up to try and use this feature (above referencing commit), I note that:

cargo-features = ["publish-lockfile"]

...is retained in (re-generated) Cargo.toml that is included in the package. In testing a local install at least, I get the following error:

% cargo +1.31.0 install --path ./barc-cli
error: `/home/david/src/body-image/barc-cli` is not a crate root; specify a crate to install from crates.io, or use --path or --git to specify an alternate source

Caused by:
  failed to parse manifest at `/home/david/src/body-image/barc-cli/Cargo.toml`

Caused by:
  the cargo feature `publish-lockfile` requires a nightly version of Cargo, but this is the `stable` channel

The first error line is confusing or perhaps a different issue, but I think I understand the "Caused by" part. Before I go through the trouble of publishing a potentially broken patch release of barc-cli, could anyone confirm/deny if the same error will occur when attempting to install such a published crate on a stable-channel rust via crates.io? If this is the case, was it intentional that the feature is limited to publishing crates (using a nightly), that can only be consumed by a nightly rust? I find that surprisingly narrow of a target. It would be helpful to clarify that, here, ether way.

@dekellum
Copy link
Contributor

Well, if its not ok to publish via nighty cargo a crate with MSRV to some stable version, then that seems like a much bigger problem than (lack of) this feature! Isn't that how many or most crates are published? The tendency to develop on nightly, CI and publish for nightly+stable+MSRV?

@dwijnand
Copy link
Member

Yeah, I think we do this by making cargo forward-compatible first, let it ride the trains to stable, then make the change to master/nightly cargo. For example what we're doing in #6180.

bors added a commit that referenced this issue Jan 17, 2019
Better error message for bad manifest with `cargo install`.

The old code assumed that any error loading a manifest meant that the
manifest didn't exist, but there are many other reasons it may fail.
Add a few helpful messages for some common cases.

First noticed at #5654 (comment)
@nrc
Copy link
Member

nrc commented Mar 13, 2019

We discussed this today in the Cargo meeting. We do think it is good to stabilise the core concept soon, we want to change a few aspects of the UX:

  • by default, publishing a binary would include the Cargo.lock file. This could be overridden in the Cargo.toml.
  • by default, installing a binary does not use the Cargo.lock file, but this could be overridden with a command line argument
  • Cargo.lock files should be verified as part of the pre-publication verification step.

@Shnatsel
Copy link
Member

Shnatsel commented Mar 13, 2019

I would strongly oppose this because this makes propagating security updates to the ecosystem effectively impossible, at least until a first-class mechanism for security updates is in place.

Right now the user is not notified about security updates, but at least cargo install pulls in latest versions with presumably latest fixes. If binary crates ship with a specific Cargo.lock, propagating a security update to a library would require action not only from the library author, but also from maintainers of every single binary crate that depends on the library, including indirect dependencies.

@dwijnand
Copy link
Member

@Shnatsel as per @nrc's comment, cargo install will, by default, continue to pull in the latest versions of dependencies (ignoring the lock file). The change is that a lock file will be published (after being verified) and that lock file may be used, if a user chooses to.

@dekellum
Copy link
Contributor

Right now and without this feature, some users (myself included) are opting to use a specific/controlled Cargo.lock, by simply committing and deploying a project's Cargo.lock with the source, building the application/service on the target (or an intermediary) host/container using that lock file, then running it. This makes good sense for repeatable deployment of non-public applications, which we must assume are the vast majority. I think rust-lang/docs.rs is one prominent, public example of this strategy, though. There are likely others.

It seems to me that any security concerns should put that in perspective, and any security efforts would be much better spent on doing the thing of most obvious utility: Any cargo command that downloads crates, and ideally any subsequent build or run, should emit WARNINGs for any crates with known security issues, or at the very least, crates that have been yanked (for any reason).

Said another way: Security and stabilizing this feature isn't an either/or situation, particularly if actually using the Cargo.lock is stabilized here as a non-default option.

@dekellum
Copy link
Contributor

@nrc:

  • Cargo.lock files should be verified as part of the pre-publication verification step.

What does that mean in this context? Would cargo, for example, refuse publication if a lock dependency has been yanked (already, at that time)? Security wise I still think the above described warnings are both sufficient and more broadly useful. Users should hopefully see such warnings sooner in there process. But it would make sense to fully re-run such checks at publication time.

@sanmai-NL
Copy link

Fully agree @dekellum! There are plenty of vulnerability scanning and auditing tools to deal with vulnerable dependencies (e.g., container layers, Linux distro packages, malware patterns within executables, and even open source components). @Shnatsel, see https://github.com/RustSec/cargo-audit.

@ehuss ehuss added the T-cargo Team: Cargo label May 31, 2019
@ehuss
Copy link
Contributor

ehuss commented May 31, 2019

This is a proposal to stabilize the publish-lockfile feature (in a limited sense).

@rfcbot fcp merge

Stabilization Target: 1.37 — Release date August 15th 2019

What is being stabilized

Cargo will be changed to always include Cargo.lock in the .crate file if the package has binaries or examples.

This has no immediate impact on users. cargo install ignores the lock file by default. A user must use cargo install --locked to use the packaged lock file.

NOTE: The publish-lockfile key in Cargo.toml is specifically not being stabilized, and will be removed at some date in the future if we decide it is ultimately not useful. At this time there does not seem to be compelling use cases to forcefully prevent Cargo.lock from being included.

Stabilization will also include documentation updates to make note of this change, and calling out the use of cargo install --locked.

The primary motivation for this feature is to allow cargo install to use the same set of dependencies that was tested when the crate is published. This can be useful for automated environments to always install the same thing. It is also useful when a semver-incompatible change is accidentally published, providing a way for users to fall back to a compatible set.

More details

  • Cargo.lock is regenerated when the .crate file is created, guided by any existing Cargo.lock. The general consequence is that the Cargo.lock may be different from the one in the current workspace. The published version will not include unrelated workspace members or [patch] entries, and some things like path dependencies may resolve differently.
  • Warnings have previously been added for:
    • Creating a .crate file with yanked dependencies.
    • cargo install --locked where an entry in the Cargo.lock is yanked.

@rfcbot
Copy link
Collaborator

rfcbot commented May 31, 2019

Team member @ehuss has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels May 31, 2019
@Nemo157
Copy link
Member

Nemo157 commented May 31, 2019

Cargo.lock is regenerated when the .crate file is created, guided by any existing Cargo.lock. The general consequence is that the Cargo.lock may be different from the one in the current workspace.

Can --frozen/locked be used to ensure it is identical; or does that only apply to updating the in-workspace lockfile, not the newly generated one?

@ehuss
Copy link
Contributor

ehuss commented May 31, 2019

Can --frozen/locked be used to ensure it is identical; or does that only apply to updating the in-workspace lockfile, not the newly generated one?

No. IIRC, that applies to the verify-step only.

In the cases where the lock file changes, using the original would either be broken, or just contain extra entries that are ignored.

@rfcbot rfcbot added final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Jun 5, 2019
@rfcbot
Copy link
Collaborator

rfcbot commented Jun 5, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

bors added a commit that referenced this issue Jun 11, 2019
Stabilize publish-lockfile.

This stabilizes the publish-lockfile feature. Specifically:

- Makes `Cargo.lock` included by default for packages with executables.
- Deprecates the `publish-lockfile` manifest key. It is no longer used.

Additional notes:

- Fixed issue where if a `Cargo.lock` file didn't exist, `cargo package` would fail the
  VCS dirty check.
- Changed it so that `cargo publish` or `cargo package` will now show manifest
  warnings. I believe this was an oversight.

Closes #5654
@rfcbot rfcbot added finished-final-comment-period FCP complete and removed final-comment-period FCP — a period for last comments before action is taken labels Jun 15, 2019
roblabla added a commit to roblabla/KFS that referenced this issue Mar 10, 2020
Ensure we build xargo and cargo-make in a reproducible way to prevent
bitrot. We now specify a version and ask for a locked build. My
understanding is that --locked will use the lockfile packaged by the
crate, see rust-lang/cargo#5654.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for something unstable. disposition-merge FCP with intent to merge finished-final-comment-period FCP complete T-cargo Team: Cargo
Projects
None yet
Development

Successfully merging a pull request may close this issue.