Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

include default policy.json when doing 'make podman-remote' #21855

Closed
benoitf opened this issue Feb 28, 2024 · 16 comments
Closed

include default policy.json when doing 'make podman-remote' #21855

benoitf opened this issue Feb 28, 2024 · 16 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. machine

Comments

@benoitf
Copy link
Contributor

benoitf commented Feb 28, 2024

Feature request description

today, if you clone podman and do make podman-remote

if you launch then the binary you get
Error: no DefaultPolicyJSONPath defined and no local overwrites found: ["stat /Users/benoitf/.config/containers/policy.json: no such file or directory" "stat /etc/containers/policy.json: no such file or directory"]

#21765 introduced a default policy.json and MACHINE_POLICY_JSON_DIR setting

but it's not applied on a default macOS build

Suggest potential solution

Today I need to use

MACHINE_POLICY_JSON_DIR="$(pwd)/pkg/machine/ocipull" make podman-remote

I would hope that by default using the default make command would compile a binary including that default policy.json

Have you considered any alternatives?

A clear and concise description of any alternative solutions or features you've considered.

Additional context

Add any other context or screenshots about the feature request here.

@benoitf benoitf added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 28, 2024
@Luap99
Copy link
Member

Luap99 commented Feb 28, 2024

While I don't disagree that for local development this is the right choice the issue that the podman-remote target is basically used by all other targets to build the binary and we really should not be setting this by default for package builds. Packagers should include the config file and set the option to the path were they will install the file.

@Luap99 Luap99 added the machine label Feb 28, 2024
@rhatdan
Copy link
Member

rhatdan commented Feb 28, 2024

For MAC and Windows, aren't we the packagers?

@Luap99
Copy link
Member

Luap99 commented Feb 28, 2024

We have packages sure, but we are not the only one, i.e. brew on mac but I know there are more on mac and windows.

I mean there is nothing fundamental wrong with doing this by default, the only issue I see is the difference in error message if a packager is build with default options and then users report the "weird" path that was used during compile which makes no sense. But then again it is not like the current error is much better. Either way packagers will need to update their packages, the difference would just be the error message the users see in the end.

@benoitf
Copy link
Contributor Author

benoitf commented Feb 28, 2024

I would also be fine if there is a special target for local development different than make podman-remote

it's more about how to find a convenient way that people should follow to build & run, not dig into all custom parameters when wanting to do a local build to try.

@afbjorklund
Copy link
Contributor

I thought it was a known fact that Podman has some mandatory config, main issue here seems to be that it has spread from Linux to Mac (i.e. Podman-remote)

@rhatdan
Copy link
Member

rhatdan commented Feb 29, 2024

Yes podman machine is now pulling an image from the registry so it needs to follow the rules.

@cgwalters
Copy link
Contributor

  • I think we should include a default compiled into the binary
  • Why are we not looking in ~/.config/containers/ for this?

@Luap99
Copy link
Member

Luap99 commented Mar 4, 2024

Why are we not looking in ~/.config/containers/ for this?

We are, you even see the path shown in the error message.

I think we should include a default compiled into the binary

The goal is to not have a default in the binary, we always require a default policy.json on linux as well.
Once we add signing for these images we will need to add keys to the policy and then embedding them in the binary would make it impossible to rotate keys (if required) since all the old binaries will be broken.
Lastly there is at least some security benifit in erroring out instead of "silently accepting" everything in case a packager/admin forgot to install their policy or put them in the wrong location.

@cgwalters
Copy link
Contributor

The goal is to not have a default in the binary, we always require a default policy.json on linux as well.

Well, yes but it's also conflicting with the general trend to support "empty /etc", so on Linux we should also support a /usr/lib/containers/policy.json e.g. too. But anyways, you are right that the tooling expects it.

Once we add signing for these images we will need to add keys to the policy and then embedding them in the binary would make it impossible to rotate keys (if required) since all the old binaries will be broken.

But if we rotate keys, old clients would be broken regardless of whether it's a config file or in the binary. It's just that having it in a separate config file would make it easier for people to manually fix it, but that's pretty ugly.

@Luap99
Copy link
Member

Luap99 commented Mar 4, 2024

Once we add signing for these images we will need to add keys to the policy and then embedding them in the binary would make it impossible to rotate keys (if required) since all the old binaries will be broken.

But if we rotate keys, old clients would be broken regardless of whether it's a config file or in the binary. It's just that having it in a separate config file would make it easier for people to manually fix it, but that's pretty ugly.

Sure an update needs to happen no matter what. But in general patching a file is much easier. Also consider the case of running something like git bisect on our source tree.

@benoitf
Copy link
Contributor Author

benoitf commented Mar 4, 2024

I think I don't care if I need to rebuild the binary. I just want that the binary compiled for development mode just works out of the box

@Luap99
Copy link
Member

Luap99 commented Mar 5, 2024

I think I don't care if I need to rebuild the binary. I just want that the binary compiled for development mode just works out of the box

Yes this was meant as an answer to why not embed the default in the binary.

As for local development I have no problem with setting the build option like you suggested.

@rhatdan
Copy link
Member

rhatdan commented Mar 6, 2024

We should check /etc/containers/policy.json and then check /usr/share/containers/policy.json , Then distributions could ship the /usr/share and admin can modify in /etc/containers.

That solves Colin's problem and allows us to be secure by default.

@Luap99
Copy link
Member

Luap99 commented Mar 6, 2024

We should check /etc/containers/policy.json and then check /usr/share/containers/policy.json , Then distributions could ship the /usr/share and admin can modify in /etc/containers.

That does nothing for windows/macos or local development. For linux the /usr chnage is tracked in image: containers/image#2157 and not really related to this.

@rhatdan
Copy link
Member

rhatdan commented Mar 6, 2024

Well it solves the problem Colin is complaining about in that he does not want packages dropping files in /etc.

@Luap99
Copy link
Member

Luap99 commented Apr 3, 2024

I guess this can be closed now as it is no longer required: #22014

@Luap99 Luap99 closed this as completed Apr 3, 2024
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Jul 3, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Jul 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/feature Categorizes issue or PR as related to a new feature. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. machine
Projects
None yet
Development

No branches or pull requests

5 participants