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

Tracking Issue for credential-process RFC 2730 #8933

Closed
16 tasks done
ehuss opened this issue Dec 3, 2020 · 26 comments · Fixed by #12649
Closed
16 tasks done

Tracking Issue for credential-process RFC 2730 #8933

ehuss opened this issue Dec 3, 2020 · 26 comments · Fixed by #12649
Assignees
Labels
A-credential-provider Area: credential provider for storing and retreiving credentials A-registry-authentication Area: registry authentication and authorization (authn authz) C-tracking-issue Category: A tracking issue for something unstable. disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns. T-cargo Team: Cargo

Comments

@ehuss
Copy link
Contributor

ehuss commented Dec 3, 2020

Summary

RFC: rust-lang/rfcs#2730
Implementation: #8934
Documentation: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#credential-process
Issues: A-credential-provider Area: credential provider for storing and retreiving credentials

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

Unresolved issues

  • 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.
  • 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
  • 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 Implement RFC 3139: alternative registry authentication support #10592.
  • 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
  • Figure out the name for cargo:basic: cargo:token-from-stdout
  • Remove cargo-1password as a built-in (add the fn main so it can be published separately).
  • Create a new -Z flag for cargo:paseto.
    • Publish cargo-credential-1password to crates.io
  • Figure out MSRV support. fix: Set MSRV for internal packages #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? Add MSRV validation GitHub Action for cargo-credential #12623
  • 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. cargo-credential: change serialization of cache expiration #12622
    • A test with some manually generated JSON with an invalid version file fed into a credential provider should provide the correct error message.
    • 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.
  • Resolve Cargo suggests _TOKEN environment variable even when it would not be applicable #12642
  • Stabilization PR - feat: stabilize credential-process and registry-auth #12649
  • Publish cargo-credential v0.4 to crates.io
  • 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.

@ehuss ehuss added the C-tracking-issue Category: A tracking issue for something unstable. label Dec 3, 2020
@ehuss ehuss self-assigned this Dec 4, 2020
@jethrogb
Copy link
Contributor

@Eh2406
Copy link
Contributor

Eh2406 commented Nov 19, 2021

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 ehuss added the A-credential-provider Area: credential provider for storing and retreiving credentials label Jul 13, 2022
@ehuss ehuss added the A-registry-authentication Area: registry authentication and authorization (authn authz) label Dec 11, 2022
@ehuss
Copy link
Contributor Author

ehuss commented Mar 29, 2023

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

@ehuss ehuss added S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing. S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns. labels Apr 25, 2023
@ehuss
Copy link
Contributor Author

ehuss commented Jul 22, 2023

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

@arlosi arlosi removed the S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing. label Aug 2, 2023
bors added a commit that referenced this issue Aug 10, 2023
cargo-credential: reset stdin & stdout to the Console

Credential providers run with stdin and stdout piped to Cargo to communicate. This makes it more difficult for providers to do anything interactive. The current workaround is for a provider to use the `cargo_credential::tty()` function when reading from the console by re-opening stdin using `/dev/tty` or `CONIN$`.

This PR makes the credential provider to re-attach itself to the current console so that reading from stdin and writing to stdout "just works" when inside the `perform` method of the provider. stderr is unaffected since it's not redirected by Cargo.

Only the `cargo-credential` crate is changed. No changes are needed to Cargo.

cc #8933
bors added a commit that referenced this issue Aug 17, 2023
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
bors added a commit that referenced this issue Aug 17, 2023
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
@arlosi arlosi removed the S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. label Aug 28, 2023
@BatmanAoD
Copy link
Member

@ehuss should this issue be closed, then?

@ehuss
Copy link
Contributor Author

ehuss commented Aug 29, 2023

@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
Copy link
Member

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

@epage epage moved this to Unstable, baking in Cargo Roadmap Sep 5, 2023
@arlosi
Copy link
Contributor

arlosi commented Sep 7, 2023

Checklist in this issue updated

@mojindri
Copy link

mojindri commented Sep 7, 2023

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
Copy link
Contributor

arlosi commented Sep 8, 2023

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 #10474

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

@arlosi
Copy link
Contributor

arlosi commented Sep 8, 2023

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 #12649.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Sep 8, 2023

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

Concerns:

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.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Sep 8, 2023
@epage
Copy link
Contributor

epage commented Sep 8, 2023

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
Copy link
Contributor

epage commented Sep 8, 2023

@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
Copy link
Contributor

arlosi commented Sep 8, 2023

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
Copy link
Contributor

epage commented Sep 8, 2023

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.

bors added a commit that referenced this issue Sep 14, 2023
fix: emit a warning for `credential-alias` shadowing

### What does this PR try to resolve?
If a `credential-alias` shadows a built-in provider the user could be confused about which provider is being used.

### How should we review this PR?
See the test to see what the warning looks like.

r? `@epage` who listed this as a concern on the FCP in #8933
@epage
Copy link
Contributor

epage commented Sep 16, 2023

@rfcbot resolve future-compatibility

See #12671

@rfcbot rfcbot added the final-comment-period FCP — a period for last comments before action is taken label Sep 16, 2023
@rfcbot
Copy link
Collaborator

rfcbot commented Sep 16, 2023

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

@rfcbot rfcbot removed the proposed-final-comment-period An FCP proposal has started, but not yet signed off. label Sep 16, 2023
@bors bors closed this as completed in 7149418 Sep 18, 2023
@BatmanAoD
Copy link
Member

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

@Eh2406
Copy link
Contributor

Eh2406 commented Sep 18, 2023

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

@rfcbot rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Sep 26, 2023
@ehuss ehuss moved this from Unstable, baking to Done in Cargo Roadmap Sep 26, 2023
@Fishrock123
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-credential-provider Area: credential provider for storing and retreiving credentials A-registry-authentication Area: registry authentication and authorization (authn authz) C-tracking-issue Category: A tracking issue for something unstable. disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns. T-cargo Team: Cargo
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

9 participants