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

make ironic-image runnable as non-root #410

Merged
merged 1 commit into from
Mar 17, 2023

Conversation

tuminoid
Copy link
Member

@tuminoid tuminoid commented Mar 10, 2023

BMO ironic has no reason to run as root. Make it run as "ironic" user.

dnsmasq requires elevated capabiities. k8s is missing the feature of ambient capabilities, so it requires us to setcap the binaries with expected capabilities and container must be running with "allowPrivilegeEscalation: true" in the manifest to allow elevation.

Read the ambient capabilities KEP for more details: https://github.com/kubernetes/enhancements/blob/master/keps/sig-security/2763-ambient-capabilities/README.md

Most of the changed here are making runtime configuration possible by changing file and directory ownership from root:root to root:ironic and applying 775/664 perms to allow modification/creation of files.

@metal3-io-bot metal3-io-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 10, 2023
@tuminoid
Copy link
Member Author

/test-ubuntu-integration-main

To get any benefit from this change, the BMO PR need to be applied, but as standalone, this should be non-invasive and should pass testing. So let's run it for smoke test.

@tuminoid
Copy link
Member Author

Yeah, it passes smoke, nice. Now to figure out the TODOs and how to test in CI.

@tuminoid
Copy link
Member Author

/cc @elfosardo @dtantsur

This is still WIP, but I'd appreciate early comments.

@elfosardo
Copy link
Member

@tuminoid thanks, two things for now:

  • please run centos job as well
  • let's not add too many things to Dockerfile, I'm pretty sure all the changes odne there can be moved to existing scripts

@tuminoid
Copy link
Member Author

@tuminoid thanks, two things for now:

  • please run centos job as well
  • let's not add too many things to Dockerfile, I'm pretty sure all the changes odne there can be moved to existing scripts

The things I’ve changed in/to Dockerfile are there because they need root privileges, and during build we are root. When were running the container, we are nonroot and cannot do any of them.

@elfosardo
Copy link
Member

@tuminoid thanks, two things for now:

  • please run centos job as well
  • let's not add too many things to Dockerfile, I'm pretty sure all the changes odne there can be moved to existing scripts

The things I’ve changed in/to Dockerfile are there because they need root privileges, and during build we are root. When were running the container, we are nonroot and cannot do any of them.

@tuminoid we run scripts in the Dockerfile to avoid that being too long and create too many layers
please follow the same example

tuminoid added a commit to Nordix/metal3-dev-env that referenced this pull request Mar 10, 2023
@tuminoid
Copy link
Member Author

OK, now I got you. Sure, this is just POC at this point, I'll put all the non-root related configuration to single script when I'm done with the FIXME's and TODO's.

@tuminoid
Copy link
Member Author

/test-centos-integration-main

tuminoid added a commit to Nordix/metal3-dev-env that referenced this pull request Mar 10, 2023
tuminoid added a commit to Nordix/metal3-dev-env that referenced this pull request Mar 10, 2023
@tuminoid
Copy link
Member Author

/test-centos-integration-main
/test-ubuntu-integration-main

Combined tests are running at metal3-io/metal3-dev-env#1172 but this should pass standalone as well.

@tuminoid tuminoid force-pushed the tuomo/ironic-nonroot branch 3 times, most recently from b84bc57 to b2ae2fb Compare March 13, 2023 12:07
configure-nonroot.sh Outdated Show resolved Hide resolved
scripts/runhttpd Outdated Show resolved Hide resolved
@dtantsur
Copy link
Member

Tuomo, this is a great idea, but I wonder if it would be easier to exclude dnsmasq initially. It's pretty natural for it to run as root.

/cc @zaneb

Zane, I know you were interested in such a thing.

@tuminoid
Copy link
Member Author

/test-ubuntu-integration-main-ironic-source
/test-centos-integration-main

tuminoid added a commit to Nordix/baremetal-operator that referenced this pull request Mar 15, 2023
BMO ironic has no reason to run as root. Make it run as "ironic" user.

dnsmasq requires elevated capabiities. k8s is missing the feature of
ambient capabilities, so it requires us to setcap the binaries with
expected capabilities and container must be running with
"allowPrivilegeEscalation: true" in the manifest to allow elevation.

Read the ambient capabilities KEP for more details:
https://github.com/kubernetes/enhancements/blob/master/keps/sig-security/2763-ambient-capabilities/README.md

Add securityContext to BMO deployment manifest and keepalived
component, with correct UIDs and GIDs. This is important to be able
to share files via /shared.

Modify keepalived image to run as ironic user, which we use the same
UID and GID as the ironic-image.

This commit requires ironic-image with PR
metal3-io/ironic-image#410 to be merged to
work.
@tuminoid
Copy link
Member Author

@tuminoid the ironic user/group is valid for the ironic processes and configuration, but ironic-inspector uses different user/group, that's where the conflict is for the source configuration you can check the prepare-image.sh script at L22 to L52

OK, I made the users/groups match the rpm install image. I also changed inspector related files to inspector, and on the BMO side made the inspector container run as inspector.

/test-ubuntu-integration-main-ironic-source
/test-centos-integration-main

tuminoid added a commit to Nordix/metal3-dev-env that referenced this pull request Mar 15, 2023
@elfosardo
Copy link
Member

@tuminoid thanks!
I beleive the name of the source CI job changed and I missed it!
I'll ask during the meeting today

@tuminoid
Copy link
Member Author

@tuminoid thanks! I beleive the name of the source CI job changed and I missed it! I'll ask during the meeting today

I found out that they're the regular job names plus -ironic-source postfix.

/test-ubuntu-integration-main-ironic-source
/test-centos-integration-main

tuminoid added a commit to Nordix/metal3-dev-env that referenced this pull request Mar 15, 2023
@furkatgofurov7
Copy link
Member

test-ubuntu-integration-main-ironic-source

this is correct, even though you run ^ , jenkins would not show it with that name in the list of running jobs, rather with usual name test-ubuntu-integration-main but taking into account the postfix specified: https://jenkins.nordix.org/view/Metal3/job/metal3_ironic_image_main_integration_test_ubuntu_ironic_from_source/

Copy link
Member

@zaneb zaneb left a comment

Choose a reason for hiding this comment

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

This is great, thank you 👍

configure-nonroot.sh Show resolved Hide resolved
BMO ironic has no reason to run as root. Make it run as "ironic" user.

dnsmasq requires elevated capabiities. k8s is missing the feature of
ambient capabilities, so it requires us to setcap the binaries with
expected capabilities and container must be running with
"allowPrivilegeEscalation: true" in the manifest to allow elevation.

Read the ambient capabilities KEP for more details:
https://github.com/kubernetes/enhancements/blob/master/keps/sig-security/2763-ambient-capabilities/README.md

Most of the changed here are making runtime configuration possible by
changing file and directory ownership from root:root to root:ironic
and applying 775/664 perms to allow modification/creation of files.
@tuminoid
Copy link
Member Author

/test-ubuntu-integration-main-ironic-source
/test-centos-integration-main

@tuminoid
Copy link
Member Author

OK, tests are passing here as well as metal3-io/metal3-dev-env#1172, and all the comments are addressed.

Can I get LGTM and approval? @zaneb @dtantsur @elfosardo

@dtantsur
Copy link
Member

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 17, 2023
@elfosardo
Copy link
Member

/approve

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elfosardo

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

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 17, 2023
@metal3-io-bot metal3-io-bot merged commit bd1050a into metal3-io:main Mar 17, 2023
tuminoid added a commit to Nordix/baremetal-operator that referenced this pull request Mar 17, 2023
BMO ironic has no reason to run as root. Make it run as "ironic" user.

dnsmasq requires elevated capabiities. k8s is missing the feature of
ambient capabilities, so it requires us to setcap the binaries with
expected capabilities and container must be running with
"allowPrivilegeEscalation: true" in the manifest to allow elevation.

Read the ambient capabilities KEP for more details:
https://github.com/kubernetes/enhancements/blob/master/keps/sig-security/2763-ambient-capabilities/README.md

Add securityContext to BMO deployment manifest and keepalived
component, with correct UIDs and GIDs. This is important to be able
to share files via /shared.

Modify keepalived image to run as ironic user, which we use the same
UID and GID as the ironic-image.

This commit requires ironic-image with PR
metal3-io/ironic-image#410 to be merged to
work.
@tuminoid tuminoid deleted the tuomo/ironic-nonroot branch March 17, 2023 15:23
tuminoid added a commit to Nordix/metal3-dev-env that referenced this pull request Mar 17, 2023
tuminoid added a commit to Nordix/baremetal-operator that referenced this pull request Mar 31, 2023
BMO ironic has no reason to run as root. Make it run as "ironic" user.

dnsmasq requires elevated capabiities. k8s is missing the feature of
ambient capabilities, so it requires us to setcap the binaries with
expected capabilities and container must be running with
"allowPrivilegeEscalation: true" in the manifest to allow elevation.

Read the ambient capabilities KEP for more details:
https://github.com/kubernetes/enhancements/blob/master/keps/sig-security/2763-ambient-capabilities/README.md

Add securityContext to BMO deployment manifest and keepalived
component, with correct UIDs and GIDs. This is important to be able
to share files via /shared.

Modify keepalived image to run as ironic user, which we use the same
UID and GID as the ironic-image.

This commit requires ironic-image with PR
metal3-io/ironic-image#410 to be merged to
work.

This v2 of the PR fixes issues identified after merging 1st PR:
- mariadb was missing securityContext and failed to run
- keepalived changes were not backwards compatible, and due using only
  single tag for all versions, new image broke all release branches
tuminoid added a commit to Nordix/baremetal-operator that referenced this pull request Mar 31, 2023
BMO ironic has no reason to run as root. Make it run as "ironic" user.

dnsmasq requires elevated capabiities. k8s is missing the feature of
ambient capabilities, so it requires us to setcap the binaries with
expected capabilities and container must be running with
"allowPrivilegeEscalation: true" in the manifest to allow elevation.

Read the ambient capabilities KEP for more details:
https://github.com/kubernetes/enhancements/blob/master/keps/sig-security/2763-ambient-capabilities/README.md

Add securityContext to BMO deployment manifest and keepalived
component, with correct UIDs and GIDs. This is important to be able
to share files via /shared.

Modify keepalived image to run as ironic user, which we use the same
UID and GID as the ironic-image.

This commit requires ironic-image with PR
metal3-io/ironic-image#410 to be merged to
work.

This v2 of the PR fixes issues identified after merging 1st PR:
- mariadb was missing securityContext and failed to run
- keepalived changes were not backwards compatible, and due using only
  single tag for all versions, new image broke all release branches
kashifest pushed a commit to Nordix/baremetal-operator that referenced this pull request Apr 5, 2023
BMO ironic has no reason to run as root. Make it run as "ironic" user.

dnsmasq requires elevated capabiities. k8s is missing the feature of
ambient capabilities, so it requires us to setcap the binaries with
expected capabilities and container must be running with
"allowPrivilegeEscalation: true" in the manifest to allow elevation.

Read the ambient capabilities KEP for more details:
https://github.com/kubernetes/enhancements/blob/master/keps/sig-security/2763-ambient-capabilities/README.md

Add securityContext to BMO deployment manifest and keepalived
component, with correct UIDs and GIDs. This is important to be able
to share files via /shared.

Modify keepalived image to run as ironic user, which we use the same
UID and GID as the ironic-image.

This commit requires ironic-image with PR
metal3-io/ironic-image#410 to be merged to
work.
tuminoid added a commit to Nordix/baremetal-operator that referenced this pull request Apr 6, 2023
BMO ironic has no reason to run as root. Make it run as "ironic" user.

dnsmasq requires elevated capabiities. k8s is missing the feature of
ambient capabilities, so it requires us to setcap the binaries with
expected capabilities and container must be running with
"allowPrivilegeEscalation: true" in the manifest to allow elevation.

Read the ambient capabilities KEP for more details:
https://github.com/kubernetes/enhancements/blob/master/keps/sig-security/2763-ambient-capabilities/README.md

Add securityContext to BMO deployment manifest and keepalived
component, with correct UIDs and GIDs. This is important to be able
to share files via /shared.

Modify keepalived image to run as ironic user, which we use the same
UID and GID as the ironic-image.

This commit requires ironic-image with PR
metal3-io/ironic-image#410 to be merged to
work.

This v2 of the PR fixes issues identified after merging 1st PR:
- mariadb was missing securityContext and failed to run
- keepalived changes were not backwards compatible, and due using only
  single tag for all versions, new image broke all release branches
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants