fix(credential)!: Fallback when an auth method isn't available on the current machine #13558

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

src/cargo/util/credential/adaptor.rs

+                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

Cargo.toml

@@ -72,7 +72,6 @@ os_info = "3.7.0"
 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

src/cargo/util/auth/mod.rs

+                            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

src/cargo/util/auth/mod.rs

-                })
-            }
+            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

src/doc/src/reference/credential-provider-protocol.md

@@ -177,6 +181,10 @@ available.
 {"Err":{
    // 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.

epage added 10 commits March 5, 2024 21:12

refactor(credential): Pull our error deserialization logic

5f9e0d3

refactor(credential): Remove use of thiserror

de77d20

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

test(credential): Verify existing serde behavior for NotSupported

8de4ecb

refactor(credential): Pull out alias for BoxError

321291f

refactor(credential): Consolidate Other construction

459cab9

refactor(credential): Switch to an Other constructor

52af0c6

fix(credential)!: Decouple ErrorKind from BoxError

0a56d63

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

19e2165

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(credential)!: Fallback when an auth method isn't available on the current machine #13558

fix(credential)!: Fallback when an auth method isn't available on the current machine #13558

epage commented Mar 7, 2024

rustbot commented Mar 7, 2024

epage commented Mar 7, 2024

arlosi Mar 9, 2024

arlosi Mar 20, 2024

arlosi Mar 20, 2024

arlosi Mar 20, 2024

epage Mar 28, 2024

epage Mar 28, 2024

epage Mar 28, 2024

epage Mar 28, 2024

arlosi Mar 20, 2024

bors commented Mar 25, 2024

rustbot commented Dec 20, 2024

fix(credential)!: Fallback when an auth method isn't available on the current machine #13558

Are you sure you want to change the base?

fix(credential)!: Fallback when an auth method isn't available on the current machine #13558

Conversation

epage commented Mar 7, 2024

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Mar 7, 2024

epage commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 25, 2024

rustbot commented Dec 20, 2024