containers / common

Location for shared common files in github.com/containers repos.
Apache License 2.0
191 stars 200 forks source link

pkg/subscriptions: Modernize FIPS mounts #2174

Closed neverpanic closed 1 week ago

neverpanic commented 1 month ago

/etc/system-fips is deprecated in CentOS Stream 9 and has been removed from CentOS Stream 10. UBI8 containers still contain /etc/system-fips -> /run/secrets/system-fips, but UBI9 containers do not, so creating /run/secrets/system-fips on UBI9 (or later) does not serve a useful purpose. See [1, 2].

Instead of checking /etc/system-fips to determine whether FIPS mode is enabled on the host, read /proc/sys/crypto/fips_enabled, which works for all supported RHEL versions and likely even earlier.

In CentOS 10 Stream, the crypto-policies package does now contain /usr/share/crypto-policies/default-fips-config, which is meant to serve as a file to bind-mount over /etc/crypto-policies/config when in FIPS mode [3]. Manual creation of this file is thus no longer required in containers/common for modern containers. Using this file as a source also enables improvements in crypto-policies tooling which will now

Closes: containers/common#2130 Related: https://issues.redhat.com/browse/CRYPTO-13556

Luap99 commented 1 month ago

Just looking at the mechanism, all the filepath.Join(mountPoint, …) in this file make me worried about symlink breakouts. There might not be much actually wrong there (at worst, I guess, triggering an an audit message about a DAC or SELinux permission violation by the container runtime when checking for presence of an out-of-contaienr file), but that’s just one small refactor away from disaster.

I do appreciate that almost all of that is pre-existing and not directly relevant to this PR.

I noticed this last week when I looked at this PR and reported this internally so yes this is an pre existing issue, fix in https://github.com/containers/common/pull/2185 @neverpanic once this is merged please rebase and make sure all joins with of the container mount point use securejoin.

neverpanic commented 1 month ago

I'm on PTO until Oct 14, I'll fix the comments after that.

kwilczynski commented 2 weeks ago

Nice!

kwilczynski commented 2 weeks ago

/approve /lgtm

openshift-ci[bot] commented 2 weeks ago

@kwilczynski: changing LGTM is restricted to collaborators

In response to [this](https://github.com/containers/common/pull/2174#issuecomment-2451602519): >/approve >/lgtm Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
Luap99 commented 1 week ago

@mtrmac Mind giving this a final review/merge? I think it would be good to get this into podman 5.3.

openshift-ci[bot] commented 1 week ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, kwilczynski, Luap99, neverpanic

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: - ~~[OWNERS](https://github.com/containers/common/blob/main/OWNERS)~~ [Luap99,giuseppe] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment