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

Add support for using lock files #991

Open
leighmcculloch opened this issue Nov 4, 2024 · 6 comments
Open

Add support for using lock files #991

leighmcculloch opened this issue Nov 4, 2024 · 6 comments
Labels
C-enhancement Category: raise the bar on expectations

Comments

@leighmcculloch
Copy link

Describe your use case

When running cargo semver-checks no lock files appear to be used, either for the current / local version of the crate, or for versions of the crate downloaded from crates.io.

This creates a reproducibility issue. When crates publish releases that do not follow semver exactly, semver may fail to compile if no lock files is used. It also creates a situation where one run of the semver-check and a subsequent run can have different inputs and outputs, meaning no reproducibility in CI.

This creates a security issue. Some times use lock files to ensure that the developer is exclusively in control of what versions of dependencies are installed and in use. This is to avoid the problem that just before running a command a crate is compromised and a malware version is uploaded for it. Today using semver-checks as it is appears to expose us to that possibility when other cargo commands provide the --locked option.

Describe the solution you'd like

The ability to pass --locked to the semver-checks command, and for the lock file to be used.

Alternatives, if applicable

No response

Additional Context

No response

@leighmcculloch leighmcculloch added the C-enhancement Category: raise the bar on expectations label Nov 4, 2024
@obi1kenobi
Copy link
Owner

Hi, thanks for opening this issue.

I'd like to get a bit more context on the workflow you're using and the specific use case, to make sure I understand everything properly.

Additionally, could you say more about this part?

Some times use lock files to ensure that the developer is exclusively in control of what versions of dependencies are installed and in use.

I'm having trouble reconciling this with the fact that developers specify the ranges or exact versions they want in Cargo.toml. The lockfile would only contain a version that is explicitly allowed by the range or exact bound set in Cargo.toml.

Separately, could you describe the possible security issue you are referring to?

Note that --locked exists for cargo install which is specific to binary crates. Library crates, which is what cargo-semver-checks analyzes, do not always contain a Cargo.lock file as part of their package index entry. There has been some motion toward changing that in the future, but unfortunately we can't add those files retroactively: rust-lang/cargo#13447

@leighmcculloch
Copy link
Author

leighmcculloch commented Nov 5, 2024

Hi @obi1kenobi! Thanks for asking more questions.

We use cargo-semver-checks in the following workflow:

The soroban-sdk crate in the repos is dependent on the arbitrary crate with version specifier 1.3.0, so all versions >= 1.3.0, < 2.0.0 are supported. Which I think is reasonable assuming the crate follows semver.

arbitrary 1.4.0 was released that unfortunately contains some breaking changes (rust-fuzz/arbitrary#203).

When cargo-semver-checks runs now, comparing the main branch against the last release 21.7.6, it fails to build because ignoring the lock file it pulls in arbitrary 1.4.0.

Even if I update the dependency version specifier on the main branch to ~1.3.0, so compatible versions are constrained to >= 1.3.0, < 1.4.0, that will only apply to the main branch and not the previous release being compared to. 21.7.6 will fail to build because it was dependent on 1.3.0 which says it is compatible with 1.4.0.

https://github.com/stellar/rs-soroban-sdk/actions/runs/11672542695/job/32501403700

Some times use lock files to ensure that the developer is exclusively in control of what versions of dependencies are installed and in use.

We commit lock files so that our builds are consistent, and we're 100% in control of what dependencies, and what versions of dependencies, are in use in our CI environments (that may have access to credentials like a crates.io auth token), and in use on our workstations. We choose when we change dependencies in our development workflows.

@obi1kenobi
Copy link
Owner

Ah hmm interesting. Tricky case for sure, thank you for bringing it to my attention. Thanks also for the additional context.

In addition to worrying about rust-lang/cargo#13447, I need to figure out a couple more things here:

When crates re-export foreign crates' types, ignoring the lockfile is usually the desirable behavior because it accurately reflects what would happen on a new cargo add. Otherwise it's possible for a project's cargo-semver-checks run to succeed based on the lockfiles but to be broken for downstream projects on cargo add due to a dependency. For example, if your crate had been re-exporting things form arbitrary, that case would have happened right here.

We also cannot say "use the lockfile in the current project but if it's missing on crates.io just re-lock from there." That would lead to "back to the future" style bugs: say your library re-exports a 3rd party crate's type, then re-locking the crates.io version can produce a newer version of that dependency in the baseline than in the current version. API additions to that newer dependency version (in the baseline) are not present in the older dependency version (in your current lockfile) — it looks like your current package has deleted things from its public API! This has happened before e.g. in #167.

A good solution to this issue shouldn't be "just a different kind of footgun" than we already have. I need to figure out how to make that happen, and whether rust-lang/cargo#13447 makes progress or not is also a key question here.

Sorry I don't have a more immediately-useful answer here!

Btw, if cargo-semver-checks has been useful to your project, I'd really appreciate if your org started sponsoring me on GitHub 💖

@leighmcculloch
Copy link
Author

leighmcculloch commented Nov 5, 2024

Similar tensions exist with the use of --locked on the cargo install command. On that command for reproducibility one often wants to lock, but when there are security fixes in dependencies and versions have been yanked it's often desirable to cargo install without --locked to get that one dependency upgraded and there's no way to easily install with that one dependency upgraded. It's not quite the same thing, but the presence of --locked at least allows the option to test with a dependency graph set in time to something.

It may be desirable for some folks to run cargo semver-checks with and without --locked so as to get a view on how things have changed with what is likely (but not guaranteed) to be the minimum supported versions, and the latest bleeding edge versions. Perfect may be the enemy of good here, but I understand the path forward is pretty unclear.

@obi1kenobi
Copy link
Owner

A specific concern is whether --locked for us should mean:

  • use the locked dependencies both for the baseline (e.g. crates.io version) and current code in the repo, and error if crates.io doesn't have a lockfile
  • use it for one but not the other (2 options)
  • one of the above variants, but the crates.io lockfile is best-effort and move on if it doesn't exist

This axis is something other cargo components don't have to worry about, so we don't have a good precedent to go on =/

@leighmcculloch
Copy link
Author

Based on the options on the current command, I think it would be reasonable to have --locked which applied to only the current, not baseline, since that's what most options already do on the command. Then have --baseline-locked that applies to the baseline, following the existing pattern.

In either case if there's no lock file, then I think it's reasonable to fail. The tool doesn't need to solve problems that the developer can control.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: raise the bar on expectations
Projects
None yet
Development

No branches or pull requests

2 participants