daemon: Load current node FIPS state on firstboot #3074
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
On firstboot, we synthesize a machineConfig which represents what we *actually see* on the node. We just fill this in with what rpm-ostree says is `osImageURL`. However, FIPS is also special in that it's handled by a special "FIPS boot" before what we think of as firstboot. It cleans up our later comparisons if we have the correct FIPS state in here. (In a layering future, I think what we'd do actually is just move `fips=1` into a kernel argument passed via MachineConfig, then this code wouldn't need to special case it; it'd just be another kernel argument)
cgwalters
force-pushed
the
firstboot-fips
branch
from
07515e3 to
af77748
Compare
| // Also inject the current fips state, which was handled before we run. | ||
| content, err := ioutil.ReadFile(fipsFile) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("Error reading FIPS file at %s: %w", fipsFile, err) |
There was a problem hiding this comment.
checkFIPS has
if os.IsNotExist(err) {
// we just exit cleanly if we're not even on linux
glog.Infof("no %s on this system, skipping FIPS check", fipsFile)
return nil
}
I think it would make more sense if we moved the FIPS code to a getFIPS() function so we handle cases like that consistently. And then maybe instead of my PR just have
if desired.Spec.FIPS != getFIPS() {
return fmt.Errorf(...)
}
There was a problem hiding this comment.
Agree a factored out getFIPS() function would make sense, though I don't understand how we could reach that particular bit since the MCD only runs on Linux. (maybe somehow the unit tests reach it when run from MacOS e.g.?)
|
If |
|
lgtm - happy to make any of the changes I suggested in a followup PR |
|
/retest |
|
@cgwalters: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
@cgwalters this is lower priority than anything we want to demo, but I'd like to rework my PR on top of this one. Do you think you'll want to merge as is or make more changes here? |
|
Well I agree that |
|
I'd say merge this and then maybe I'll factor out I am still curious about whether we want to error if |
|
Factored out |
|
adding a hold as this uses the deprecated errors package removed in: /hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. DetailsIn response to this:
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. |
4 participants
On firstboot, we synthesize a machineConfig which represents
what we actually see on the node. We just fill this in with
what rpm-ostree says is
osImageURL. However, FIPS is alsospecial in that it's handled by a special "FIPS boot" before what
we think of as firstboot.
It cleans up our later comparisons if we have the correct FIPS
state in here.
(In a layering future, I think what we'd do actually is just move
fips=1into a kernel argument passed via MachineConfig, thenthis code wouldn't need to special case it; it'd just be another
kernel argument)