-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Use .rcdata to store trampoline type + path to python binary
#15068
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
Conversation
|
I'm not super qualified to review this idea, cc @samypr100 & @konstin |
T-256
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, it's not necessary to done before merging, but can you confirm that it can bring back python3.7 support? (i.e. uvx -p 3.7 ruff most work)
|
Added an even sloppier (but working) refactor of It is now verifiably robust to code-signing, so that solves my original problem. I have not verified the py3.7 case, though a similar integration test can be added probably very easily? @T-256 Also note that the Win32 APIs are all really rather |
|
I don't see any immediate issues with using rcdata for the trampoline kind and runnable path, it's mainly a different way to accomplish the same thing. What I'm not very sure of is using it for the script contents. From a code signing perspective is the goal to sign the launcher regardless of what it may end up executing? |
The goal is to be able to sign a (finished) entrypoint executable. |
81627de to
da84e64
Compare
|
Getting there...
If anyone is able to give me a hand on any of these, it would be amazing! If not I will try to keep chipping away over the next few days |
You can build the binaries on your machine for testing and CI (there are cross-compile instructions in the readme), before merging the PR a maintainer will build the binaries on their machine and push them to this branch. |
a98aeea to
b7bab03
Compare
7716271 to
12bdf11
Compare
|
Close...
|
crates/uv-python/src/managed.rs
Outdated
| if cfg!(unix) { | ||
| #[cfg(unix)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the previous use of if cfg! instead of #[cfg] was intentional so you can edit and check this code when not on a matching platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'm cleaning this up)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's my commit f2b69d2 — I can't push to your repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very weird -- I've ticked the 'accept pushes from maintainers' option. (I have invited you as a collaborator to my fork anyway)
| let mut file = fs_err::OpenOptions::new() | ||
| .create_new(true) | ||
| .write(true) | ||
| .open(target)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these dropped? These are important?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change of semantics. write_to_file() now takes a file path, not a file handle, and therefore takes care of file creation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need it to fail if it's not creating a new file, that's why the original API took a handle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little more context at #14790 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I wasn't aware of this requirement. (I still don't get it, but I trust that it's valid)
IIRC I refactored it this way in order to avoid double-opening the file, which is fraught on Windows. Using a fs_err::write_file() one-and-done is nice because it leaves no contention for write_resources() which likewise opens the file, but in a winapi way. (none of the winapi methods accept native Rust File objects, so you couldn't even properly re-use the file handle either.)
Maybe add a boolean exists_ok arg to write_to_file()? And replace fs_err::write_file() with a deliberately scoped block with the appropriate OpenOptions... maybe that will do the trick?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will look a little closer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the most immediate solution is to include an API which performs the same two-step operation as windows_script_launcher where it writes everything to a temporary file, reads that to get the bytes, then writes that to an open file handle.
As a separate note, the two step operation in windows_script_launcher is pretty weird for normal operation! We're writing everything just to read it then write it again :) I worry about the performance implications of that and would like to refactor to avoid that unless the caller needs ownership of the file handle (e.g., as above).
Unfortunately, the general pattern of using write_file then write_resources does leave room for races (and this is not just theoretical, things like this have bit us before). I think it's okay for now, but we'll probably want to look for a way to avoid that too.
Circling back, I guess the two step approach of windows_script_launcher actually does resolve this problem... which hints to a simple solution: just construct the launcher in a temporary file that we're the sole writer to, then rename it into place (this is what we do for atomic writes in general, e.g., see write_atomic).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy for you to decide which way you want to go? I do think that the two-step operation is not optimal (or at least not aesthetically pleasing), although I kept it as such to avoid gratuitous API churn -- it's a chunky PR already. But if it does incidentally give us safety, then I guess that's alright. Although I imagine it is better to make the atomicity explicit rather than incidental!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it looks like there'd be a bunch of API churn here because we need to hash the contents for the RECORD when writing entrypoints.
I think we should change it. I'm vaguely worried about this doubling the overhead of writing entrypoints on Windows, but they're small so I can live with it for now. It makes sense to do separately.
d4303fd addresses my blocking comment, I think.
…path to python binary `.rsrc` is the idiomatic way of storing metadata and non-code resources in PE binaries. This should make the resulting binaries more robust as they are no longer dependent on the exact location of a certain magic number. Addresses: astral-sh#15022
|
FWIW before merge, I just tested binary of x86_64 got from ci summary with Python3.7. |
But then it wouldn't work with code-signing, because fixed positioning of the zip blob gets wrecked by code signing tools. Also -- the current trampoline is already incompatible with 3.7. (Do we actively want to reintroduce compatibility?) |
No — if this introduced compatibility that'd be a nice property, but it's not an active goal for the project. |
|
Hello! Sorry for not engaging much on this PR in the last month and a bit. I am still interested in it -- is there any way I can help? |
|
Sorry I lost track of this too. Let me take a look again! |
.rcdata to store trampoline type + path to python binary.rcdata to store trampoline type + path to python binary
Cargo.toml
Outdated
| hyper-util = { version = "0.1.8", features = ["tokio"] } | ||
| ignore = { version = "0.4.23" } | ||
| insta = { version = "1.40.0", features = ["json", "filters", "redactions"] } | ||
| p12 = { version = "0.6.3" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should be using https://docs.rs/pkcs12/latest/pkcs12/ instead which is maintained?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(It actually looks like that doesn't support writing archives)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a reason P12 format was needed in particular?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure.
I also briefly looked at https://github.com/ancwrd1/p12-keystore which seems fine too.
It'd be nice to use a dependency that comes from a higher trust source, even if it's just for a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the test and the Set-AuthenticodeSignature docs, I wonder if its possible to avoid P12 entirely by manually instantiating a System.Security.Cryptography.X509Certificates.X509Certificate2 bundle. I believe that would allows us to only rely on rcgen and not p12 (if it's possible). I'll give it a shot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Success, although it will require use of pwsh rather than powershell because System.Security.Cryptography.X509Certificates.X509Certificate2::CreateFromPemFile is .NET 5+
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI should have pwsh already so I don't think its a problem and I don't think we truly need to test in legacy powershell in this particular case as we've proven it works with legacy powershell already using pfx. I'll push my changes soon in case you're interested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 153d854
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! Re the 'why' question: I did it because of a (possibly incorrect) inference that pwsh on CI did not have the ability to modify the credential store
Cargo.toml
Outdated
| walkdir = { version = "2.5.0" } | ||
| which = { version = "8.0.0", features = ["regex"] } | ||
| windows = { version = "0.59.0", features = ["Win32_Globalization", "Win32_Security", "Win32_System_Console", "Win32_System_Kernel", "Win32_System_Diagnostics_Debug", "Win32_Storage_FileSystem", "Win32_System_Registry", "Win32_System_IO", "Win32_System_Ioctl"] } | ||
| windows = { version = "0.61.0", features = ["std", "Win32_Globalization", "Win32_System_LibraryLoader", "Win32_System_Console", "Win32_System_Kernel", "Win32_System_Diagnostics_Debug", "Win32_Storage_FileSystem", "Win32_Security", "Win32_System_Registry", "Win32_System_IO", "Win32_System_Ioctl"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zanieb I also don't think we need to change the windows version as part of this PR either. Everything should work fine if we keep the previous windows versions intact. That will minimize the changes to the lock file to be just isolated to rcgen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After f7e549d windows version is now the same as main. It also removed small unrelated changes to windows_exception in this PR as a result of the upgrade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.9.8` -> `0.9.9` | 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.9.9`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#099) [Compare Source](astral-sh/uv@0.9.8...0.9.9) Released on 2025-11-12. ##### Deprecations - Deprecate use of `--project` in `uv init` ([#​16674](astral-sh/uv#16674)) ##### Enhancements - Add iOS support to Python interpreter discovery ([#​16686](astral-sh/uv#16686)) - Reject ambiguously parsed URLs ([#​16622](astral-sh/uv#16622)) - Allow explicit values in `uv version --bump` ([#​16555](astral-sh/uv#16555)) - Warn on use of managed pre-release Python versions when a stable version is available ([#​16619](astral-sh/uv#16619)) - Allow signing trampolines on Windows by using `.rcdata` to store metadata ([#​15068](astral-sh/uv#15068)) - Add `--only-emit-workspace` and similar variants to `uv export` ([#​16681](astral-sh/uv#16681)) ##### Preview features - Add `uv workspace dir` command ([#​16678](astral-sh/uv#16678)) - Add `uv workspace metadata` command ([#​16516](astral-sh/uv#16516)) ##### Configuration - Add `UV_NO_DEFAULT_GROUPS` environment variable ([#​16645](astral-sh/uv#16645)) ##### Bug fixes - Remove `torch-model-archiver` and `torch-tb-profiler` from PyTorch backend ([#​16655](astral-sh/uv#16655)) - Fix Pixi environment detection ([#​16585](astral-sh/uv#16585)) ##### Documentation - Fix `CMD` path in FastAPI Dockerfile ([#​16701](astral-sh/uv#16701)) </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:eyJjcmVhdGVkSW5WZXIiOiI0MS4xNzMuMSIsInVwZGF0ZWRJblZlciI6IjQxLjE3My4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
.rsrcis the idiomatic way of storing metadata and non-code resources in PEbinaries. This should make the resulting binaries more robust as they are no longer
dependent on the exact location of a certain magic number.
Addresses: #15022
Test Plan
Existing integration test for
uv-trampoline-builder+ addition to ensure robustnessto code signing.