Use unified configfile parser for policy.json by jankaluza · Pull Request #711 · containers/container-libs

jankaluza · 2026-03-23T12:23:58Z

This PR switches policy.json loading to the unified storage/pkg/configfile parser instead of maintaining separate lookup logic in image/signature.

The SignaturePolicyPath support is preserved, so explicit callers continue to work as before. The policy loader also now uses CONTAINERS_POLICY_JSON for environment-based override. The CONTAINERS_POLICY_JSON_OVERRIDE remains ignored for policy.json because drop-in loading is disabled for that this configuration file.

It also extends the configfile API to return the paths it searched. This is showed to users when there is no policy.json found.

Fixes: #207
Fixes: #202

common/libimage/manifests/manifests_test.go

image/signature/policy_config.go

jankaluza · 2026-03-23T12:50:59Z

@Luap99 , Hi, could you please check if this PR makes a sense? I also added two questions for parts of code I'm not sure about...

image/types/types.go

common/pkg/config/config.go

mtrmac

Just an extremely brief skim.

image/signature/policy_config.go

image/signature/policy_config_test.go

common/pkg/config/config.go

common/libimage/manifests/manifests_test.go

common/libimage/copier.go

image/signature/policy_config.go

Luap99

it would be good to if you can squash the re-add SignaturePolicyPath commits, i.e when reviewing this it makes no sense to look at something getting remove to be added again. It also break git bisect most likely when vendoring from another repo.

image/signature/policy_config.go

Luap99 · 2026-03-24T12:11:47Z

storage/pkg/configfile/parse.go

 		}

-		if conf.EnvironmentName != "" {
+		if conf.EnvironmentName != "" && !conf.DoNotLoadDropInFiles {


please add a new test case here for that behavior

And document that behavior! It’s rather unexpected based on plain reading of the current option (although there is some logic to it).

Probably not, but maybe we’d want s/DoNotLoadDropInFiles/DropInFilesUnsupported/ ?!

I am in favour of naming it DropInFilesUnsupported, though then I guess we would want to rename DoNotLoadMainFiles as well to MainFilesUnsupported? I suppose we could rename that outside of the scope of this PR if wanted.

I kind of think of them as different dimensions: “do not load a file at $this path” is more of a directive vs. “the consumer cannot process multiple drop-ins regardless of names” is more of a capability declaration, but that’s a very weak reasoning.

I’m also quite fine with leaving the name as is.

I would remove that in another PR if possible to not add more unrelated changes to this one if that's fine for you. I added the documentation part, that's good point :-).

mtrmac

Thanks!

image/signature/policy_config.go

image/signature/policy_paths_common.go

mtrmac · 2026-03-24T16:58:17Z

storage/pkg/configfile/parse.go

 		}

-		if conf.EnvironmentName != "" {
+		if conf.EnvironmentName != "" && !conf.DoNotLoadDropInFiles {


And document that behavior! It’s rather unexpected based on plain reading of the current option (although there is some logic to it).

Probably not, but maybe we’d want s/DoNotLoadDropInFiles/DropInFilesUnsupported/ ?!

storage/pkg/configfile/parse.go

image/signature/policy_config.go

image/signature/policy_config_test.go

image/signature/policy_config.go

storage/pkg/configfile/parse.go

mtrmac

I didn’t carefully read the added tests or think about coverage, I’m hoping for a simpler version first.

There are also a bunch of earlier outstanding review comments.

storage/pkg/configfile/parse_test.go

mtrmac · 2026-03-30T17:57:29Z

storage/pkg/configfile/parse.go

 // Expected ENOENT errors are already ignored in this function and must not be handled again by callers.
 // The given File options must not be nil and populated with valid options.
+//
+// The _OVERRIDE environment is ignored if DoNotLoadDropInFiles is set.


This does not talk about _OVERRIDE so this is a bit of an unexpected place to suddenly go into this detail.

OTOH the EnvironmentName field does not document _OVERRIDE, and I think it should. That might also be a place to add that note.

This comment is still open, documenting this behavior on the function comment feels odd and duplicated. Having it as part of the struct field comments seem enough.

image/signature/policy_config_test.go

image/signature/policy_config.go

packit-as-a-service · 2026-03-31T09:47:06Z

Packit jobs failed. @containers/packit-build please check.

Luap99

(commit 3 should be squashed into commit 1 and that one needs WIP stripped)

image/signature/policy_config.go

storage/pkg/configfile/parse.go

Luap99 · 2026-04-08T15:39:57Z

@mtrmac are you fine with the new approach suggested by me?

@jankaluza Generally speaking I would like to see a "clean" history, i.e. not add ReadWithPaths just to remove it again as this makes reviewing harder but lets with for @mtrmac feedback first

jankaluza · 2026-04-09T04:38:57Z

@Luap99 , yeah, I will squash the commits before merge.

mtrmac

Sure, the ErrorIfNotFound approach is (given the signature-side test) fine. We can rework storage/configfile as needed. ACK overall.

Some outstanding nits:

#711 (comment) (that would not have test coverage, that’s fine)
#711 (comment)
#711 (comment)
#711 (comment)

please squash, per @Luap99 .

image/docs/containers-policy.json.5.md

image/signature/policy_config.go

Replace custom path resolution with configfile.Read and remove legacy SignaturePolicyPath defaults and helpers. Fixes: containers#207 Fixes: containers#202 Signed-off-by: Jan Kaluza <jkaluza@redhat.com>

This commit introduces `File.ErrorIfNotFound`. If it is true, the Read returns an error which contains all the paths it tried when searching for a config file. It uses this in DefaultPolicy() to include the searched paths in the error message when no policy file is found. The useless policy_paths_*.go files are removed. Signed-off-by: Jan Kaluza <jkaluza@redhat.com>

Luap99 · 2026-04-13T10:45:49Z

storage/pkg/configfile/parse.go

 // Expected ENOENT errors are already ignored in this function and must not be handled again by callers.
 // The given File options must not be nil and populated with valid options.
+//
+// The _OVERRIDE environment is ignored if DoNotLoadDropInFiles is set.


This comment is still open, documenting this behavior on the function comment feels odd and duplicated. Having it as part of the struct field comments seem enough.

image/docs/containers-policy.json.5.md

Return an error if multiple policy files are encountered, which should not happen with configfile.Read. Update tests to use full Policy comparisons and cover nil/empty SystemContext cases. Clarify CONTAINERS_POLICY_JSON and *_OVERRIDE behavior in docs. Signed-off-by: Jan Kaluza <jkaluza@redhat.com>

jankaluza · 2026-04-13T11:46:25Z

@Luap99 , I fixed the issues you've pointed out.

Luap99

Thanks, LGTM overall though I have not checked the policy.json tests in detail. I defer that to @mtrmac.

Did you run this trough podman and buildah CI to make sure they are no problmes and we can vendor this cleanly there?

jankaluza · 2026-04-13T16:22:05Z

I have not checked buildah. I'll do that first thing tomorrow morning.

Introduce ErrConfigFileNotFound error. This can be used by API caller to detect the File not found error and handle this situation. Signed-off-by: Jan Kaluza <jkaluza@redhat.com>

jankaluza · 2026-04-14T10:46:17Z

@Luap99, I've added one more commit.

jankaluza · 2026-04-14T10:56:03Z

@Luap99, I'm using it here: containers/podman@b767918#diff-8d637d03bead23f5af7c1aabd6167fa0d18309f805bc2f18bdbfe0311de4d631R62

jankaluza · 2026-04-14T11:03:14Z

@Luap99,

buildah PR: WIP: Test policy.json changes from container-libs. buildah#6787. It needs "No tests required" label.
podman PR: WIP: trust: switch policy.json lookup to configfile podman#28508.

mtrmac · 2026-04-14T17:24:07Z

image/signature/policy_config.go

+		}
+		if policy != nil {
+			// Coverage: This should never happen, configfile.Read ensures at most one policy file.
+			return nil, fmt.Errorf("internal error: expected at most one policy file, got another item %q", item.Name) //nolint:revive


What is the nolint directive supposed to silence? (We don’t have revive enabled and) when removing that directive and manually running golangci-lint run --max-issues-per-linter 0 --max-same-issues 0 --enable=revive ./image/signature I see nothing reported on this line.

If it is truly necessary, please use //nolint:revive // a short explanation what this is trying to silence and/or why silencing that is OK

mtrmac · 2026-04-14T17:28:36Z

storage/pkg/configfile/parse.go

 		}
+
+		if conf.ErrorIfNotFound && !foundAny {
+			yield(nil, fmt.Errorf("%w: no %s file found; searched paths: %q", ErrConfigFileNotFound, configFileName, usedPaths))


I’m not asking for any change, just noting the possibility: If you want ErrConfigFileNotFound to be used without using any part of its error text, it is possible to use %.0w with Errorf.

mtrmac · 2026-04-14T17:33:51Z

image/docs/containers-policy.json.5.md

+By default, the policy is read from `$XDG_CONFIG_HOME/containers/policy.json` (or from `$HOME/.config/containers/policy.json` if `$XDG_CONFIG_HOME` is unset), if it exists; otherwise from `/etc/containers/policy.json`;  otherwise from `/usr/share/containers/policy.json`. Applications performing verification may allow using a different policy instead.
+
+If `CONTAINERS_POLICY_JSON` is set, it specifies the policy file to use,
+unless overridden by application-specific configuration.


(Absolutely non-blocking: This works fine.

“the policy is read from $CONTAINERS_POLICY_JSON if set; otherwise from $HOME … ; … /usr/share…. Applications … may allow using a different policy instead” might be a simpler way to say the same thing, representing this as a single linear order, and not repeating the application-specific part.

Also, pre-existing, maybe s/may allow using/may choose to use.)

github-actions bot added common Related to "common" package image Related to "image" package labels Mar 23, 2026

jankaluza commented Mar 23, 2026

View reviewed changes

common/libimage/manifests/manifests_test.go Outdated Show resolved Hide resolved

jankaluza commented Mar 23, 2026

View reviewed changes

image/signature/policy_config.go Show resolved Hide resolved

Luap99 reviewed Mar 23, 2026

View reviewed changes

image/types/types.go Show resolved Hide resolved

Luap99 reviewed Mar 23, 2026

View reviewed changes

common/pkg/config/config.go Show resolved Hide resolved

mtrmac reviewed Mar 23, 2026

View reviewed changes

image/signature/policy_config.go Show resolved Hide resolved

image/signature/policy_config_test.go Show resolved Hide resolved

common/pkg/config/config.go Show resolved Hide resolved

common/libimage/manifests/manifests_test.go Outdated Show resolved Hide resolved

mtrmac reviewed Mar 23, 2026

View reviewed changes

common/libimage/copier.go Show resolved Hide resolved

mtrmac reviewed Mar 23, 2026

View reviewed changes

image/signature/policy_config.go Show resolved Hide resolved

github-actions bot added the storage Related to "storage" package label Mar 24, 2026

jankaluza changed the title ~~WIP: Use unified configfile parser for policy.json~~ Use unified configfile parser for policy.json Mar 24, 2026

Luap99 reviewed Mar 24, 2026

View reviewed changes

This was referenced Mar 24, 2026

Multiple copies of the default signature policy path #226

Open

containers-policy.json: provide default config in /usr/ #207

Open

policy.json overwrite not honouring $XDG_CONFIG_HOME #202

Open

mtrmac reviewed Mar 24, 2026

View reviewed changes

Luap99 linked an issue Mar 24, 2026 that may be closed by this pull request

Provide a customization point for policy.json #49

Open

Luap99 mentioned this pull request Mar 24, 2026

Provide a customization point for policy.json #49

Open

This was linked to issues Mar 25, 2026

policy.json overwrite not honouring $XDG_CONFIG_HOME #202

Open

containers-policy.json: provide default config in /usr/ #207

Open

mtrmac reviewed Mar 26, 2026

View reviewed changes

storage/pkg/configfile/parse.go Outdated Show resolved Hide resolved

storage/pkg/configfile/parse.go Outdated Show resolved Hide resolved

jankaluza force-pushed the policy.json branch 3 times, most recently from 89fd676 to d9da724 Compare March 30, 2026 14:07

mtrmac reviewed Mar 30, 2026

View reviewed changes

jankaluza force-pushed the policy.json branch from 8888d15 to 12ae511 Compare March 31, 2026 09:46

Luap99 requested changes Apr 1, 2026

View reviewed changes

image/signature/policy_config.go Outdated Show resolved Hide resolved

storage/pkg/configfile/parse.go Outdated Show resolved Hide resolved

jankaluza force-pushed the policy.json branch from 30d9835 to ff4e0c0 Compare April 2, 2026 11:06

jankaluza mentioned this pull request Apr 9, 2026

image/docker: use unified configfile for registries.d #753

Open

mtrmac reviewed Apr 9, 2026

View reviewed changes

mtrmac reviewed Apr 10, 2026

View reviewed changes

image/docs/containers-policy.json.5.md Outdated Show resolved Hide resolved

image/signature/policy_config.go Show resolved Hide resolved

policy: use configfile parser for policy.json loading

deaaf71

Replace custom path resolution with configfile.Read and remove legacy SignaturePolicyPath defaults and helpers. Fixes: containers#207 Fixes: containers#202 Signed-off-by: Jan Kaluza <jkaluza@redhat.com>

jankaluza force-pushed the policy.json branch from 690d0d9 to f3af7ae Compare April 13, 2026 10:26

jankaluza force-pushed the policy.json branch from f3af7ae to 8aed54b Compare April 13, 2026 10:36

Luap99 requested changes Apr 13, 2026

View reviewed changes

jankaluza force-pushed the policy.json branch from 8aed54b to 5a468c3 Compare April 13, 2026 11:06

jankaluza force-pushed the policy.json branch from 5a468c3 to c9bb623 Compare April 13, 2026 11:45

Luap99 approved these changes Apr 13, 2026

View reviewed changes

configfile: expose ErrConfigFileNotFound

87ec862

Introduce ErrConfigFileNotFound error. This can be used by API caller to detect the File not found error and handle this situation. Signed-off-by: Jan Kaluza <jkaluza@redhat.com>

mtrmac reviewed Apr 14, 2026

View reviewed changes

Conversation

jankaluza commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jankaluza commented Mar 23, 2026

Uh oh!

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

packit-as-a-service bot commented Mar 31, 2026

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Luap99 commented Apr 8, 2026

Uh oh!

jankaluza commented Apr 9, 2026

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jankaluza commented Mar 23, 2026 •

edited

Loading