credential provider implementation #12334

arlosi · 2023-07-07T22:00:05Z

The current credential process protocol only allows sending the credential without any additional information. This changes the protocol in two important ways: Cargo will tell the credential provider what the token is needed for, and the credential provider can tell Cargo how the token can be used.

Since the credential provider knows why Cargo needs a token (publish for example), it can produce a signed token specifically for that operation. This would enable a credential process to produce an asymmetric token, or a token with restricted scope such as PASETO or Biscuit.

The credential process can also indicate back to Cargo if the token can be cached in-memory for subsequent requests. For example, if a credential provider integrates with an SSO identity provider that provides short-lived tokens, Cargo will only continue to use the token while it is valid.

Summary of changes

Rename credential-process to credential-provider in config.
Add a new line-oriented JSON protocol for communicating with external credential providers via stdin/stdout.
Allow built-in credential providers to run in the Cargo process.
Move support for asymmetric tokens (RFC3231) into a built-in credential provider (cargo:paseto).
Change the unstable key for asymmetric tokens from registry-auth to credential-process
Add a new built-in provider to represent the current config/token based system (cargo:token).
Add a new built-in provider for the a "basic" provider that prints only the token on stdout (cargo:basic).
Create a new config key for the fallback credential providers (registry.credential-providers) as a list.
The provider for crates.io no longer also acts as a fallback for other registries.
Adds a [credential-alias] table for defining aliases of credential providers.
Collect all headers from http_registry requests, passing them through to the cred provider.

Everything remains unstable under the -Zcredential-process flag.

How to review this:

I recommend starting with the changes in unstable.md for a more detailed description.

Open questions

Should we pass all the HTTP headers rather than just www-authenticate

rustbot · 2023-07-07T22:00:10Z

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

ehuss

This is looking great!

This is just a partial review. I'll try to finish the rest tomorrowish.

src/doc/src/reference/unstable.md

credential/cargo-credential/Cargo.toml

credential/cargo-credential/src/lib.rs

src/cargo/util/credential/process.rs

src/cargo/ops/registry/publish.rs

ehuss · 2023-07-11T23:53:53Z

@arlosi Your most recent commit seems to have introduced a bunch of formatting changes.

ehuss · 2023-07-11T23:56:31Z

Oh, I think the let-else formatting changes just landed in nightly. Lemme look at that.

ehuss · 2023-07-12T00:00:09Z

Posted #12351 to update the formatting.

ehuss

Review part 2. I think I've gone through everything except the tests, will try to get to those tomorrow.

credential/cargo-credential-macos-keychain/src/lib.rs

src/cargo/util/auth/mod.rs

ehuss · 2023-07-12T00:44:01Z

credential/cargo-credential-1password/src/lib.rs

    }
 }
-
-fn main() {


In the future, how hard would it be to translate the 1password provider back to a standalone executable? We haven't decided if we are going to support these providers directly. One option was to migrate these to a separate repo (perhaps under my name) if we decide that rust-lang isn't going to support these. The maintenance burden isn't too bad, so I'm not sure if it matters (and we can document that we can drop providers in the future if we can't support it).

It's very easy to translate back. Just need to change it back to a bin crate and add:

fn main() { cargo_credential::main(OnePasswordCredential); }

I think I'd also prefer that 1password was a standalone provider that you could cargo install.

src/cargo/util/auth/mod.rs

bors · 2023-07-12T01:10:26Z

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

ehuss

Something weird happens if you have a configuration with registries.NAME.credential-providers = ["..."] (notice the s in the key). It seems to ignore that value, and doesn't generate a warning about an unused key. I would expect a warning about an unused key. It also seems to do some basic validation, since if you have credential-providers = "...", it will generate an error about not being a list. In general I'm a little concerned that users will get confused about the subtle distinction of whether or not there is an s in the key.

src/doc/src/reference/unstable.md

src/cargo/ops/registry/login.rs

tests/testsuite/credential_process.rs

src/cargo/util/auth/mod.rs

bors · 2023-07-20T23:24:20Z

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

arlosi · 2023-07-21T19:37:01Z

Something weird happens if you have a configuration with registries.NAME.credential-providers = ["..."] (notice the s in the key). It seems to ignore that value, and doesn't generate a warning about an unused key. I would expect a warning about an unused key. It also seems to do some basic validation, since if you have credential-providers = "...", it will generate an error about not being a list. In general I'm a little concerned that users will get confused about the subtle distinction of whether or not there is an s in the key.

There was a shared struct that was used for both registries.NAME and registry, with registry allowing more keys. I've separated it out so we get better warnings for unused keys.

I've also renamed the credential-providers to global-credential-providers (name subject to bikeshedding), to make it more distinct from credential-provider.

arlosi · 2023-07-21T22:32:36Z

@ehuss I think this is ready for another round of review.

Given that #12310 was just merged, I've also made changes here to pass all the headers into the credential provider.

ehuss · 2023-07-22T17:31:42Z

Looks great, thanks! If there are thoughts about what needs followup, please file new issues, but I can't think of anything right now.

@bors r+

bors · 2023-07-22T17:31:44Z

📌 Commit 6151a41 has been approved by ehuss

It is now in the queue for this repository.

bors · 2023-07-22T17:31:50Z

⌛ Testing commit 6151a41 with merge a7b6a3c...

bors · 2023-07-22T18:15:23Z

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing a7b6a3c to master...

Update cargo 8 commits in 1b15556767f4b78a64e868eedf4073c423f02b93..7ac9416d82cd4fc5e707c9ec3574d22dff6466e5 2023-07-18 14:44:47 +0000 to 2023-07-24 14:29:38 +0000 - fix(cargo-credential): should enable feature `serde/derive` (rust-lang/cargo#12396) - fix: encode URL params correctly for SourceId in Cargo.lock (rust-lang/cargo#12280) - docs: format config override caveat as a note (rust-lang/cargo#12392) - credential provider implementation (rust-lang/cargo#12334) - feat(crates-io): expose HTTP headers and Error type (rust-lang/cargo#12310) - chore: Don't update test data (rust-lang/cargo#12380) - fix: only skip mtime check on `~/.cargo/{git,registry}` (rust-lang/cargo#12369) - Update docs for artifact JSON debuginfo levels. (rust-lang/cargo#12376) Since rust-lang/cargo#12334 makes built-in credential providers part of the cargo binary, it's no longer needed to build them in bootstrap.

DrChat · 2023-08-02T21:17:34Z

src/cargo/ops/registry/login.rs

+    if !std::io::stdin().is_terminal() {
+        let token = std::io::read_to_string(std::io::stdin()).unwrap_or_default();
+        if !token.is_empty() {
+            token_from_stdin = Some(token);
        }
    }


Just a friendly heads up - this is causing our CI jobs to hang if we provide the token via the command-line instead of stdin.
I'm happy to open a bug for this, but just wanted to make sure you were aware.

Sorry for the inconvenience!

I didn't quite follow the PR at that time. Is that a regression? If yes, please file an issue with reproduction, and we can then arrange a fix!

Yes, this is a regression.
Sounds good, thanks :)

login: allow passing additional args to provider As part of moving asymmetric token support to a credential provider in #12334, support for passing `--key-subject` to `cargo login` was removed. This change allows passing additional arguments to credential providers when running `cargo login`. For example: `cargo login -- --key-subject foo`. The asymmetric token provider (`cargo:paseto`) is updated to take advantage of this and re-enables setting `--key-subject` from `cargo login`. r? `@Eh2406` cc #8933

rustbot assigned ehuss Jul 7, 2023

weihanglo mentioned this pull request Jul 9, 2023

Tracking Issue for alternative registry authentication (RFC 3139) #10474

Closed

5 tasks

arlosi changed the title ~~Draft credential provider implementation~~ credential provider implementation Jul 10, 2023

ehuss reviewed Jul 11, 2023

View reviewed changes

src/doc/src/reference/unstable.md Outdated Show resolved Hide resolved

credential/cargo-credential/Cargo.toml Outdated Show resolved Hide resolved

credential/cargo-credential/src/lib.rs Show resolved Hide resolved

src/cargo/util/credential/process.rs Show resolved Hide resolved

weihanglo reviewed Jul 11, 2023

View reviewed changes

src/cargo/ops/registry/publish.rs Outdated Show resolved Hide resolved

rustbot added A-git Area: anything dealing with git A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-manifest Area: Cargo.toml issues labels Jul 11, 2023

ehuss reviewed Jul 12, 2023

View reviewed changes

arlosi force-pushed the cred-ext branch 2 times, most recently from e592978 to 3b8e2c0 Compare July 18, 2023 22:59

ehuss reviewed Jul 20, 2023

View reviewed changes

arlosi mentioned this pull request Jul 21, 2023

Error message with credential-process and a token is confusing. #12320

Closed

arlosi added 2 commits July 21, 2023 16:02

Credential provider implementation

2b39792

Send all headers to cred provider

6151a41

arlosi force-pushed the cred-ext branch from 3b8e2c0 to 6151a41 Compare July 21, 2023 22:30

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 22, 2023

ehuss mentioned this pull request Jul 22, 2023

Tracking Issue for credential-process RFC 2730 #8933

Closed

16 tasks

bors merged commit a7b6a3c into rust-lang:master Jul 22, 2023
20 checks passed

bors mentioned this pull request Jul 22, 2023

Support cargo owner add #11879

Closed

arlosi mentioned this pull request Jul 24, 2023

Update cargo rust-lang/rust#114027

Merged

ehuss added this to the 1.73.0 milestone Jul 30, 2023

DrChat reviewed Aug 2, 2023

View reviewed changes

DrChat mentioned this pull request Aug 2, 2023

Providing a token to cargo login via command line causes a hang on CI #12438

Closed

arlosi mentioned this pull request Aug 14, 2023

login: allow passing additional args to provider #12499

Merged

decathorpe mentioned this pull request Nov 9, 2023

non-target-specific / unconditional dependencies on potentially unusable credential providers #12945

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

credential provider implementation #12334

credential provider implementation #12334

arlosi commented Jul 7, 2023 •

edited

Loading

rustbot commented Jul 7, 2023

ehuss left a comment

ehuss commented Jul 11, 2023

ehuss commented Jul 11, 2023

ehuss commented Jul 12, 2023

ehuss left a comment

ehuss Jul 12, 2023

arlosi Jul 13, 2023 •

edited

Loading

bors commented Jul 12, 2023

ehuss left a comment

bors commented Jul 20, 2023

arlosi commented Jul 21, 2023

arlosi commented Jul 21, 2023

ehuss commented Jul 22, 2023

bors commented Jul 22, 2023

bors commented Jul 22, 2023

bors commented Jul 22, 2023

DrChat Aug 2, 2023 •

edited

Loading

weihanglo Aug 2, 2023

DrChat Aug 2, 2023

DrChat Aug 2, 2023

credential provider implementation #12334

credential provider implementation #12334

Conversation

arlosi commented Jul 7, 2023 • edited Loading

Summary of changes

How to review this:

Open questions

rustbot commented Jul 7, 2023

ehuss left a comment

Choose a reason for hiding this comment

ehuss commented Jul 11, 2023

ehuss commented Jul 11, 2023

ehuss commented Jul 12, 2023

ehuss left a comment

Choose a reason for hiding this comment

ehuss Jul 12, 2023

Choose a reason for hiding this comment

arlosi Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

bors commented Jul 12, 2023

ehuss left a comment

Choose a reason for hiding this comment

bors commented Jul 20, 2023

arlosi commented Jul 21, 2023

arlosi commented Jul 21, 2023

ehuss commented Jul 22, 2023

bors commented Jul 22, 2023

bors commented Jul 22, 2023

bors commented Jul 22, 2023

DrChat Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

weihanglo Aug 2, 2023

Choose a reason for hiding this comment

DrChat Aug 2, 2023

Choose a reason for hiding this comment

DrChat Aug 2, 2023

Choose a reason for hiding this comment

arlosi commented Jul 7, 2023 •

edited

Loading

arlosi Jul 13, 2023 •

edited

Loading

DrChat Aug 2, 2023 •

edited

Loading