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

Add support for trusted-host pip option #4944

Closed
wants to merge 5 commits into from

Conversation

fkapsahili
Copy link

Summary

The goal of this PR is to enable the trusted-host option like pip and to make it possible to bypass SSL validation when resolving and installing packages. With the trusted-host option it should be possible to specify a list of hosts that are ignored during validation. This option can be especially helpful when installing packages from private registries and behind corporate proxies.

@zanieb Can you take a look at the draft to see if I'm on the right track, especially with regard to the request handling of "secure" and "insecure" requests? Or do you see a possibility here with less refactoring involved? Currently I see no other option than setting danger_accept_invalid_certs to true when instantiating the BaseClient, but this would lead to two clients being maintained, as in my draft, since the request URL obviously cannot be determined at client build time.

Please note that this PR is still at an early stage and I would like to get early feedback on the possible implementation as I have very limited experience with Rust, but would love to help add this option to uv! 🙂

References: #1339

Test Plan

  • We should probably add tests to test the behavior where we normally get an SSL verification error with a self-signed certificate
  • Add tests to make sure that the trusted-host option is correctly applied to all existing configuration options

@zanieb zanieb self-assigned this Jul 9, 2024
@samypr100
Copy link
Collaborator

samypr100 commented Jul 10, 2024

Curious, would the option mentioned in #1339 (comment) be more feasible and help avoid having to maintain two clients or would a small change be needed upstream in reqwest to support passing such configuration? I haven't looked into it too much but found the references of where its being used in reqwest. Ideally all it takes is one configuration to pass a NoVerifier-style impl to a selected set of hosts, that way only one client config would be needed.

@timvink
Copy link

timvink commented Jul 11, 2024

Looking forward to this one! We can promote the new feature in the README also:

uv/README.md

Lines 111 to 113 in 42cb254

uv's `pip-install` and `pip-compile` commands support many of the same command-line arguments
as existing tools, including `-r requirements.txt`, `-c constraints.txt`, `-e .` (for editable
installs), `--index-url`, and more.

@zanieb
Copy link
Member

zanieb commented Jul 11, 2024

Thanks you for contributing! No worries that you don't have Rust experience we're happy to help out.

Did you explore the comment @samypr100 noted? I think we're all on the same page that it'd be ideal not to maintain multiple clients.

@fkapsahili
Copy link
Author

Curious, would the option mentioned in #1339 (comment) be more feasible and help avoid having to maintain two clients or would a small change be needed upstream in reqwest to support passing such configuration? I haven't looked into it too much but found the references of where its being used in reqwest. Ideally all it takes is one configuration to pass a NoVerifier-style impl to a selected set of hosts, that way only one client config would be needed.

Thanks for pointing this out!
Since the NoVerifier struct is responsible for disabling the certificate checks, I think we would need to add a method in the ClientBuilder to configure a NoVerifier for specific hosts, store these verifiers in a HashMap, and modify the connection logic to use these custom verifiers when making requests to the specified hosts. This would allow us to skip certificate checks for specific hosts and would allow us to continue to maintain only one client in uv:
https://github.com/seanmonstar/reqwest/blob/master/src/async_impl/client.rs

If this makes sense, I'll try to setup a PR in the reqwest library.

@zanieb
Copy link
Member

zanieb commented Jul 19, 2024

Do you mind if I close this so it's not in my review stack?

@fkapsahili
Copy link
Author

I am closing this as I am currently unable to continue working on this PR or the upstream changes in the reqwest library, although I would love to support the UV community with this! 😕

I also found that setting the SSL_CERT_FILE environment variable to the path of the corresponding root certificate solved my problem with an internal registry.

I hope this helps someone who has similar problems!

@fkapsahili fkapsahili closed this Jul 21, 2024
charliermarsh added a commit that referenced this pull request Aug 27, 2024
## Summary

This PR revives #4944, which I think
was a good start towards adding `--trusted-host`. Last night, I tried to
add `--trusted-host` with a custom verifier, but we had to vendor a lot
of `reqwest` code and I eventually hit some private APIs. I'm not
confident that I can implement it correctly with that mechanism, and
since this is security, correctness is the priority.

So, instead, we now use two clients and multiplex between them.

Closes #1339.

## Test Plan

Created self-signed certificate, and ran `python3 -m http.server --bind
127.0.0.1 4443 --directory . --certfile cert.pem --keyfile key.pem` from
the packse index directory.

Verified that `cargo run pip install
transitive-yanked-and-unyanked-dependency-a-0abad3b6 --index-url
https://127.0.0.1:8443/simple-html` failed with:

```
error: Request failed after 3 retries
  Caused by: error sending request for url (https://127.0.0.1:8443/simple-html/transitive-yanked-and-unyanked-dependency-a-0abad3b6/)
  Caused by: client error (Connect)
  Caused by: invalid peer certificate: Other(OtherError(CaUsedAsEndEntity))
```

Verified that `cargo run pip install
transitive-yanked-and-unyanked-dependency-a-0abad3b6 --index-url
'https://127.0.0.1:8443/simple-html' --trusted-host '127.0.0.1:8443'`
failed with the expected error (invalid resolution) and made valid
requests.

Verified that `cargo run pip install
transitive-yanked-and-unyanked-dependency-a-0abad3b6 --index-url
'https://127.0.0.1:8443/simple-html' --trusted-host '127.0.0.2' -n` also
failed.
@zanieb
Copy link
Member

zanieb commented Aug 27, 2024

Hey @fkapsahili just wanted to say thanks — we ended up pursuing a multi-client approach in #6591 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants