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

Configuration option to support deployment with K8s readOnlyRootFilesystem security context #613

Open
Rozzii opened this issue Jan 28, 2025 · 9 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue is ready to be actively worked on.

Comments

@Rozzii
Copy link
Member

Rozzii commented Jan 28, 2025

This proposal is aimed to introduce an optional new way to handle configuration generation at deployment time.
This proposal does not aim to change the default behavior of the ironic-image.

This new feature would be implemented as at least 1 new environment variable that could result in a different way of rendering the different configuration files. The configuration files would be moved to a path designated by the new configuration variable and no file writing operation would take place on the container's own file system.

Requirements for users:

  • The path specified by the new environment variable has to be backed by a mounted volume .

Impact:

  • Most likely files at these locations will be directed to the new custom location:
  • /etc/keepalived/*
  • /var/lib/misc/*
  • /etc/dnsmasq.conf
  • /tmp/ipa.conf
  • /etc/ironic/*
  • /bin/*
  • /usr/local/lib/python*
  • /var/lib/ironic/*
  • /etc/apache2/*

Unknowns

  • There is a possibility that more than one new environment variable needs to be introduced as there might be certain executables that wouldn't allow custom paths for config files (this needs to be checked and solved accordingly)

The impact has to be considered in the context of a K8s pod so because of the "run scripts" of Ironic the impact on a individual container level will be smaller but if all the effected paths of all the possible Ironic container deployments are congregated we will have the above described impact.

Non goals

  • Changing the default behavior of configuration handling
  • Deprecating any existing features
  • Introducing new dependencies
  • Making the new way of deployment the a recommended or defacto way of using ironic

Additional goals discovered during issue discussion

  • Move some of the config files to sane default places.
@metal3-io-bot metal3-io-bot added the needs-triage Indicates an issue lacks a `triage/foo` label and requires one. label Jan 28, 2025
@Rozzii Rozzii self-assigned this Jan 28, 2025
@Rozzii Rozzii added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 28, 2025
@Rozzii Rozzii moved this from Backlog to Ironic-image WIP in Metal3 - Roadmap Jan 28, 2025
@Rozzii Rozzii added this to the ironic-image - v28.0 milestone Jan 28, 2025
@tuminoid
Copy link
Member

First thing is to make each file mountable, and then the second thing is to make it either default or create a kustomization to actually run ironic-image with read-only filesystem. This then required adding some emptyDir mounts so the config files are not persisted, as they're not persisted now either.

  • /bin/*

This is going to be annoying as in /bin we have liveness and readiness probes, whose path is defined in BMO manifests.

  • /usr/local/lib/python*

This is pretty weird one as well. Why are we writing here?

  • /etc/dnsmasq.conf
  • /tmp/ipa.conf

These two are the most important ones to move into subdirectories. The other config files already are.

@Rozzii
Copy link
Member Author

Rozzii commented Jan 28, 2025

First thing is to make each file mountable, and then the second thing is to make it either default or create a kustomization to actually run ironic-image with read-only filesystem. This then required adding some emptyDir mounts so the config files are not persisted, as they're not persisted now either.

  • /bin/*

This is going to be annoying as in /bin we have liveness and readiness probes, whose path is defined in BMO manifests.

  • /usr/local/lib/python*

This is pretty weird one as well. Why are we writing here?

  • /etc/dnsmasq.conf
  • /tmp/ipa.conf

These two are the most important ones to move into subdirectories. The other config files already are.

So dividing the response here to a few parts;

  • I agree that it would be reasonable to move the defaults also to more reasonable places, it can be done as part of this work so I don't object.
  • As we have discussed ofline I would like to do it with the least amount of mounts so I aim to put the feature behind "feature gate" so if no custom location is specified nothing would happen thus we would not break the BMO ironic manifests or IrSO, I don't want to prioritize making the default "sane" locations mountable but as a side effect I can do that too but from K8s user perspective the less mounts the user have to deal with the better the UX is.

@tuminoid tuminoid changed the title Configuration option to support deployment with K8s readOnlyRootFilesystem securit context Configuration option to support deployment with K8s readOnlyRootFilesystem security context Jan 29, 2025
@dtantsur
Copy link
Member

dtantsur commented Feb 5, 2025

I think the goal is worth the effort, but I have reservations about adding a new non-default mode of operation. Especially in the bash-based ironic-image, it's going to increase the maintenance burden. How is the new mode going to be tested? If it's something that you only use downstream, we're going to regress eventually.

@Rozzii
Copy link
Member Author

Rozzii commented Feb 11, 2025

I think the goal is worth the effort, but I have reservations about adding a new non-default mode of operation. Especially in the bash-based ironic-image, it's going to increase the maintenance burden. How is the new mode going to be tested? If it's something that you only use downstream, we're going to regress eventually.

The only reason I have decided to introduce the feature as a non default option was to not overburden the community and as other communities e.g. Ironic is also using this image I wanted to avoid backward incompatibility.

From my POV I have no objection making the new mode of operation the default or the only one.

But before I would make this new feature a default I would really like to have a community consensus (fine if it is a lazy one) on how to roll this out. I would prefer to introduce it first as optional then e.g. 1 release later it would become the default then yet an other release later it would be the only way to manage config files.

Also after our discussion about @lentzi90 's old proposal , I think the two proposals compliment each other very nicely and I could implement that as a next step also.

@tuminoid
Copy link
Member

I think the goal is worth the effort, but I have reservations about adding a new non-default mode of operation. Especially in the bash-based ironic-image, it's going to increase the maintenance burden. How is the new mode going to be tested? If it's something that you only use downstream, we're going to regress eventually.

The only reason I have decided to introduce the feature as a non default option was to not overburden the community and as other communities e.g. Ironic is also using this image I wanted to avoid backward incompatibility.

From my POV I have no objection making the new mode of operation the default or the only one.

But before I would make this new feature a default I would really like to have a community consensus (fine if it is a lazy one) on how to roll this out.

My proposal would be placing template (source) files in directories where they can be left read-only and always, regardless of target directories are mounts or not, write them into target directories. It is backwards compatible way and allow the user flexibility of using it or not using it. (Disclaimer: I did not check yet what we're actually modifying in paths like /usr/local/bin/python*).

For example:

  • dnsmasq.conf would be /etc/dnsmasq.conf.template and output file would be /etc/dnsmasq/dnsmasq.conf. Here the /etc/dnsmasq can be either a mount or just a plain directory, we don't care (in Ironic-image that is, manifests that enable read-only FS are in IRSO or BMO).
  • dnsmasq launch script would always say dnsmasq <options> /etc/dnsmasq/dnsmasq.conf

This way we:

  • actually make template file use explicit, and not just "randomly" sed config files
  • target directories are standard config dirs, so files are still easy to find
  • user has flexibility to configure the read-only FS option in their manifests, and if they do, they need in same place also define the documented directories to be mounted (emptyDir is obvious choice as these files should not persist).
  • we can provide kustomize component for this hardening
  • we can run the e2e tests with this hardening on, to ensure it does not break

@Rozzii
Copy link
Member Author

Rozzii commented Feb 11, 2025

I think the goal is worth the effort, but I have reservations about adding a new non-default mode of operation. Especially in the bash-based ironic-image, it's going to increase the maintenance burden. How is the new mode going to be tested? If it's something that you only use downstream, we're going to regress eventually.

The only reason I have decided to introduce the feature as a non default option was to not overburden the community and as other communities e.g. Ironic is also using this image I wanted to avoid backward incompatibility.
From my POV I have no objection making the new mode of operation the default or the only one.
But before I would make this new feature a default I would really like to have a community consensus (fine if it is a lazy one) on how to roll this out.

My proposal would be placing template (source) files in directories where they can be left read-only and always, regardless of target directories are mounts or not, write them into target directories. It is backwards compatible way and allow the user flexibility of using it or not using it. (Disclaimer: I did not check yet what we're actually modifying in paths like /usr/local/bin/python*).

For example:

* `dnsmasq.conf` would be `/etc/dnsmasq.conf.template` and output file would be `/etc/dnsmasq/dnsmasq.conf`. Here the `/etc/dnsmasq` can be either a mount or just a plain directory, we don't care (in Ironic-image that is, manifests that enable read-only FS are in IRSO or BMO).

* dnsmasq launch script would always say `dnsmasq <options> /etc/dnsmasq/dnsmasq.conf`

This way we:

* actually make template file use explicit, and not just "randomly" sed config files

* target directories are standard config dirs, so files are still easy to find

* user has flexibility to configure the read-only FS option in their manifests, and if they do, they need in same place also define the documented directories to be mounted (`emptyDir` is obvious choice as these files should not persist).

* we can provide kustomize component for this hardening

* we can run the e2e tests with this hardening on, to ensure it does not break

This sounds good but based on my test we can get away with 1 directory for all the config files, and I would advocate for 1 location for all the config files with 1 mounted volume backing it .

So I think I can incorporate the fix paths but not in /etc or such places but under e.g. /mnt/cfg for example. Maybe even /etc/ironic-image-cfg/ would work I have to test how does this security context react if I mount something "under" the read only root file system so like under /etc/something if the /etc/somthing is already present.

Anyhow I already have code I am testing it now, I think it will be easier to discuss this when I have code example to show.

@tuminoid
Copy link
Member

This sounds good but based on my test we can get away with 1 directory for all the config files, and I would advocate for 1 location for all the config files with 1 mounted volume backing it .

So I think I can incorporate the fix paths but not in /etc or such places but under e.g. /mnt/cfg for example

I mean it is a choice and we just need to decide what we want to prioritize.

@lentzi90
Copy link
Member

I would also favor making this the default/only option. It is a best practice for a reason so I don't see why we would avoid it.

Maybe even /etc/ironic-image-cfg/ would work I have to test how does this security context react if I mount something "under" the read only root file system so like under /etc/something that if the /etc/somthing is already present.

This should work well and is a common practice. I don't have a strong preference if we put it separate (e.g. /config) or mixed in under /etc.

@Rozzii
Copy link
Member Author

Rozzii commented Feb 19, 2025

/triage accepted
Community had no objections in the general need for this feature, we are just discussing the exact way to do it and how to roll it out to not cause a lot of compatibility issues.
I am working on this.

@metal3-io-bot metal3-io-bot added triage/accepted Indicates an issue is ready to be actively worked on. and removed needs-triage Indicates an issue lacks a `triage/foo` label and requires one. labels Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue is ready to be actively worked on.
Projects
Status: Ironic-image WIP
Development

No branches or pull requests

5 participants