Copilot commented on code in PR #875: URL: https://github.com/apache/solr-operator/pull/875#discussion_r4407868804
########## controllers/util/security_context_util.go: ########## @@ -0,0 +1,164 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package util + +import ( + solr "github.com/apache/solr-operator/api/v1beta1" + corev1 "k8s.io/api/core/v1" + "k8s.io/utils/ptr" +) + +// Names of the containers and init containers that the Solr Operator generates. +const ( + VolumePrepInitContainer = "volume-prep" + SetupZkInitContainer = "setup-zk" + Pkcs12InitContainer = "gen-pkcs12-keystore" + TLSWrapperScriptInitContainer = "create-tls-wrapper-script" + // InitdbInitContainer is defined in solr_tls_util.go +) + +// operatorContainerOptions describes a container that the operator generates. +type operatorContainerOptions struct { + // imageRunsAsRoot is true if the container's image has no non-root user of its own. + // The "restricted" profile requires runAsNonRoot, which the kubelet refuses to honor for such + // an image unless a runAsUser is also given, so these containers get one. + imageRunsAsRoot bool + + // readOnlyRootFilesystem is true if the container is known not to write to its root filesystem. + // This is not part of the "restricted" profile and is not applied to containers that did not + // already have it, see https://github.com/apache/solr-operator/issues/801. + readOnlyRootFilesystem bool +} + +// operatorContainers lists every container and init container that the operator generates, and is +// therefore responsible for hardening. Containers the user supplies through podOptions are not +// listed: their security context is the user's to set. +// +// A container missing from this map gets no security context, which makes the pod fail "restricted" +// evaluation. The unit tests evaluate the generated pods against the profile, so a new operator +// container that is not listed here fails the build. +var operatorContainers = map[string]operatorContainerOptions{ + SolrNodeContainer: {}, + SolrPrometheusExporterContainer: {}, + SetupZkInitContainer: {readOnlyRootFilesystem: true}, + Pkcs12InitContainer: {}, + VolumePrepInitContainer: {imageRunsAsRoot: true}, + InitdbInitContainer: {imageRunsAsRoot: true}, + TLSWrapperScriptInitContainer: {imageRunsAsRoot: true}, +} + +// defaultPodSecurityContext builds the pod-level security context, starting from the one the user +// provided, if any, and filling in every field they left unset. User-provided fields always win. +// +// The "restricted" profile asks for a seccomp profile, which is set once here rather than on every +// container. fsGroup is not part of the profile, it is a long-standing operator default. +func defaultPodSecurityContext(userProvided *corev1.PodSecurityContext, defaultFSGroup int64) *corev1.PodSecurityContext { + securityContext := &corev1.PodSecurityContext{} + if userProvided != nil { + securityContext = userProvided.DeepCopy() + } + + if securityContext.SeccompProfile == nil { + securityContext.SeccompProfile = &corev1.SeccompProfile{Type: corev1.SeccompProfileTypeRuntimeDefault} + } + if securityContext.FSGroup == nil { + securityContext.FSGroup = ptr.To(defaultFSGroup) + } + + return securityContext +} + +// defaultContainerSecurityContext builds the security context for one operator-generated container, +// starting from the one the user provided, if any, and filling in every field they left unset. +// User-provided fields always win, so a single field is enough to opt out of a single default. +// +// podRunAsUser is the pod-level runAsUser, which containers inherit when they do not set their own. +func defaultContainerSecurityContext(userProvided *corev1.SecurityContext, options operatorContainerOptions, podRunAsUser *int64) *corev1.SecurityContext { + securityContext := &corev1.SecurityContext{} + if userProvided != nil { + securityContext = userProvided.DeepCopy() + } + + // The user can ask for a container to run as root, which some setups need (see the backup + // repository preparation in generateSolrSetupInitContainers). Such a container cannot also run + // as non-root, and dropping all capabilities would leave it unable to do the work root was + // asked for, so neither default is applied to it. + effectiveRunAsUser := securityContext.RunAsUser + if effectiveRunAsUser == nil { + effectiveRunAsUser = podRunAsUser + } + runsAsRoot := effectiveRunAsUser != nil && *effectiveRunAsUser == 0 + + if securityContext.AllowPrivilegeEscalation == nil { + securityContext.AllowPrivilegeEscalation = ptr.To(false) + } Review Comment: When a user supplies `containerSecurityContext.privileged: true` (or the equivalent init-container override) without `allowPrivilegeEscalation`, this adds `allowPrivilegeEscalation: false`. Kubernetes rejects that combination, so a previously valid override now prevents the StatefulSet or Deployment from being created, even outside a restricted namespace. Please leave this default unset when privileged mode is explicitly requested, while preserving an explicitly supplied value. ########## controllers/util/security_context_util.go: ########## @@ -0,0 +1,164 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package util + +import ( + solr "github.com/apache/solr-operator/api/v1beta1" + corev1 "k8s.io/api/core/v1" + "k8s.io/utils/ptr" +) + +// Names of the containers and init containers that the Solr Operator generates. +const ( + VolumePrepInitContainer = "volume-prep" + SetupZkInitContainer = "setup-zk" + Pkcs12InitContainer = "gen-pkcs12-keystore" + TLSWrapperScriptInitContainer = "create-tls-wrapper-script" + // InitdbInitContainer is defined in solr_tls_util.go +) + +// operatorContainerOptions describes a container that the operator generates. +type operatorContainerOptions struct { + // imageRunsAsRoot is true if the container's image has no non-root user of its own. + // The "restricted" profile requires runAsNonRoot, which the kubelet refuses to honor for such + // an image unless a runAsUser is also given, so these containers get one. + imageRunsAsRoot bool + + // readOnlyRootFilesystem is true if the container is known not to write to its root filesystem. + // This is not part of the "restricted" profile and is not applied to containers that did not + // already have it, see https://github.com/apache/solr-operator/issues/801. + readOnlyRootFilesystem bool +} + +// operatorContainers lists every container and init container that the operator generates, and is +// therefore responsible for hardening. Containers the user supplies through podOptions are not +// listed: their security context is the user's to set. +// +// A container missing from this map gets no security context, which makes the pod fail "restricted" +// evaluation. The unit tests evaluate the generated pods against the profile, so a new operator +// container that is not listed here fails the build. +var operatorContainers = map[string]operatorContainerOptions{ + SolrNodeContainer: {}, + SolrPrometheusExporterContainer: {}, + SetupZkInitContainer: {readOnlyRootFilesystem: true}, + Pkcs12InitContainer: {}, + VolumePrepInitContainer: {imageRunsAsRoot: true}, + InitdbInitContainer: {imageRunsAsRoot: true}, + TLSWrapperScriptInitContainer: {imageRunsAsRoot: true}, +} + +// defaultPodSecurityContext builds the pod-level security context, starting from the one the user +// provided, if any, and filling in every field they left unset. User-provided fields always win. +// +// The "restricted" profile asks for a seccomp profile, which is set once here rather than on every +// container. fsGroup is not part of the profile, it is a long-standing operator default. +func defaultPodSecurityContext(userProvided *corev1.PodSecurityContext, defaultFSGroup int64) *corev1.PodSecurityContext { + securityContext := &corev1.PodSecurityContext{} + if userProvided != nil { + securityContext = userProvided.DeepCopy() + } + + if securityContext.SeccompProfile == nil { + securityContext.SeccompProfile = &corev1.SeccompProfile{Type: corev1.SeccompProfileTypeRuntimeDefault} + } + if securityContext.FSGroup == nil { + securityContext.FSGroup = ptr.To(defaultFSGroup) + } + + return securityContext +} + +// defaultContainerSecurityContext builds the security context for one operator-generated container, +// starting from the one the user provided, if any, and filling in every field they left unset. +// User-provided fields always win, so a single field is enough to opt out of a single default. +// +// podRunAsUser is the pod-level runAsUser, which containers inherit when they do not set their own. +func defaultContainerSecurityContext(userProvided *corev1.SecurityContext, options operatorContainerOptions, podRunAsUser *int64) *corev1.SecurityContext { + securityContext := &corev1.SecurityContext{} + if userProvided != nil { + securityContext = userProvided.DeepCopy() + } + + // The user can ask for a container to run as root, which some setups need (see the backup + // repository preparation in generateSolrSetupInitContainers). Such a container cannot also run + // as non-root, and dropping all capabilities would leave it unable to do the work root was + // asked for, so neither default is applied to it. + effectiveRunAsUser := securityContext.RunAsUser + if effectiveRunAsUser == nil { + effectiveRunAsUser = podRunAsUser + } + runsAsRoot := effectiveRunAsUser != nil && *effectiveRunAsUser == 0 + + if securityContext.AllowPrivilegeEscalation == nil { + securityContext.AllowPrivilegeEscalation = ptr.To(false) + } + if !runsAsRoot { + if securityContext.RunAsNonRoot == nil { + securityContext.RunAsNonRoot = ptr.To(true) + } + if securityContext.Capabilities == nil { + securityContext.Capabilities = &corev1.Capabilities{Drop: []corev1.Capability{"ALL"}} + } + if options.imageRunsAsRoot && effectiveRunAsUser == nil { + securityContext.RunAsUser = ptr.To(int64(DefaultSolrUser)) + } + } + if options.readOnlyRootFilesystem && securityContext.ReadOnlyRootFilesystem == nil { + securityContext.ReadOnlyRootFilesystem = ptr.To(true) + } + + return securityContext +} + +// setSecurityContextOnOperatorContainers sets the security context on every container and init +// container that the operator generated for the given pod, leaving user-supplied containers alone. +// +// Call this once the pod spec is otherwise complete, so that containers added along the way (for +// TLS, for example) are covered too. +func setSecurityContextOnOperatorContainers(podSpec *corev1.PodSpec, containerSecurityContext *corev1.SecurityContext, initContainerSecurityContext *corev1.SecurityContext) { + var podRunAsUser *int64 + if podSpec.SecurityContext != nil { + podRunAsUser = podSpec.SecurityContext.RunAsUser + } + + for i, container := range podSpec.Containers { + if options, isOperatorContainer := operatorContainers[container.Name]; isOperatorContainer { + podSpec.Containers[i].SecurityContext = defaultContainerSecurityContext(containerSecurityContext, options, podRunAsUser) Review Comment: A name in this global map does not establish that the container was generated by the operator: a user-supplied `solr-prometheus-exporter` sidecar in a SolrCloud has its explicit security context replaced, potentially losing its UID or capabilities and failing to start. The init-container loop at lines 145–146 has the same issue when a custom init container uses the name of an absent operator init container. Track which containers were actually generated for each pod and apply defaults only to those, preserving user-supplied security contexts. ########## docs/modules/solr-cloud/pages/custom-kube-options.adoc: ########## @@ -93,3 +93,50 @@ spec: clientIP: timeoutSeconds: 3600 ---- + +== Pod Security Standards + +The pods that the operator creates satisfy the Kubernetes https://kubernetes.io/docs/concepts/security/pod-security-standards/#restricted[Pod Security Standards "restricted" profile] out of the box, so they can run in a namespace that enforces it without any extra configuration. +This applies to the Solr pods, the Prometheus exporter pods, and every init container the operator generates for them. + +The operator sets a seccomp profile once at the pod level, and the following on each container it generates: + +[source,yaml] +---- +securityContext: + runAsNonRoot: true + allowPrivilegeEscalation: false + capabilities: + drop: + - ALL +---- + +The init containers that use the `busyBoxImage` additionally get `runAsUser: 8983`, because that image has no non-root user of its own and `runAsNonRoot` cannot be honoured without an explicit user. + +Three things are outside the operator's control: + +* **Containers you add yourself** through `podOptions.sidecarContainers` and `podOptions.initContainers` keep whatever security context you give them. +Set one on each if the namespace enforces the profile. +* **Volumes you add yourself** must be of a type the profile allows, which rules out `hostPath`. +* **ZooKeeper** is not covered. +A `provided` ensemble's pods are created by the ZooKeeper operator, whose `zookeeperPodPolicy` exposes only a pod-level `securityContext`, so they cannot be made to satisfy the profile from the SolrCloud resource. +The `zookeeper-operator` deployment that the Solr Operator Helm chart installs as a dependency is likewise configured by its own chart. +Use an `external` ZooKeeper ensemble if the namespace enforces the profile. + +=== Overriding the defaults + +`podOptions.podSecurityContext`, `podOptions.containerSecurityContext` and `podOptions.defaultInitContainerSecurityContext` are merged over these defaults field by field, so setting one field overrides one default and leaves the rest in place. +To run a custom Solr image that needs to start as root, for example: + +[source,yaml] +---- +spec: + customSolrKubeOptions: + podOptions: + containerSecurityContext: + runAsNonRoot: false Review Comment: For a custom Solr image that starts as root, opting out only on the main container leaves any generated `setup-zk` or PKCS12 conversion init container with `runAsNonRoot: true`, so the kubelet refuses to start it. Include the init-container override in this example and in `docs/modules/upgrade-notes/pages/upgrade-notes.adoc:158`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
