Repository navigation
Conversation
janhoy
marked this pull request as draft
October 29, 2026 02:03
Copilot stopped reviewing on behalf of
janhoy due to an error
October 29, 2026 08:14
Contributor
Author
|
@HoustonPutman This is something I have been wanting to have for a long time. Things should just be secure by default. This is AI generated, but Claude has done lots of verification steps, and I've done three rounds of AI self review. I have not yet got around to reading all the code, thus the draft state. Do you agree with the approach - that the restricted profile is the default, and if user override certain values in a pod/container, they are overlaid the security defaults instead of overwriting. I suppose that in order to nullify a security setting, you'd have to ovveride with I'll add a comment on each of the linked issues and ask them to have a look. |
This was referenced Oct 29, 2026
janhoy
force-pushed
the
restricted-pss-defaults
branch
from
November 5, 2026 23:37
96328b6 to
16c5e20
Compare
janhoy
marked this pull request as ready for review
November 6, 2026 00:13
Set a seccompProfile on the Solr and Prometheus exporter pods, and runAsNonRoot, allowPrivilegeEscalation=false and capabilities.drop=[ALL] on every container and initContainer the operator generates. The busybox-based initContainers also get runAsUser, since that image has no non-root user of its own. podSecurityContext, containerSecurityContext and defaultInitContainerSecurityContext are now merged over these defaults field by field, so one field opts out of one default. This also extends the initContainer securityContext to the TLS initContainers, which it did not reach before. The volume-prep initContainer can no longer chown a backup repository directory, so it checks that the directory is writable and fails with a message naming it. Setting defaultInitContainerSecurityContext.runAsUser to 0 restores the previous behaviour. A unit test evaluates the generated pods against the upstream profile checks, so a new operator container that is not hardened fails the build. Closes apache#873 Closes apache#582
Skip the security context defaults for containers the user supplied through podOptions, matching them by name rather than relying on the operatorContainers map, which a user-supplied container can collide with. Leave allowPrivilegeEscalation unset when the user asked for privileged mode or an added CAP_SYS_ADMIN, since Kubernetes rejects either of those alongside allowPrivilegeEscalation: false. Document that a root-based custom Solr image needs the init container override too, as setup-zk and gen-pkcs12-keystore run the same image.
Send the backup repository rejection to stderr and set FallbackToLogsOnError on the volume-prep container, so the reason shows up in the pod status rather than only in that container's log. Document that capabilities is overridden as a whole rather than per sub-field, with a test pinning that, since an empty block is the only way to keep a container's default capabilities. Note that spec.dataStorage.ephemeral.hostPath breaks the "restricted" profile too, not just the volumes a user adds through podOptions.
The init container override reaching the TLS init containers was the point of moving the hardening to the end of pod assembly, and nothing tested it: the three containers added after assembly had no coverage of a user-supplied securityContext at all. Also cover the exporter container, which only had pod-level assertions; the exporter's TLS-from-secret path, which generates a keystore conversion init container that no fixture reached; a pod that stays compliant when the user sets an unrelated field, which is the common case for the merging behaviour; and the volume prep container with several repositories, where the Solr user must only be added once.
Two of the operator's security context defaults are numeric IDs: fsGroup 8983 on the pod (8080 for the exporter), and runAsUser 8983 on the init containers whose busybox image has no non-root user of its own. OpenShift's restricted SCCs only accept IDs from a per-namespace range and fill them in themselves when unset, and the field-by-field merge gives the user no way to leave a default out. Everything else the operator sets is accepted. The new podOptions.omitDefaultIDs leaves both IDs out, for the SolrCloud and the SolrPrometheusExporter, and the solr chart exposes it. IDs given explicitly are still used, and the generated pods still satisfy the "restricted" profile. The volume-prep check now reports the user and groups it ran as, instead of assuming the default. Fixes apache#466 without platform detection.
Code comments explained user-facing behaviour and the docs repeated themselves. Say what each piece needs for its audience and no more. The readOnlyRootFilesystem note now points at apache#884, which tracks setting it.
janhoy
force-pushed
the
restricted-pss-defaults
branch
from
November 9, 2026 10:17
8ee139a to
5a61978
Compare
Both test harnesses give a SolrCloud a volume backupRepository backed by a hostPath directory created by root. The kubelet applies no fsGroup to hostPath volumes, so uid 8983 cannot write there, and the volume-prep initContainer now fails the pod instead of chowning it as root. The Solr pods never became ready: the smoke test timed out after 30 minutes, and the e2e "Local Directory - Single" backup spec after 4. Prepare the directory in both harnesses, which is what the docs tell users to do for a volume that comes with its own ownership. The smoke test owns its directory inside the Kind node, so it can chown it. The e2e Kind config shares one host directory with every node, so that one is made writable before Kind creates it as root.
janhoy
force-pushed
the
restricted-pss-defaults
branch
from
November 9, 2026 13:44
5a61978 to
70d6f18
Compare
Contributor
|
I do agree with the general approach I think, but this is almost certainly a v1 (not a v0.10) feature, so it can wait for a full review. |
The volume-prep initContainer runs as the Solr user and only checks that each backup repository directory is writable. Each repository is mounted at a subPath, which the kubelet creates owned by root with the mode of the directory above it, so chowning that directory leaves the repository unwritable, volume-prep rejects it, and the Solr pods never start. The smoke test hung on this until the job timed out. Give the smoke test's directory a permissive mode instead, as the e2e harness already does, and say in the initContainer's error message and in the docs that the owner is not inherited, only the mode.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #873, fixes #582, fixes #466
The pods the operator generates now satisfy the Kubernetes Pod Security Standards "restricted" profile by default, so they run in a namespace that enforces it without any extra configuration. There is no opt-in flag and no platform detection: a platform-conditional default would make the hardened path the least tested one.
Defaults
seccompProfile: RuntimeDefaulton the pod.runAsNonRoot: true,allowPrivilegeEscalation: falseandcapabilities.drop: [ALL]on every container and init container the operator generates. This now includes the TLS init containers, whichdefaultInitContainerSecurityContextnever reached before.runAsUser: 8983on the busybox-based init containers. That image has no non-root user of its own, and the kubelet will not honourrunAsNonRootwithout one.Overrides
podSecurityContext,containerSecurityContextanddefaultInitContainerSecurityContextare merged over the defaults field by field, rather than replacing them. Setting one field overrides one default and leaves the rest in place, so a custom image that must run as root only needsrunAsNonRoot: false.allowPrivilegeEscalation: falseis skipped for a container asked to runprivilegedor to addCAP_SYS_ADMIN, since Kubernetes rejects that combination, and a container asked to run as root gets neitherrunAsNonRootnorcapabilities.drop. Containers the user adds throughpodOptionsare left untouched.Behaviour changes
volume-prepruns as the Solr user now, so it can no longerchowna backup repository directory. It checks that the directory is writable and fails the pod with a message naming it.defaultInitContainerSecurityContext.runAsUser: 0restores the old behaviour.fsGroupwhen given apodSecurityContextthat omits it, as Solr pods always did.OpenShift
The default
restricted-v2SCC accepts everything above except hardcoded IDs: it assignsrunAsUserandfsGroupfrom a per-namespace range and rejects any other value, and the field-by-field merge gives no way to leave a numeric default out. The newpodOptions.omitDefaultIDsleavesfsGroupand the busyboxrunAsUserunset so the platform assigns them, while the rest of the defaults still apply and explicit IDs are still used. The solr chart exposes it. This fixes #466 without the platform detection proposed in #706.Not covered
ZooKeeper pods. A provided ensemble's pods come from the ZooKeeper operator, whose
zookeeperPodPolicyexposes only a pod-levelsecurityContext, and thezookeeper-operatorchart dependency is configured by its own chart.readOnlyRootFilesystemis outside the profile and not defaulted here; #884 tracks setting it on the busybox init containers and as an opt-in for the Solr pods.Testing
A unit test evaluates the generated pod templates against the upstream profile checks, using
k8s.io/pod-security-admissionas a test-only dependency pinned to thek8s.io/apiversion, so a new operator container that is not hardened fails the build. Verified in a kind cluster against Kyverno's full restricted policy set and native Pod Security Admission withenforce=restricted, covering plain, backup-volume, TLS and provided-ZooKeeper SolrClouds, the exporter and the operator itself.omitDefaultIDshas unit tests only, it has not been run on an OpenShift cluster.Builds on the
defaultInitContainerSecurityContextfield added by @alexalex89 in #796.🤖 Written with AI assistance 🤖