fix(auth): add auth scheme hint to token rejected error for alt registries#16794
fix(auth): add auth scheme hint to token rejected error for alt registries#16794enricobolzonello wants to merge 4 commits intorust-lang:masterfrom
Conversation
rustbot
added
A-registry-authentication
|
r? @weihanglo rustbot has assigned @weihanglo. Use Why was this reviewer chosen?The reviewer was selected based on:
|
src/cargo/util/auth/mod.rs
Outdated
| f, | ||
| "\nnote: the token does not include an authorization scheme prefix \ | ||
| (e.g. `Bearer `); if the registry requires one, prefix the token value \ | ||
| in credentials.toml" |
There was a problem hiding this comment.
I'd prefer we avoid the reference to credentials.toml, and recommend using cargo login to set the token. Directly editing the credentials files doesn't support the more secure credential provider workflow.
There was a problem hiding this comment.
got it, I will remove the mention to credentials.toml
src/cargo/util/auth/mod.rs
Outdated
| Some((prefix, _)) if !prefix.is_empty() => Some(match prefix { | ||
| "Bearer" => AuthorizationScheme::Bearer, | ||
| "Basic" => AuthorizationScheme::Basic, | ||
| other => AuthorizationScheme::Unrecognized(other.to_string()), |
There was a problem hiding this comment.
Should we require that other is also shorter than maybe 20 characters before passing it through? I'm a little worried that this could end up leaking secrets.
There was a problem hiding this comment.
I see the concern. We could combine the length check with the token specification (which disallows characters like = or :). The other option would be to remove the Unrecognized entirely, but we would lose the warning which is helpful
src/cargo/util/auth/mod.rs
Outdated
| )?, | ||
| AuthorizationScheme::Unrecognized(x) => write!( | ||
| f, | ||
| "\nwarning: the token has an unrecognized authorization scheme `{x}`" |
There was a problem hiding this comment.
I'm worried about this as a potential user confusion source. Say for example a registry uses a scheme of AWS4-HMAC-SHA256, but the the token part of the scheme is invalid. In this case, just because Cargo didn't recognize the scheme doesn't mean the server didn't. Only the registry server can say whether a scheme is unrecognized or not (which it can do, since we print the body of the HTTP response).
I think given this situation, I'd prefer that we reduce this to only emit the note if there is no authentication scheme present (the NoScheme case), and don't emit anything extra for the other cases (keep existing behavior).
src/cargo/util/auth/mod.rs
Outdated
| match scheme { | ||
| AuthorizationScheme::NoScheme => write!( | ||
| f, | ||
| "\nnote: the token does not include an authorization scheme prefix \ |
There was a problem hiding this comment.
The proper name for a "authorization scheme prefix" seems like an "authentication scheme"
4 participants
What does this PR try to resolve?
Based on the POC PR #15985, expand token rejected error message with authorization scheme.
Addresses issue #15021.
How to test and review this PR?
Run the tests in
tests/testsuite/registry_auth.rs:incorrect_token_unrecognized_schemeincorrect_token_bearer_schemeincorrect_token