pkg/subscriptions: Modernize FIPS mounts #2174

neverpanic · 2024-09-25T12:00:32Z

/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

unmount the two bind mounts when a user manually changes the policy using update-crypto-policies --set, something which was previously broken in containers because /etc/crypto-policies/config was a read-only bind-mount, and
unmount and restore the two bind-mounts when the crypto-policies package is updated. The crypto-policies package will only do these steps if the the bind mounts for crypto-policies use the
/usr/share/crypto-policies/default-fips-config
file as source, so it makes sense for containers/common to switch to that.

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

Honny1

Thanks!

The approach looks good overall, but I have a few suggestions that should reduce maintenance costs. Also, vendoring will need to be done to the projects that using c/common for example podman and buildah to make the changes take effect.

PTAL @mtrmac, @Luap99

pkg/subscriptions/subscriptions.go

mtrmac · 2024-10-01T14:33:34Z

pkg/subscriptions/subscriptions.go

+				logrus.Errorf("Adding FIPS mode bind mounts to container: %v", err)
+			}
+		} else {
+			logrus.Debug("/proc/sys/crypto/fips_enabled does not contain '1', not adding FIPS mode bind mounts")


Is it worth logging the value here?

I don't think so. The value can't change without a reboot, and it's going to be the the same across the entire host, regardless of whether one is inside of a container or outside. It should be simple to obtain the actual value using cat if necessary for debugging.

The only case where this could differ from the global value is if somebody bind-mounted a file over it for testing purposes. I don't think we need to handle this eventuality.

mtrmac · 2024-10-01T14:42:40Z

pkg/subscriptions/subscriptions.go

-		file, err := os.Create(fipsFile)
+	fipsFileHost := filepath.Join(mountPoint, "etc/system-fips")
+	if fileutils.Lexists(fipsFileHost) != nil {
+		fipsFileTarget, err := filepath.EvalSymlinks(fipsFileHost)


This conceptually allows an attacker to set up /etc/system-fips inside the container to be a symlink with ../../… to break out of the container and refer to hosts’ files.

Yes, we actually only compare the fipsFileTarget text without referring to the possibly-unwanted file, but, still…

In other words, either we care about the specific symlink text, and we can just Readlink (probably not), or we care about where the symlink evaluates, whatever its form, and in that case .. in the / directory of the container should be evaluated correctly (restricted to the containers’ root).

(https://github.com/cyphar/filepath-securejoin is used for that purpose in some parts of the codebase.)

Good catch, fixed by using securejoin. I still need a fileutils.Lexists, because the log messages should probably differ between /etc/system-fips not existing (which is a common config) and /etc/system-fips existing but not pointing to /run/secrets/system-fips (which is probably an uncommon config). That shouldn't be an issue, though, since fileutils.Lexists doesn't follow symlinks.

pkg/subscriptions/subscriptions.go

mtrmac

I didn’t actually review the substance of the PR, whether the code correctly handles all relevant versions of the OSes and behaviors.

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.

Luap99 · 2024-10-01T15:56:52Z

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 #2185
@neverpanic once this is merged please rebase and make sure all joins with of the container mount point use securejoin.

neverpanic · 2024-10-06T08:53:49Z

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

pkg/subscriptions/subscriptions.go

Luap99 · 2024-10-22T17:16:25Z

pkg/subscriptions/subscriptions.go

+	fips_enabled, err := os.ReadFile("/proc/sys/crypto/fips_enabled")
+	if err != nil {
+		logrus.Errorf("Failed to read /proc/sys/crypto/fips_enabled to determine FIPS state: %v", err)
+		return false
+	}


This will log an error when the file does not exists, it not clear to me does the kernel always has the file or is that set to some CONFIG_ options in the kernel. If so we should certainly ignore ENOENT?

Second, this logs an error and then assumes fips is disabled, Given this is a security option should we not outright fail with the error (e.g. return the error to the caller)? I do realize MountsWithUIDGID() does not return an error so we would need to add a new function so maybe it is not worth it. Maybe it is not important enough but I always very much find it odd to log an error but then start the container anyway. Either way the issue is pre existing so I would not block this on it.

The kernel always has that file, so there's no need to ignore ENOENT.

I can make the change to bubble up the error, but as you said, it's going to be a larger change because MountsWithUIDGID() does not currently fail. Maybe we should do that in a separate PR afterwards?

The kernel always has that file, so there's no need to ignore ENOENT.

@neverpanic not always. I work with systems where CONFIG_CRYPTO_FIPS is not always enabled.

Should be fixed now.

pkg/subscriptions/subscriptions.go

Luap99

one small nit otherwise LGTM

cc @TomSweeneyRedHat not sure if the fips changes here need any updates in the container RHEL docs.

Luap99 · 2024-10-30T17:24:05Z

pkg/subscriptions/subscriptions.go

+func shouldAddFIPSMounts() bool {
+	fips_enabled, err := os.ReadFile("/proc/sys/crypto/fips_enabled")
+	if err != nil {
+		if !os.IsNotExist(err) {


new code should use errors.Is(err, fs.ErrNotExist)

I've used os.ErrNotExist, since that was already imported.

/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 - unmount the two bind mounts when a user manually changes the policy using update-crypto-policies --set, something which was previously broken in containers because /etc/crypto-policies/config was a read-only bind-mount, and - unmount and restore the two bind-mounts when the crypto-policies package is updated. The crypto-policies package will only do these steps if the the bind mounts for crypto-policies use the /usr/share/crypto-policies/default-fips-config file as source, so it makes sense for containers/common to switch to that. [1]: https://gitlab.com/redhat-crypto/fedora-crypto-policies/-/merge_requests/111 [2]: https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/9/html/9.0_release_notes/deprecated_functionality#deprecated-functionality_security [3]: https://gitlab.com/redhat-crypto/fedora-crypto-policies/-/commit/04ceadccfc07e5946b08157d06ca5c0d5a229d92 Closes: containers#2130 Related: https://issues.redhat.com/browse/CRYPTO-13556 Signed-off-by: Clemens Lang <[email protected]>

Luap99

LGTM

kwilczynski · 2024-11-01T09:44:06Z

Nice!

kwilczynski · 2024-11-01T09:44:12Z

/approve
/lgtm

openshift-ci · 2024-11-01T09:44:15Z

@kwilczynski: changing LGTM is restricted to collaborators

In response to this:

/approve
/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Luap99 · 2024-11-07T11:04:59Z

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

giuseppe

/lgtm

openshift-ci · 2024-11-07T11:14:22Z

[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~~ [Luap99,giuseppe]

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

First, creating a global file /etc/system-fips was never a good idea for testing as it affects other running tests at the same time. And as of a recent change to FIPS mounts[1] we no longer use the file so the test breaks with c/common v0.61. Instead it uses the kernel file /proc/sys/crypto/fips_enabled which requires the real fips mode to be activated and that in turn requires a reboot. As such this is not somthing that can be tested in upstream CI like that. [1] containers/common#2174 Signed-off-by: Paul Holzinger <[email protected]>

Honny1 suggested changes Oct 1, 2024

View reviewed changes

mtrmac reviewed Oct 1, 2024

View reviewed changes

neverpanic force-pushed the cllang-modernize-fips-mounts branch from fada40b to affa37c Compare October 22, 2024 16:19

Luap99 reviewed Oct 22, 2024

View reviewed changes

neverpanic force-pushed the cllang-modernize-fips-mounts branch from affa37c to 7e169c6 Compare October 24, 2024 09:52

mtrmac reviewed Oct 24, 2024

View reviewed changes

pkg/subscriptions/subscriptions.go Outdated Show resolved Hide resolved

neverpanic force-pushed the cllang-modernize-fips-mounts branch 4 times, most recently from 3f4818e to 1b8afdf Compare October 30, 2024 17:09

Luap99 reviewed Oct 30, 2024

View reviewed changes

neverpanic force-pushed the cllang-modernize-fips-mounts branch from 1b8afdf to abf2f6b Compare October 31, 2024 13:05

Luap99 approved these changes Nov 1, 2024

View reviewed changes

openshift-ci bot added the approved label Nov 1, 2024

giuseppe approved these changes Nov 7, 2024

View reviewed changes

openshift-ci bot assigned giuseppe Nov 7, 2024

openshift-ci bot added the lgtm label Nov 7, 2024

openshift-merge-bot bot merged commit e522662 into containers:main Nov 7, 2024
16 checks passed

neverpanic deleted the cllang-modernize-fips-mounts branch November 7, 2024 19:14

edsantiago mentioned this pull request Nov 11, 2024

[v5.3] Bump to Buildah v1.38.0 containers/podman#24528

Merged

Luap99 mentioned this pull request Nov 12, 2024

vendor containers projects to tagged versions containers/podman#24540

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/subscriptions: Modernize FIPS mounts #2174

pkg/subscriptions: Modernize FIPS mounts #2174

neverpanic commented Sep 25, 2024

Honny1 left a comment •

edited

Loading

mtrmac Oct 1, 2024

neverpanic Oct 22, 2024

mtrmac Oct 1, 2024

neverpanic Oct 22, 2024

mtrmac left a comment

Luap99 commented Oct 1, 2024

neverpanic commented Oct 6, 2024

Luap99 Oct 22, 2024

neverpanic Oct 24, 2024

kwilczynski Oct 25, 2024 •

edited

Loading

neverpanic Oct 30, 2024

Luap99 left a comment

Luap99 Oct 30, 2024

neverpanic Oct 31, 2024

Luap99 left a comment

kwilczynski commented Nov 1, 2024

kwilczynski commented Nov 1, 2024

openshift-ci bot commented Nov 1, 2024

Luap99 commented Nov 7, 2024

giuseppe left a comment

openshift-ci bot commented Nov 7, 2024

pkg/subscriptions: Modernize FIPS mounts #2174

pkg/subscriptions: Modernize FIPS mounts #2174

Conversation

neverpanic commented Sep 25, 2024

Honny1 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

Luap99 commented Oct 1, 2024

neverpanic commented Oct 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwilczynski Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

kwilczynski commented Nov 1, 2024

kwilczynski commented Nov 1, 2024

openshift-ci bot commented Nov 1, 2024

Luap99 commented Nov 7, 2024

giuseppe left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 7, 2024

Honny1 left a comment •

edited

Loading

kwilczynski Oct 25, 2024 •

edited

Loading