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

server: allow containers within a cluster to opt out of FIPS mode #8011

Merged
merged 3 commits into from
Apr 26, 2024
Merged

server: allow containers within a cluster to opt out of FIPS mode #8011

merged 3 commits into from
Apr 26, 2024

Conversation

sohankunkerkar
Copy link
Member

With this change, we can disable FIPS mode in containers for a cluster that is FIPS-enabled.

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

This isn't fully tested; I'm relying on our CI, which can build runc with Go 1.21

Does this PR introduce a user-facing change?

Allow containers within a Kubernetes cluster to opt out of FIPS mode when required

@openshift-ci openshift-ci bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/feature Categorizes issue or PR as related to a new feature. labels Apr 12, 2024
@openshift-ci openshift-ci bot requested review from klihub and QiWang19 April 12, 2024 14:21
@sohankunkerkar sohankunkerkar force-pushed the fix-fips branch 2 times, most recently from 15bb36c to 548fb5e Compare April 12, 2024 14:55
@sohankunkerkar
Copy link
Member Author

/retest

@openshift-ci openshift-ci bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Apr 18, 2024
@openshift-ci openshift-ci bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Apr 18, 2024
@sohankunkerkar sohankunkerkar force-pushed the fix-fips branch 3 times, most recently from 2605555 to b7607ef Compare April 22, 2024 23:17
kolyshkin
kolyshkin reviewed Apr 22, 2024
View reviewed changes
test/pod.bats Outdated Show resolved Hide resolved
kolyshkin
kolyshkin reviewed Apr 22, 2024
View reviewed changes
.golangci.yml Outdated Show resolved Hide resolved
@sohankunkerkar sohankunkerkar force-pushed the fix-fips branch 6 times, most recently from f129540 to c441eab Compare April 23, 2024 15:46
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 23, 2024
kolyshkin
kolyshkin approved these changes Apr 25, 2024
View reviewed changes
Copy link
Collaborator

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

lgtm

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 25, 2024
kwilczynski
kwilczynski reviewed Apr 25, 2024
View reviewed changes
test/pod.bats Outdated Show resolved Hide resolved
kwilczynski
kwilczynski reviewed Apr 25, 2024
View reviewed changes
pkg/config/config.go Outdated Show resolved Hide resolved
kwilczynski
kwilczynski reviewed Apr 25, 2024
View reviewed changes
docs/crio.conf.5.md Outdated Show resolved Hide resolved
@kwilczynski kwilczynski removed the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2024
sohankunkerkar
sohankunkerkar commented Apr 26, 2024
View reviewed changes
server/container_create_linux.go
Comment on lines +956 to +961
ctr.SpecAddMount(rspec.Mount{
Destination: "/proc/sys/crypto/fips_enabled",
Source: fileName,
Type: "bind",
Options: []string{"noexec", "nosuid", "nodev", "ro", "bind"},
})
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if I understood this. Do we need to include/exclude anything from options?

@sohankunkerkar
server: allow containers within a cluster to opt out of FIPS mode whe…
9977160 
…n necessary

Signed-off-by: Sohan Kunkerkar <sohank2602@gmail.com>
@sohankunkerkar
test: add coverage for disabling crypto.fips_enabled when FIPS_DISABL…
81a98de 
…E is set

Signed-off-by: Sohan Kunkerkar <sohank2602@gmail.com>
@sohankunkerkar
contrib/test: skip fips test for kata containers
2e81eed 
Kata containers doesn't support disabling the crypto.fips_enabled
kernel parameter. Attempting to mount /proc/sys/crypto/fips_enabled
within Kata Containers results in an error because the /proc directory
does not allow such operations

Signed-off-by: Sohan Kunkerkar <sohank2602@gmail.com>
@kwilczynski
Copy link
Member

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 26, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Apr 26, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kolyshkin, kwilczynski, sohankunkerkar

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

@kwilczynski
Copy link
Member

/retest

@openshift-merge-bot openshift-merge-bot bot merged commit a3de11a into cri-o:main Apr 26, 2024
57 of 58 checks passed
@sohankunkerkar sohankunkerkar deleted the fix-fips branch April 26, 2024 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kwilczynski kwilczynski kwilczynski left review comments

@kolyshkin kolyshkin kolyshkin approved these changes

@mrunalp mrunalp Awaiting requested review from mrunalp mrunalp is a code owner

@klihub klihub Awaiting requested review from klihub

@QiWang19 QiWang19 Awaiting requested review from QiWang19

Assignees

@kwilczynski kwilczynski

@kolyshkin kolyshkin

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sohankunkerkar @kwilczynski @kolyshkin @openshift-merge-robot