-
Notifications
You must be signed in to change notification settings - Fork 29
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
Fixes #30465 - Use libexec wrappers for SELinux #116
Conversation
So it turns out #115 is not needed for this. I verified this manually on an EL7 VM (using the beaker tests). |
8a70a4a
to
2eeb921
Compare
-w pulpcore.tasking.worker.PulpWorker -n resource-manager \ | ||
--pid=/var/run/pulpcore-resource-manager/resource-manager.pid \ |
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.
These pid files gave SELinux errors and https://python-rq.org/patterns/systemd/ doesn't suggest it needs pid files.
Probably because /var/run
is not being labeled correctly since it's a symlink to /run
making https://github.com/pulp/pulpcore-selinux/blob/64b99b463186305a371498ce1da8d1f3bb956a20/pulpcore.fc#L12-L16 useless.
templates/pulpcore-api.service.erb
Outdated
@@ -8,9 +8,10 @@ Environment="DJANGO_SETTINGS_MODULE=pulpcore.app.settings" | |||
Environment="PULP_SETTINGS=<%= scope['pulpcore::settings_file'] %>" | |||
Environment="PULP_STATIC_ROOT=<%= scope['pulpcore::pulp_static_root'] %>" | |||
User=<%= scope['pulpcore::user'] %> | |||
PIDFile=/run/pulpcore-api.pid | |||
Group=<%= scope['pulpcore::group'] %> | |||
WorkingDirectory=%t/pulpcore-api |
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 think %t resolves to /run
when it runs as a system service. I'm a bit unsure about this. I've considered setting it to ~
or explicit /var/lib/pulp
. That way systemd also automatically ensures /var/lib/pulp
is present and mounted. I'm not sure why these services should have a different workdir.
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'm unclear from the comment what we want to end up with. If /var/lib/pulp
is the proper working directory then I vote set it explicitly to that.
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 think %t
resolves to /run
for system services. This is to preserve the previous /var/run
(and on all modern systems that's a symlink to /run
. I'm leaning to ~
or /var/lib/pulp
because systemd will ensure it's mounted which can be very useful if it comes from NFS or a separate partition (which is very common), but I might missing something.
Here I'd like some feedback from Pulp upstream since I don't know what their preference is.
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.
On Pulp upstream, we never considered the $PWD
of the pulp processes AFAIK.
I agree with ~
(pulp_installer defaults the pulp home dir to /var/lib/pulp
, but lets users change it), in case we do end up writing anything there, and so that it gets mounted.
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.
(pulp_installer defaults the pulp home dir to
/var/lib/pulp
, but lets users change it)
So does this module, but since it also manages the user, the homedir will be updated and that causes service restarts. Technically I think that Linux doesn't allow changing the homedir if a user is logged in, so you need to manually stop all services before, but after that it should work. Of course there's no data migration either.
When running this, you do see AVCs: # grep type=AVC /var/log/audit/audit.log
type=AVC msg=audit(1595519054.448:2345): avc: denied { search } for pid=28778 comm="gunicorn" name="httpd" dev="vda1" ino=34201106 scontext=system_u:system_r:pulpcore_t:s0 tcontext=system_u:object_r:httpd_config_t:s0 tclass=dir permissive=0
type=AVC msg=audit(1595519054.448:2346): avc: denied { search } for pid=28778 comm="gunicorn" name="httpd" dev="vda1" ino=34201106 scontext=system_u:system_r:pulpcore_t:s0 tcontext=system_u:object_r:httpd_config_t:s0 tclass=dir permissive=0
type=AVC msg=audit(1595519054.723:2347): avc: denied { read } for pid=28822 comm="sh" name="meminfo" dev="proc" ino=4026532028 scontext=system_u:system_r:pulpcore_t:s0 tcontext=system_u:object_r:proc_t:s0 tclass=file permissive=0
type=AVC msg=audit(1595519058.657:2361): avc: denied { search } for pid=28799 comm="rq" name="httpd" dev="vda1" ino=34201106 scontext=system_u:system_r:pulpcore_t:s0 tcontext=system_u:object_r:httpd_config_t:s0 tclass=dir permissive=0
type=AVC msg=audit(1595519058.657:2362): avc: denied { search } for pid=28799 comm="rq" name="httpd" dev="vda1" ino=34201106 scontext=system_u:system_r:pulpcore_t:s0 tcontext=system_u:object_r:httpd_config_t:s0 tclass=dir permissive=0 |
@mikedep333 mind having a look as well? In particular the systemd unit files. |
manifests/service.pp
Outdated
content => "#!/bin/bash\nexec ${bin} \"$@\"\n", | ||
owner => 'root', | ||
group => 'root', | ||
mode => '0755', |
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.
Is worldable executable applicable in our case?
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.
It doesn't hurt since there's nothing secret in there.
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.
Does it imply any other user could run those commands? Starting any rogue services?
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.
Yes, but they can already do that today using /usr/bin/gunicorn
so there is no difference. I have a suggestion to use unix sockets instead of TCP ports but the current SELinux policy in Pulpcore doesn't allow that now (https://pulp.plan.io/issues/7189). By using systemd socket activation and letting systemd create /run/pulpcore-api.sock
(or similar) owned by apache:root
and mode 0600
we can prevent any local user from spinning up a rogue service. It also prevents local users from impersonating admin by connecting directly to the TCP port.
I'm still not clear on why the wrappers are needed versus using the system paths. |
What you execute needs to have some SELinux label. IMHO Pulpcore doesn't own |
This is just my lack of knowledge in the SELinux area. Is it common to create wrappers? It feels generally safe to assume, or rather to enforce that Pulpcore is the only thing using This question is geared at:
I'm checking on these more than I am trying to comment on them one way or another. |
Yes: # ls -lZ /usr/share/foreman/bin/rails /usr/libexec/foreman/sidekiq-selinux
-rwxr-xr-x. root root system_u:object_r:foreman_rails_exec_t:s0 /usr/libexec/foreman/sidekiq-selinux
-rwxr-xr-x. root root system_u:object_r:foreman_rails_exec_t:s0 /usr/share/foreman/bin/rails Note that setting a context on a file is not security thing. As a normal non-privileged user you can set it: $ touch test
$ chcon -t foreman_rails_exec_t test
$ ls -lZ test
-rw-rw-r--. ekohl ekohl unconfined_u:object_r:foreman_rails_exec_t:s0 test
I don't think it is. You may need another Python service with another SELinux context. By not claiming any specific resources in a shared path, you keep it open in the future.
I think we are.
I suggested the wrappers and upstream replied with "This is a really good idea IMO.". See https://pulp.plan.io/issues/7178#note-3 (last paragraph). It also allows upstream to use the same wrapper locations in their setup with a virtualenv. This allows further standardization of the systemd unit files. I still hope that one day we can share those between both projects rather than having them duplicated in two places. |
Thanks for the explanations. All sounds good. Are we waiting on Pulp to merge this? |
I'm hoping to get an answer about WorkingDirectory and the generated denials. Then I'll also introduce the execdir variable. |
e8c30ba
to
6673e62
Compare
@dkliban @mikedep333 could you have a look at these 2? I should have mentioned this earlier today
|
@zpytela is working on updating our policies right now. He should be able to help. I'm running pulplift now with your nightly RPM packages to see if I get this error. |
Updated. There is now a parameter With this, I think it's ready to merge. I think those SELinux AVCs don't need to block merging. Maybe we first want to get nightly green with 3.6 before adding more changes though. @jlsherrill? |
Yeah that's probably a good idea, hopefully it'll all be fixed in a couple
days time
…On Mon, Sep 7, 2020, 12:23 PM Ewoud Kohl van Wijngaarden < ***@***.***> wrote:
Updated. There is now a parameter libexecdir and the WorkingDirectory in
the services is set to ~, meaning systemd will now also ensure the FS is
mounted before starting services.
With this, I think it's ready to merge. I think those SELinux AVCs don't
need to block merging.
Maybe we first want to get nightly green with 3.6 before adding more
changes though. @jlsherrill <https://github.com/jlsherrill>?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#116 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADAORMATT3GCPLA25246RLSEUCG5ANCNFSM4PE4GRCA>
.
|
1b77a5b
to
b8de25b
Compare
Rebased to resolve the merge conflict, no real change. |
b8de25b
to
e3a2886
Compare
Looks like I had a bad rebase: it managed the assets dir twice. Should be fixed now. |
e3a2886
to
e153fbe
Compare
@wbclark tests are green now and the pipeline is ready for it. Please have a look and approve/merge it if you think it's good. |
I get errors testing this on current nightly. The issues look SELinux related:
Or possibly this actually broken in nightly:
|
There was a big pulpcore-selinux update last week, but I'm not sure if that was already released. I'd like to test with that. |
@ehelms adding these permissions should be fairly easy. I just need to ask: does pulpcore need the access to objects with the specified types? |
@zpytela I don't think your patch made it into nightly RPMs yet so I think that would be a good first step. Since it now labels |
Here ya go:
|
So should we label settings.py with the new label pulpcore_etc_t as it was in the policy previously? If this file is for pulp3 only, the answer is probably yes. What about the keys, private_key.pem and public_key.pem?
This one is to add, likely with kernel_read_all_proc().
No information about the path; also search may not be the only permission needed. The permissive= entry was stripped off the record? Supposing 0 anyway; could you run again with |
I extracted the service changes to #125. With the updated policy in 3.7 the libexec change isn't really needed and those service benefits should go in already. |
I'm not entirely happy with the libexec changes. Marking it as a draft now. |
In python3-pulpcore 3.7.1-2 the /usr/libexec/pulpcore wrappers have been introduced to enter the proper SELinux domain. It has also been cherry picked to 3.6.3-2 but in the SELinux policy is incomplete so it has no effect. The main benefit of that cherry pick is to keep the module compatible with both 3.6 and 3.7.
Updated the PR to use the libexec wrappers provided by packaging. |
In python3-pulpcore 3.7.1-2 the
/usr/libexec/pulpcore
wrappers have been introduced to enter the proper SELinux domain.It has also been cherry picked to 3.6.3-2 but in the SELinux policy is incomplete so it has no effect. The main benefit of that cherry pick is to keep the module compatible with both 3.6 and 3.7.
The README is updated with a support policy. Since 3.7 is not being tested in CI yet, I have not yet added it to the supported versions.