Bump git2 to 0.15 and libgit2-sys to 0.14

[jonhoo]

This will allow cargo to avoid vendored builds of git2 in up-to-date
environments going forward, and brings in the libgit2 1.4.4 CVE fix.

[rust-highfive]

r? @weihanglo

(rust-highfive has picked a reviewer for you, use r? to override)

[ehuss]

I think to move forward with this cargo will need to call libgit2_sys::set_verify_owner_validation(false) in order to disable owner validation. We don't need that for security (cargo doesn't do discovery in any way that is a concern) and there are scenarios where it will trigger in typical environments.

[jonhoo]

Ah, yes, I remember from the discussion in rust-lang/rfcs#3279. Will update!

[jonhoo]

One issue with setting that is that it's a global property. Which means that if someone has a project that uses git2 separately from cargo, but also runs cargo, then operations they do with git2 will also not have the owner check applied, which is probably unexpected. We could set it before each git operation and reset it after, though that feels brittle and racy. I wish it were possible to configure this option per repository...

[jonhoo]

There's git_repository_open_ext, but the flags it supports don't include the owner check.

[jonhoo]

Having now dug through the libgit2 PR that introduced the ownership check, libgit2/libgit2#6266, I don't see a way around this. Either we have to disable the check globally in something like cargo::Config::new, or we have to surround every call to git2::Repository::open (or ::init? or ::clone?) with a disable + do + enable dance which still risks racing with other (non-Cargo) code to allow opening a repository there with the check unexpectedly disabled.

[ehuss]

I was thinking it can be called from init_git_transports or somewhere around there on the binary side. That shouldn't affect cargo-lib users, and I believe is the only safe place to do it since it is an unprotected global.

[jonhoo]

We could do that, although it would mean that anyone using Cargo as a library would need to know to explicitly call that method themselves too. We could perhaps add a method like Config::globally_disable_git_ownership_verification so that consumers will at least a) not have to add git2 as a direct dependency and b) hopefully discover that this is something they may need to call?

[jonhoo]

One option might be to set the ceiling directories, which can be done per open, but as far as I can tell from the implementation that doesn't actually bypass the check either?

[weihanglo]

We could perhaps add a method like Config::globally_disable_git_ownership_verification

This sounds lovely to me. And I believe people will eventually find it. As Cargo lib users, they must be accustomed to breakages between versions sometimes. (Sorry about that 😅)

[weihanglo]

I am going to merge this. Thanks for the pull request.

If @ehuss or anyone still concerns with this, feel free to comment.

[weihanglo]

@bors r+

[bors]

📌 Commit 222e0e5 has been approved by weihanglo

It is now in the queue for this repository.

[bors]

⌛ Testing commit 222e0e5 with merge e5ec3a8...

[weihanglo]

I feel like this worth mentioning in release note, at least for library user?

[bors]

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing e5ec3a8 to master...

[jonhoo]

I agree this should be mentioned in relnotes!

[ehuss]

fwiw, I always include libgit2 bumps in the Cargo changelog. But I don't think this needs to be in the Rust release notes, does it? Are you concerned that third-party packages that link to the cargo API might run into problems here?

[jonhoo]

Oh, sorry, I thought relnotes was the Cargo release notes, not the Rust release notes. I agree with you it probably doesn't need to be in the Rust release notes.

I will point out that this bump means that libgit2 will now be built from source for anyone without a fairly recent libgit2 installed locally, which may cause some headache (it did for me), but that should only hit a small number of entities I suspect.

[ehuss]

Ah. Ok, I'll untag the relnotes. That label is only for RELEASES.md.

[ehuss]

Oh, it was already removed. I was confused. 😄