rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.54k stars 2.38k forks source link

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

Open rbtcollins opened 1 year ago

rbtcollins commented 1 year ago

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

Eh2406 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

sassman commented 1 year ago

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 commented 1 year ago

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

Eh2406 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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)

ykitamura-mdsol commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

qartik commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

qartik commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 12 months ago

@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

cpaika commented 5 months ago

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: https://github.com/rust-lang/cargo/issues/13574

wichert commented 3 months ago

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 commented 3 months ago

That's actually incorrect - Artifactory can get some endpoints without authentication, like terraform discovery API, npm login (start of the npm auth flow), or this config.json.

On Fri, Jun 7, 2024, 17:09 Wichert Akkerman @.***> wrote:

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 https://doc.rust-lang.org/nightly/cargo/reference/registry-index.html#index-configuration, and convincing jFrog to update Artifactory.
  2. support a registries..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.

— Reply to this email directly, view it on GitHub https://github.com/rust-lang/cargo/issues/11460#issuecomment-2154927702, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACO4J6OJIUPCX7CL6ZT4MZDZGG5LJAVCNFSM6AAAAAASVXHJR2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJUHEZDONZQGI . You are receiving this because you were mentioned.Message ID: @.***>

wichert commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

@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.