Skip to content

WIP: Add FIPS to MachineConfig #800

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

Closed
cgwalters wants to merge 1 commit into from
Closed

WIP: Add FIPS to MachineConfig #800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions pkg/apis/machineconfiguration.openshift.io/v1/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
// It sorts all the configs in increasing order of their name.
// It uses the Ignition config from first object as base and appends all the rest.
// Kernel arguments are concatenated.
// FIPS uses the last specified value.
Copy link

@ericavonb ericavonb Jun 21, 2019

Choose a reason for hiding this comment

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

I think this will be confusing for users, especially because unset values will decode into false. Like, the following would have FIPS mode OFF:

metadata:
  name: 00_config_controller
spec:
  config: ...
---
metadata:
  name: 99_config_fips
spec:
  FIPS: true
---
metadata:
  name: 99_config_master
spec:
  osImageURL: thelatestversion

Copy link
Member

@runcom runcom Jun 21, 2019

Choose a reason for hiding this comment

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

We might want to go pointer then to differentiate between set/unset as well?

Copy link

@ericavonb ericavonb Jun 21, 2019

Choose a reason for hiding this comment

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

Yeah, probably. Another option would be to fail closed, i.e. if ANY configs have FIPS=true, it will be turned on.

// It uses only the OSImageURL provided by the CVO and ignores any MC provided OSImageURL.
func MergeMachineConfigs(configs []*MachineConfig, osImageURL string) *MachineConfig {
if len(configs) == 0 {
Expand All @@ -24,15 +25,18 @@ func MergeMachineConfigs(configs []*MachineConfig, osImageURL string) *MachineCo
for idx := 1; idx < len(configs); idx++ {
outIgn = ign.Append(outIgn, configs[idx].Spec.Config)
}
fips := false
kargs := []string{}
for _, cfg := range configs {
kargs = append(kargs, cfg.Spec.KernelArguments...)
fips = cfg.Spec.FIPS
}

return &MachineConfig{
Spec: MachineConfigSpec{
OSImageURL: osImageURL,
KernelArguments: kargs,
FIPS: fips,
Config: outIgn,
},
}
Expand Down
2 changes: 2 additions & 0 deletions pkg/apis/machineconfiguration.openshift.io/v1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,8 @@ type MachineConfigSpec struct {
Config igntypes.Config `json:"config"`

KernelArguments []string `json:"kernelArguments"`

FIPS bool `json:"FIPS"`
}

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
Expand Down
20 changes: 20 additions & 0 deletions pkg/daemon/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,26 @@ func (dn *Daemon) updateKernelArguments(oldConfig, newConfig *mcfgv1.MachineConf
return exec.Command("rpm-ostree", args...).Run()
}

// updateFIPS handles changes in FIPS
func (dn *Daemon) updateFIPS(oldConfig, newConfig *mcfgv1.MachineConfig) error {
if oldConfig.Spec.FIPS != newConfig.Spec.FIPS {
Copy link
Member

@runcom runcom Jun 21, 2019

Choose a reason for hiding this comment

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

wait, shouldn't this be ==?

Copy link
Member

@runcom runcom Jun 21, 2019

Choose a reason for hiding this comment

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

I know this is wip, just leaving also a comment that we still miss a call to updateFIPS 💪

return nil
}
if dn.OperatingSystem != machineConfigDaemonOSRHCOS {
return fmt.Errorf("Updating FIPS on non-RHCOS nodes is not supported")
}

arg := "enable"
if !newConfig.Spec.FIPS {
arg = "disable"
}

cmd := "/usr/libexec/rhcos-tools/coreos-fips"
args := []string{arg}
dn.logSystem("Running %s %v", cmd, args)
return exec.Command(cmd, args...).Run()
Copy link
Member

@ashcrow ashcrow May 29, 2019

Choose a reason for hiding this comment

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

This looks like the right command to me. One thing to keep in mind is that, within OCP, the coreos-fips will not default to updating the kernel args and, instead, defer to MCO to own those args.

Copy link
Member Author

@cgwalters cgwalters May 29, 2019

Choose a reason for hiding this comment

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

Let's debate that in openshift/rhcos-tools#2 since...it seems to me doing it that way would make everything unnecessarily more complex.

Copy link
Member

@ashcrow ashcrow May 29, 2019

Choose a reason for hiding this comment

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

I closed the PR which would offload kernel args to the MCO. This call should be good as is.

}

// updateFiles writes files specified by the nodeconfig to disk. it also writes
// systemd units. there is no support for multiple filesystems at this point.
//
Expand Down