GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Fix permission errors for mounts in rootless docker by matthewhughes934 · Pull Request #3446 · pre-commit/pre-commit

Conversation

matthewhughes934 commented Apr 13, 2025 •

edited by asottile

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

By running containers in a rootless docker context as root. This is because user and group IDs are remapped in the user namespaces uses by rootless docker, and it's unlikely that the current user ID will map to the same ID under this remap (see docs[1] for some more details). Specifically, it means ownership of mounted volumes will not be for the current user and trying to write can result in permission errors.

This change borrows heavily from an existing PR[2].

The output format of docker system info I don't think is documented/guaranteed anywhere, but it should corresponding to the format of a /info API request to Docker[3]

The added test hopes to avoid regressions in this behaviour, but since tests aren't run in a rootless docker context on the PR checks (and I couldn't find an easy way to make it the case) there's still a risk of regressions sneaking in.

Link: https://docs.docker.com/engine/security/rootless/ [1]
Link: #1484 [2]
Link: https://docs.docker.com/reference/api/engine/version/v1.48/#tag/System/operation/SystemAuth [3]

resolves #1243

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ypid-work reacted with thumbs up emoji

All reactions

👍 1 reaction

matthewhughes934 force-pushed the fix-docker-rootless-permission-mounts branch 2 times, most recently from 26b203c to 399f2f0 Compare

April 13, 2025 19:03

matthewhughes934 commented

Apr 13, 2025

View reviewed changes

Comment thread

tests/languages/docker_test.py Outdated

		),
		)
		def test_docker_user_rootless_docker(info_ret, expect_root):
		docker._is_rootless_docker.cache_clear()

matthewhughes934 Apr 13, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

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.

🤔 maybe we should clear this before and after the test run: otherwise we risk polluting following tests with whatever value we last injected (and not the real value it reads from docker)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

asottile Apr 19, 2025

Copy link

Copy Markdown

Member

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.

usually a better approach is something like this:

@pytest.fixture(autouse=True)
def _avoid_cache():
    with mock.patch.object(docker, '_is_rootless_docker', docker._is_rootless_docker.__wrapped__):
        yield

this bypasses the cache for the duration of the test

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

matthewhughes934 marked this pull request as ready for review

April 13, 2025 19:10

asottile reviewed

Apr 19, 2025

View reviewed changes

Comment thread

pre_commit/languages/docker.py Outdated

Comment on lines +107 to +115

		retcode, out, _ = cmd_output_b(
		'docker', 'system', 'info', '--format', '{{ json .SecurityOptions }}',
		)
		# some failures are to be expected, e.g. for 'podman' aliased as 'docker'
		if retcode != 0:
		return False

		info = json.loads(out)
		return any(opt == 'name=rootless' for opt in info)

asottile Apr 19, 2025

Copy link

Copy Markdown

Member

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.

this doesn't seem to work for podman :(

$ readlink -f $(which docker)
/usr/bin/podman
$ docker system  info --format '{{ json .SecurityOptions }}'
Error: template: info:1:8: executing "info" at <.SecurityOptions>: can't evaluate field SecurityOptions in type *define.Info

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

matthewhughes934 Apr 20, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

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.

this doesn't seem to work for podman :(

$ readlink -f $(which docker)
/usr/bin/podman
$ docker system  info --format '{{ json .SecurityOptions }}'
Error: template: info:1:8: executing "info" at <.SecurityOptions>: can't evaluate field SecurityOptions in type *define.Info

It looks like the invocation we need for podman is: podman system info --format '{{ json .Host.Security.Rootless }}', I guess we could either:

Run the docker command and on failure try the podman one, or
Run a separate command to figure out if docker invokes docker or podman and then run the corresponding command (ignoring any other container engines for now)

Trying to find a suitable command:

Naively I just want to run docker --version and search the output for "podman"
Or use the entire JSON output from docker system info
- If .host exists: check ["host"]["Security"]["Rootless"], else
- Check in ["SecurityOptions"]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

matthewhughes934 Apr 20, 2025

Copy link

Copy Markdown

Contributor Author

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.

Parsing the entire info response seemed the most robust: 9bc412d

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

tests/languages/docker_test.py Outdated

Comment on lines +78 to +81

		if expect_root:
		assert docker.get_docker_user() == ()
		else:
		assert docker.get_docker_user() != ()

asottile Apr 19, 2025

Copy link

Copy Markdown

Member

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.

don't write logic in tests -- these are two separate disparate behaviours and should be tested separately

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

matthewhughes934 reacted with thumbs up emoji

All reactions

👍 1 reaction

matthewhughes934 Apr 20, 2025

Copy link

Copy Markdown

Contributor Author

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.

don't write logic in tests -- these are two separate disparate behaviours and should be tested separately

909c165 also included your cache suggestion from above

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

asottile force-pushed the fix-docker-rootless-permission-mounts branch from 3b3d042 to 026db3a Compare

May 23, 2025 20:54

asottile approved these changes

May 23, 2025

View reviewed changes

asottile left a comment

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

invisiblepancake reacted with thumbs up emoji

All reactions

👍 1 reaction


          Fix permission errors for mounts in rootless docker

466f6c4

By running containers in a rootless docker context as root. This is
because user and group IDs are remapped in the user namespaces uses by
rootless docker, and it's unlikely that the current user ID will map to
the same ID under this remap (see docs[1] for some more details).
Specifically, it means ownership of mounted volumes will not be for the
current user and trying to write can result in permission errors.

This change borrows heavily from an existing PR[2].

The output format of `docker system info` I don't think is
documented/guaranteed anywhere, but it should corresponding to the
format of a `/info` API request to Docker[3]

The added test _hopes_ to avoid regressions in this behaviour, but since
tests aren't run in a rootless docker context on the PR checks (and I
couldn't find an easy way to make it the case) there's still a risk of
regressions sneaking in.

Link: https://docs.docker.com/engine/security/rootless/ [1]
Link: pre-commit#1484 [2]
Link: https://docs.docker.com/reference/api/engine/version/v1.48/#tag/System/operation/SystemAuth [3]
Co-authored-by: Kurt von Laven <[email protected]>
Co-authored-by: Fabrice Flore-Thébault <[email protected]>

asottile force-pushed the fix-docker-rootless-permission-mounts branch from 026db3a to 466f6c4 Compare

May 23, 2025 21:01

asottile enabled auto-merge

May 23, 2025 21:01

asottile merged commit 8a4af02 into pre-commit:main

May 23, 2025

tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request

Aug 12, 2025


          build(deps): update pre-commit to v4.3.0

2caedcd

This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [pre-commit](https://github.com/pre-commit/pre-commit) | minor | `4.2.0` -> `4.3.0` |

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>pre-commit/pre-commit (pre-commit)</summary>

### [`v4.3.0`](https://github.com/pre-commit/pre-commit/blob/HEAD/CHANGELOG.md#430---2025-08-09)

[Compare Source](pre-commit/pre-commit@v4.2.0...v4.3.0)

\==================

##### Features

- `language: docker` / `language: docker_image`: detect rootless docker.
  - [#&#8203;3446](pre-commit/pre-commit#3446) MR by [@&#8203;matthewhughes934](https://github.com/matthewhughes934).
  - [#&#8203;1243](pre-commit/pre-commit#1243) issue by [@&#8203;dkolepp](https://github.com/dkolepp).
- `language: julia`: avoid `startup.jl` when executing hooks.
  - [#&#8203;3496](pre-commit/pre-commit#3496) MR by [@&#8203;ericphanson](https://github.com/ericphanson).
- `language: dart`: support latest dart versions which require a higher sdk
  lower bound.
  - [#&#8203;3507](pre-commit/pre-commit#3507) MR by [@&#8203;bc-lee](https://github.com/bc-lee).

</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:eyJjcmVhdGVkSW5WZXIiOiI0MS41OC4yIiwidXBkYXRlZEluVmVyIjoiNDEuNTguMiIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90Il19-->

Reviewers

asottile

asottile approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Permission issue with rootless containers

Uh oh!

There was an error while loading. Please reload this page.

2 participants

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

matthewhughes934 commented Apr 13, 2025 • edited by asottile Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthewhughes934 Apr 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asottile Apr 19, 2025

Choose a reason for hiding this comment

Uh oh!

asottile Apr 19, 2025

Choose a reason for hiding this comment

Uh oh!

matthewhughes934 Apr 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewhughes934 Apr 20, 2025

Choose a reason for hiding this comment

Uh oh!

asottile Apr 19, 2025

Choose a reason for hiding this comment

Uh oh!

matthewhughes934 Apr 20, 2025

Choose a reason for hiding this comment

Uh oh!

asottile left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

matthewhughes934 commented Apr 13, 2025 •

edited by asottile

Loading

Uh oh!

There was an error while loading. Please reload this page.

matthewhughes934 Apr 13, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

matthewhughes934 Apr 20, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.