-
Notifications
You must be signed in to change notification settings - Fork 202
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
Rework default list of capabilities to minimal #1240
Conversation
@vrothberg PTAL |
Fixes: #1234 |
@TomSweeneyRedHat 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.
Just nits but LGTM 👍
Thank you, Dan!
For a couple of years now we have been shipping containers.conf settings to use only 10 capabities. Docker uses 14, with the difference being CAP_AUDIT_ALLOW, CAP_NET_RAW, CAP_MKNOD, CAP_CHROOT. This allows us to run with a tighter security then Docker. This PR makes the change to default config, so that if no containers.conf overrides are present, we will run with the tighter security. Currently most distros that ship the containers.conf already run with this setting, but this makes it formal, and changes the man page to match the defaults. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, rhatdan 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 |
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
One problem with this change, is we can not let the containers.conf change out until we release updated versions of Podman/Buildah |
Are you thinking about CI? |
No sometimes we release containers-common rpm package with updated content, prior to a new release of podman or on a bugfix release of podman. |
That's a fair concern but I think we can avoid it by being careful: Unless there's a bug, we shouldn't bump c/common in the package. If there's a bug, we should backport it to exactly the version c/common that is being used by the containers-common package. |
Yup usually there is only docs changes, but in this case there would be security changes. |
Note, by default container engines using containers.conf, run with less | ||
capabilities than Docker. Docker runs additionally with "AUDIT_WRITE", "MKNOD", | ||
"NET_RAW", "CHROOT". If you need to add one of these capabilities for a | ||
particular container, you can use the --cap-add option or edit your system's containers.conf. |
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.
Late to the party, but pronouns aren't hip in man pages
Note, by default, container engines using containers.conf run with fewer
capabilities than Docker. Docker runs additionally with "AUDIT_WRITE", "MKNOD",
"NET_RAW" and "CHROOT" capabilities. If one of these capabilities is required for a particular container, the --cap-add option can be used, or the system's containers.conf can be edited.
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.
We need to go through all man pages and remove you, and then add a test to look for you in a man page PR and block it.
A reminder that buildah's chroot isolation, which we configure in the default buildah image, depends on chroot. |
For a couple of years now we have been shipping containers.conf settings to use only 10 capabities. Docker uses 14, with the difference being CAP_AUDIT_ALLOW, CAP_NET_RAW, CAP_MKNOD, CAP_CHROOT.
This allows us to run with a tighter security then Docker.
This PR makes the change to default config, so that if no containers.conf overrides are present, we will run with the tighter security. Currently most distros that ship the containers.conf already run with this setting, but this makes it formal, and changes the man page to match the defaults.
Signed-off-by: Daniel J Walsh dwalsh@redhat.com