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

cargo:token not being used by default #13343

Open
davidcorrigan714 opened this issue Jan 24, 2024 · 9 comments
Open

cargo:token not being used by default #13343

davidcorrigan714 opened this issue Jan 24, 2024 · 9 comments
Labels
A-credential-provider Area: credential provider for storing and retreiving credentials A-documenting-cargo-itself Area: Cargo's documentation C-bug Category: bug S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@davidcorrigan714
Copy link

Problem

Fetching dependencies from an authenticate provider using credentials stored in credentials.toml isn't working unless I explicitly add:

[registry]
global-credential-providers = ["cargo:token"]

Though the documentation says that should be the default: "the cargo:token provider is used if no providers are configured."

I don't see any CARGO_ env variables set in my environment so it doesn't seem like I have conflicting configurations anywhere. My full config.toml looks like this now:

[registry]
global-credential-providers = ["cargo:token"]

[registries.ni]
index = "sparse+https://provider..../"

[registries.ni_pre]
index = "sparse+https://provider..../"

Steps

No response

Possible Solution(s)

No response

Notes

No response

Version

cargo 1.75.0 (1d8b05cdd 2023-11-20)
release: 1.75.0
commit-hash: 1d8b05cdd1287c64467306cf3ca2c8ac60c11eb0
commit-date: 2023-11-20
host: x86_64-pc-windows-msvc
libgit2: 1.7.1 (sys:0.18.1 vendored)
libcurl: 8.4.0-DEV (sys:0.4.68+curl-8.4.0 vendored ssl:Schannel)
os: Windows 10.0.19044 (Windows 10 Enterprise) [64-bit]
@davidcorrigan714 davidcorrigan714 added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Jan 24, 2024
@epage epage added the A-credential-provider Area: credential provider for storing and retreiving credentials label Jan 24, 2024
@Eh2406
Copy link
Contributor

Eh2406 commented Jan 24, 2024

This is intentional, if you want to store you tokens in plain text you need to opted into it. Better documentation is welcome!

cc @arlosi

@davidcorrigan714
Copy link
Author

It's just blatantly wrong at the moment. The default value specified here should be updated too.

@Eh2406
Copy link
Contributor

Eh2406 commented Jan 24, 2024

Yes. That needs to be clearer! Thanks for the issues!

From the first sentence of the paragraph you quoted:

Using alternative registries with authentication requires a credential provider to be configured to avoid unknowingly storing unencrypted credentials on disk.

@davidcorrigan714
Copy link
Author

davidcorrigan714 commented Jan 24, 2024

Using alternative registries with authentication requires a credential provider to be configured to avoid unknowingly storing unencrypted credentials on disk.

Which is directly contradictory to the cargo:token provider is used if no providers are configured. Cause I have no provider configured, and the "cargo:token" provider certainly wasn't used or treated as the default value for the configuration option.

Regardless, I really don't care if the "bug" is in the behavior of the code or the docs but the docs and implementation are not consistent at the moment. Well the docs aren't consistent with itself apparently.

@weihanglo
Copy link
Member

The full context of it is:

public (non-authenticated) registries do not require credential provider configuration, and the cargo:token provider is used if no providers are configured.

Which is not really a contradictory to me, as alternative registries are not considered as public registries. Granted, there are too many jargons not immediately clear to users.

Since it looks like we handle crates.io specially, for the paragraph we might adapt it as

public (non-authenticated) registries , for example crates.io, do not require credential provider configuration, and the cargo:token provider is used if no providers are configured.

And the default could change to:

Default: none (["cargo:token"] for crates.io)

@weihanglo weihanglo added A-documenting-cargo-itself Area: Cargo's documentation S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review and removed S-triage Status: This issue is waiting on initial triage. labels Jan 24, 2024
@davidcorrigan714
Copy link
Author

davidcorrigan714 commented Jan 24, 2024

Don't mind cleaning up the docs. But the docs match the code but I really can't parse out what the intention was. Why have a default value at all if it's never used and confusing.

So what's the definition of a "public registry" from the code's perspective?

Requoting what you said public (non-authenticated) registries how would the phrase about credential providers apply to a non-authenticated registry? The password for the passwordless thing?

@davidcorrigan714
Copy link
Author

davidcorrigan714 commented Jan 24, 2024

I also just don't see any value in discerning "public" vs "alternative" in any technical decisions unless the technical implementation is trying to push a registry owner's agenda (yayyy Docker) . "alternative" has no clear definition to me other than crates.io probably being "official".

Edit: should keep myself on topic.

@davidcorrigan714
Copy link
Author

davidcorrigan714 commented Jan 24, 2024

Out of time for this today and haven't quite parsed through all the code yet to find where the token provider might ever be used without ever being explicitly set but I'm guessing it's for publishing(?) So more accurately it is something like: "Registries with unauthenticated download access will use the default value for global-credential-providers for publishing, but global-credential-providers must be explicitly set for authenticated download operations". Want to poke through more later and confirm that before proposing some updated docs. It's super weird to me though that there's a default value that is arbitrarily ignored.

@Eh2406
Copy link
Contributor

Eh2406 commented Jan 24, 2024

Thank you for that proposed text. It is a big step in the write direction, removing much jargon that I didn't realize I was using.

We would've liked to make it required for everything. but "Registries with unauthenticated download access" (for which we use the jargon "public registries") were already stable without the opt in, and they can use tokens for publish/yank/unyank/owners.

epage added a commit to epage/cargo that referenced this issue Mar 7, 2024
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
epage added a commit to epage/cargo that referenced this issue Mar 7, 2024
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
epage added a commit to epage/cargo that referenced this issue Mar 7, 2024
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
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-documenting-cargo-itself Area: Cargo's documentation C-bug Category: bug S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
Development

No branches or pull requests

4 participants