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.

Sign up for GitHub

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

Jump to bottom

Unhelpful cloudfront error when using bad API token #11571

Closed
Boddlnagg opened this issue Jan 8, 2023 · 5 comments · Fixed by #11952
Closed

Unhelpful cloudfront error when using bad API token #11571

Boddlnagg opened this issue Jan 8, 2023 · 5 comments · Fixed by #11952
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. A-interacts-with-crates.io Area: interaction with registries A-registry-authentication Area: registry authentication and authorization (authn authz) C-bug Category: bug E-easy Experience: Easy

Comments

@Boddlnagg
Copy link
Contributor

Boddlnagg commented Jan 8, 2023 •
edited
Loading

Current Behavior

I (unintentionally and without noticing) entered a bad API token via cargo login (see below). Then cargo publish and cargo owner, including cargo owner --list, give the following error:

> cargo owner --list windows
    Updating crates.io index
error: failed to list owners of crate `windows` on registry at https://crates.io

  failed to get a 200 OK response, got 400
  headers:
        HTTP/2 400
        content-length: 0
        server: nginx
        date: Sun, 08 Jan 2023 17:06:47 GMT
        strict-transport-security: max-age=31536000; includeSubDomains
        via: 1.1 vegur, 1.1 319f376925908156190f5fc160137b42.cloudfront.net (CloudFront)
        x-cache: Error from cloudfront
        x-amz-cf-pop: FRA60-P3
        x-amz-cf-id: eclND8Emct0Dx-QEsF7G9Wn4NP_6iHfFegsmX7UxsQxr1X89yRw6og==

  body:

The credentials file ended up looking like this after I hit Ctrl+V to insert my token on the command line (and at first I didn't notice that this didn't work correctly):

[registry]
token = "\u0016"

Expected Behavior

A more helpful error message that tells me that my API token is wrong or ill-formed.

Steps To Reproduce

No response

Environment

Windows 10, using PowerShell within Windows Terminal or VS Code

Anything else?

No response
@Eh2406
Copy link
Contributor

Eh2406 commented Jan 9, 2023

The most direct fix is that cargo should escape characters that have special meanings in HTTP before putting in ill formed authentication token into CUrl, I'm honestly a little surprised that CUrl does not have this escaping already.

More generally, we should probably make a breaking change banning these kinds of control characters from tokens in general. We recently made a similar "breaking" change to ban empty tokens.

@Turbo87
Copy link
Member

Turbo87 commented Jan 12, 2023

@Eh2406 should we move this to the cargo issue tracker?

@Eh2406 Eh2406 transferred this issue from rust-lang/crates.io Jan 12, 2023
@Eh2406
Copy link
Contributor

Eh2406 commented Jan 12, 2023

Done.

@weihanglo
Copy link
Member

Out of curiosity, does crates.io allow control characters in tokens?

@weihanglo weihanglo added C-bug Category: bug A-diagnostics Area: Error and warning messages generated by Cargo itself. A-interacts-with-crates.io Area: interaction with registries A-registry-authentication Area: registry authentication and authorization (authn authz) labels Jan 13, 2023
@Eh2406
Copy link
Contributor

Eh2406 commented Jan 13, 2023

I don't believe any of the tokens generated by crates.io have control characters. I believe they are all ASCII.

@weihanglo weihanglo added the E-easy Experience: Easy label Jan 18, 2023
@Akida31 Akida31 mentioned this issue Jan 19, 2023
Merged
bors added a commit that referenced this issue Feb 16, 2023
@bors
Auto merge of #11600 - Akida31:issue-11571-bad-token, r=Eh2406
a66f123 
Error on invalid alphanumeric token for crates.io

ref #11571

When using `cargo login` and calling an api which requires authentification there will be an error if the given token is not a valid alphanumerical string.
This check is only enabled for crates.io because
only for that registry we can be certain, that the generated token should have been alphanumeric, see [the code here](https://github.com/rust-lang/crates.io/blob/7ea41e9d345f05566ee776b7cbb62e46ccf6b393/src/util/token.rs#L15). So if I'm not mistaken, this should not be a breaking change, since crates.io only generates fitting tokens. (Should I add a comment to the crates.io code that modifying this logic can break cargo?)

I'm not sure if the fix works and is enough to close the issue, please say if you have any corrections or improvements!

I don't know if the check should also be enabled for other registries and it would be really bad if the check is too strict.
In the linked issue it was recommended to encode invalid characters, but I don't know in which encoding. I saw in [this http rfc](https://www.rfc-editor.org/rfc/rfc7230#section-3.2.4) that only the ISO-8859-1 charset is allowed and everything else must be [encoded](https://www.rfc-editor.org/rfc/rfc7230#section-3.2.4) but this seems somewhat complex and hard to implement. There is a crate `rust-encoding` which should be capable doing this (from a first look), but I don't know if a new dependency only for this is justified. There seems to be `percent encoding` already in the dependency tree but I have no idea if it would be correct and work.
If you have any idea about this encoding, please say so.

r? `@Eh2406` (since you suggested the encoding part)
@ehuss ehuss mentioned this issue Apr 9, 2023
Merged
bors added a commit that referenced this issue Apr 10, 2023
@bors
Auto merge of #11952 - ehuss:check-token-publish, r=weihanglo
49c62f5 
Validate token on publish.

The `publish` path was not validating the token like the other API routes were (like owner, or yank). This does not appear to be intentional from what I can tell. This consolidates the relevant code so that it is shared with all the API calls.

cc #11600

Closes #11571
@bors bors closed this as completed in 1a53cdb Apr 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. A-interacts-with-crates.io Area: interaction with registries A-registry-authentication Area: registry authentication and authorization (authn authz) C-bug Category: bug E-easy Experience: Easy
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Validate token on publish. ehuss/cargo
4 participants
@Turbo87 @Boddlnagg @Eh2406 @weihanglo