Skip to content

EXE installers: inherit permissions from $INSTDIR for all-users installations #991

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

Merged
merged 15 commits into from
Jun 24, 2025

Conversation

marcoesters
Copy link
Contributor

Description

For all-user installations, the permissions of the installation directory are changed manually to remove write access for authenticated, domain, and built-in users, and to add read and execute rights for domain and built-in users. This assumes that permissions applied to the installation directories are inherited by all files and subdirectories.

If a file has the SDDL_PROTECTED flag set (see Security Descriptors), these permissions are no inherited and thus not changed. The result is that users who are not part of the Administrators group(s) cannot use these files.

One example where this flag is set is the exe entry points created by _conda.exe: conda-standalone creates a hard link from the entry point in the extracted temporary directory to the installation directory. Windows automatically sets the SDDL_PROTECTED flag in that case and the permissions are not changed at the end of the installation.

The solution is to enable inheritance for all files and directories inside $INSTDIR while still protecting $INSTDIR itself. This is done by a call to icacls because it is much faster than calling AccessControl::EnableFileInheritance on every file (local tests with only jupyter show a difference of 30 seconds).

The added tests fail for the Miniforge example without this addition, as expected.

Closes #828.

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@github-project-automation github-project-automation bot moved this to 🆕 New in 🔎 Review May 22, 2025
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label May 22, 2025
@marcoesters
Copy link
Contributor Author

I had to add the /C flag to continue on error. When I logged into the runner, icacls wasn't able to change the inheritance of one particular file in one of the Python packages, probably because of a broken hard link. So, changing those permissions is essentially a "best effort" for now.

@marcoesters marcoesters marked this pull request as ready for review May 27, 2025 18:58
@marcoesters marcoesters requested a review from a team as a code owner May 27, 2025 18:58
@@ -929,6 +960,144 @@ def test_virtual_specs_override(tmp_path, request, monkeypatch):
)


@pytest.mark.skipif(not ON_CI, reason="CI only")
@pytest.mark.skipif(not sys.platform.startswith("win"), reason="Windows only")
def test_allusers_exe(tmp_path, request):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a super useful test that nonetheless might feel foreign when compared to the other tests in this module. Can you add a docstring explaining what we are testing here and why? It's obvious now in the context of this PR, but it might not be in the future when this all of a sudden fails in the future for whatever reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good call. I added a detailed doc string to the function that also talks about the limitations of this test. It only tests that permissions are correctly applied. A more complete test would go through all permissions attached to each file and ensures that write permissions are unset for users who are not part of the local administrator group. However, this isn't part of the installer code either.

jaimergp
jaimergp previously approved these changes Jun 13, 2025
@github-project-automation github-project-automation bot moved this from 🆕 New to ✅ Approved in 🔎 Review Jun 13, 2025
@marcoesters marcoesters requested a review from jaimergp June 17, 2025 20:05
@moorepants
Copy link

I built our installer with this branch if you'd like to test: https://github.com/moorepants/anaconda-dee-config/actions/runs/15697333735

@marcoesters marcoesters merged commit db1ea11 into main Jun 24, 2025
72 of 87 checks passed
@marcoesters marcoesters deleted the all-users-permissions-win branch June 24, 2025 14:12
@github-project-automation github-project-automation bot moved this from ✅ Approved to 🏁 Done in 🔎 Review Jun 24, 2025
@moorepants
Copy link

🎉 Thanks for this fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Status: 🏁 Done
Development

Successfully merging this pull request may close these issues.

Struggling with permissions issues for the AllUsers settings on Windows
4 participants