cargo deny check updates Cargo.lock file

[Veetaha]

Describe the bug
We've got a very spectacular master CI failure in our private repo where we use cargo deny.
This bug wasn't hitting us until we started migrating some of our crates to a private crates registry.
In our Cargo.lock file there are currently a bunch of packages with

source = "registry+https://dl.cloudsmith.io/basic/elastio/private/cargo/index.git"

(elastio is our company name)

But this isn't enough, for that to work we have to add our private registry token to ~/.git-credentials and only then cargo will be able to download crates from our private Cloudsmith registry.
Our cargo deny check step didn't write the token to ~/.git-credentials however. It used the other way, where it was setting the env var:

CARGO_REGISTRIES_ELASTIO_PRIVATE_INDEX=https://dl.cloudsmith.io/{redacted_private_token}/elastio/private/cargo/index.git

However, it appears that when this env var is set cargo metadata (that is used by cargo-deny) is updating the Cargo.lock file and it replaces all source = "registry+..." entries with the URL from the env var. But it does not only that, it also updates the versions of the dependencies (just like cargo update does).
For some time it didn't cause us troubles, but one day we published a new major version of our private crate, started incrementally updating Cargo.toml files in our repos and at some point our main repo's master became red because cargo deny check started detecting duplicated entries of the private crate.
And that's because our lock file was updating all the time by cargo deny check
To Reproduce
Steps to reproduce the behavior:

1. Create a cargo project that uses a crate from some custom registry. For this you need to use some crates registry provider (I hope you should have this at Embark).
2. Add the following to .cargo/config in your cargo project root:

[registries]
my-private-registry = { index = "https://dl.cloudsmith.io/basic/my-private-registry/private/cargo/index.git" }

3. Add a crate from the private registry to the dependencies

my-private-crate = { version = "1.0", registry = "my-private-registry" }

4. Run cargo check to generate the Cargo.lock
5. Then run the following (we added -2 suffix to the URL):

CARGO_REGISTRIES_MY_PRIVATE_REGISTRY_INDEX=https://https://dl.cloudsmith.io/basic/my-private-registry/private/cargo/index.git-2 cargo-deny check

6. You should see that the lock file is updating

Expected behavior
I expect that the lock file is not updated if it exists
Proposals
I suppose cargo deny check calls cargo metadata without the --locked parameter. I suggest that you do add --locked to the cargo metadata invocation so that cargo-deny doesn't update the Cargo.lock file. However, this will fail if Cargo.lock is absent, so this should be conditional on the presence of Cargo.lock

Or maybe we should add --locked flag to cargo deny check itself? I am not sure... Is it expected behavior that cargo deny check updates the lock file at all?

[briansmith]

I suppose cargo deny check calls cargo metadata without the --locked parameter. I suggest that you do add --locked to the cargo metadata invocation so that cargo-deny doesn't update the Cargo.lock file. However, this will fail if Cargo.lock is absent, so this should be conditional on the presence of Cargo.lock

Or maybe we should add --locked flag to cargo deny check itself? I am not sure... Is it expected behavior that cargo deny check updates the lock file at all?

I propose that we follow the principle of least surprise and have --locked be the default. I think for anybody who has a Cargo.lock then they don't want it updated. If they don't have Cargo.lock checked into CI then it would be strange to rely on the order of cargo deny and the execution of other tools to generate it; Instead, we should document how to use cargo generate-lockfile or cargo update to generate the lock file if one isn't checked into CI, prior to invoking cargo-deny.

[Jake-Shadle]

I agree in general about needing --locked to be able to be used to avoid mutation on what should be an immutable operation, however I think this would actually be even more confusing if we somehow made it the default as that would be the opposite of every other regular cargo command, as well as potentially breaking users, at least the ones that don't use the github action we supply.

I think the best and clearest option is to expose --locked at the same level as --all-features etc that will be passed down to command to gather the metadata, so that users can use the same flags they would with any other command that could potentially modify the lockfile.

[briansmith]

@Jake-Shadle IMO --locked should be the default for every command. But, given that that isn't the case, I also agree it is reasonable to be consistent with the other (built-in) commands.

[briansmith]

A potential compromise would be to have the failure to pass --locked should cause a warning to be omitted when there is a Cargo.lock, since I don't think using the tool without --locked is what people want, at least in CI, when they have a Cargo.lock.

[Jake-Shadle]

The problem with that is it would require users to actually look at their CI runs for warnings, but in my experience, CI logs are only checked when there is a build failure/error. That being said, we could add a top-level configuration option to emit errors if a lockfile is present and the --locked flag is not supplied if an environment variable is present/set to a specific value (ie most CI environments set the CI environment variable to ""/"true")

[briansmith]

(ie most CI environments set the CI environment variable to ""/"true")

I generally try to make my CI scripts do the same thing when run locally as they'd do when run in CI, to help with debugging and troubleshooting and to allow developers to check that CI will pass before submitting, so I generally don't like tools to act differently in CI like that.

BTW, I noticed in rustsec/rustsec#322 that cargo audit acts like --locked by default. I think cargo deny and cargo audit should be consistent with each other w.r.t. the "locked" behavior and defaults.

I propose:

1. We add --locked regardless of what is decided for the default behavior, to give people a clear explicit way to express the intent, without changing the default behavior.
2. Separately, we then decide what to do by default.

Regarding the default, I think we need to lay out explicitly what constraints we have before we try to solve for a solution.

• Do we want to be consistent with cargo build w.r.t. default behavior (of --locked) when there is no Cargo.lock file?

• Do we want to be consistent with cargo audit w.r.t. default behavior (of --locked) when there is no Cargo.lock file?

• If we have to choose differently between the above two, which should we favor?

• Do we want to be consistent with cargo build w.r.t. default behavior (of --locked) when there is a Cargo.lock file?

• Do we want to be consistent with cargo audit w.r.t. default behavior (of --locked) when there is a Cargo.lock file?

• If we have to choose differently between the above two, which should we favor?

Maybe cargo {build, test, update} are actually the exception case for --locked? IMO we should be advocating, perhaps in the next edition, to change the default to be --locked for them; however, I am not sure that's a winnable battle.

I am starting to think that cargo audit has better, if not perfect default behavior, based on the description in rustsec/rustsec#322.

[adam-azarchs]

I think it would be nice if it also supported the --frozen flag, which (for other cargo commands) is basically --locked but also fails if Cargo.lock is out of date.

[Jake-Shadle]

I've closed this issue, but please comment in #360 for thoughts on default behavior with regard to these flags.