-
Notifications
You must be signed in to change notification settings - Fork 200
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
Add support for enabling/disabling kernel keyring in engines #354
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@containers/podman-maintainers PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I
@@ -179,6 +179,7 @@ func DefaultConfig() (*Config, error) { | |||
DNSServers: []string{}, | |||
DNSOptions: []string{}, | |||
DNSSearches: []string{}, | |||
EnableKeyring: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want this to be set unconditionally. This code will likely be greeted on older kernels, where keyrings are not namespaced.
I think we need to run a kernel version check for the built-in default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate kernel version checks, becasue they usually are wrong on systems like RHEL that back port fixes. We have been defaulting to on for a long time now without an issue. This has only been asked for inside of a locked down container. I don't believe this is a security issue, and has nothing to do with whether the container can use the kernel keyring. This is the podman/buildah command attempting to create a kernel keyring though conmon.
When people run podman build within a container from Docker, the seccomp rules block this and cause podman to not run, we have been requested to set this field in the podman container to allow it to run with seccomp enabled.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan, saschagrunert, vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
# a kernel keyring for use within the container. | ||
# keyring = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiny nit: consistent formatting
otherwise, LGTM once other stuff is addressed
# a kernel keyring for use within the container. | |
# keyring = true | |
# a kernel keyring for use within the container. | |
# | |
# keyring = true |
…rde-1.0.140 build(deps): bump serde from 1.0.139 to 1.0.140
Signed-off-by: Daniel J Walsh dwalsh@redhat.com