Conversation
isc-dchui
requested review from
isc-cborbonm,
isc-egabhart,
isc-eneil,
isc-jili,
isc-jlechtne,
isc-kiyer,
isc-pbarton and
isc-tleavitt
as code owners
September 14, 2026 19:40
isc-kiyer
requested changes
Sep 15, 2026
isc-kiyer
left a comment
Collaborator
There was a problem hiding this comment.
@isc-dchui Looks good! Few minor comments. Question: does github actions support any windows runners so we can have some basic tests run on windows to catch such issues in future?
Collaborator
Author
|
@isc-kiyer Addressed the comments! As for Windows runners, my understanding is that yes they exist, but the problem is that there's no Windows IRIS container, so we'll need to install an IRIS kit onto the runner, set up a license key (secretly), and then install IPM. So not very straightforward unfortunately. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #1137.
On Windows, the Python packages IPM installs under
<install>\mgr\pythoncan end up unreadable tonon-elevated processes. The ORAS code path is the only part of IPM that imports one of those packages,
so that is where it surfaces:
The permissions are not oras-specific. Every wheel IPM ships is affected, so application code
importing
requests,urllib3,jsonschemaor the rest from that directory fails the same way.Cause, and why Windows
IPM installs its bundled wheels with
pip install <wheel> -t <mgr>/python(PythonWheel.cls), and a module's
requirements.txtthe same way (Base.cls,
InstallOrDownloadPythonRequirements).pip's
--targethandling stages the install in a temporary directory and then moves the result intothe target. Three Windows behaviours combine from there:
copies, so the staged tree's ACL travels into
mgr\pythonunchanged: inherited entries becomeexplicit and inheritance is switched off. A copy behaves the opposite way, letting the destination's
permissions apply. Unix has no equivalent, since a file's mode is set by pip rather than derived
from the directory it lands in.
the IRIS service account rather than the launching user, and IPM's wheel install is JOB'd
(
InstallORASIfMissingusesjob ..InitializeAndSignal()to pickup the new
%IPM.Storage.Moduledefinition). pip'sgettempdir()therefore resolves toC:\WINDOWS\TEMP, which grantsUsersno read access at all. In a container every IRIS processruns as one account, which is why this never appears in CI.
SYSTEM,BUILTIN\AdministratorsandOWNER RIGHTSfull control and nothing else. A non-elevated tokencarries its Administrators SID as deny-only, so the same user on the same machine has access from
an elevated shell and none otherwise.
The fix
Stage, then copy. New shared helper
%IPM.Utils.Module.StagedPipInstall()pointspip install -tat atemporary directory and copies the result into
mgr/pythonwith%IPM.Utils.File.CopyDir(). Neithercopy path carries the source security descriptor (
ROBOCOPY /Eon Windows without/SEC,%File.CopyDiron Unix), so every new file and directory inherits frommgr\pythonand thedestination's permissions become authoritative regardless of where pip staged or which account ran it.
No ACL calls and no platform branch. Both pip call sites go through it, and the staging directory is
removed on the failure path as well.
Replace same-named entries. Copying alone does not repair an instance that is already affected:
robocopy leaves an existing directory's security descriptor untouched, a file it overwrites in place
keeps its old ACL, and a file whose size and timestamp match the source is skipped entirely. So for
each top-level entry pip staged, the entry of the same name under
mgr/pythonis deleted first. Thatmakes reinstalling IPM the repair, with no permission change and no
icacls. The delete happens onlyafter pip succeeds, so a failed pip cannot leave
mgr/pythonemptier than it started. A failed copystill can, since the delete precedes it, so the copy reports that the package it was replacing may be
left partially installed.
bin,Scriptsand__pycache__are merged rather than replaced. pip's--targethandling makes themtop-level entries by construction, so every wheel with a console script writes into the same
bin, andreplacing it would delete another distribution's scripts. Neither needs repairing anyway.
A module's
requirements.txtkeeps the installed package instead, dropping the staged entry and warning.That is what pip does without
--upgrade, so the behaviour matchesmain; replacing would let a module'spin change the
requestsorurllib3that IPM's own ORAS client and everything else in the instancegets. Which version should win in a shared
mgr/pythonis in issue #1126.Report it usefully.
GetClient()mapped only<OBJECT DISPATCH>to a real message and rethrew everything else, so the user saw a barePermissionError. It now recognizes a permission failure and reports "Reinstall IPM to repair theinstallation", with the original error embedded so the failing path stays visible. The match is on the
exception details rather than
e.Name, because the same underlying error has been seen as both<SYNTAX>and<PYTHON EXCEPTION>, and it requires the details to namemgr/pythonso a permissionfailure elsewhere on
sys.pathis not blamed on IPM.ORASInstalled()used a bare
except:, reporting a permission failure as "oras not installed" and sending IPM off tore-run a wheel install that cannot succeed; it now catches only
ImportError, and the caller warnsinstead of retrying.
Reviewer notes
pDeleteFirststays 0 on theCopyDir. Passing 1 would delete all ofmgr/python, including theother ten wheels and IPM's own
modules/python/files.package), installing the second removes the first's contribution. None of IPM's own wheels share a
top-level name once the shared pip entries are excluded, but other modules' wheels use this resource
processor too. Not a regression:
pip install -talready refused to write into an existing target directory and installed only the.dist-info, losing the second wheel's files instead and claiming the package was installed.requirements.txtinstall no longer prints pip's progress, since its output now goes tothe same sink stream the wheel path uses. A failed install still prints it.
in a module's flags would otherwise produce, and a stray
zwriteis gone from a test helper.Initialize, whichruns as whichever account performs the reinstall: the user's own account for a terminal
IPM.Installer.setup(), or the IRIS service account forzpm install zpmand the autoinstall path.The bad DACL grants SYSTEM and Administrators, so a default
LocalSystemservice has the rights andthe reported case is covered. A non-elevated user reinstalling from their own session is not, and is
back to their machine administrator or
icacls /inheritance:e /T.Rejected alternatives
icacls /inheritance:eon what pip just created, or the same from theJOB'd process, which was verified to work with no elevation. Rejected: it widens permissions on a
shared directory inside the IRIS install, unwanted at a site that hardened
mgr\pythondeliberately.Staging and copying reaches the same end state without touching an ACL.
TMP/TEMPfor the child process, removing the bad sourcedescriptor at its origin. Rejected:
$zf(-100)has no environment option, so this needsos.environmutation in the calling process or a wrapper script, and it covers only pip rather than anything else
that writes into the staging area.
sys.pathshadow copy of the wheels somewhere the user can write. Verified to work,rejected: installing IPM's Python dependencies outside
mgr\pythonis not acceptable, andsys.path[0]shadowing is process-global, so a pinned copy can silently downgrade therequestsorurllib3that unrelated application code gets.temporary directory for that one path while leaving every other write into
mgr\pythonat the mercyof whichever account performed it.
%Net.HttpRequestagainst the OCI distribution API. Muchlarger, and it only hides this bug, since
publishstill importsorasand the other packages stayunreadable for everyone else.
installed, including
RECORD/dist-infoand entry points.Testing
New integration test
TestWheelInstallsIntoSharedTopLevelDirectoryinProcessPythonWheel.cls, with two
fixture wheels sharing the
ipm_shared_nstop-level directory. It asserts the second wheel's modulereaches
mgr/python, whichpip install -talone would not have delivered. What becomes of the firstwheel's module in that directory is deliberately not asserted, since that is the part #1126 decides.
The
requirements.txtpath has no new test.lune-wheel-reqs-offline,lune-no-wheel-reqs-offline,lune-wheel-reqs-onlineandpackage-with-python-depsinstall through it and assert the packages areimportable, which covers the staging and copy. The branch that keeps an already-installed package and
warns is left untested for the same reason: it holds
main's behaviour until #1126 settles what itshould be.
Both branches of
ResolveStagedEntrieswere exercised directly against a staging and target pair in thecontainer, as was the tightened
PermissionErrormatch.Every claim above about move, copy and robocopy behaviour was established by a spike run directly with
icacls/robocopyon Windows, and through%IPM.Utils.File.CopyDirin the container for the Unixbranch. What
pip install -tdoes with an entry the target already has, with and without--upgrade,is from its
_handle_target_dirand was reproduced with two versions ofrequests.Manual verification on Windows: on an affected instance,
##class(%SYS.Python).Import("oras")failed withPermissionErrorfrom a non-elevated session beforethe fix, and succeeds after reinstalling IPM from this branch.
Checklist
mainbranch rebased or merged.zpm test -only) and integration tests (zpm verify -only) pass.