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.

Sign up for GitHub

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

Jump to bottom

RHEL 10 FIPS adjustments #21264

Merged
merged 4 commits into from
Nov 15, 2024
Merged

RHEL 10 FIPS adjustments #21264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
24b5660
systemd: Fix detection of current crypto policy in FIPS mode
martinpitt Nov 14, 2024
90becdd
systemd: Rewrite crypto policy setting as async
martinpitt Nov 15, 2024
8dca495
systemd: Move crypto policies components to useInit()
martinpitt Nov 15, 2024
dae0b2f
systemd: Only change FIPS policy with fips-mode-setup
martinpitt Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 68 additions & 37 deletions pkg/systemd/overview-cards/cryptoPolicies.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/

import cockpit from "cockpit";
import React, { useState, useEffect } from 'react';
import React, { useState } from 'react';
import { Button } from "@patternfly/react-core/dist/esm/components/Button/index.js";
import { Flex, FlexItem } from "@patternfly/react-core/dist/esm/layouts/Flex/index.js";
import { Modal } from "@patternfly/react-core/dist/esm/components/Modal/index.js";
Expand All @@ -29,6 +29,7 @@ import { ModalError } from 'cockpit-components-inline-notification.jsx';
import { PrivilegedButton } from "cockpit-components-privileged.jsx";
import { ProfilesMenuDialogBody } from "./profiles-menu-dialog-body.jsx";
import { useDialogs } from "dialogs.jsx";
import { useInit } from "hooks";

import "./cryptoPolicies.scss";

Expand All @@ -37,63 +38,87 @@ const _ = cockpit.gettext;
const displayProfileText = profile => profile === "DEFAULT" ? _("Default") : profile;
const isInconsistentPolicy = (policy, fipsEnabled) => policy === "FIPS" !== fipsEnabled;

const getFipsConfigurable = () => cockpit.spawn(["/bin/sh", "-c", "command -v fips-mode-setup"], { error: "ignore" })
.then(() => true)
.catch(() => false);

export const CryptoPolicyRow = () => {
const Dialogs = useDialogs();
const [currentCryptoPolicy, setCurrentCryptoPolicy] = useState(null);
const [fipsEnabled, setFipsEnabled] = useState(null);
const [fipsConfigurable, setFipsConfigurable] = useState(null);
const [shaSubPolicyAvailable, setShaSubPolicyAvailable] = useState(null);

useEffect(() => {
useInit(() => {
cockpit.file("/proc/sys/crypto/fips_enabled").read()
.then(content => setFipsEnabled(content ? content.trim() === "1" : false));
cockpit.file("/etc/crypto-policies/state/current")
.watch(content => setCurrentCryptoPolicy(content ? content.trim() : null));
getFipsConfigurable().then(v => setFipsConfigurable(v));
cockpit.file("/etc/crypto-policies/config")
.watch(async contents => {
// Ask crypto-policies to get correct FIPS state, as that dominates the configured policy
try {
setCurrentCryptoPolicy((await cockpit.spawn(["update-crypto-policies", "--show"])).trim());
} catch (error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test.

console.warn("Failed to get current crypto policy:", error.toString(),
"; falling back to /etc/crypto-policies/config");
setCurrentCryptoPolicy(contents.trim());
}
});
// RHEL-8-8 has no SHA1 subpolicy
cockpit.file("/usr/share/crypto-policies/policies/modules/SHA1.pmod").read()
.then(content => setShaSubPolicyAvailable(content ? content.trim() : false));
}, []);
});

if (!currentCryptoPolicy) {
if (currentCryptoPolicy === null || fipsEnabled === null || fipsConfigurable === null)
return null;
}

const policyRender = (currentCryptoPolicy.startsWith("FIPS") && !fipsConfigurable)
/* read-only mode; can't switch away from FIPS without fips-mode-setup */
? <span id="crypto-policy-current">{displayProfileText(currentCryptoPolicy)}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test.

: <PrivilegedButton variant="link" buttonId="crypto-policy-button" tooltipId="tip-crypto-policy"
excuse={ _("The user $0 is not permitted to change cryptographic policies") }
onClick={() => Dialogs.show(<CryptoPolicyDialog
currentCryptoPolicy={currentCryptoPolicy}
setCurrentCryptoPolicy={setCurrentCryptoPolicy}
fipsEnabled={fipsEnabled}
fipsConfigurable={fipsConfigurable}
shaSubPolicyAvailable={shaSubPolicyAvailable} />)}>
{displayProfileText(currentCryptoPolicy)}
</PrivilegedButton>;

return (
<tr className="pf-v5-c-table__tr">
<th className="pf-v5-c-table__th" scope="row">{_("Cryptographic policy")}</th>
<td className="pf-v5-c-table__td">
<PrivilegedButton variant="link" buttonId="crypto-policy-button" tooltipId="tip-crypto-policy"
excuse={ _("The user $0 is not permitted to change cryptographic policies") }
onClick={() => Dialogs.show(<CryptoPolicyDialog
currentCryptoPolicy={currentCryptoPolicy}
setCurrentCryptoPolicy={setCurrentCryptoPolicy}
fipsEnabled={fipsEnabled}
shaSubPolicyAvailable={shaSubPolicyAvailable} />)}>
{displayProfileText(currentCryptoPolicy)}
</PrivilegedButton>
</td>
<td className="pf-v5-c-table__td">{policyRender}</td>
</tr>
);
};

const setPolicy = (policy, setError, setInProgress) => {
const setPolicy = async (policy, setError, setInProgress, fipsConfigurable) => {
setInProgress(true);

let promise;
if (policy === "FIPS") {
promise = cockpit.spawn(["fips-mode-setup", "--enable"], { superuser: "require", err: "message" });
} else {
promise = cockpit.spawn(["fips-mode-setup", "--disable"], { superuser: "require", err: "message" }).then(() =>
cockpit.spawn(["update-crypto-policies", "--set", policy], { superuser: "require", err: "message" }));
}
try {
if (policy === "FIPS") {
cockpit.assert(fipsConfigurable, "calling setPolicy(FIPS) without fips-mode-setup");
await cockpit.spawn(["fips-mode-setup", "--enable"], { superuser: "require", err: "message" });
} else {
if (fipsConfigurable)
await cockpit.spawn(["fips-mode-setup", "--disable"], { superuser: "require", err: "message" });
await cockpit.spawn(["update-crypto-policies", "--set", policy], { superuser: "require", err: "message" });
}

promise.then(() => cockpit.spawn(["shutdown", "--reboot", "now"], { superuser: "require", err: "message" }))
.catch(error => setError(error))
.finally(() => setInProgress(false));
await cockpit.spawn(["shutdown", "--reboot", "now"], { superuser: "require", err: "message" });
Copy link
Member

Choose a reason for hiding this comment

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

Technically should use cockpit.hint('restart'); but that is another issue.

} catch (error) {
setError(error);
} finally {
Comment on lines +111 to +113
Copy link
Contributor

Choose a reason for hiding this comment

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

These 3 added lines are not executed by any test.

setInProgress(false);
}
};

const CryptoPolicyDialog = ({
currentCryptoPolicy,
fipsEnabled,
fipsConfigurable,
reApply,
shaSubPolicyAvailable,
}) => {
Expand Down Expand Up @@ -125,6 +150,8 @@ const CryptoPolicyDialog = ({

const policies = Object.keys(cryptopolicies)
.filter(pol => pol.endsWith(':SHA1') ? shaSubPolicyAvailable : true)
// cannot enable fips without fips-mode-setup
.filter(pol => pol.startsWith("FIPS") ? fipsConfigurable : true)
.map(policy => ({
name: policy,
title: displayProfileText(policy),
Expand Down Expand Up @@ -185,7 +212,8 @@ const CryptoPolicyDialog = ({
<Flex spaceItems={{ default: 'spaceItemsSm' }} alignItems={{ default: 'alignItemsCenter' }}>
{_("Applying new policy... This may take a few minutes.")}
</Flex>}
<Button id="crypto-policy-save-reboot" variant='primary' onClick={() => setPolicy(selected, setError, setInProgress)}
<Button id="crypto-policy-save-reboot" variant='primary'
onClick={() => setPolicy(selected, setError, setInProgress, fipsConfigurable)}
isDisabled={inProgress} isLoading={inProgress}
>
{reApply ? _("Reapply and reboot") : _("Apply and reboot")}
Expand All @@ -209,16 +237,18 @@ export const CryptoPolicyStatus = () => {
const Dialogs = useDialogs();
const [currentCryptoPolicy, setCurrentCryptoPolicy] = useState(null);
const [fipsEnabled, setFipsEnabled] = useState(null);
const [fipsConfigurable, setFipsConfigurable] = useState(null);

useEffect(() => {
if (currentCryptoPolicy === null) {
cockpit.file("/etc/crypto-policies/state/current")
.watch(content => setCurrentCryptoPolicy(content ? content.trim().split(':', 1)[0] : undefined));
}

useInit(() => {
cockpit.file("/etc/crypto-policies/state/current")
.watch(content => setCurrentCryptoPolicy(content ? content.trim().split(':', 1)[0] : undefined));
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test.

getFipsConfigurable().then(v => setFipsConfigurable(v));
cockpit.file("/proc/sys/crypto/fips_enabled").read()
.then(content => setFipsEnabled(content ? content.trim() === "1" : false));
}, [currentCryptoPolicy]);
});

if (currentCryptoPolicy === null || fipsConfigurable === null)
return null;

if (isInconsistentPolicy(currentCryptoPolicy, fipsEnabled)) {
return (
Expand All @@ -232,6 +262,7 @@ export const CryptoPolicyStatus = () => {
<Button isInline variant="link" className="pf-v5-u-font-size-sm"
onClick={() => Dialogs.show(<CryptoPolicyDialog currentCryptoPolicy={currentCryptoPolicy}
fipsEnabled={fipsEnabled}
fipsConfigurable={fipsConfigurable}
reApply />)}>
{_("Review cryptographic policy")}
</Button>
Expand Down
44 changes: 39 additions & 5 deletions test/verify/check-system-info
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,14 @@ class TestSystemInfo(testlib.MachineCase):
# Most OSes don't set nosmt by default, but there are some exceptions
self.expect_smt_default = self.machine.image in ["fedora-coreos"]

self.supportsFIPS = re.match('fedora|((centos|rhel)-(8|9)).*', self.machine.image)
# HACK: temporary: we are in the middle of transitioning centos/rhel-10 images away from fips-mode-setup;
# until https://github.com/cockpit-project/bots/pull/7095 and https://github.com/cockpit-project/bots/pull/7091
# land, do runtime detection
if self.machine.image in ["centos-10", "rhel-10-0"]:
if '/bin' in self.machine.execute("command -v fips-mode-setup", check=False):
self.supportsFIPS = True

def testBasic(self):
m = self.machine
b = self.browser
Expand Down Expand Up @@ -811,8 +819,6 @@ password=foobar
profile = m.execute(cmd + " --show").strip()
self.assertEqual(profile, new_profile)
b.wait_text("#crypto-policy-button", shown_profile_text(profile))
new_profile = "FIPS"
change_profile(profile, new_profile)

# Select a custom policy (non-selectable option)
profile = "EMPTY"
Expand All @@ -824,6 +830,24 @@ password=foobar
b.wait_in_text(".pf-v5-c-menu__item.pf-m-selected", "Custom cryptographic policy")
b.click("#crypto-policy-dialog button.pf-v5-c-button.pf-m-link")

# FIPS mode
if self.supportsFIPS:
change_profile(profile, "FIPS")
else:
# FIPS is not an available choice
b.click("#crypto-policy-button")
b.wait_in_text(".pf-v5-c-menu__item.pf-m-selected", shown_profile_text(profile))
self.assertNotIn("FIPS", b.text("#crypto-policy-dialog"))
b.click("#crypto-policy-dialog button:contains('Cancel')")
b.wait_not_present("#crypto-policy-dialog")

# pretend the machine already has FIPS enabled, then it can't be disabled
m.execute(cmd + " --set FIPS") # this is known incomplete/invalid, but suffices to set the UI state
b.wait_text("#crypto-policy-current", "FIPS")
self.assertFalse(b.is_present("#crypto-policy-button"))
m.execute(cmd + f" --set {profile}") # this is known incomplete/invalid, but suffices to set the UI state
b.wait_text("#crypto-policy-button", shown_profile_text(profile))

@testlib.skipImage("crypto-policies not available", "debian-*", "ubuntu-*", "arch")
@testlib.skipOstree("crypto-policies not available")
def testInconsistentCryptoPolicy(self):
Expand All @@ -837,14 +861,24 @@ password=foobar
self.login_and_go("/system")
b.wait_text("#inconsistent_crypto_policy", "FIPS is not properly enabled")
b.click(".system-health-crypto-policies button.pf-v5-c-button.pf-m-link")
b.wait_in_text(".pf-v5-c-menu__item.pf-m-selected .pf-v5-c-label.pf-m-orange", "inconsistent")
if self.supportsFIPS:
# fix the FIPS policy
b.wait_in_text(".pf-v5-c-menu__item.pf-m-selected .pf-v5-c-label.pf-m-orange", "inconsistent")
else:
# pick any valid non-FIPS policy
b.click("#crypto-policy-dialog .pf-v5-c-menu__list-item[data-value='DEFAULT'] button")
b.click("#crypto-policy-save-reboot")
# Initramfs re-generation takes a while
m.wait_reboot(timeout_sec=600)
m.start_cockpit()
self.login_and_go("/system")
b.wait_text("#crypto-policy-button", "FIPS")
self.assertEqual(m.execute("cat /proc/sys/crypto/fips_enabled").strip(), "1")
if self.supportsFIPS:
b.wait_text("#crypto-policy-button", "FIPS")
self.assertEqual(m.execute("cat /proc/sys/crypto/fips_enabled").strip(), "1")
else:
b.wait_text("#crypto-policy-button", "Default")
# rest of the test does not apply any more
return

m.execute(cmd + " --set DEFAULT")
b.wait_text("#inconsistent_crypto_policy", "Cryptographic policy is inconsistent")
Expand Down