Sentinel-command without nsenter by default

[ckotzbauer]

As long as there's no custom sentinel-command kured expects that the sentinel file (/var/run/reboot-required by default) to be mounted into the container. This would cause the sentinel-command to be executed without nsenter.

close #526

[rptaylor]

@ckotzbauer Great! The kube-api-access projected volume is also read-only BTW, but indeed overlapping volumeMounts would not be good.

Fortunately I think there is a relatively easy way to allow the hostPath to be read-only as a security improvement.
The sentinel path is a single configurable value currently interpreted as one path but we need a way to use it for two different purposes: the path on the host (needs to be configurable) and the path in the container (can be arbitrary), since hostPath can map between different paths.

Moreover I think this can be done entirely in Helm, without further go changes. I can put together a PR to the Helm chart illustrating the idea. It would not require any changes to kured-ds.yaml; is that just an example or for people who want to manually apply static YAML?

[ckotzbauer]

Yeah, you are absolutely right, I thought about the same approach today. We could leave the default sentinel-path in go as is and just map in the helm-chart to an arbitrary path which does not overlap. This would be absolutely feasible, but it would make kustomize-based installations a bit more complicated, as you have to configure the deployment properly.
That's what you mentioned, the kured-ds.yaml is not just an example it is the "trusted source" for all which are not using helm to install kured.

But however, maybe this would be a good improvement and we should just take the risk of making the kustomize-setups a bit more complicated. We would just need to adjust my PR kubereboot/charts#49

[rptaylor]

Actually I noticed the chart already allows arbitrary volume(Mounts) to be defined, so it could be done without any further modifications to the chart at all. This is what I would do in my Helm values:

volumes:
  - hostPath:
    name: sentinel
      path: /opt/whatever/
      type: Directory
      read-only: true
volumeMounts:
  - name: sentinel
    mountPath: /kured

configuration:
  rebootSentinel: /kured/reboot-required
  rebootSentinelCommand: ""

Then I place a sentinel file in /opt/whatever/reboot-required, and kured will look for the hostPath-mapped location /kured/reboot-required.
As long as the empty sentinel command causes kured to look for the file via 'test -f' without nsenter, it will work.

If the Helm chart is expanded via something like kubereboot/charts#49, it could help make the configuration easier, but IMHO it should be done in a way that makes the hostPath read-only by default, since the goal is to improve security. That would require using some Helm path helpers as I mentioned here which would be a bit more complicated: #526 (comment)
Personally I think complexity is better managed in plain data structures (YAML dict of values) rather than by adding logic (more if statements and path parsing in the Helm template) so I would be happier with just defining the volumes via my values. We could write a simple doc or add some comments explaining how to do this.

[ckotzbauer]

The kured-ds.yaml and the helm-chart are updated with a /sentinel read-only mount.

[jackfrancis]

lgtm pending consensus on concerns about renaming the container mount path

[ckotzbauer]

@rptaylor Yes, this is ready to merge. I will hold it back until we know when we do the next minor release (around Kubernetes 1.29.0). Until that we can do patch releases from main without separate branching.

[github-actions]

This PR was automatically considered stale due to lack of activity. Please refresh it and/or join our slack channels to highlight it, before it automatically closes (in 7 days).

[rptaylor]

This will be superseded by #814 right?

[ckotzbauer]

@rptaylor No, they are both needed. This PR is "only" needed to make the check if a reboot is required more secure.

[rptaylor]

Okay I wasn't sure because the commit descriptions in this PR are a subset of those in https://github.com/kubereboot/kured/pull/814/commits. Maybe once that is merged and this is rebased it will be more clear. Thanks!

[jackfrancis]

lgtm

[ckotzbauer]

Superseded by #814