pkg/daemon: support FIPS #889

runcom · 2019-06-25T12:29:14Z

Signed-off-by: Antonio Murdaca [email protected]

- What I did

resurrected from #826
close #800

Added a new FIPS field to MachineConfig to enable/disable FIPS on rhcos (disabled by default). This is, at this point, only intended as a day-2 operation.

FIPS would also greatly benefit a dedicated CRD (oc edit fips?) - I'm experimenting with that but it will ofc require a new controller.

- How to verify it

added an e2e and manually verified

- Description for the changelog

Added FIPS support

ashcrow · 2019-06-25T14:22:14Z

pkg/daemon/update.go

@@ -369,6 +400,7 @@ func Reconcilable(oldConfig, newConfig *mcfgv1.MachineConfig) (*MachineConfigDif
 	return &MachineConfigDiff{
 		osUpdate: oldConfig.Spec.OSImageURL != newConfig.Spec.OSImageURL,
 		kargs:    !reflect.DeepEqual(oldConfig.Spec.KernelArguments, newConfig.Spec.KernelArguments),
+		fips:     oldConfig.Spec.Fips != newConfig.Spec.Fips,


This will force the reboot so that the change takes effect, correct?

nope, the sync itself does the reboot the node (and it can't be avoided today as we know). This is just to log what we're changing during a sync.

WFM. Just wanted to ensure a reboot does happen when FIPS changes 👍

👍 the e2e added here also takes care of validating fips has been enabled on nodes so it will fail if we skip a reboot after enabling fips (iiuic)

ashcrow

👍

cgwalters · 2019-06-25T14:53:48Z

pkg/daemon/update.go

@@ -37,6 +37,8 @@ const (
 	coreUserName = "core"
 	// SSH Keys for user "core" will only be written at /home/core/.ssh
 	coreUserSSHPath = "/home/core/.ssh/"
+	// fipsCommand is the command to use when enabling or disabling FIPS
+	fipsCommand = "/usr/libexec/rhcos-tools/coreos-fips"


Sort of a side comment but I recently was thinking that we probably should have taken the path of replacing /usr/bin/fips-mode-setup; we can totally do that now via override layers.

cgwalters · 2019-06-25T14:56:03Z

docs/MachineConfiguration.md

@@ -51,6 +51,7 @@ type MachineConfigSpec struct {
    // Config is a Ignition Config object.
    Config ign.Config `json:"config"`
    KernelArguments []string `json:"kernelArguments"`
+    Fips bool `json:"fips"`


What about Erica's comment here?

cgwalters · 2019-06-25T14:57:08Z

pkg/apis/machineconfiguration.openshift.io/v1/helpers.go

 	outIgn := configs[0].Spec.Config
 	for idx := 1; idx < len(configs); idx++ {
+		// if any of the config has FIPS enabled, it'll be set


Ah OK I see we went with the last approach. I think that's sane for this indeed.

cgwalters · 2019-06-25T15:54:28Z

LGTM, giving to
/assign ericavonb
since she reviewed an earlier version.

ericavonb · 2019-06-25T16:28:17Z

docs/MachineConfiguration.md

@@ -94,6 +95,10 @@ Ignition config keys as well.

 This extends the host's kernel arguments.  Use this for e.g. [nosmt](https://access.redhat.com/solutions/rhel-smt).

+### FIPS
+
+This allows to enable/disable [FIPS mode](https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/security_guide/chap-federal_standards_and_regulations).


Can you add a comment here about if any of the config has FIPS enabled, it'll be set as well?

ericavonb · 2019-06-25T16:33:48Z

pkg/daemon/update.go

+	cmd := exec.Command(fipsCommand, arg)
+	dn.logSystem("Running %s %s", fipsCommand, arg)
+	if out, err := cmd.CombinedOutput(); err != nil {
+		return errors.Wrapf(err, "enabling FIPS: %s", string(out))


Can you include arg in the error to distinguish between errors from enabling and disabling FIPS?

oh good catch this is completely wrong indeed

ericavonb · 2019-06-25T16:39:27Z

test/e2e/mcd_test.go

@@ -546,3 +546,91 @@ func TestDontDeleteRPMFiles(t *testing.T) {
 		}
 	}
 }
+
+func TestFIPS(t *testing.T) {


non-blocking suggestion: it might be nice to have this test structured as a table test with a couple variations of machineconfig sets.

yeah, that would be amazing but test timeout is 75m today and going through various reboots for applying the MC + other tests would exceed that

ericavonb · 2019-06-25T16:42:02Z

pkg/daemon/update.go

 	return dn.updateOSAndReboot(newConfig)
 }

+func (dn *Daemon) updateFIPS(current, desired *mcfgv1.MachineConfig) error {
+	if current.Spec.Fips == desired.Spec.Fips {


Does the current get written to Status anywhere?

Co-authored-by: Colin Walters <[email protected]> Co-authored-by: Antonio Murdaca <[email protected]> Signed-off-by: Antonio Murdaca <[email protected]>

runcom · 2019-06-25T17:38:29Z

Took care of @ericavonb comments

runcom · 2019-06-25T17:45:49Z

/retest

ericavonb · 2019-06-25T17:50:56Z

/lgtm

openshift-ci-robot · 2019-06-25T17:51:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashcrow, ericavonb, runcom

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~~ [ashcrow,ericavonb,runcom]

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

openshift-bot · 2019-06-25T18:06:53Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-06-25T18:46:19Z

/retest

Please review the full test history for this PR and help us cut down flakes.

kikisdeliveryservice · 2019-06-25T18:50:30Z

Gah. This is hitting AWS/ECS errors now: Error: error attaching EC2 Internet Gateway...

Part of: openshift/enhancements#15 We added FIPS to the MCO a while ago: openshift/machine-config-operator#889 However, during some discussion it became clear that the main use case for FIPS is "day 1" - it doesn't make sense to turn it on "day 2" because the standard requires that e.g. long-term key material was created with FIPS enabled. Further, it's unlikely that admins will want to turn it *off* if they ever had it on. This is a good candidate for an install config.

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 25, 2019

openshift-ci-robot requested review from cgwalters and kikisdeliveryservice June 25, 2019 12:29

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 25, 2019

ashcrow reviewed Jun 25, 2019

View reviewed changes

ashcrow approved these changes Jun 25, 2019

View reviewed changes

cgwalters reviewed Jun 25, 2019

View reviewed changes

openshift-ci-robot assigned ericavonb Jun 25, 2019

ericavonb reviewed Jun 25, 2019

View reviewed changes

pkg/daemon: support FIPS

eb9186b

Co-authored-by: Colin Walters <[email protected]> Co-authored-by: Antonio Murdaca <[email protected]> Signed-off-by: Antonio Murdaca <[email protected]>

runcom force-pushed the fips-all-the-things branch from 3037ec7 to eb9186b Compare June 25, 2019 17:37

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2019

openshift-merge-robot merged commit c73d4ad into openshift:master Jun 25, 2019

runcom deleted the fips-all-the-things branch June 25, 2019 19:32

cgwalters mentioned this pull request Oct 30, 2019

Add a fips flag to install-config.yaml openshift/installer#2594

Merged

cgwalters mentioned this pull request Nov 7, 2019

Need to enable FIPS mode on the bootstrap node if FIPS mode requested openshift/installer#2634

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/daemon: support FIPS #889

pkg/daemon: support FIPS #889

runcom commented Jun 25, 2019

ashcrow Jun 25, 2019

runcom Jun 25, 2019

ashcrow Jun 25, 2019

runcom Jun 25, 2019

ashcrow left a comment

cgwalters Jun 25, 2019

cgwalters Jun 25, 2019

cgwalters Jun 25, 2019

cgwalters commented Jun 25, 2019

ericavonb Jun 25, 2019

ericavonb Jun 25, 2019

runcom Jun 25, 2019

ericavonb Jun 25, 2019

runcom Jun 25, 2019

ericavonb Jun 25, 2019

runcom Jun 25, 2019

runcom commented Jun 25, 2019

runcom commented Jun 25, 2019

ericavonb commented Jun 25, 2019

openshift-ci-robot commented Jun 25, 2019

openshift-bot commented Jun 25, 2019

openshift-bot commented Jun 25, 2019

kikisdeliveryservice commented Jun 25, 2019

pkg/daemon: support FIPS #889

pkg/daemon: support FIPS #889

Conversation

runcom commented Jun 25, 2019

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

ashcrow left a 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

cgwalters commented Jun 25, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runcom commented Jun 25, 2019

runcom commented Jun 25, 2019

ericavonb commented Jun 25, 2019

openshift-ci-robot commented Jun 25, 2019

openshift-bot commented Jun 25, 2019

openshift-bot commented Jun 25, 2019

kikisdeliveryservice commented Jun 25, 2019