Tracking Issue for credential-process RFC 2730

[ehuss]

Summary

RFC: https://github.com/rust-lang/rfcs/pull/2730 Implementation: #8934 Documentation: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#credential-process Issues: https://github.com/rust-lang/cargo/labels/A-credential-provider

This feature provides a configuration option to specify a process to fetch a token to authenticate with a registry.

Unresolved issues

• [X] Is this approach useful enough? Things like macos keychain don't protect against being executed to extract tokens (and I don't see a way to require a password, or force the process to be untrusted). The lack of signatures also cause issues (each update of the toolchain will cause it to become untrusted again).
  • Making the built-in providers like cargo:macos-keychain part of the cargo binary improves security here, since only the Cargo binary is accessing the Keychain. If we get Cargo signed by Apple, then it would be further improved.
• [X] Should the login API be changed? Providers that need to be interactive need to be able to read from stdin.
  • This is worked around by having the credential-provider re-open stdin from /dev/tty or $CONIN
• [X] Should the storage key off the registry name? RFC 3139 discusses problems with this. RESOLVED: credential helpers are now keying off the index url via #10592.
• [x] Should it be possible to pass additional arguments on the command line for cargo login that would be sent to the provider? E.g. cargo login -- --extra-arg-for-provider
  • Implemented in #12499
• [X] Figure out the name for cargo:basic: cargo:token-from-stdout
• [X] Remove cargo-1password as a built-in (add the fn main so it can be published separately).
• [x] Create a new -Z flag for cargo:paseto.
  • [x] Publish cargo-credential-1password to crates.io
• [x] Figure out MSRV support. rust-lang/cargo#12381 is proposing to set MSRV for all packages in the repo. I think it will be important that the credential provider libraries have a relatively relaxed requirement (like stable-2 or something). I think this will be important to figure out before stabilization. This may be complicated with CI costs, so I'm wondering if cargo-credential could grow a few independent tests on its own so we could test just the library with an older rust? https://github.com/rust-lang/cargo/pull/12623
• [x] Testing of forwards-compatibility support. It is important that when a user installs a new credential provider that it does not necessarily cause older cargos to fail. There is a version field when we need to do breaking changes, but we need tests that are actually verifying that is working. https://github.com/rust-lang/cargo/pull/12622
  • [x] A test with some manually generated JSON with an invalid version file fed into a credential provider should provide the correct error message.
  • [x] A test with some manually generated JSON with the current V1 format that is fed into a credential provider, and validate that it behaves correctly. This will make sure that if any changes to the credential provider are made that are not compatible with the old format that the test will fail.
• [x] Resolve rust-lang/cargo#12642
• [x] Stabilization PR - #12649
• [x] Publish cargo-credential v0.4 to crates.io
• [x] Publish cargo-credential-1password v0.4 to crates.io

About tracking issues

Tracking issues are used to record the overall progress of implementation. They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions. A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature. Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

[jethrogb]

Also see the discussion in https://internals.rust-lang.org/t/securing-cargo-publishing-credentials/14050

[Eh2406]

There's still a lot of unanswered questions in this tracking issue. But to make it clear basic functionality from the RFC is implemented and available for testing. The questions are all about comparatively small details.

[ehuss]

I have posted a proposal to stabilize just the cargo logout portion at #11910.

[ehuss]

12334 has been merged which is a redesign of this feature. It should land in nightly within the next week.

[BatmanAoD]

@ehuss should this issue be closed, then?

[ehuss]

@ehuss should this issue be closed, then?

I'm not sure I understand the question, why would this be closed? This is still tracking the credential-process feature. Its design has changed some, but it is still an unstable feature we are tracking.

[BatmanAoD]

@ehuss 🤦🏻 Sorry, I forgot that just because it's implemented doesn't mean it's stabilized.

[Fishrock123]

All the checkboxes in the original issue post here have been checked off - can an updated checklist be made if this is not ready to be stabilized?

[ehuss]

All the checkboxes in the original issue post here have been checked off - can an updated checklist be made if this is not ready to be stabilized?

I'm not sure which original issue you are referring to. Work is still actively being performed on this, mainly the amazing effort from @arlosi:

12424

12439

12440

12461

12469

12499

12507

12512

12515

12518

12521

12526

12551

12587

12588

12590

12622

12623

12626

12628

We are getting very close to being ready to stabilize. However, some of these changes haven't even made it to nightly, yet.

[BatmanAoD]

@Fishrock123 The checkbox list in the comment at the top of this thread are "unresolved issues", meaning design questions that needed to be answered. It is not a complete list of steps necessary to actually implement this feature.

[BatmanAoD]

...I agree this is a bit confusing, though. Several of those do look like implementation requirements, and are linked to merged PRs.

[Fishrock123]

I'm not sure which original issue you are referring to. Work is still actively being performed on this, mainly the amazing effort from @arlosi:

... pr links ...

We are getting very close to being ready to stabilize. However, some of these changes haven't even made it to nightly, yet.

All but one of those are merged. I am asking if it would be possible to update a high-level checklist like the one at the top of this issue with at least some kind of bullet point of what is left to be done.

[arlosi]

Checklist in this issue updated

[mojindri]

Hi, A question if "--token" for cargo build not included, is there any way to pass token to Private artifactory such as jfrog that requires token for crate download ?? Regards, Moji

[arlosi]

is there any way to pass token to Private artifactory such as jfrog that requires token for crate download

Yes, that's covered by the registry-auth unstable feature tracked as https://github.com/rust-lang/cargo/issues/10474

I'll be posting a stabilization PR for both registry-auth and credential-process soon. Hang in there.

[arlosi]

Stabilize registry-auth and credential-process

Users have been requesting a way to have authenticated private registries for some time, and the registry-auth feature has received significant testing, with multiple commercial and non-commercial offerings implementing the current unstable protocol.

The Cargo team has been reluctant to stabilize it as-is because it uses static plaintext tokens which are at greator risk of being leaked (accidentally or maliciously).

To improve the security situation, work continued on credential-process and asymmetric-token unstable features and the team agreed to only stabilize registry-auth if it was combined with one of these two approaches for improved security. credential-process allows the token to be stored securely by the operating system, or for people to build external credential providers that can generate short-lived tokens for registries. I believe the credential-process feature is ready to be stabilized, which also unblocks registry-auth.

Attempts to use an authenticated registry without a credential provider configured will encounter an error such as:

authenticated registries require a credential-provider to be configured
see https://doc.rust-lang.org/cargo/reference/registry-authentication.html for details

For contexts like CI where a more secure credential provider may not be available, the existing behavior of reading from config or the *_TOKEN environment variables can be enabled by selecting the cargo:token provider.

Stabilization summary (registry-auth) (#10474)

• The auth-required field in the registry index's config.json
  • Enables authenticated sparse index, crate downloads, and search API
• Automatically retrying requests to config.json in sparse registries with authorization included on HTTP 401 errors.
• Note: Attempting to use these features without a credential provider configured is an error.

Stabilization summary (credential-process):

• The built-in credential providers: cargo:token, cargo:wincred, cargo:macos-keychain, cargo:libsecret, cargo:token-from-stdout <path> <args>
• The credential provider v1 JSON protocol for external credential providers
• The registry.global-credential-providers config value
• The registries.<NAME>.credential-provider config value
• The [credential-alias] config table

To review these changes in more detail, I recommend looking at the documentation changes in the draft stabilization PR rust-lang/cargo#12649.

@rfcbot fcp merge

[rfcbot]

Team member @arlosi has proposed to merge this. The next step is review by the rest of the tagged team members:

• [x] @Eh2406
• [x] @Muscraft
• [x] @arlosi
• [x] @ehuss
• [x] @epage
• [ ] @joshtriplett
• [x] @weihanglo

Concerns:

• future-compatibility resolved by https://github.com/rust-lang/cargo/issues/8933#issuecomment-1722134167

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[epage]

Initially, I was concerned about allowing a plugin to be used multiple times with different configurations. This mostly came up as a theoretical because I was thinking or process plugins for per-user cache where that might be of more interest. Arlo pointed me to the fact that CLI flags can be used for this which I had forgotten about and overlooked it when I went back over the docs.

[epage]

@rfcbot concern future-compatibility

Built-in providers start with cargo: but on non-Windows systems, a binary could be created with a name like that or on all platforms a credential alias could be named that. Either approach means we have a potential compatibility hazard if we want to add more built-ins in the future.

EDIT: Do we care enough about this vs taking the risk of breaking people? Allowing this intentionally allows polyfills.

[arlosi]

We could reserve the cargo: namespace for credential providers, but I think we shouldn't.

• It's unlikely that a user will accidentally set up a credential-provider with a cargo: prefix, so the risk of unintentional shadowing is low (significantly less than the current issue of subcommand shadowing).
• If in the future Cargo adds a new built-in provider cargo:foo, it would be possible to make a polyfill provider available for older cargos that could be added via cargo install cargo-credential-foo and a credential-alias.

  
  [credential-alias]
  'cargo:foo' = 'cargo-credential-foo'

OTOH there may be a reason I'm not currently thinking of why we need to reserve something that we know can't be user-defined, and the current proposal does not allow for that.

[epage]

We also consider shadowing fine for built-in subcommands, aliases, and third-party subcommands though we provide warnings when it happens: https://github.com/rust-lang/cargo/blob/master/src/bin/cargo/cli.rs#L258

Maybe that is the route we should go with using that as our precedence.

[epage]

@rfcbot resolve future-compatibility

See #12671

[rfcbot]

:bell: This is now entering its final comment period, as per the review above. :bell:

[BatmanAoD]

Will this be in the beta on October 5th and available in the stable channel on November 16th?

[Eh2406]

Yes, that is the plan and so far we are still on schedule for it.

[Fishrock123]

Stabilizing this without making cargo:token a default option (which would have respected the world as it functioned before 1.74) has broken all of our CI: https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Cargo.201.2E74.20broke.20all.20our.20CI.20due.20to.20credentials-process