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

401 Installing from Internal Index due to .netrc #2323

Closed
hmc-cs-mdrissi opened this issue Mar 10, 2024 · 19 comments · Fixed by #2325
Closed

401 Installing from Internal Index due to .netrc #2323

hmc-cs-mdrissi opened this issue Mar 10, 2024 · 19 comments · Fixed by #2325
Assignees
Labels
bug Something isn't working

Comments

@hmc-cs-mdrissi
Copy link

hmc-cs-mdrissi commented Mar 10, 2024

I was excited by the work done in this issue, so I tried checking out main and testing uv to see if it improved performance on 1 package I found slow. Instead I found that using main uv now gives me a 401 error for installing any internal package with index server. Packages that previously worked to install now fail. I've bisected commits to find the first commit that fails is this commit.

The exact command being run is cargo run -- pip install --index-url=SOME_URL numpy. The url has credentials in it. I do have .netrc file, but they are not intended for this installation especially when --index-url has all credentials needed already and did work prior to that pr. Similarly pip works for similar command pip install --index-url=SOME_URL numpy and I'd guess is sticking to credentials inside index-url over mixing it with .netrc.

Here's the verbose logs from the first failing commit,

Logs
    0.000830s DEBUG uv_client::registry_client Using registry request timeout of 300s
 uv::requirements::from_source source=numpy
    0.257274s DEBUG uv_interpreter::python_environment Found a virtualenv through VIRTUAL_ENV at: /Users/pa-loaner/Snapchat/Dev/.venvs/bento_uv2
    0.257714s DEBUG uv_interpreter::interpreter Probing interpreter info for: /Users/pa-loaner/Snapchat/Dev/.venvs/bento_uv2/bin/python
    0.539855s DEBUG uv_interpreter::interpreter Found Python 3.9.16 for: /Users/pa-loaner/Snapchat/Dev/.venvs/bento_uv2/bin/python
    0.540817s DEBUG uv::commands::pip_install Using Python 3.9.16 environment at /Users/pa-loaner/Snapchat/Dev/.venvs/bento_uv2/bin/python
 uv_client::flat_index::from_entries
 uv_resolver::resolver::solve
      0.548281s   0ms DEBUG uv_resolver::resolver Solving with target Python version 3.9.16
   uv_resolver::resolver::choose_version package=root
   uv_resolver::resolver::get_dependencies package=root, version=0a0.dev0
        0.548614s   0ms DEBUG uv_resolver::resolver Adding direct dependency: numpy*
   uv_resolver::resolver::choose_version package=numpy
     uv_resolver::resolver::package_wait package_name=numpy
 uv_resolver::resolver::process_request request=Versions numpy
   uv_client::registry_client::simple_api package=numpy
     uv_client::cached_client::get_cacheable
       uv_client::cached_client::read_and_parse_cache file=/Users/pa-loaner/Library/Caches/uv/simple-v3/dc13ddb23a33f0c4/numpy.rkyv
 uv_resolver::resolver::process_request request=Prefetch numpy *
 uv_client::cached_client::from_path_sync path="/Users/pa-loaner/Library/Caches/uv/simple-v3/dc13ddb23a33f0c4/numpy.rkyv"
          0.549991s   0ms DEBUG uv_client::cached_client No cache entry for: https://registry.snapchat.com/python/virtual/numpy/
       uv_client::cached_client::fresh_request url="https://registry.snapchat.com/python/virtual/numpy/"
error: HTTP status client error (401 Unauthorized) for url (https://registry.snapchat.com/python/virtual/numpy/)

Like before, I'm happy to test out any specific commands/extra log statements that would be helpful.

edit: Credentials in netrc are not wrong though and maybe could be used somehow. The netrc looks like,

machine registry.snapchat.com login LCAv1 password AUTH

while --index-url passed is https://LCAv1:password@registry.snapchat.com/python/virtual. Login and password are same either way, but guessing the two aren't mixing as expected. I wonder if /python/virtual part is lost.

@hmc-cs-mdrissi hmc-cs-mdrissi changed the title 401 Installing from Internal Index 401 Installing from Internal Index due to .netrc Mar 10, 2024
@charliermarsh
Copy link
Member

Yeah this sounds like a mistake. Thanks for reporting.

@charliermarsh charliermarsh added the bug Something isn't working label Mar 10, 2024
@charliermarsh
Copy link
Member

Perhaps @zanieb can take a look when they're back since they've been owning auth.

@hmc-cs-mdrissi
Copy link
Author

I did one extra test and main works if I include index url without auth.

uv pip install --index-url=https://LCAv1:password@registry.snapchat.com/python/virtual # Fail
uv pip install --index-url=https://registry.snapchat.com/python/virtual # Works

So it's using netrc + index-url well if index-url doesn't also have authenication data in it. But if they both have it even if it's same password/login, then it fails.

@charliermarsh
Copy link
Member

I can try to take a look at it tomorrow.

@charliermarsh
Copy link
Member

The code for the netrc middleware is actually pretty small, and I'm surprised it's failing to handle this. It does look like it would override the credentials though. I'm not sure what's expected.

@charliermarsh
Copy link
Member

It looks like pip does explicitly prioritize existing credentials: pypa/pip#10998. I'm still not sure why it would fail here though since the credentials are exactly the same, right?

@charliermarsh
Copy link
Member

Oh wait, it was then reverted: pypa/pip#11134.

@hmc-cs-mdrissi
Copy link
Author

The credentials are identical. At work we have some script that we need to run daily to refresh our credentials. That script updates both .netrc and pip.conf with same credentials. I then manually copy the index url in pip.conf and pass it to uv.

@charliermarsh
Copy link
Member

Makes sense. You could try running with RUST_LOG=trace cargo run -- pip install --index-url=SOME_URL numpy --verbose to see if there's any other helpful info.

@charliermarsh
Copy link
Member

Okay, the error I get when I provide both sets of credentials is:

error: error sending request for url (https://pkgs.dev.azure.com/charlie0806/_packaging/charlie0806/pypi/simple/requests/): http2 error: stream error received: endpoint requires HTTP/1.1
  Caused by: http2 error: stream error received: endpoint requires HTTP/1.1
  Caused by: stream error received: endpoint requires HTTP/1.1

@charliermarsh
Copy link
Member

I think the problem is that we now send up two authorization headers.

@hmc-cs-mdrissi
Copy link
Author

I can confirm there's two headers for me too when it fails. I added the logs from this pr you did before. I also tried RUST_LOG=trace but that doesn't log headers.

For my use case it does not really matter which of the two credentials you pick. My personal intuition though is if url has credentials it'd be used first over a separate file.

@charliermarsh
Copy link
Member

Just trying to figure out how to replace a header with reqwest, the API doesn't seem to want you to do this...

@charliermarsh
Copy link
Member

@zanieb - I think this might be impossible to do with middleware, because the reqwest middleware seems to be purely additive. We might need to remove this middleware, and add a safe_copy_url_auth-like method to every HTTP path that gets the relevant netrc credentials, and replaces the authorization header with them.

@charliermarsh
Copy link
Member

Think I figured out a way to do it with the middleware API.

@charliermarsh
Copy link
Member

Should be fixed in #2325, thanks for filing and sorry about that regression.

@hmc-cs-mdrissi
Copy link
Author

I’ll test the PR in ~1 hour when I’m back home. The diagnosis makes full sense to me.

@charliermarsh
Copy link
Member

No rush! I’m going to bed anyway.

@hmc-cs-mdrissi
Copy link
Author

I've tested it and confirmed your fix resolves the issue. Installation works well trying both internal and public packages.

charliermarsh added a commit that referenced this issue Mar 10, 2024
## Summary

The netrc middleware we added in
#2241 has a slight problem. If you
include credentials in your index URL, _and_ in the netrc file, the
crate blindly adds the netrc credentials as a header. And given the
`ReqwestBuilder` API, this means you end up with _two_ `Authorization`
headers, which always leads to an invalid request, though the exact
failure can take different forms.

This PR removes the middleware crate in favor of our own middleware.
Instead of using the `RequestInitialiser` API, we have to use the
`Middleware` API, so that we can remove the header on the request
itself.

Closes #2323.

## Test Plan

- Verified that running against a private index with credentials in the
URL (but no netrc file) worked without error.
- Verified that running against a private index with credentials in the
netrc file (but not the URL) worked without error.
- Verified that running against a private index with a mix of
credentials in both _also_ worked without error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants