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

Include all securityContext fields in injection template #17318

Closed
rlenglet opened this issue Sep 23, 2019 · 9 comments · Fixed by #17427 or istio/installer#628
Closed

Include all securityContext fields in injection template #17318

rlenglet opened this issue Sep 23, 2019 · 9 comments · Fixed by #17427 or istio/installer#628

Comments

@rlenglet
Copy link
Member

@rlenglet rlenglet commented Sep 23, 2019

Bug description

Istio's sidecar injection webhook rewrites the pod spec to inject the Istio containers, after the pod spec has been rewritten by the PodSecurityPolicy admission controller to match the applied PSP, but also before the PSP validation (see #12231 for more details). In the case the spec injected by Istio would need to be mutated by the PSP admission controller, esp. if the Istio container specs are missing some fields in their securityContext which would be injected by the PSP admission controller, the PSP validation would reject the resulting pod spec, and the pods couldn't be created. In the case of a Deployment, the ReplicaSet description would show an error like:

Error creating: pods "httpgo-796d5fdfbf-xhz2x" is forbidden: unable to validate against any pod security policy: []

This problem concerns only automatic sidecar injection. Manual injection doesn't have this problem, because in this case the istioctl kube-inject is executed before the deployment is created, and therefore before the PSP admission controller mutates the pod spec.

To observe the mutations done by the PSP admission controller (and other controllers):

  1. Enable pod security policy in the cluster, and configure a PSP to be applied on your Deployment.
  2. Inject the Deployment manually with istioctl kube-inject and create it.
  3. Observe the diff between the pod specs of the Deployment and the created Pod.

For instance for an injected istio-proxy container, the diff may look like:

       - args:
         - proxy
         - sidecar
...
         - name: POD_NAME
           valueFrom:
             fieldRef:
+              apiVersion: v1
               fieldPath: metadata.name
         - name: POD_NAMESPACE
           valueFrom:
             fieldRef:
+              apiVersion: v1
               fieldPath: metadata.namespace
         - name: INSTANCE_IP
           valueFrom:
             fieldRef:
+              apiVersion: v1
               fieldPath: status.podIP
         - name: ISTIO_META_POD_NAME
           valueFrom:
             fieldRef:
+              apiVersion: v1
               fieldPath: metadata.name
         - name: ISTIO_META_CONFIG_NAMESPACE
           valueFrom:
             fieldRef:
+              apiVersion: v1
               fieldPath: metadata.namespace
         - name: ISTIO_META_INTERCEPTION_MODE
           value: REDIRECT
...
           httpGet:
             path: /healthz/ready
             port: 15020
+            scheme: HTTP
           initialDelaySeconds: 1
           periodSeconds: 2
+          successThreshold: 1
+          timeoutSeconds: 1
         resources:
           limits:
             cpu: "2"
...
             cpu: 100m
             memory: 128Mi
         securityContext:
+          allowPrivilegeEscalation: false
+          capabilities:
+            drop:
+            - ALL
           readOnlyRootFilesystem: true
           runAsUser: 1337
+        terminationMessagePath: /dev/termination-log
+        terminationMessagePolicy: File
         volumeMounts:
         - mountPath: /etc/istio/proxy
           name: istio-envoy
         - mountPath: /etc/certs/
           name: istio-certs
           readOnly: true
+        - mountPath: /var/run/secrets/kubernetes.io/serviceaccount
+          name: testuser-token-lsxfd
+          readOnly: true
+      dnsPolicy: ClusterFirst
+      enableServiceLinks: true
+      nodeName: ...
+      priority: 0
+      restartPolicy: Always
+      schedulerName: default-scheduler
       securityContext:
         fsGroup: 10001
         runAsGroup: 10001
         runAsUser: 10000
+        supplementalGroups:
+        - 1
+      serviceAccount: testuser
       serviceAccountName: testuser
+      terminationGracePeriodSeconds: 30
+      tolerations:
+      - effect: NoExecute
+        key: node.kubernetes.io/not-ready
+        operator: Exists
+        tolerationSeconds: 300
+      - effect: NoExecute
+        key: node.kubernetes.io/unreachable
+        operator: Exists
+        tolerationSeconds: 300
       volumes:
       - emptyDir:
           medium: Memory
         name: istio-envoy
       - name: istio-certs
         secret:
+          defaultMode: 420
           optional: true
           secretName: istio.testuser

The relevant mutations are in the securityContext of the injected containers, and some annotations:

       annotations:
+        container.apparmor.security.beta.kubernetes.io/httpgo: runtime/default
+        container.apparmor.security.beta.kubernetes.io/istio-proxy: runtime/default
+        kubernetes.io/psp: unprivileged
         readiness.status.sidecar.istio.io/applicationPorts: ""
+        seccomp.security.alpha.kubernetes.io/pod: docker/default
         sidecar.istio.io/status: '{"version":"7b4662597b3e803e9e0c812acba79e8dfca00ce0f570cbae5a5cc18940e19ce2","initContainers":null,"containers":["istio-proxy"],"volumes":["istio-envoy","istio-certs"],"imagePullSecrets":null}'
...
         securityContext:
+          allowPrivilegeEscalation: false
+          capabilities:
+            drop:
+            - ALL
           readOnlyRootFilesystem: true
           runAsUser: 1337

What annotations and fields are injected depends on which PSP the PSP admission controller matched with the pod spec.

Both the security annotations and the fields in securityContext need to be injected by Istio's injection webhook in order for the resulting pod spec to pass PSP validation.

This issue tracks updating Istio's default injection template to include all the securityContext fields in injected containers, even if to set them to default values, so that the PSP admission controller wouldn't need to add / mutate them.

Injecting custom annotations is tracked in #17331.

Affected product area (please put an X in all that apply)

[x] Configuration Infrastructure
[ ] Docs
[ ] Installation
[ ] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure

Version (include the output of istioctl version --remote and kubectl version)

All Istio versions.

Environment where bug was observed (cloud vendor, OS, etc)

Any cluster with pod security policy enabled.

@ssuchter

This comment has been minimized.

Copy link
Contributor

@ssuchter ssuchter commented Sep 24, 2019

Do we have a maintenance problem? Will we need to keep changing Istio as the PSP admission controller has new fields?

Would it be better to figure out a way to invert the PSP admission controller and the Istio sidecar injector webhook? Maybe this is a problem with PSP validation and we should discuss with that project?

@rlenglet

This comment has been minimized.

Copy link
Member Author

@rlenglet rlenglet commented Sep 24, 2019

Do we have a maintenance problem? Will we need to keep changing Istio as the PSP admission controller has new fields?

Adding all fields in the securityContext will be sufficient to prevent further mutations to the securityContext from the PSP admission controller. And I believe it doesn't mutate other fields.

Would it be better to figure out a way to invert the PSP admission controller and the Istio sidecar injector webhook? Maybe this is a problem with PSP validation and we should discuss with that project?

This problem only exists when a webhook injects new containers into the pod spec, which is rather specific to Istio.
The long-term, proper fix will be to stop injecting containers into the pod specs.

@ozevren ozevren removed the area/config label Sep 26, 2019
rlenglet added a commit that referenced this issue Sep 26, 2019
rlenglet added a commit that referenced this issue Sep 27, 2019
rlenglet added a commit that referenced this issue Sep 27, 2019
@howardjohn

This comment has been minimized.

Copy link
Member

@howardjohn howardjohn commented Oct 18, 2019

What is the status on those?

@rlenglet

This comment has been minimized.

Copy link
Member Author

@rlenglet rlenglet commented Oct 18, 2019

It's WIP in #17427.

rlenglet added a commit that referenced this issue Dec 27, 2019
rlenglet added a commit that referenced this issue Dec 27, 2019
rlenglet added a commit that referenced this issue Dec 27, 2019
rlenglet added a commit that referenced this issue Dec 28, 2019
rlenglet added a commit that referenced this issue Dec 28, 2019
rlenglet added a commit to istio/installer that referenced this issue Dec 28, 2019
@rlenglet rlenglet added this to the 1.5 milestone Dec 28, 2019
istio-testing added a commit to istio/installer that referenced this issue Dec 28, 2019
istio-testing added a commit that referenced this issue Dec 28, 2019
* Add all securityContext fields in injected containers

Fixes #17318

* Update injection unit tests
istio-testing added a commit to istio-testing/istio that referenced this issue Dec 28, 2019
istio-testing added a commit that referenced this issue Dec 28, 2019
…19832)

* Add all securityContext fields in injected containers

Fixes #17318

* Update injection unit tests

Co-authored-by: Romain Lenglet <romain.lenglet@berabera.info>
rlenglet added a commit that referenced this issue Dec 28, 2019
rlenglet added a commit to istio/installer that referenced this issue Dec 28, 2019
istio-testing added a commit to istio/installer that referenced this issue Dec 28, 2019
rlenglet added a commit to istio/installer that referenced this issue Dec 28, 2019
istio-testing added a commit that referenced this issue Jan 13, 2020
…19836)

* Add all securityContext fields in injected containers

Fixes #17318

* Update injection unit tests
@myidpt

This comment has been minimized.

Copy link
Contributor

@myidpt myidpt commented Jan 16, 2020

@rlenglet one of our customers is asking this feature to be supported by istioctl and cherry picked in 1.4 release. Can we do that?

@rlenglet

This comment has been minimized.

Copy link
Member Author

@rlenglet rlenglet commented Jan 16, 2020

The installer was patched in time for 1.4.3, but the operator was not updated for that release for some reason.

@michaelbannister

This comment has been minimized.

Copy link

@michaelbannister michaelbannister commented Jan 17, 2020

@rlenglet that's a shame! Is that because of the way that istio/operator, istio/installer and istio/istio were related for 1.4.x releases? (I see that installer & operator have both been subsumed into the main istio repo now.)
(If the istio Slack or Discuss forum would be a better place to ask such things, please say so.)
I'm interested in how they relate as I might want to try and build an updated operator image myself to try this out.

@rlenglet

This comment has been minimized.

Copy link
Member Author

@rlenglet rlenglet commented Jan 17, 2020

Is that because of the way that istio/operator, istio/installer and istio/istio were related for 1.4.x releases?

Yes, commit SHAs have to be explicitly picked up in each release branch. Istio 1.5 won't have that problem. But we still have to update SHAs manually in release-1.3 and release-1.4.
I've done the work in release-1.3 for the upcoming 1.3.7: istio/operator#754, #20256.
Someone else updated the istio/installer SHA in istio/operator:release-1.4 in istio/operator#747, but that operator version is not yet been picked in istio/istio:release-1.4.

@rlenglet

This comment has been minimized.

Copy link
Member Author

@rlenglet rlenglet commented Jan 17, 2020

Fixing this with #20286. This will make it into Istio 1.4.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.