fix: judge docker plugin install against allow-registries - #443
Merged
Merged
Conversation
Closes #420. The plugin endpoints were grouped with swarm in unattributablePath and so inherited the BUILD default: allowed unless deny-unattributable-builds is explicitly set, which is off. A plain allow-registries therefore let docker plugin install evil.example.com/rogue through. A plugin gets host device and mount access where an image gets a container, so that was a worse hole than the build one the permissive default was chosen to tolerate. The premise was wrong, not just the default ------------------------------------------- I filed #420 saying these endpoints "carry no attributable image reference". True of swarm. Not true of plugins -- and the comment directly above the regex said so all along: they "fetch from an arbitrary registry named in `remote`". The reference is in the query. So this is not a new policy or a stricter default. It is the rule the operator already wrote, applied to an endpoint that was missed. pluginPullTarget reads `remote` with the same body-shadows-query precedence pullTarget uses -- dockerd's own -- and the answer goes to DenyPull. Consequences, stated rather than discovered later: - allow-registries refuses a plugin from an unlisted registry, with no opt-in. That is the fix. - require-digest applies too, because a plugin IS an image from a registry and DenyPull is one rule set. An unpinned plugin install on a machine with require-digest set will now be refused. - a plugin from a LISTED registry installs exactly as before. - swarm is unchanged: a TaskSpec genuinely cannot be attributed, and refusing beats parsing it and getting it subtly wrong. Fail closed on what we cannot read ---------------------------------- The plugin paths stay in unattributablePath as a FALLBACK. pluginPullTarget runs first and judges the attributable case precisely; a plugin pull that names no remote -- malformed, or a shape not anticipated -- falls through to the old conservative treatment rather than passing unjudged. Verification ------------ Negative control, with the plugin branch disabled: status = 200, want 403 -- allow-registries alone must refuse this reached the engine The permitted case is asserted too, so this cannot have bought safety by banning plugins: a pull from an allowed registry reaches the engine and returns 200, and the gate is checked to have seen the remote rather than an empty string. An existing test asserted the old grouping. It is split rather than deleted: swarm and the no-remote plugin pull keep their DenyBuild assertions, and the attributable plugin pulls move to a test that names what changed.
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 #420.
The plugin endpoints were grouped with swarm in
unattributablePath, so they inherited the build default — allowed unlessdeny-unattributable-buildsis explicitly set, which is off. A plainallow-registriestherefore let this through:A plugin gets host device and mount access where an image gets a container, so that was a worse hole than the build one the permissive default was chosen to tolerate.
The premise was wrong, not just the default
I filed #420 saying these endpoints "carry no attributable image reference". True of swarm. Not true of plugins — and the comment directly above the regex said so all along: they "fetch from an arbitrary registry named in
remote". The reference is right there in the query.So this is not a new policy or a stricter default. It is the rule the operator already wrote, applied to an endpoint that was missed.
pluginPullTargetreadsremotewith the same body-shadows-query precedencepullTargetuses — dockerd's own — and the answer goes toDenyPull.Consequences, stated rather than discovered later:
allow-registriesrefuses a plugin from an unlisted registry, with no opt-in. That is the fix.require-digestapplies too, because a plugin is an image from a registry andDenyPullis one rule set. An unpinned plugin install on a machine withrequire-digestset will now be refused.Fail closed on what we cannot read
The plugin paths stay in
unattributablePath, as a fallback.pluginPullTargetruns first and judges the attributable case precisely; a plugin pull naming noremote— malformed, or a shape I did not anticipate — falls through to the old conservative treatment rather than passing unjudged.Verification
Negative control, plugin branch disabled:
The permitted case is asserted too, so this cannot have bought safety by banning plugins: a pull from an allowed registry reaches the engine and returns 200, and the gate is checked to have seen the remote rather than an empty string.
An existing test asserted the old grouping. It is split rather than deleted: swarm and the no-remote plugin pull keep their
DenyBuildassertions, and the attributable plugin pulls move to a test that names what changed.