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

git dependencies in a local-registry require a non-existent checksum #10939

Open
jarhodes314 opened this issue Aug 4, 2022 · 6 comments · May be fixed by #11188
Open

git dependencies in a local-registry require a non-existent checksum #10939

jarhodes314 opened this issue Aug 4, 2022 · 6 comments · May be fixed by #11188
Labels
A-local-registry-source Area: local registry sources (vendoring) C-bug Category: bug S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@jarhodes314
Copy link

jarhodes314 commented Aug 4, 2022

Problem

Ordinarily cargo doesn't require checksums to be present for git dependencies, and therefore it does not include a checksum in the lockfile for any git dependencies. If, however, the git dependency is vendored in a local registry (e.g. with cargo local-registry), cargo verifies the checksum when unpacking the crate, due to

let actual = Sha256::new().update_file(&crate_file)?.finish_hex();
if actual != checksum {
anyhow::bail!("failed to verify the checksum of `{}`", pkg)
}
executing regardless of whether a checksum actually exists for the given crate.

I've attempted to work around this by adding any missing checksums to the index of the local registry after generating/updating the local registry. This does allow my crate to compile, but I can then no longer update the registry as the checksums cargo has added to the lockfile are no longer expected when source replacement is disabled since git dependencies do not really have a checksum.

It appears to me that the checksum should just not be verified in cases where it is not present in the index.

Steps

  1. Create a new cargo project
  2. Add libc as a dependency with the git url: libc = { git = "https://github.com/rust-lang/libc" }
  3. Run cargo install cargo-local-registry --version 0.2.2
  4. Run cargo local-registry -s Cargo.lock registry
  5. Create a .cargo/config.toml in the current directory with the following contents:
[source.local-registry]
local-registry = "./registry"

[source.libc-git]
git = "https://github.com/rust-lang/libc"
replace-with = "local-registry"
  1. Run cargo build and observe that libc cannot be found in the local registry (to ensure the local-registry is correctly configured).
  2. Run cargo local-registry -s Cargo.lock registry --git to create a local registry including git dependencies.
  3. Run cargo build again and observe the "failed to verify checksum" error.

Possible Solution(s)

Where a checksum does not exist, for a package in a local registry, cargo should not attempt to verify it.

Notes

No response

Version

cargo 1.61.0 (a028ae4 2022-04-29)
release: 1.61.0
commit-hash: a028ae42fc1376571de836be702e840ca8e060c2
commit-date: 2022-04-29
host: x86_64-unknown-linux-gnu
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.80.0-DEV (sys:0.4.51+curl-7.80.0 vendored ssl:OpenSSL/1.1.1m)
os: Ubuntu 20.04 (focal) [64-bit]
@weihanglo
Copy link
Member

weihanglo commented Sep 23, 2022

Sorry for replying this thread late.

I am not sure whether we can simply skip the checksum verification. From my understanding, the present of checksums is for basic filesystem corruption detection. It would be robust as before if we can keep this mechanism intact. In addition, Cargo knows nothing about the original source of a package from a local registry. Skipping checksum verifications sounds like leaking the details behind the local registry implementation (both in Cargo and cargo-local-registry).

Personally, I lean toward generating a checksum from cargo-local-registry if possible. For reference, a similar command cargo vendor generates a .cargo-checksum.json for git dependency as well. That doesn't mean refusing any change in Cargo. We can still explore possible approaches to circumvent it from Cargo side.

Edited: mentioned cargo vendor

@jarhodes314
Copy link
Author

jarhodes314 commented Sep 29, 2022

Using cargo vendor with a git dependency generates a .cargo-checksum.json that includes "package":null, which as far as I can tell is entirely equivalent to what cargo-local-registry is doing.

if let Some(package) = &cksum.package {
pkg.manifest_mut()
.summary_mut()
.set_checksum(package.clone());
}
looks like we skip the check in the case where a checksum does not exist (e.g. a git dependency). I think the local source should have a similar exception.

We could conditionally skip the check by changing the condition to something like:

actual != checksum && !checksum.is_empty()

If skipping the check is not an option, generating a checksum from cargo-local-registry is entirely possible, but attempting to update the registry causes cargo's resolver to fail with the following error (package name and url changed for anonymity):

error: failed to sync

Caused by:
  failed to load pkg lockfile

Caused by:
  checksum for `my-package v0.1.2 (ssh://git@...)` could not be calculated, but a checksum is listed in the existing lock file

  this could be indicative of a few possible situations:

      * the source `ssh://git@...` supports checksums,
        but was replaced with one that doesn't
      * the lock file is corrupt

  unable to verify that `my-package v0.1.2 (ssh://git@...)` is the same as when the lockfile was generated

As far as I can tell, in order to avoid this error, Cargo would need to generate checksums for all git sources, which would presumably be a big breaking change as similar errors would be observed on any crates with git dependencies.

@weihanglo
Copy link
Member

Totally valid. When a lockfile already lacks checksums for git dependencies, the only possible way to get around is indeed skipping that check. Cargo doesn't even require a package checksum for a git dependency to validate the integrity.

I am fine with the solution you propose. Thank you for the well-written explanation! Would you mind sending a pull request for this patch?

@jarhodes314
Copy link
Author

Sure. I've made the changes locally. I'll just tidy up the test I've added for this and create a PR later today.

@weihanglo weihanglo added S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review and removed E-help-wanted labels Apr 19, 2023
@mehulmathur001
Copy link

Hi @weihanglo @jarhodes314 are there any other alternatives for getting this to work ?

@weihanglo
Copy link
Member

@mehulmathur001
For the linked PR, see #11188 (comment) and #11188 (comment).

As far as I understand, you could do this to bypass some checks:

echo '{"package":null,"files":{}}' > $some-crate/.cargo-checksum.json 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-local-registry-source Area: local registry sources (vendoring) C-bug Category: bug S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants