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.

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

daemon/update.go: fix FIPS check and remove day 2 support #1252

Merged
merged 1 commit into from Nov 8, 2019

Conversation

@yuqi-zhang
Copy link
Contributor

yuqi-zhang commented Nov 7, 2019

We no longer carry coreos-fips script in RHCOS, so that check fill
fail regardless. As a temporary fix, check state of node via
cat /proc/sys/crypto/fips_enabled instead.

Fixes: #1250

pkg/daemon/update.go Outdated Show resolved Hide resolved
@nee1esh

This comment has been minimized.

Copy link

nee1esh commented Nov 7, 2019

/retest

@yuqi-zhang yuqi-zhang force-pushed the yuqi-zhang:fips-fixes branch from 3ff1125 to fa3c675 Nov 7, 2019
@yuqi-zhang yuqi-zhang changed the title WIP: monkey patch MCD fips daemon/update.go: fix FIPS check and remove day 2 support Nov 7, 2019
@ashcrow
ashcrow approved these changes Nov 7, 2019
Copy link
Member

ashcrow left a comment

👍

@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Nov 7, 2019

/override ci/prow/e2e-aws-scaleup-rhel7

@openshift-ci-robot

This comment has been minimized.

Copy link

openshift-ci-robot commented Nov 7, 2019

@ashcrow: Overrode contexts on behalf of ashcrow: ci/prow/e2e-aws-scaleup-rhel7

In response to this:

/override ci/prow/e2e-aws-scaleup-rhel7

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/test-infra repository.

@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Nov 7, 2019

verify failure:

pkg/daemon/update.go:331: File is not `goimports`-ed (goimports)
		return errors.Wrapf(err, "Error parsing FIPS file at %s", fipsFile,)
pkg/daemon/update.go:339:21: error strings should not be capitalized or end with punctuation or a newline (golint)
		return errors.New("Refusing to update FIPS mode. This is not a supported operation.")
		                  ^
return errors.Wrapf(err, "%s FIPS: %s", arg, string(out))
nodeFIPS, err := strconv.ParseBool(string(content))
if err != nil {
return errors.Wrapf(err, "Error parsing FIPS file at %s", fipsFile,)

This comment has been minimized.

Copy link
@jlebon

jlebon Nov 7, 2019

Member

Extra comma here?

This comment has been minimized.

Copy link
@yuqi-zhang

yuqi-zhang Nov 7, 2019

Author Contributor

Whoops, fixing

return nil
}

if current.Spec.FIPS != desired.Spec.FIPS {

This comment has been minimized.

Copy link
@jlebon

jlebon Nov 7, 2019

Member

Hmm, should we move this check higher up?

This comment has been minimized.

Copy link
@yuqi-zhang

yuqi-zhang Nov 7, 2019

Author Contributor

Hm so we ran into this error initially because we detected that the current current.Spec.FIPS != desired.Spec.FIPS right? Because it was nil?

This comment has been minimized.

Copy link
@yuqi-zhang

yuqi-zhang Nov 7, 2019

Author Contributor

As in, maybe I should change this just to return an error if !desired.Spec.FIPS == nodeFIPS

This comment has been minimized.

Copy link
@yuqi-zhang

yuqi-zhang Nov 7, 2019

Author Contributor

And flat out ignore what current.Spec.FIPS is

This comment has been minimized.

Copy link
@yuqi-zhang

yuqi-zhang Nov 7, 2019

Author Contributor

but then the Spec would not be reflective of the state of the system, perhaps I should update that as well just above?

This comment has been minimized.

Copy link
@yuqi-zhang

yuqi-zhang Nov 7, 2019

Author Contributor

I changed it to update current.Spec.FIPS so we don't get stuck in a reboot loop. This should definitely be improved of course but I think this can get us past the errors (not tested yet)

// fipsCommand is the command to use when enabling or disabling FIPS
fipsCommand = "/usr/libexec/rhcos-tools/coreos-fips"
// fipsFile is the file to check if FIPS is enabled
fipsFile = "/proc/sys/crypto/fips_enabled"

This comment has been minimized.

Copy link
@ericavonb

ericavonb Nov 7, 2019

Contributor

@cgwalters didn't approve of this method (should use the coreos-fips --status or whatever command): https://github.com/openshift/machine-config-operator/pull/1013/files#r307428010

This comment has been minimized.

Copy link
@yuqi-zhang

yuqi-zhang Nov 7, 2019

Author Contributor

Yeah we removed that binary unfortunately, which is what lead to this

This comment has been minimized.

Copy link
@ashcrow

ashcrow Nov 7, 2019

Member

fipscheck

This comment has been minimized.

Copy link
@jlebon

jlebon Nov 7, 2019

Member

See #1250 (comment). That said, checking for /proc/sys/crypto/fips_enabled is a good start.

We don't want to use coreos-fips anymore. RHCOS has embedded support for turning on FIPS now.

This comment has been minimized.

Copy link
@ashcrow

ashcrow Nov 7, 2019

Member

I think using the proc path above is fine. fipscheck is an alternate possibility. coreos-fips has been removed.

@yuqi-zhang yuqi-zhang force-pushed the yuqi-zhang:fips-fixes branch from fa3c675 to 1b679bb Nov 7, 2019
@yuqi-zhang

This comment has been minimized.

Copy link
Contributor Author

yuqi-zhang commented Nov 7, 2019

@jlebon

This comment has been minimized.

Copy link
Member

jlebon commented Nov 7, 2019

/approve

This looks sane to me!

@yuqi-zhang

This comment has been minimized.

Copy link
Contributor Author

yuqi-zhang commented Nov 7, 2019

/retest

@yuqi-zhang yuqi-zhang force-pushed the yuqi-zhang:fips-fixes branch from 1b679bb to 0be6403 Nov 7, 2019
@yuqi-zhang

This comment has been minimized.

Copy link
Contributor Author

yuqi-zhang commented Nov 7, 2019

Minor update: Instead of just returning nil, if FIPS changes were detected, do:
return errors.New("Detected change to FIPS flag. Refusing to modify FIPS on a running cluster.")
instead

@yuqi-zhang yuqi-zhang force-pushed the yuqi-zhang:fips-fixes branch from 0be6403 to 542f116 Nov 7, 2019
@cgwalters

This comment has been minimized.

Copy link
Contributor

cgwalters commented Nov 8, 2019

/approve

@sinnykumari

This comment has been minimized.

Copy link
Contributor

sinnykumari commented Nov 8, 2019

/retest

@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Nov 8, 2019

/override ci/prow/e2e-aws-scaleup-rhel7

@openshift-ci-robot

This comment has been minimized.

Copy link

openshift-ci-robot commented Nov 8, 2019

@ashcrow: Overrode contexts on behalf of ashcrow: ci/prow/e2e-aws-scaleup-rhel7

In response to this:

/override ci/prow/e2e-aws-scaleup-rhel7

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/test-infra repository.

@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Nov 8, 2019

/retest

@yuqi-zhang

This comment has been minimized.

Copy link
Contributor Author

yuqi-zhang commented Nov 8, 2019

/hold

Found an error while testing, investigating

We no longer carry coreos-fips script in RHCOS, so we can no longer
update FIPS mode via Machineconfig. For now, let us instead check
whether FIPS is expected via /proc/sys/crypto/fips_enabled.

Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>
@yuqi-zhang yuqi-zhang force-pushed the yuqi-zhang:fips-fixes branch from 542f116 to 9075a83 Nov 8, 2019
@yuqi-zhang

This comment has been minimized.

Copy link
Contributor Author

yuqi-zhang commented Nov 8, 2019

/hold cancel

Can confirm this should work in existing clusters, as well as properly catch and reject day 2 fips applications as we expect. Have not tested with day 1 pending RHCOS updates

@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Nov 8, 2019

Have not tested with day 1 pending RHCOS updates

Understood. I don't think an issue will occur but if one does the fix can be done in a different PR.

/lgtm

@openshift-ci-robot

This comment has been minimized.

Copy link

openshift-ci-robot commented Nov 8, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashcrow, cgwalters, jlebon, yuqi-zhang

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:

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

@openshift-ci-robot

This comment has been minimized.

Copy link

openshift-ci-robot commented Nov 8, 2019

@yuqi-zhang: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-aws-scaleup-rhel7 9075a83 link /test e2e-aws-scaleup-rhel7

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Nov 8, 2019

/override ci/prow/e2e-aws-scaleup-rhel7

@openshift-ci-robot

This comment has been minimized.

Copy link

openshift-ci-robot commented Nov 8, 2019

@ashcrow: Overrode contexts on behalf of ashcrow: ci/prow/e2e-aws-scaleup-rhel7

In response to this:

/override ci/prow/e2e-aws-scaleup-rhel7

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/test-infra repository.

@cgwalters

This comment has been minimized.

Copy link
Contributor

cgwalters commented Nov 8, 2019

@ashcrow FWIW that context is non-blocking, you don't need to override it.

@openshift-merge-robot openshift-merge-robot merged commit 0cffa48 into openshift:master Nov 8, 2019
8 checks passed
8 checks passed
ci/prow/e2e-aws Job succeeded.
Details
ci/prow/e2e-aws-scaleup-rhel7 Overridden by ashcrow
Details
ci/prow/e2e-gcp-op Job succeeded.
Details
ci/prow/e2e-gcp-upgrade Job succeeded.
Details
ci/prow/images Job succeeded.
Details
ci/prow/unit Job succeeded.
Details
ci/prow/verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.