fix(credential)!: Fallback when an auth method isn't available on the current machine by epage · Pull Request #13558 · rust-lang/cargo

epage · 2024-03-07T21:26:01Z

What does this PR try to resolve?

This changes cargo-credential-libsecret to report UrlNotSupported errors when libsecret can't be loaded. This was also extended to token-from-stdout when the process can't be spawned.

The goal with this change is to make it easier to support a
cross-platform, cross-machine config file.
Before, someone couldn't enable libsecret for the machines that
supported it and then fallback to something else on machines that don't.

After this change, we should be able to fallback to other providers.

To help with debugability, we preserve the "libsecret can't be loaded"
message by reporting the first "rich" UrlNotSupported error message.
This is a newly invented construct as part of this PR.

This was discussed on
zulip
and in the cargo team meeting.

This is to improve the cross-platform config support in an effort to
deprecate having config be unset as part of #13343

How should we test and review this PR?

Additional information

Compatibility

Breaking: cargo-credential's Error went from an enum to an opaque struct with a separate ErrorKind
cargo-credential json messages can now contain more, optional fields
cargo-credential-libsecret will no longer hard error if libsecret can't be loaded

This isn't really providing much and gets in the way of tweaking the errors.

BREAKING CHANGE: `Error` because an opaque type with the enum broken out into `ErrorKind` - To construct an error from an `ErrorKind`, you can use `Into`. - To wrap an existing error, use `Error::other`

This changes cargo-credential-libsecret to report `UrlNotSupported` errors when `libsecret` can't be loaded. The goal with this change is to make it easier to support a cross-platform, cross-machine config file. Before, someone couldn't enable `libsecret` for the machines that supported it and then fallback to something else on machines that don't. After this change, we should be able to fallback to other providers. To help with debugability, we preserve the "`libsecret` can't be loaded" message by reporting the first "rich" `UrlNotSupported` error message. This is a newly invented construct as part of this PR. This was discussed on [zulip](https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/reg.20auth.20and.20libsecret) and in the cargo team meeting. This is to improve the cross-platform config support in an effort to deprecate having config be unset as part of rust-lang#13343

Like with `libsecret` not being present, process spawn errors can be treated as an indication that that config entry isn't relevant for the current machine and we should try other entries. If there is nothing else to fallback to, we'll still report this error.

rustbot · 2024-03-07T21:26:06Z

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

epage · 2024-03-07T21:26:23Z

r? @arlosi

arlosi · 2024-03-09T15:32:59Z

+                let mut child = cmd
+                    .spawn()
+                    .context("failed to spawn credential process")
+                    .map_err(|err| Error::from(err).with_kind(ErrorKind::UrlNotSupported))?;


Please add a comment saying why UrlNotSupported is used here. (So that we get the behavior of attempting further providers).

arlosi · 2024-03-20T22:58:52Z

 pasetors = { version = "0.6.8", features = ["v3", "paserk", "std", "serde"] }
 pathdiff = "0.2"
 percent-encoding = "2.3"
-pkg-config = "0.3.30"


I don't see this referenced anywhere else in the change. Is it just an unneeded dependency?

arlosi · 2024-03-20T23:06:56Z

+                            Err::<(), _>(e)
+                                .with_context(|| {
+                                    format!(
+                                        "credential provider `{}` could not handle the request",


Could you please add a UI test so we can see the output when there are multiple failed providers?

arlosi · 2024-03-20T23:23:56Z

-                })
-            }
+            Err(e) => match e.kind() {
+                cargo_credential::ErrorKind::UrlNotSupported => {


I'm not sure that the UrlNotSupported error makes sense for "non-fatal" errors. It was intended to indicate that the provider only supports some registry urls (like a provider only for a specific type of registry). It then grew to also be used for the platform-specific providers on the wrong platform - and that still made some sense since they are providers that don't work with any registry.

This use case of non-fatal (continuable) error is much further from that intent. However, I'm not sure what the right solution is.

Here are some other options:

Make all errors "non-fatal" and continue trying providers for all error types

Keep the existing behavior, but add an option in the global credential providers config to mark a provider as "optional"

Add a new error type of OtherContinuable that that works as you've implemented it here.

Talked about this in office hours

Add an alternative to Other that is Backtrack

Backtrack will need an MSRV note in docs

Maybe add an alias for Other that is Critical

Be careful about forcing MSRV bump

As for whether every error could have a message, this was a bit unclear.

url-not-supported: doesn't need it

not-found: doesn't need it

operation-not-supported: could benefit from it because a login failure on a provider that doesn't support it could give a better message saying why.

In the end, we decided that since the code was written, we should just move forward with a message on each one.

For message printing:

On error (everything backtracked):

Print every provider that backtracked and the message

On success (a provider matched)

In extra verbose, print the providers that backtracked and the message

Since we're doing a breaking change, we should also do #13615

@arlosi will do that breaking change after this is merged but within the same release

arlosi · 2024-03-20T23:25:06Z

    // Error: The credential could not be found in the provider.
    "kind":"not-found"
+    // (Optional) Error message string to be displayed
+    "message": "free form string error message",


Do these messages from NotFound errors show anywhere?

bors · 2024-03-25T22:00:38Z

☔ The latest upstream changes (presumably #13577) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot · 2024-12-20T15:43:46Z

☔ The latest upstream changes (possibly 081d7ba) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot · 2025-11-03T20:45:49Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

epage · 2026-01-28T15:32:49Z

My problem was actually #13927 and with that fixed, this isn't needed as much

epage added 10 commits March 5, 2024 21:12

refactor(credential): Pull our error deserialization logic

42f58ea

refactor(credential): Remove use of thiserror

33da53a

This isn't really providing much and gets in the way of tweaking the errors.

test(credential): Verify existing serde behavior for NotSupported

bfbd3a7

refactor(credential): Pull out alias for BoxError

2a15abd

refactor(credential): Consolidate Other construction

206a382

refactor(credential): Switch to an Other constructor

459e6b2

fix(credential)!: Decouple ErrorKind from BoxError

1df5f16

BREAKING CHANGE: `Error` because an opaque type with the enum broken out into `ErrorKind` - To construct an error from an `ErrorKind`, you can use `Into`. - To wrap an existing error, use `Error::other`

feat(credential): Allow any ErrorKind to have a custom error

4b297bc

rustbot assigned ehuss Mar 7, 2024

rustbot assigned arlosi and unassigned ehuss Mar 7, 2024

epage force-pushed the secret-fallback branch from da59163 to 368444f Compare March 7, 2024 22:22

rustbot added A-credential-provider Area: credential provider for storing and retreiving credentials A-documenting-cargo-itself Area: Cargo's documentation A-registry-authentication Area: registry authentication and authorization (authn authz) labels Mar 7, 2024

epage force-pushed the secret-fallback branch from 368444f to 0f3b391 Compare March 7, 2024 22:38

arlosi reviewed Mar 20, 2024

View reviewed changes

bors added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Mar 25, 2024

epage force-pushed the secret-fallback branch from 0f3b391 to 560efc9 Compare November 3, 2025 20:45

epage closed this Jan 28, 2026

Conversation

epage commented Mar 7, 2024

What does this PR try to resolve?

How should we test and review this PR?

Additional information

Uh oh!

rustbot commented Mar 7, 2024

Uh oh!

epage commented Mar 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Mar 25, 2024

Uh oh!

rustbot commented Dec 20, 2024

Uh oh!

rustbot commented Nov 3, 2025

Uh oh!

epage commented Jan 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants