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 #6591

Merged
merged 5 commits into from
Aug 27, 2024
Merged

Add support for --trusted-host #6591

merged 5 commits into from
Aug 27, 2024

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Aug 24, 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.

@charliermarsh charliermarsh added compatibility Compatibility with a specification or another tool configuration Settings and such security labels Aug 24, 2024
@charliermarsh charliermarsh force-pushed the charlie/trust branch 2 times, most recently from 0c76816 to 8aafa26 Compare August 24, 2024 23:15
}
})
{
&self.dangerous_client
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only way to access this client is via for_host. It's not exposed anywhere else.

@charliermarsh charliermarsh force-pushed the charlie/trust branch 2 times, most recently from fba6f6a to 7c440dd Compare August 24, 2024 23:20
@zanieb
Copy link
Member

zanieb commented Aug 25, 2024

Also might be helpful for #5726

@gaby
Copy link

gaby commented Aug 25, 2024

What happens if --trusted-host is supplied multiple times, is that taken into account? Or do the later override the first one?

Edit: I see the code allows a list, which if it's like pip it would be space-separated.

crates/uv-cli/src/lib.rs Outdated Show resolved Hide resolved
super::TrustedHost::HostPort("example.com".to_string(), 8080)
);

assert_eq!(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test case for something with a path? e.g. https://example.com/hello/world?

Comment on lines 4 to 10
/// A trusted host, which could be a host or a host-port pair.
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub enum TrustedHost {
Host(String),
HostPort(String, u16),
}
Copy link
Member

@zanieb zanieb Aug 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can / should use Realm in place of this (crates/uv-auth/src/realm.rs) — I think you can just move the from_str implementation over. I'm not sure if that adds complexity to the setting schema part — but it seems like we could just have Realm in the setting schema.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"should" because it's our canonical way to do this and there's From<&Url> for Realm.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't that require a scheme? These values don't require a scheme.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does yeah, hm. But if someone provides a scheme here (which is allowed per the parser), shouldn't that match be enforced?

Copy link
Member

@zanieb zanieb Aug 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scheme could become optional on Realm, I'm not sure if that causes problems. I don't feel strongly if it seems like a pain. There's not a lot of complexity in the matching anymore (Realm got simplified at some point, relying on some behaviors from reqwest)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need to test what happens if you set HTTPS_PROXY or ALL_PROXY to an http:// URL?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zanieb That's typically what I use when using something like Squid-Proxy + Pypi. I set both HTTP_PROXY and HTTPS_PROXY to "http://squid-hostname`, and let Squid handle the request/redirect/etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should allow http://. It's normal practice to run internal pypi mirror that runs on http. That's the whole point of trusting a host.

This is a reason as to why I'm suggesting a rename away from --trusted-host as it's not about trusting a host, rather skipping TLS verification 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zanieb - I opted for now to keep TrustedHost, because the requirements are different than Realm... Realm requires a scheme, TrustedHost requires a host (Realm does not), etc. I was also hesitant to make uv-auth a dependency everywhere, and add Schemars as a dep to that crate.

@charliermarsh charliermarsh force-pushed the charlie/trust branch 2 times, most recently from 0cd60b3 to 64715d3 Compare August 25, 2024 15:49
@pradyunsg
Copy link

You’re receiving notifications because you were mentioned.

Not sure why GitHub thinks I have a mention here... this seems fine to me, in case someone mentioned me for asking my opinion on the idea. :)

Copy link
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@graingert
Copy link

graingert commented Aug 27, 2024

You’re receiving notifications because you were mentioned.

Not sure why GitHub thinks I have a mention here... this seems fine to me, in case someone mentioned me for asking my opinion on the idea. :)

You were tagged here #6591 (comment)

@charliermarsh charliermarsh merged commit d86075f into main Aug 27, 2024
56 checks passed
@charliermarsh charliermarsh deleted the charlie/trust branch August 27, 2024 13:36
@jonathanasdf
Copy link

I seem to be getting some errors, eg.

[tool.uv]
allow-insecure-host = [
  "amazonaws.com",
]
warning: Failed to parse `pyproject.toml` during settings discovery:
  TOML parse error at line 93, column 1
     |
  93 | [tool.uv]
     | ^^^^^^^^^
  invalid type: string "amazonaws.com", expected struct TrustedHost

@charliermarsh
Copy link
Member Author

Apologies, that's my bad. Can you open a separate issue? It should work on the command-line though.

@charliermarsh
Copy link
Member Author

See: #6716

tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Aug 28, 2024
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.3.2` -> `0.3.5` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>astral-sh/uv (astral-sh/uv)</summary>

### [`v0.3.5`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#035)

[Compare Source](astral-sh/uv@0.3.4...0.3.5)

##### Enhancements

-   Add support for `--allow-insecure-host` (aliased to `--trusted-host`) ([#&#8203;6591](astral-sh/uv#6591))
-   Read requirements from `requires.txt` when available ([#&#8203;6655](astral-sh/uv#6655))
-   Respect `tool.uv.environments` in `pip compile --universal` ([#&#8203;6663](astral-sh/uv#6663))
-   Use relative paths by default in `uv add` ([#&#8203;6686](astral-sh/uv#6686))
-   Improve messages for empty solves and installs ([#&#8203;6588](astral-sh/uv#6588))

##### Bug fixes

-   Avoid reusing state across tool upgrades ([#&#8203;6660](astral-sh/uv#6660))
-   Detect musl and error for musl Python builds ([#&#8203;6643](astral-sh/uv#6643))
-   Ignore `send` errors in installer ([#&#8203;6667](astral-sh/uv#6667))

##### Documentation

-   Add development section to Docker guide and reference new example project ([#&#8203;6666](astral-sh/uv#6666))
-   Add docs for `constraint-dependencies` and `override-dependencies` ([#&#8203;6596](astral-sh/uv#6596))
-   Clarify package priority order in pip compatibility guide ([#&#8203;6619](astral-sh/uv#6619))
-   Fix docs for disabling build isolation with `uv sync` ([#&#8203;6674](astral-sh/uv#6674))
-   Improve consistency of directory lookup instructions in Docker ([#&#8203;6665](astral-sh/uv#6665))
-   Improve lockfile concept documentation, add coverage for upgrades ([#&#8203;6698](astral-sh/uv#6698))
-   Shift the order of some of the Docker guide content ([#&#8203;6664](astral-sh/uv#6664))
-   Use `python` to highlight requirements and use more content tabs ([#&#8203;6549](astral-sh/uv#6549))

### [`v0.3.4`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#034)

[Compare Source](astral-sh/uv@0.3.3...0.3.4)

##### CLI

-   Show `--editable` on the `uv add` CLI ([#&#8203;6608](astral-sh/uv#6608))
-   Add `--refresh` to `tool run` warning for `--with` dependencies ([#&#8203;6609](astral-sh/uv#6609))

##### Bug fixes

-   Allow per dependency build isolation for `setup.py`-based projects ([#&#8203;6517](astral-sh/uv#6517))
-   Avoid un-strict syncing by-default for build isolation ([#&#8203;6606](astral-sh/uv#6606))
-   Respect `--no-build-isolation-package` in `uv sync` ([#&#8203;6605](astral-sh/uv#6605))
-   Respect extras and markers on virtual dev dependencies ([#&#8203;6620](astral-sh/uv#6620))
-   Support PEP 723 scripts in GUI files ([#&#8203;6611](astral-sh/uv#6611))
-   Update lockfile after setting minimum bounds in `uv add` ([#&#8203;6618](astral-sh/uv#6618))
-   Use relative paths for `--find-links` and local registries ([#&#8203;6566](astral-sh/uv#6566))
-   Use separate types to represent raw vs. resolver markers ([#&#8203;6646](astral-sh/uv#6646))
-   Parse wheels `WHEEL` and `METADATA` files as email messages ([#&#8203;6616](astral-sh/uv#6616))
-   Support unquoted hrefs in `--find-links` and other HTML sources ([#&#8203;6622](astral-sh/uv#6622))
-   Don't canonicalize paths to user requirements ([#&#8203;6560](astral-sh/uv#6560))

##### Documentation

-   Add FastAPI guide to overview ([#&#8203;6603](astral-sh/uv#6603))
-   Add docs for disabling build isolation with `uv sync` ([#&#8203;6607](astral-sh/uv#6607))
-   Add example of reading script from stdin using echo ([#&#8203;6567](astral-sh/uv#6567))
-   Add tip to use intermediate layers in Docker builds ([#&#8203;6650](astral-sh/uv#6650))
-   Clarify need to include `pyproject.toml` with `--no-install-project` ([#&#8203;6581](astral-sh/uv#6581))
-   Move `WORKDIR` directive in Docker examples ([#&#8203;6652](astral-sh/uv#6652))
-   Remove duplicate `WORKDIR` directive in Docker example ([#&#8203;6651](astral-sh/uv#6651))

### [`v0.3.3`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#033)

[Compare Source](astral-sh/uv@0.3.2...0.3.3)

##### Enhancements

-   Add `uv sync --no-install-project` to skip installation of the project ([#&#8203;6538](astral-sh/uv#6538))
-   Add `uv sync --no-install-workspace` to skip installation of all workspace members ([#&#8203;6539](astral-sh/uv#6539))
-   Add `uv sync --no-install-package` to skip installation of specific packages ([#&#8203;6540](astral-sh/uv#6540))
-   Show previous version in self update message ([#&#8203;6473](astral-sh/uv#6473))

##### CLI

-   Add `--no-project` alias for `uv python pin --no-workspace` ([#&#8203;6514](astral-sh/uv#6514))
-   Ignore `.python-version` files in `uv venv` with `--no-config` ([#&#8203;6513](astral-sh/uv#6513))
-   Include virtual environment interpreters in `uv python find` ([#&#8203;6521](astral-sh/uv#6521))
-   Respect `-` as stdin channel for `uv run` ([#&#8203;6481](astral-sh/uv#6481))
-   Revert changes to pyproject.toml when sync fails duing `uv add` ([#&#8203;6526](astral-sh/uv#6526))

##### Configuration

-   Add `UV_COMPILE_BYTECODE` environment variable ([#&#8203;6530](astral-sh/uv#6530))

##### Bug fixes

-   Set `VIRTUAL_ENV` for `uv run` invocations ([#&#8203;6543](astral-sh/uv#6543))
-   Ignore errors in workspace discovery with `--no-project` ([#&#8203;6554](astral-sh/uv#6554))

##### Documentation

-   Add documentation for `uv python find` ([#&#8203;6527](astral-sh/uv#6527))
-   Add uv tool install example in Docker ([#&#8203;6547](astral-sh/uv#6547))
-   Document why we do lower bounds ([#&#8203;6516](astral-sh/uv#6516))
-   Fix to miss string termination in PowerShell commands for shell autocompletion documentation ([#&#8203;6491](astral-sh/uv#6491))
-   Fix incorrect workspace members keyword ([#&#8203;6502](astral-sh/uv#6502))
-   Use proper environment variables for Windows ([#&#8203;6433](astral-sh/uv#6433))
-   Improve caveat in `uvx` note ([#&#8203;6546](astral-sh/uv#6546))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility Compatibility with a specification or another tool configuration Settings and such security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pip install --trusted-host support
9 participants