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

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...

mtrmac

Just an extremely brief skim.

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.

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

 		}

-		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!

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

 		}

-		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/ ?!

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.

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

 // 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.

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)

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 .

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

 // 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.

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.

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: Update to use shared configfile implementation buildah#6787. It needs "No tests required" label.
podman PR: trust: switch policy.json lookup to configfile podman#28508.

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

 		}
+
+		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

+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.)

jankaluza · 2026-04-15T09:26:36Z

@mtrmac , thanks for the review. I've fixed the blocking lint issue.

mtrmac

Thanks, LGTM for this repo.

Are the unknowns in containers/podman#28508 resolved enough that we don’t need to block here? If so, please go ahead and merge.

Luap99 · 2026-04-15T17:27:48Z

@mheon is working on a clean container-libs@main vendor in podman right now, we should wait for that before merging here as there are test fixes we need to land first.

Also this PR has not been rebased onto main in a while, if you force push it is best to always rebase against upstream main to avoid any unexpected issues post merge (stuff that does not show up as git conflict). I don't think there is anything to worry about here but it is a good habit to rebase.

mtrmac · 2026-04-15T17:36:05Z

Right — I don’t think there is anything either, but I think we did find the old base causing a Buildah test failure.

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>

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>

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-16T04:09:55Z

Rebased.

Luap99 · 2026-04-20T10:36:25Z

Please update your podman/buildah PRs with the code from main to unblock others.

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

Comment thread common/libimage/manifests/manifests_test.go Outdated

jankaluza commented Mar 23, 2026

View reviewed changes

Comment thread image/signature/policy_config.go

Luap99 reviewed Mar 23, 2026

View reviewed changes

Comment thread image/types/types.go

Luap99 reviewed Mar 23, 2026

View reviewed changes

Comment thread common/pkg/config/config.go

mtrmac reviewed Mar 23, 2026

View reviewed changes

Comment thread image/signature/policy_config.go

Comment thread image/signature/policy_config_test.go

Comment thread common/pkg/config/config.go

Comment thread common/libimage/manifests/manifests_test.go Outdated

mtrmac reviewed Mar 23, 2026

View reviewed changes

Comment thread common/libimage/copier.go

mtrmac reviewed Mar 23, 2026

View reviewed changes

Comment thread image/signature/policy_config.go

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

Closed

policy.json overwrite not honouring $XDG_CONFIG_HOME #202

Closed

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

Closed

Luap99 mentioned this pull request Mar 24, 2026

Provide a customization point for policy.json #49

Closed

This was linked to issues Mar 25, 2026

policy.json overwrite not honouring $XDG_CONFIG_HOME #202

Closed

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

Closed

mtrmac reviewed Mar 26, 2026

View reviewed changes

Comment thread storage/pkg/configfile/parse.go Outdated

Comment thread storage/pkg/configfile/parse.go Outdated

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

Comment thread image/signature/policy_config.go Outdated

Comment thread storage/pkg/configfile/parse.go Outdated

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

Comment thread image/docs/containers-policy.json.5.md Outdated

Comment thread image/signature/policy_config.go

jankaluza force-pushed the policy.json branch 2 times, most recently 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 2 times, most recently from 5a468c3 to c9bb623 Compare April 13, 2026 11:45

Luap99 approved these changes Apr 13, 2026

View reviewed changes

mtrmac reviewed Apr 14, 2026

View reviewed changes

jankaluza force-pushed the policy.json branch from 87ec862 to e86bff4 Compare April 15, 2026 09:25

mtrmac reviewed Apr 15, 2026

View reviewed changes

jankaluza added 4 commits April 16, 2026 06:09

policy: use configfile parser for policy.json loading

43a7fd6

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>

configfile: expose ErrConfigFileNotFound

8aa084a

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 force-pushed the policy.json branch from e86bff4 to 8aa084a Compare April 16, 2026 04:09

Luap99 merged commit 618304d into containers:main Apr 20, 2026
37 checks passed

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!

jankaluza commented Apr 9, 2026

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

jankaluza commented Mar 23, 2026 •

edited

Loading