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 info requires login #14409

Closed
ehuss opened this issue Aug 15, 2024 · 9 comments · Fixed by #14418
Closed

cargo info requires login #14409

ehuss opened this issue Aug 15, 2024 · 9 comments · Fixed by #14418
Assignees
Labels
C-bug Category: bug Command-info S-triage Status: This issue is waiting on initial triage.

Comments

@ehuss
Copy link
Contributor

ehuss commented Aug 15, 2024

Problem

Running cargo info foo seems to require a login to crates.io. I would not expect a simple information command to require authentication (or for those with a hardware token, a hardware key) to access it.

Steps

  1. Set up config.toml with:
[registry]
global-credential-providers = ["false"]
  1. Run cargo info syn
  2. See that it fails:
    Updating crates.io index
error: credential provider `false` failed action `get`

Caused by:
  failed to deserialize hello

Caused by:
  EOF while parsing a value at line 1 column 0

Possible Solution(s)

I'm guessing this is because of the call to try_list_owners. I do not think cargo info should be trying to access API endpoints that require authentication.

Notes

No response

Version

commit-hash: ff6df29857a5fc5f6bb0b9da8436cb501ee5894c
@ehuss ehuss added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. Command-info labels Aug 15, 2024
@Rustin170506
Copy link
Member

@rustbot claim

I will take a look.

@Rustin170506
Copy link
Member

I can reproduce it.

@Rustin170506
Copy link
Member

Rustin170506 commented Aug 15, 2024

I do not think cargo info should be trying to access API endpoints that require authentication.

From the current design, if we can access the owner list, then we will display it; otherwise, it will skip the missing token error.

I think the bug from here:

(AuthorizationErrorReason::TokenMissing)

We only checked the missing token error, maybe we should ignore all errors here.

The reason we introduced this information is that sometimes knowing who maintained the crate should be helpful.

@ehuss
Copy link
Contributor Author

ehuss commented Aug 15, 2024

We only checked the missing token error, maybe we should ignore all errors here.

It's not an issue of checking for errors. Credentials can pop up requests for passwords, biometrics, or hardware keys. I do not think it would be good if cargo info pops up a password request every time you try to use it.

AFAIK, the owners list API endpoint does not require authentication on crates.io. Does the call to registry() require the Operation::Read entitlement?

@Rustin170506
Copy link
Member

It's not an issue of checking for errors. Credentials can pop up requests for passwords, biometrics, or hardware keys. I do not think it would be good if cargo info pops up a password request every time you try to use it.

I see. I didn't consider that. You are right. It doesn't make sense at all.

AFAIK, the owners list API endpoint does not require authentication on crates.io. Does the call to registry() require the Operation::Read entitlement?

Good point. The owners list API from crates.io does not require any authentication.

Yes, we passed the 'Operation::Read' entitlement. However, I found that even if we do not pass it, the 'crates-io' crate will still require us to pass a token.

I believe that whenever we send a request, we simply pass Auth::Authorized to the req function.

fn req(&mut self, path: &str, body: Option<&[u8]>, authorized: Auth) -> Result<String> {

I will dig a little deeper to see why we are forcing it.

@Rustin170506
Copy link
Member

Since this commit 0c25226, authentication is required for this API.

@Rustin170506
Copy link
Member

I guess we shouldn't "fix" this problem because our registry specification states that this API requires authentication.

So there are two ways to fix it:

  1. Add a special case for crates.io.
  2. Remove this feature at all.

@ehuss @epage What do you think?

@epage
Copy link
Contributor

epage commented Aug 16, 2024

Let's just remove it for now and we can always re-evaluate it later.

@Rustin170506
Copy link
Member

Let's just remove it for now and we can always re-evaluate it later.

I agree, I will go head to remove it tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug Command-info S-triage Status: This issue is waiting on initial triage.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants