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

Expose security attribute errors with their own messages #8946

Merged
merged 1 commit into from
Jan 12, 2021

Conversation

JAORMX
Copy link
Contributor

@JAORMX JAORMX commented Jan 12, 2021

This creates error objects for runtime errors that might come from the
runtime. Thus, indicating to users that the place to debug should be in
the security attributes of the container.

When creating a container with a SELinux label that doesn't exist, we
get a fairly cryptic error message:

$ podman run --security-opt label=type:my_container.process -it fedora bash
Error: OCI runtime error: write file `/proc/thread-self/attr/exec`: Invalid argument

This instead handles any errors coming from LSM's /proc API and
enhances the error message with a relevant indicator that it's related
to the container's security attributes.

A sample run looks as follows:

$ bin/podman run --security-opt label=type:my_container.process -it fedora bash
Error: `/proc/thread-self/attr/exec`: OCI runtime error: unable to assign security attribute

With debug log level enabled it would be:

Error: write file `/proc/thread-self/attr/exec`: Invalid argument: OCI runtime error: unable to assign security attribute

Note that these errors wrap ErrOCIRuntime, so it's still possible to to
compare these errors with errors.Is/errors.As.

One advantage of this approach is that we could start handling these
errors in a more efficient manner in the future.

e.g. If a SELinux label doesn't exist (yet), we could retry until it
becomes available.

This creates error objects for runtime errors that might come from the
runtime. Thus, indicating to users that the place to debug should be in
the security attributes of the container.

When creating a container with a SELinux label that doesn't exist, we
get a fairly cryptic error message:

```
$ podman run --security-opt label=type:my_container.process -it fedora bash
Error: OCI runtime error: write file `/proc/thread-self/attr/exec`: Invalid argument
```

This instead handles any errors coming from LSM's `/proc` API and
enhances the error message with a relevant indicator that it's related
to the container's security attributes.

A sample run looks as follows:

```
$ bin/podman run --security-opt label=type:my_container.process -it fedora bash
Error: `/proc/thread-self/attr/exec`: OCI runtime error: unable to assign security attribute
```

With `debug` log level enabled it would be:

```
Error: write file `/proc/thread-self/attr/exec`: Invalid argument: OCI runtime error: unable to assign security attribute
```

Note that these errors wrap ErrOCIRuntime, so it's still possible to to
compare these errors with `errors.Is/errors.As`.

One advantage of this approach is that we could start handling these
errors in a more efficient manner in the future.

e.g. If a SELinux label doesn't exist (yet), we could retry until it
becomes available.

Signed-off-by: Juan Antonio Osorio Robles <[email protected]>
@mheon
Copy link
Member

mheon commented Jan 12, 2021

/approve
@rhatdan PTAL

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JAORMX, mheon

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2021
@rhatdan
Copy link
Member

rhatdan commented Jan 12, 2021

@JAORMX Wouldn't this code be better in https://github.com/opencontainers/selinux?

@rhatdan
Copy link
Member

rhatdan commented Jan 12, 2021

That way buildah, Docker, and other container engines could use it.

@JAORMX
Copy link
Contributor Author

JAORMX commented Jan 12, 2021

@JAORMX Wouldn't this code be better in https://github.com/opencontainers/selinux?

It would be! and that's the direction that I initially tried to take. However, in some flows we won't see that error when setting the label in exec. I would have expected https://github.com/opencontainers/selinux/blob/master/go-selinux/selinux_linux.go#L677 to throw an error when one uses a selinux label that doesn't exist. But that's not the case, seems that an error only manifests itself after execve, e.g. after conmon is spawned. And hence why I submitted this as it is. It was the only way I figured to capture the error.

@rhatdan
Copy link
Member

rhatdan commented Jan 12, 2021

Ok, understood.
LGTM

@rhatdan
Copy link
Member

rhatdan commented Jan 12, 2021

@containers/podman-maintainers PTAL

@mheon
Copy link
Member

mheon commented Jan 12, 2021

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2021
@openshift-merge-robot openshift-merge-robot merged commit db52828 into containers:master Jan 12, 2021
edsantiago added a commit to edsantiago/libpod that referenced this pull request Jan 25, 2021
- stop: test --all and --ignore (containers#9051)
- build: test /run/secrets (containers#8679, but see below)
- sensitive mount points: deal with 'stat' failures
- selinux: confirm useful diagnostics on unknown labels (containers#8946)

The 'build' test is intended as a fix for containers#8679, in which
'podman build' does not mount secrets from mounts.conf.
Unfortunately, as of this writing, 'podman build' does
not pass the --default-mounts-file option to buildah,
so there's no reasonable way to test this path. Still,
we can at least confirm /run/secrets on 'podman run'.

The /sys thing is related to containers#8949: RHEL8, rootless, cgroups v1.
It's just a workaround to get gating tests to pass on RHEL.

Signed-off-by: Ed Santiago <[email protected]>
iwita pushed a commit to iwita/podman that referenced this pull request Jan 26, 2021
- stop: test --all and --ignore (containers#9051)
- build: test /run/secrets (containers#8679, but see below)
- sensitive mount points: deal with 'stat' failures
- selinux: confirm useful diagnostics on unknown labels (containers#8946)

The 'build' test is intended as a fix for containers#8679, in which
'podman build' does not mount secrets from mounts.conf.
Unfortunately, as of this writing, 'podman build' does
not pass the --default-mounts-file option to buildah,
so there's no reasonable way to test this path. Still,
we can at least confirm /run/secrets on 'podman run'.

The /sys thing is related to containers#8949: RHEL8, rootless, cgroups v1.
It's just a workaround to get gating tests to pass on RHEL.

Signed-off-by: Ed Santiago <[email protected]>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants