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

update apparmor profile to allow podman to send any signal #2228

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

terencehonles
Copy link

This change updates the default apparmor profile to allow podman to send any signal rather than the allow listed "SIGINT", "SIGQUIT", "SIGKILL", and "SIGTERM". This fixes podman with signal proxying turned on (--sig-proxy) not being able to forward signals from the terminal such as "SIGWINCH" when attached to a TTY.

Copy link
Contributor

openshift-ci bot commented Nov 4, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: terencehonles
Once this PR has been reviewed and has the lgtm label, please assign jwhonce for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@terencehonles
Copy link
Author

I'm not completely familiar with podman's architecture, but I modified two apparmor profiles based the template. One explicitly disabled winch and the other did not. I saw the following message for both the default profile and the one disabling winch, but didn't see it for one that had the signal list omitted.

ERRO[0000] unable to signal init: permission denied     
                                                        ERRO[0007] forwarding signal 28 to container 15ba309cc529d743778f8d16d4311aa2077afdb77e864b2fb6fa0138d29ebdd5: sending signal to container 15ba309cc529d743778f8d16d4311aa2077afdb77e864b2fb6fa0138d29ebdd5: `/usr/bin/runc kill 15ba309cc529d743778f8d16d4311aa2077afdb77e864b2fb6fa0138d29ebdd5 28` failed: exit status 1 

So when quickly trying to see which process should be sending the signal I came across remoteProxySignal, but it wasn't completely clear if that should not be sending the signals directly from podman.

This change updates the default apparmor profile to allow podman to send
any signal rather than the allow listed "SIGINT", "SIGQUIT", "SIGKILL",
and "SIGTERM". This fixes podman with signal proxying turned on
(``--sig-proxy``) not being able to forward signals from the terminal
such as "SIGWINCH" when attached to a TTY.

Signed-off-by: Terence D. Honles <[email protected]>
@terencehonles
Copy link
Author

I'm not completely familiar with podman's architecture, but I modified two apparmor profiles based the template. One explicitly disabled winch and the other did not. I saw the following message for both the default profile and the one disabling winch, but didn't see it for one that had the signal list omitted.

ERRO[0000] unable to signal init: permission denied     
                                                        ERRO[0007] forwarding signal 28 to container 15ba309cc529d743778f8d16d4311aa2077afdb77e864b2fb6fa0138d29ebdd5: sending signal to container 15ba309cc529d743778f8d16d4311aa2077afdb77e864b2fb6fa0138d29ebdd5: `/usr/bin/runc kill 15ba309cc529d743778f8d16d4311aa2077afdb77e864b2fb6fa0138d29ebdd5 28` failed: exit status 1 

So when quickly trying to see which process should be sending the signal I came across remoteProxySignal, but it wasn't completely clear if that should not be sending the signals directly from podman.

This error message looks like it comes from:

https://github.com/containers/podman/blob/63b577e03e5946c0c1b973d368b2d1128cd4ac74/pkg/domain/infra/abi/terminal/sigproxy_commn.go#L39

and:

https://github.com/containers/podman/blob/63b577e03e5946c0c1b973d368b2d1128cd4ac74/libpod/oci_conmon_common.go#L312

@mheon
Copy link
Member

mheon commented Nov 4, 2024

LGTM, but I'm not the most familiar with Apparmor, so other reviews would be appreciated.

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused here, the signal proxy is not actually causing podman to send the signals but rather we call the ociruntime crun/runc and they are already allowed to send all signals per the lines above?

@terencehonles
Copy link
Author

terencehonles commented Nov 4, 2024

Well I'm a bit confused too, and was why I wrote

I'm not completely familiar with podman's architecture

But I'm not really sure what's blocking the SIGWINCH. I have added --security-opt=apparmor=podman to use the podman rule (from ubuntu I believe) which is very permissive:

$ cat /etc/apparmor.d/podman

# This profile allows everything and only exists to give the
# application a name instead of having the label "unconfined"

abi <abi/4.0>,
include <tunables/global>

profile podman /usr/bin/podman flags=(unconfined) {
  userns,

  # Site-specific additions and overrides. See local/README for details.
  include if exists <local/podman>
}

and this also does not suffer from this issue, but removing the set=(...) also seemed to fix the issue and is much more targeted.

@Luap99
Copy link
Member

Luap99 commented Nov 4, 2024

What podman and ubuntu version are you testing this on?

@terencehonles
Copy link
Author

I'm using podman 4.9.3 since that's what ubuntu noble (24.04) has 🙁, but I'm considering upgrading to oracular so I can get podman 5 and proper quadlet support for template files but using a non LTS is going to require increased maintenance and continuous version bumping until the next LTS so that's not ideal.

@terencehonles
Copy link
Author

They've added a slight variation of #2004 with https://git.launchpad.net/ubuntu/+source/golang-github-containers-common/commit/?id=1c376e5b2720a6dc015a746938275708255ff040 (doesn't have crun* since they didn't like the wildcard which is also mentioned in #2023)

@Luap99
Copy link
Member

Luap99 commented Nov 4, 2024

Well if you have a bug with that particular version then reporting this to via the ubuntu bug tracker (if not already) would be a good start because AFAICT this change cannot fix the mentioned error message, runc is failing to send a signal not podman. As such I fail to see how allowing podman to send other signals can possible fix that. Now I believe you that you tested this profile but just applying security relevant fixes without understanding why there are needed is not something I am comfortable with so I would love to get the input from someone who actually understands this.

@terencehonles
Copy link
Author

I am intending (and have been trying) to create a bug report in the tracker, but I was previously not registered and there have been getting server errors when trying to log in. I planned to link to this PR, but I wasn't sure what was sending the signals and figured I would get some context here.

If runc is indeed sending the signals, why does podman need to be specified at all in the apparmor config? I would expect podman itself to use the same machinery signal proxying is using, and if that were the case it should be able to ask runc to SIG{INT,QUIT,KILL,TERM} the process, right? Or does apparmor apply an intersection of podman and runc's permissions because one forked the other?

@terencehonles
Copy link
Author

terencehonles commented Nov 4, 2024

Still no luck with the bug tracker, but I'm noticing the following in journalctl

kernel: audit: type=1400 audit(1730750015.280:133): apparmor="DENIED" operation="signal" class="signal" profile="containers-default-0.57.4-apparmor1" pid=14034 comm="runc" requested_mask="receive" denied_mask="receive" signal=winch peer="podman"

I noticed I also see different behavior between the two commands:

sudo podman run -it --rm docker.io/library/python python -c '
import signal, time
signal.signal(signal.SIGWINCH, lambda x, y: print(x, y))
try:
  while True: time.sleep(5)
except KeyboardInterrupt:
  pass
'
sudo podman run -it --rm docker.io/library/python
>>> import signal; signal.signal(signal.SIGWINCH, lambda x, y: print(x, y))

Both print an error about forwarding the signal as I mentioned:

ERRO[0000] unable to signal init: permission denied     
                                                        ERRO[0012] forwarding signal 28 to container 5ca4ae26493a4e97b2ea5124ff5ad04f52bb37f9b7f5b8e3659648e6a0ae0ff3: sending signal to container 5ca4ae26493a4e97b2ea5124ff5ad04f52bb37f9b7f5b8e3659648e6a0ae0ff3: `/usr/bin/runc kill 5ca4ae26493a4e97b2ea5124ff5ad04f52bb37f9b7f5b8e3659648e6a0ae0ff3 28` failed: exit status 1 

however the first does receive SIGWINCH (maybe not all of them?), but the second does not receive any. This shouldn't be a problem with Python since both exhibit the same behavior when executed outside of a container.

@rhatdan
Copy link
Member

rhatdan commented Nov 20, 2024

So is this PR still valid or should it be closed?

@terencehonles
Copy link
Author

It is still valid, I finally have had my launchpad account reset, but I haven't been able to report the bug yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants