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

Run the virtiofsd container as non-root for config volumes #9609

Merged
merged 5 commits into from
Apr 27, 2023
Merged

Run the virtiofsd container as non-root for config volumes #9609

merged 5 commits into from
Apr 27, 2023

Conversation

germag
Copy link
Contributor

@germag germag commented Apr 13, 2023 •
edited

What this PR does / why we need it:

Currently, we can share configuration volumes, such as ConfigMaps, Secrets, DownwardAPI and ServiceAccount using virtiofs, but the virtiofs container requires to be run as root.

Virtiofsd supports being run without privileges, so this PR to modify the container to run as the unprivileged user when sharing configuration volumes.

Reported-by: Javier Cano Cano jcanocan@redhat.com

Tested-by: Javier Cano Cano jcanocan@redhat.com

Related #7735

Release note:

Add support for running virtiofsd in an unprivileged container when sharing configuration volumes.

@germag
Fix typo
bc973f7 
Rename a function name to match the required style.

Signed-off-by: German Maglione <gmaglione@redhat.com>
@germag
Refactor variable name
6e4620e 
Let's rename the variable that holds the uid/gid of the virtiofsd
privileged container.

Signed-off-by: German Maglione <gmaglione@redhat.com>
@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/M needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 13, 2023
@kubevirt-bot
Copy link
Contributor

Hi @germag. Thanks for your PR.

I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@germag
Copy link
Contributor Author

germag commented Apr 13, 2023

/cc @jcanocan

@xpivarc
Copy link
Member

xpivarc commented Apr 13, 2023

/ok-to-test
@germag We should onboard you to community.

@kubevirt-bot kubevirt-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 13, 2023
@germag
Add security profile
b5406f7 
This profile will then be used to select with which privileges to run
the virtiofsd container, based on the type of volume to be shared.

Signed-off-by: German Maglione <gmaglione@redhat.com>
@germag
Run virtiofsd as non-root for config volumes
532f469 
It runs the virtiofsd container as root only if the shared volume is
_not_ a config volume.

Signed-off-by: German Maglione <gmaglione@redhat.com>
@germag
Test virtiofs config volumes outside the privileged namespace
a23d66b 
Removes the test privileged namespace from virtiofs config volumes
tests.

Signed-off-by: German Maglione <gmaglione@redhat.com>
@germag
Copy link
Contributor Author

germag commented Apr 13, 2023 •
edited

v2:

  • Add AllowPrivilegeEscalation field to SecurityContext
  • Explicitly drop all capabilities if run as non-root

@jcanocan
Copy link
Contributor

/retest-required

@alicefr
Copy link
Member

alicefr commented Apr 14, 2023

@germag I have a question. IIUC, we are adding the noroot version but still keeping the root configuration option available. However, in the test now we are only testing noroot, do I understand the change correctly?

@germag
Copy link
Contributor Author

germag commented Apr 14, 2023 •
edited

@germag I have a question. IIUC, we are adding the noroot version but still keeping the root configuration option available. However, in the test now we are only testing noroot, do I understand the change correctly?

Yes, but we only keep the root configuration for PVCs(*), and we keep testing those as root tests/virtiofs/datavolume.go. The non-root (for now) it's only for config volumes, but you cannot choose sharing config volumes as root, for that reason we only test those as non-root.

(*) Currently, we can share PVC as non-root, but the functionality is very limited.

@germag
Copy link
Contributor Author

germag commented Apr 14, 2023 •
edited

We should onboard you to community.

Thanks, do I need to do something?

@germag
Copy link
Contributor Author

germag commented Apr 17, 2023

/retest-required

@jcanocan
Copy link
Contributor

Looking awesome for me! Please, add a release note.

@alicefr
Copy link
Member

alicefr commented Apr 17, 2023

/lgtm

@kubevirt-bot kubevirt-bot added 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. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 17, 2023
@alicefr
Copy link
Member

alicefr commented Apr 26, 2023 •
edited

/approve
/hold
If anyone else wants to review it.

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 26, 2023
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alicefr

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 26, 2023
xpivarc
xpivarc reviewed Apr 26, 2023
View reviewed changes
Copy link
Member

@xpivarc xpivarc left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/virt-controller/services/virtiofs.go
privileged
)

func isRestricted(profile securityProfile) bool {
Copy link
Member

Choose a reason for hiding this comment

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

I feel like you could inline these two functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean, replacing it by just profile == restricted?, I did it first, but it felt less clear, specially in:

		RunAsNonRoot:             pointer.Bool(profile == restricted),
		AllowPrivilegeEscalation: pointer.Bool(profile == privileged),

but I don't have a strong opinion about that

@xpivarc
Copy link
Member

xpivarc commented Apr 26, 2023

/hold cancel

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 26, 2023
@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-bot
Copy link
Contributor

@germag: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubevirt-fossa a23d66b link false /test pull-kubevirt-fossa
pull-kubevirt-e2e-k8s-1.25-sig-compute a23d66b link unknown /test pull-kubevirt-e2e-k8s-1.25-sig-compute

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.

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-bot kubevirt-bot merged commit a6ca211 into kubevirt:main Apr 27, 2023
28 of 29 checks passed
@germag germag mentioned this pull request Jul 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xpivarc xpivarc xpivarc left review comments

@kbidarkar kbidarkar Awaiting requested review from kbidarkar

@marceloamaral marceloamaral Awaiting requested review from marceloamaral

@jcanocan jcanocan Awaiting requested review from jcanocan

Assignees

@alicefr alicefr

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. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@germag @kubevirt-bot @xpivarc @jcanocan @alicefr @kubevirt-commenter-bot