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

cargo-audit fails to fetch advisory database: "Could initialize the http client" #1029

Open
kpreid opened this issue Sep 28, 2023 · 14 comments

Comments

@kpreid
Copy link
Contributor

kpreid commented Sep 28, 2023

I just decided to upgrade my cargo-audit from v0.17.6 to v0.18.2. The command then failed:

$ cargo audit
    Fetching advisory database from `https://github.com/RustSec/advisory-db.git`
error: couldn't fetch advisory database: git operation failed: failed to prepare fetch: Could initialize the http client

I thought that perhaps there was a problem with the existing advisory-db, so I deleted ~/.cargo/advisory-db. Now it fails with:

$ cargo audit
    Fetching advisory database from `https://github.com/RustSec/advisory-db.git`
error: couldn't fetch advisory database: git operation failed: failed to fetch repo: Could initialize the http client

cargo audit --no-fetch succeeds, given a manually updated advisory-db. cargo install --locked cargo-audit did not make a difference to the behavior.

I'm assuming this is a bug, but if it's some misconfiguration then it would be nice if the error message explained what to do about it (and wasn't missing a "not" after "Could").

  • OS: macOS 12.7 (21G816)
  • rustc 1.72.1 (d5c2e9c34 2023-09-13)
  • x86_64-apple-darwin
@Shnatsel
Copy link
Member

The error message missing a "not" originates from the gix crate, and the message seems to be cut off also due to gix behavior. I have filed an issue upstream: Byron/gitoxide#1075

Once that's fixed it will at least start reporting the underlying problem correctly, so we will be able to debug it.

@Byron
Copy link
Contributor

Byron commented Oct 24, 2023

The lack of detail is due to the source() trail not being followed by the routine that prints the error. Display will only display the error message of the respective error, but the error provided by cargo-audit seems to be made to concatenate sources of errors by hand like msg: {source} and expects the error in source to do the same. gix errors don't do that as it cannot make a choice for the user (in terms of error display and presentation) beyond the human-readable message that presents the context of the error at hand.

One could add a custom printer to additionally follow the error chain via source() and keep appending to the line to add the desired context.

@Shnatsel
Copy link
Member

@kpreid could you please try #1053 and post the error it produced? It should be more informative this time around.

@kpreid
Copy link
Contributor Author

kpreid commented Nov 16, 2023

@kpreid could you please try #1053 and post the error it produced? It should be more informative this time around.

No change!

$ git rev-parse HEAD
f23756e35353e045fbe1b316afae26cc72620e4c
$ cargo run -p cargo-audit -- audit
    Finished dev [unoptimized + debuginfo] target(s) in 0.37s
     Running `target/debug/cargo-audit audit`
    Fetching advisory database from `https://github.com/RustSec/advisory-db.git`
error: couldn't fetch advisory database: git operation failed: failed to prepare fetch: Could initialize the http client

Given that I'm sure I am running the new code, this indicates that an error type involved isn't reporting a source(). I will investigate further.

@kpreid
Copy link
Contributor Author

kpreid commented Nov 16, 2023

The error being printed here is a rustsec::error::Error which never reports any source(); it contains only a String. Therefore, the place actually needing formatting is:

.map_err(|err| format_err!(ErrorKind::Repo, "failed to prepare fetch: {}", err))?

Patching that,

            .map_err(|err| {
                format_err!(
                    ErrorKind::Repo,
                    "failed to prepare fetch: {}",
                    display_error_chain::DisplayErrorChain::new(&err)
                )
            })?

I get the following actual error:

error: couldn't fetch advisory database: git operation failed: failed to prepare fetch: Could initialize the http client
Caused by:
  -> builder error: The Trust Settings Record was corrupted.
  -> The Trust Settings Record was corrupted.

According to info turned up from a web search, this is an error from rustls-native-certs that can happen on macOS when the Keychain contains a certificate that is present but untrusted. (Unfortunately, it doesn't tell us which one, so the error still isn't giving specific enough information, but it's not rustsec's problem specifically.)

So, the remaining task for rustsec is to stop using (solely) eagerly stringified errors, so that cause chains can be properly reported. I'll take a stab at it and see how much of a mess that turns out to be.

@kpreid
Copy link
Contributor Author

kpreid commented Nov 16, 2023

rustsec::format_err! (a public macro) is expanding to a use of crate::error::Error::new() (i.e the macro caller's crate) not $crate::error::Error::new() (the rustsec crate). This seems to be used intentionally, but that's a pretty surprising and future-change-restricting thing for a public macro to do.

In order to fix all the eager stringification (and the relevant one above) it'd be necessary to change this macro (semver breaking, and a hassle for all other error types using it), introduce a different macro that has an additional parameter, or introduce a method for inline augmentation (format_err!(...).with_source(e)).

I'm not sure what the best way to proceed is. I have a self-contained change that enhances rustsec::Error and updates direct uses of Error::new(), but it doesn't address the many uses of format_err!.

@Shnatsel
Copy link
Member

Is that macro really public? It certainly does not appear in documentation. I don't see a pub or #[macro_export] on its definition either. So changing this macro should not be semver-breaking.

Let's try transparently moving the source from the error being formatted to the parent one and see how far that gets us. I'd rather avoid inline augmentation because we'll just forget to use it half the time.

@kpreid
Copy link
Contributor Author

kpreid commented Nov 16, 2023

Is that macro really public?

No, it isn't. My mistake; when constructing my understanding of the situation I mixed up #[macro_use] with #[macro_export], and was misled about its usage in other crates due to the similar abscissa_core::prelude::format_err! macro which is used in many crates.

transparently moving the source from the error being formatted

That's not possible (without hacks like autoref specialization, or looking for arguments named e or err) because there's nothing that distinguishes an "error being formatted" from some random other format argument to be Displayed and included in the error message.

(Note also that in order to avoid redundant and theoretically-exponentially-growing printing, it's important to ensure that any contained error value is either formatted in Display::fmt() or returned from Error::source(), but not both.)

@Shnatsel
Copy link
Member

I think we'll need to rework (and semver-break) the rustsec::Error type and likely ditch or rework format_err! too.

We have these requirements:

  1. Preserve information of the original error (like thiserror?)
  2. Provide some context about it, which the custom message format_err! currently fulfills (like anyhow's context?)
  3. Avoid exposing error types directly to avoid coupling our semver with that of other crates (tame-index, gix)
  4. Either preserve the source field in our own error, or failing that stringify it to present it to the user

@tarcieri
Copy link
Member

The rustsec crate already uses thiserror, although format_err! is a pre-thiserror relic which should probably just go away

Re: anyhow, abscissa uses color-eyre which has an excellent error printer which we should be using.

@kpreid
Copy link
Contributor Author

kpreid commented Nov 17, 2023

I think we'll need to rework (and semver-break) the rustsec::Error type and likely ditch or rework format_err! too. …

I don't think this work is necessarily a semver break; it just needs to become a little more generic. This is what I did in my WIP:

#[derive(Debug)]
pub struct Error {
    kind: ErrorKind,
    msg: String,
    /// Error::source() returns this
    source: Option<Box<dyn std::error::Error + Send + Sync>>,
}

Basically a mini anyhow::Error of sorts. This may not be ideal, but it is compatible with the existing structure and allows adding sources incrementally. Would you like a PR for just this + the easy changes to use the new field?

@Shnatsel
Copy link
Member

Oh, since the fields are private that's not even going to be semver-breaking. Yes, I'd love to have such a PR!

@bitfield
Copy link

Same issue here on macOS 10.15.7:

$ cargo install --locked cargo-audit
...
$ cargo audit
Fetching advisory database from `https://github.com/RustSec/advisory-db.git`
error: couldn't fetch advisory database: git operation failed: failed to fetch repo: Could not initialize the http client

@tarcieri
Copy link
Member

@bitfield that release of macOS is EOL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants