Mark /opt/esp-matter as a safe git directory - #29
Merged
Conversation
Closes #27. The checkout belongs to root, because the build created it, while the documented invocation runs the image as the caller's uid — `-u $(id -u):$(id -g)`, which is how build output in a bind-mounted workspace stays owned by the caller. Git then refuses the repository as "dubious ownership", inside the container only, on a tree the image built itself. The ESP-IDF base already marks $IDF_PATH the same way, which is why the two answered differently to one `git -C … rev-parse HEAD`. Two entries, both naming paths rather than `*`: - `/opt/esp-matter`, the superproject; - `/opt/esp-matter/*`, its submodules — git checks ownership per repository, and connectedhomeip brings dozens of nested ones, far too many to list. That suffix is version-dependent: measured as ignored by git 2.43, which this base ships, and honoured by 2.47, where it covers subdirectories but not the parent — hence both lines. Today the second is inert and harmless; it starts working when the base's git catches up, and until then a submodule wants a per-invocation `-c safe.directory=<path>`, which the README shows. Be precise about what the narrow form buys, because the first draft of this comment overstated it: `safe.directory` matches a *path*, not an owner, so a repository bind-mounted over /opt/esp-matter would be trusted too. What it keeps is the ownership check everywhere else in the container, which `*` switches off wholesale. The entries are asserted rather than assumed — the build runs as root, so nothing else here would notice their absence, which is exactly how this gap survived until a consumer running under `-u` hit it. Three assertions, each measured: - both entries are present (`grep -qFx`, fail-closed on empty output); - no blanket `*` is — the base is overridable and pinned by digest, so a fork's base, or a future espressif/idf, could trust everything while this file claims path scoping. Upstream adds `$IDF_PATH` alone today, checked; - the repository actually reads from a uid that does not own it, **through the entrypoint** as well as directly. The entrypoint runs `set -e` and sources both export.sh scripts before the command, so a working command is not the same as a working README recipe. setpriv's own failures are separated from git's first (`setpriv … true`), since without CAP_SETGID `--clear-groups` fails and blaming safe.directory for that sends the next reader to the wrong place. All six paths of that layer were exercised in a plain container: both entries present, each one missing, a blanket `*` added, a broken entrypoint, and setpriv absent. The README section is scoped to what the image actually provisions — git under `-u`. A full firmware build as a non-root uid also wants a writable HOME and cache directories, which this image does not set up and the host image does; saying so beats implying otherwise. Not verified by building the image: `espressif/idf` will not pull on this machine (traffic interception drops the connection well short of its 3.4 GB layer). The behaviour behind every claim above was established in plain containers on the two git versions. The pull request builds all three esp-matter variants on both architectures.
There was a problem hiding this comment.
Pull request overview
This PR addresses Git’s “dubious ownership” protection when the esp-matter image is run as a non-root user (via -u $(id -u):$(id -g)), by marking /opt/esp-matter as a safe Git directory (and documenting/validating that behavior) so consumers can read the image-provisioned repositories without per-invocation workarounds.
Changes:
- Add system-level
safe.directoryentries for/opt/esp-matterand/opt/esp-matter/*in the esp-matter Dockerfile. - Extend the Dockerfile verification layer to assert the safe.directory configuration and validate Git access under an unprivileged uid (including via the entrypoint).
- Document non-root Git usage and limitations (HOME/cache) in
images/esp-matter/README.md, and record the repo-wide rationale inCLAUDE.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| images/esp-matter/Dockerfile | Adds safe.directory configuration and build-time assertions/verification for non-root Git access. |
| images/esp-matter/README.md | Documents how/why Git works under -u, including submodule handling and trust boundary notes. |
| CLAUDE.md | Updates repository guidelines to capture the intended safe.directory conventions across images. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot read `grep -qFx '*' && { …; exit 1; } || true` as self-defeating. It is
not: a brace group is not a subshell, so `exit` leaves the whole RUN and `||`
never sees it — checked directly, the layer exits 1 with the message and the
following command does not run. But the form invites that reading, and the next
reader gets to make the same call, so it is now an `if`. Same behaviour, verified
both ways: passes with no blanket entry, fails the layer with one.
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.
Closes #27.
The ESP-Matter checkout at
/opt/esp-matterbelongs to root — the build createdit — while the documented invocation runs the image as the caller's uid
(
-u $(id -u):$(id -g), which is how build output in a bind-mounted workspacestays owned by the caller). Git refuses that as dubious ownership, inside the
container only, on a tree the image built itself. The ESP-IDF base already marks
$IDF_PATHthis way, which is why the two answered differently to onegit -C … rev-parse HEAD.Two entries, both naming paths
/opt/esp-matter/opt/esp-matter/*Git checks ownership per repository, and connectedhomeip brings dozens of nested
submodules, far too many to list. The suffix is version-dependent, and this was
measured rather than assumed: ignored by git 2.43 (which this base ships) and
honoured by 2.47, where it covers subdirectories but not the parent — hence
both lines. Today the second is inert and harmless; it starts working when the
base's git catches up, and until then a submodule takes a per-invocation
-c safe.directory=<path>, which the README shows.What the narrow form buys, stated precisely — the first draft of this comment
overstated it.
safe.directorymatches a path, not an owner, so a repositorybind-mounted over
/opt/esp-matterwould be trusted too. What it keeps is theownership check everywhere else in the container, which
*switches offwholesale. (The sibling host image does use
*, and for a reason that does notapply here: it exists to run against a checkout mounted at a path it cannot know
— a job
container:gets/__w/<repo>/<repo>.)Asserted, not assumed
The build runs as root, so nothing else in the image would notice these entries
missing — which is exactly how the gap survived until a consumer running under
-uhit it. Three assertions:grep -qFx, fail-closed: emptygit configoutputfails it);
*—BASE_IMAGEis overridable and pinned by digest, so afork's base, or a future
espressif/idf, could trust every repository whilethis file and the README claim path scoping. Upstream adds
$IDF_PATHalonetoday, checked;
through the entrypoint, which runs
set -eand sources bothexport.shscripts before the command. A working command is not the same as a working
README recipe, and only the latter is what a user copies. setpriv's own failures
are separated from git's first (
setpriv … true), since withoutCAP_SETGID--clear-groupsfails and blamingsafe.directoryfor that sends the nextreader to the wrong place.
All six paths of that layer were exercised in a plain container: both entries
present, each one missing, a blanket
*added, a broken entrypoint, and setprivabsent.
Scope of the README section
It covers git under
-u, which is what the image provisions for. A full firmwarebuild as a non-root uid also wants a writable
HOMEand cache directories, whichthis image does not set up and the host image does — saying so beats implying
otherwise.
What was not verified locally
espressif/idfwill not pull on the machine this was written on, so the esp-matterimage was not built here. Every behavioural claim above was established in plain
containers, on both git versions. This pull request is the real check — it
builds all three esp-matter variants on both architectures, and the new
entrypoint assertion is the most likely place to fail if the documented
-urecipe does not actually work.