-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: also try to auth for config if server returns status 403 #12446
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
feat: also try to auth for config if server returns status 403 #12446
Conversation
This makes it so that the `registry-auth` unstable feature also attempts to make authorized requests to a registry's `config.json` (in sparse mode) if the server's response code is 403, not just 401. These status codes have very similar meanings and some 3rd-party registry providers (e.g. Artifactory) prefer to return 403 for unauthorized requests. Also updates the docs on this feature to reflect this.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
r? @arlosi You're a better reviewer than me for this PR. |
The intent of only sending the token for HTTP 401 is to minimize the possibility of sending a token to a server that's not expecting it. Additionally allowing HTTP 403 relaxes that slightly. Is there a reason why Artifactory can't send HTTP 401 when Cargo is attempting to fetch |
I don’t work for JFrog / Artifactory so I cannot answer that. However it seems like 403 is generally what Artifactory returns for any unauthenticated request. |
I'd rather not merge this unless we know we need to, since it makes Cargo slightly more likely to send a token to a URL where it shouldn't. Given that a registry already has to implement the rest of the Cargo protocol, it doesn't seem too difficult to send HTTP 401 for authentication failure. I suspect that the biggest blocker for registries adding Cargo support likely that I'm happy to revisit this if we get a compelling request from a registry operator though. Thanks. |
This makes it so that the
registry-auth
unstable feature also attempts to make authorized requests to a registry'sconfig.json
(in sparse mode) if the server's response code is 403, not just 401.These status codes have very similar meanings and some 3rd-party registry providers (e.g. Artifactory) prefer to return 403 for unauthorized requests.
Also updates the docs on this feature to reflect this.
What does this PR try to resolve?
Artifactory likes to return 403 for unauthorized requests.
The meanings of HTTP
401 Unauthorized
and HTTP403 Forbidden
are very similar and largely overlapping.In order to make Cargo's
registry-auth
feature play better with 3rd-party registries, I would recommend also supporting attempting authentication for 403s even if that is not the perfectly semantic meaning.How should we test and review this PR?
Ideally, talk to a 3rd-party registry such as artifactory with this change included and all the config set on the 3rd party registry.
Additional information
Related tracking issue for the whole
registry-auth
feature: #10474