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

config.json access on sites that require auth but return 400 without auth #11460

Open
rbtcollins opened this issue Dec 6, 2022 · 39 comments
Open
Labels
A-registry-authentication Area: registry authentication and authorization (authn authz) C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted`

Comments

@rbtcollins
Copy link
Contributor

Problem

This is a usability papercut on the new registry authentication feature in nightly.

The merged code depends on a GET -> 401 -> GET-with-auth -> 200 sequence for config.json.

That doesn't work (today) for JFrog Artifactory cargo registries: they return 400 for the first request.

Obviously this is a thing that they'll fix eventually, but perhaps an option to allow prior-knowledge of auth tokens being required would be helpful, since I doubt they'll be the only site to do this weirdly.

Steps

No response

Possible Solution(s)

No response

Notes

No response

Version

No response

@rbtcollins rbtcollins added the C-bug Category: bug label Dec 6, 2022
@Eh2406 Eh2406 added C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` and removed C-bug Category: bug labels Dec 6, 2022
@Eh2406
Copy link
Contributor

Eh2406 commented Dec 6, 2022

I am reluctant to add this, as it provides an easy way for a user to accidentally send tokens to a url/registry that knows nothing about this RFC. That being said, if this over time turns out to be a real user need we can design and add it.

@rbtcollins
Copy link
Contributor Author

Fair enough. I am a bit confused about that easy way thing - I mean, any HTTP server that provides an auth challenge can harvest the token IF our configuration language permits tokens to be send to their URL, so I'm not sure how prior knowledge permission adds to that risk.

@Eh2406
Copy link
Contributor

Eh2406 commented Dec 7, 2022

I think a website has to be actively malicious to return 401 while keeping track of Auth headers. Whereas someone only needs to be careless to throw together a read-only registry (or for that matter any other website someone accidentally puts in their config.toml) that logs all inputs.

@ehuss ehuss added the A-registry-authentication Area: registry authentication and authorization (authn authz) label Dec 11, 2022
@sassman
Copy link

sassman commented Dec 17, 2022

I know the RFC 3139 does not really specify what I'm asking now for, but IMO it is very important to have.

As a user I would like to control the behavior of cargo, so that I can force-overwrite the flag "auth-required": true that comes right now from the registry index (see config.json).

The background for this is, that some of the commercial vendors, like JFrog / Artifactory support registries that are protected by an Auth-token, but they lack the flexibility to change the config.json in a way that this new feature can be turned on for cargo.

Additionally as a user I might want to be in control of wether my Auth-token is not send over to a specific registry or not.

In order to empower the user to force overwrite the very flag that is dictated by the registry it would require a to extend the config file ($CARGO_HOME/.cargo/config) in such / similar ways:

# $CARGO_HOME/.cargo/config

[registries.my-corporate-registry]
index = "https://my-corporate.jfrog.io/artifactory/git/my-corporate-registry.git"
# allowing the user to have explicit control
auth-required = true

It was already mentioned that other package managers in other languages offer such control to users by changing local configuration.

@sassman
Copy link

sassman commented Dec 19, 2022

@Eh2406 I would even contribute if that is going to be accepted, just let me know.

@Eh2406
Copy link
Contributor

Eh2406 commented Dec 19, 2022

So if I'm reading the discussion correctly there are two arguments for a auth-required in .cargo/config.toml. "Registries do not follow the RFC" and "users want more control".

As to 1. I don't yet feel justified in adding functionality to cargo to work around bugs in registries. I might be talked into a separate permanently unstable flag, if it really is the only way to get testing on the feature. It certainly true that if we require asymmetric tokens we cannot accommodate all the ways servers could implement it wrong.

As to 2. I don't fully understand the design you have intended. Can you specify the full 3x3 matrix of configurations and what should happen in each one?

Registry Config Behavior Read Behavior Mutation
Yes Yes Send token Send token
Unset Yes Send token Send token
No Yes ? ?
Yes Unset Send token? Send token
Unset Unset Don't send token Send token
No Unset Don't send token Error?
Yes No Send token? Send token
Unset No Don't send token Send token
No No Don't send token Error?

Also can you give some information on what use cases the user gets to opt into by having this new flag? What practical "more control" does this provide?

@sassman
Copy link

sassman commented Dec 26, 2022

As to 1. I don't yet feel justified in adding functionality to cargo to work around bugs in registries. I might be talked into a separate permanently unstable flag, if it really is the only way to get testing on the feature. It certainly true that if we require asymmetric tokens we cannot accommodate all the ways servers could implement it wrong

The suggestion with the separate unstable flag, to send an auth header always sounds fair enough to me.

As to 2. I don't fully understand the design you have intended. Can you specify the full 3x3 matrix of configurations and what should happen in each one?

What I had in mind was like this:

Registry Config Behavior Read Behavior Mutation
Yes Yes Send token Send token
Unset Yes Send token Send token
No Yes Send token Send token
Yes Unset Send token Send token
Unset Unset Don't send token Send token
No Unset Don't send token Send token (same as of today)
Yes No Don't send token Error with hint that the config says no
Unset No Don't send token Error with hint that the config says no
No No Don't send token Error with hint that the config says no

But to be honest, the more I'm thinking about it I see the potential confusion that it could cause.

So maybe your suggestion with a separate unstable flag would be a less confusing thing then.

@rbtcollins
Copy link
Contributor Author

FWIW I've worked around this by implementing a small HTTPS proxy to transform the Jfrog responses while JFrog work on the bug. I note that https://orth.uk/cargo-registry-authentication/ is another way to workaround this, just using different tooling.

Also going to ping @AsafZalcman-jfrog for visibility.

@nadav-yo
Copy link

nadav-yo commented Jan 3, 2023

That doesn't work (today) for JFrog Artifactory cargo registries: they return 400 for the first request.

Which endpoint returns 400?
I'm familiar with an issue we already addressed of auth-required=true always being sent on sparse-index enabled repos, even if anonymous access was enabled. this can configurable by the repo anonymous flag.

(Nadav from JFrog)

@ehuss ehuss added the Z-asymmetric-token Nightly: asymmetric-token authentication label Jan 3, 2023
@ykitamura-mdsol
Copy link

@nadav-yo ~/index/config.json endpoint seems to return 400:

$ curl https://my-corporate.jfrog.io/artifactory/api/cargo/my-corporate-registry/index/config.json
{
  "errors" : [ {
    "status" : 400,
    "message" : "Bad Request"
  } ]
}%

Interestingly, ~/index/ returns 401:

$ curl https://my-corporate.jfrog.io/artifactory/api/cargo/my-corporate-registry/index/
{
  "errors" : [ {
    "status" : 401,
    "message" : "Authentication is required"
  } ]
}%

X-JFrog-Version: Artifactory/7.47.12 74712900

@nadav-yo
Copy link

nadav-yo commented Feb 5, 2023

@ykitamura-mdsol
Assuming that sparse index flag is enabled, allow anonymous is disabled and the repository has been reindex -

Artifactory have a special handling of cargo endpoints, to allow anonymous requests to bypass the filter on very specific endpoints, as the client did not send credentials. so you get a semi-elevated permissions, only for that endpoint.

We've made a change on Artifactory 7.50 to NOT mask Cargo anonymous response statuses. You actually got 403 on the backend, but it was masked as 400.
But - why 403? it's actually a change I've noticed, as I recall at the first versions of cargo support of token authentication, either a token was always sent, or the requests was always anonymous. Now I see there is a auth-challenge request.

Anyway, I've adjusted Artifactory code and it will now return 401 as intended (removed this special handling all in all if the anoymous flag is turned off) . doing my best to make it to the next minor version.

@rbtcollins
Copy link
Contributor Author

rbtcollins commented Feb 5, 2023

Hi @nadav-yo currently we get 403 (not 401) on first request to config.json. I went through a recorded zoom call with one of your staff last week to share info on this - I missed the bug update or would have replied here earlier ;).

This is a new local registry, auth required, sparse enabled, fully indexed.

So yes, I think that your latest change will fix it.

@nadav-yo
Copy link

nadav-yo commented Feb 5, 2023

@rbtcollins makes sense, that's the behavior after the first fix I've mentioned. (not masking the responses)
As we designed the authentication much before it was actually supported, we had some gaps to fill when this nightly feature came out. we usually try to test behaviors with clients as much as we can, but we didn't had auth in the client to test with.

Anyway, my fix has been merged, So I think starting from next minor we will be fully compatible with this nightly auth feature.
For SaaS I may push it even sooner as a patch fix if I'll be able to and it's requested.

@Fishrock123
Copy link

Fishrock123 commented Aug 2, 2023

It seems to me that artifactory with sparse registry returns 401 for config.json:

Caused by:
  failed to query replaced source registry `crates-io`

Caused by:
  failed to get successful HTTP response from `https://artifactory.x.x/artifactory/x/config.json`, got 401
  body:
  {
    "errors" : [ {
      "status" : 401,
      "message" : "Unauthorized"
    } ]
  }

Edit: this might be due to our internal setup...

@nadav-yo
Copy link

nadav-yo commented Aug 2, 2023

@Fishrock123 That looks like either the Artifactory Cargo repo anonymous download/search is turned off, or you didn't configured the client to authenticate with the registry-auth nightly flag (whichever apply)

@sassman
Copy link

sassman commented Aug 29, 2023

@nadav-yo

or you didn't configured the client to authenticate with the registry-auth nightly flag

This is a misconception, the cargo nightly flag for registry-auth, does not turn the authentication on.

All it does is it would accept if the server has turned authentication on like this:

# config.json
{
    "dl": "https://artifactory.x.x/artifactory/api/cargo/x/v1/crates",
    "api": "https://artifactory.x.x/artifactory/api/cargo/x",
    "auth-required": true
}

But if this "auth-required": true is not there, cargo won't turn it on by any configuration argument.

@nadav-y
Copy link

nadav-y commented Aug 29, 2023

@sassman
I know, that was actually my point, as it is there.

Artifactory does send "auth-required": true if it set up correctly.
The client will send authentication only with the nightly flag. (and given that the first server did send the auth-required)

So, that looks like either the Artifactory Cargo repo anonymous download/search is enabled (which will make artifactory not send the "auth-required": true) or the client did not send authentication, probably because the registry-auth nightly flag wasn't added to the command.

$ curl https://RTURL/artifactory/api/cargo/cargo-local/index/config.json -uUSERNAME
{
  "dl" : "https://RTURL/artifactory/api/cargo/cargo-local/v1/crates",
  "api" : "https://RTURL/artifactory/api/cargo/cargo-local",
  "auth-required" : true
}

@qartik
Copy link

qartik commented Sep 1, 2023

@nadav-y / @nadav-yo I am facing a different issue with Artifactory's support of registry auth. After storing the token correctly, it's rejected during the request.

Anon downloads are off.

@nadav-yo
Copy link

nadav-yo commented Sep 1, 2023

@qartik Did you add
"Bearer "
before the actual token?

@qartik
Copy link

qartik commented Sep 1, 2023

Interesting, I was certainly curious why that was mentioned in the docs. But indeed adding that helped. cargo login command doesn't seem to add it, why does Artifactory's implementation require it?

@nadav-yo
Copy link

nadav-yo commented Sep 1, 2023

@qartik it's not artifactory, that's the exact authorization header cargo client will send artifactory.

From https://rust-lang.github.io/rfcs/3139-cargo-alternative-registry-auth.html -
The authorization token would be sent as an HTTP header, exactly how it is currently sent for operations such as publish or yank:

Authorization: token

As the authorization header always requires to send the authorization schema, Bearer need to be added

@qartik
Copy link

qartik commented Sep 1, 2023

@nadav-yo thanks!

Another issue I am facing is that our cloud-hosted Artifactory doesn't have Cargo among the types for a virtual repo. Hence, there doesn't seem to be any way to use Artifactory for both local uploads and external (crates.io) downloads. Is that something you are aware of as an existing issue?

@nadav-yo
Copy link

nadav-yo commented Sep 1, 2023

@qartik yeah, there is currently no virtual cargo repo support, basically because of some limitation on cargo side, as we see it.
The framework for everything is mostly done on our side.
However, if you'll follow the steps on our wiki, you'll see there is an easy way to use local and remote together.

@nadav-yo
Copy link

nadav-yo commented Sep 1, 2023

Yeah, even though most of the development is done on our side, there is no virtual repository support for cargo, because of limitation on cargo client as we see it. See https://rust-lang.github.io/rfcs/3139-cargo-alternative-registry-auth.html

However, if you'll read our documentation see that this could mostly be achieved with the replace-with.

@qartik
Copy link

qartik commented Sep 1, 2023

Thanks! I wish the documentation was clearer and updated. For example, I don't see the requirement to use nightly features such as registry-auth is mentioned there. Without that, the only option is anonymous downloads whose security implications are not described.

Further the marketing page does say "aggregate local and remote resources under a single virtual Cargo repository to access all your Rust crates from a single URL" (emphasis mine).

Coming back to the issue of using local and remote together. I don't see a clear way just yet. I know crates-io needs to be replace-with our artifactory-remote, but that cannot be resolved if I want to have artifactory repo (referring to local) as the default.

Are you just suggesting developers pass the name of the repo in all their cargo commands?

@nadav-y
Copy link

nadav-y commented Sep 1, 2023

You are correct, I don't know who wrote that, probably they copy pasted by mistake from other repository but I'll tell the team to fix that next week.
Usually the good stuff is at https://jfrog.com/help/r/jfrog-artifactory-documentation/cargo-package-registry 😆

Regarding your question, the way I use it is -
assume a package with Chart.toml of

[package]
name = "other2"
version = "0.1.3"
authors = ["nadavy <nadavy@jfrog.com>"]
edition = "2018"
publish = ["artifactory"]

[dependencies]
other =  { version = "0.1.0", registry = "artifactory" }
serde = "1.0.188"

while the config.toml has
default = "artifactory"
and artifactory-remote, with sources replace-with section of

[source.crates-io]
replace-with = "artifactory-remote"

@qartik
Copy link

qartik commented Sep 1, 2023

while the config.toml has default = "artifactory" and artifactory-remote, with sources replace-with section of

[source.crates-io]
replace-with = "artifactory-remote"

I believe I have the same setup. But, what happens when you issue let's say cargo add serde or cargo install hyperfine?

In my case, I get:

❯ cargo add serde
    Updating `artifactory` index
error: the crate `serde` could not be found in registry index.
❯ cargo install hyperfine
    Updating `artifactory` index
error: could not find `hyperfine` in registry `artifactory` with version `*`

@nadav-y
Copy link

nadav-y commented Sep 1, 2023

By client limitations, you will have to specify the registry unfortunately
cargo add hyperfine --registry artifactory-remote

@qartik
Copy link

qartik commented Sep 1, 2023

Got it, that much seemed clear enough. I was curious to see that in your example:

[dependencies]
other = { version = "0.1.0", registry = "artifactory" }
serde = "1.0.188"

you have serde without an explicit registry value. I don't see a way to enable that behavior automatically. I get:

❯ cargo add serde --registry artifactory-remote
    Updating `artifactory-remote` index
note: name of alternative registry `sparse+https://corp.jfrog.io/artifactory/api/cargo/cargo-remote/index/` set to `artifactory-remote`
      Adding serde v1.0.188 to dependencies.

and hence the following with an unnecessary registry annotation (being a mirror of crates.io) in Cargo.toml:

serde = { version = "1.0.188", registry = "artifactory-remote" }

Perhaps cargo just needs to add an option to choose whether a user wants that name recorded.

@nadav-yo
Copy link

nadav-yo commented Sep 1, 2023

Yeah, I honestly don't use cargo add myself, and prefer to edit the toml myself. In this example both of the dependencies were added manually

@Fishrock123
Copy link

Artifactory does send "auth-required": true if it set up correctly.

@nadav-y could you please elaborate on the Artifactory options you have set or unset?

@nadav-y
Copy link

nadav-y commented Sep 7, 2023

@Fishrock123
Sure!
First, I have my global anonymous access turned off (it's the default)
image

Then, in my cargo remote and local repositories I have
Allow Anonymous download and search OFF
Enable sparse index support ON
(Those are the defaults too)
image

If anything changed, or the base URL has changed, a reindex is required

@weihanglo weihanglo removed the Z-asymmetric-token Nightly: asymmetric-token authentication label Sep 20, 2023
@cpaika
Copy link

cpaika commented Mar 12, 2024

Seeing a similar but different issue where Artifactory is sending a 401 to the cargo CLI on the initial request, if multiple cargo requests happen at the same time with the same credentials they can rate limit themselves: #13574

@wichert
Copy link

wichert commented Jun 7, 2024

If I understand the discussion here, the implemented behaviour is:

  1. fetch config.json
  2. if config.json contains "auth-required": true, allow other requests will use authentication

This assumes that config.json can always be retrieved without authentication. This is not true for Artifactory: that requires authentication for all requests (unless the repository is public). Is that a correct summary?

If so, I can think of three ways to fix this:

  1. require a registry to serve config.json without authentication. That would mean an update in section 3.13.2.1 of the cargo book, and convincing jFrog to update Artifactory.
  2. support a registries.<name>.auth-required config option, which if set to true makes cargo include auth headers for all requests, including fetching config.json
  3. modify cargo to always send credentials if they are configured for the registry.

@nadav-yo
Copy link

nadav-yo commented Jun 7, 2024 via email

@wichert
Copy link

wichert commented Jun 10, 2024

@nadav-yo Perhaps it can, but the artifactory I am dealing with does not it seems.

Ignoring artifactory, is my understanding of cargo behavior correct?

@Eh2406
Copy link
Contributor

Eh2406 commented Jun 18, 2024

@wichert No, your understanding of cargo behavior is not correct.

The implemented behaviour is:

  1. fetch config.json without authentication, if it succeeds skip to step 3.
  2. if the registry responds with 401 unauthorized then refetch config.json with authentication.
  3. if config.json contains "auth-required": true, allow other requests will use authentication.

The problem reported in this issue is that the registry responds with 400 not with 401. This is simply the registry not following cargoes specification for registrys.

@wichert
Copy link

wichert commented Jun 19, 2024

@Eh2406 In my case the registry returns a 403, not a 400. What is the expected behavior in that case?

< HTTP/1.1 403 
< Date: Wed, 19 Jun 2024 06:49:24 GMT
< Content-Type: application/json
< Connection: keep-alive
< X-JFrog-Version: Artifactory/7.71.4 77104900
< 
{
  "errors" : [ {
    "status" : 403,
    "message" : "Download request for repo:path 'geo-cargo-local:config.json' is forbidden for user: 'anonymous'."
  } ]

@Eh2406
Copy link
Contributor

Eh2406 commented Jun 19, 2024

@wichert the current behavior, as planned in the RFC and as spelled out in our documentation and as implemented in codechecks explicitly for 401 (Unauthorized).

We have needed to loosen similar checks in the past, when experts in the HTTP specification point out we are being overly strict. But the documentation I'm seeing is pretty specific that 403 is not a request for retry.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/403

This status is similar to 401, but for the 403 Forbidden status code, re-authenticating makes no difference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-registry-authentication Area: registry authentication and authorization (authn authz) C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted`
Projects
None yet
Development

No branches or pull requests