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

Run Containers as Non-Root, and without Privilege Escalation by default. #400

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jk464
Copy link
Contributor

@jk464 jk464 commented Feb 13, 2024

In attempt to harden the security of running StackStorm within k8s, this PR makes it such that by default all containers run as a non-root user, generally 1000:1000 (the stanley user) - or for st2web it runs as 100:100 (which is the nginx user).

It also disables the ability of the container to escalate privileges.

Generally in my testing, this all "just" works - Aside from requiring a change to the st2web container to allow nginx to run as the nginx user. (Changes to support that are here StackStorm/st2-dockerfiles#66).

NOTE: This PR depends on StackStorm/st2-dockerfiles#66 to work.

@pull-request-size pull-request-size bot added the size/M PR that changes 30-99 lines. Good size to review. label Feb 13, 2024
Comment on lines +345 to +354
/bin/cp -dR /opt/stackstorm/packs/. /opt/stackstorm/packs-shared &&
/bin/cp -dR /opt/stackstorm/virtualenvs/. /opt/stackstorm/virtualenvs-shared
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-d only preserves symlinks as-is. -a also copied the mode, including execute bits.

We need to make sure to preserve the execute bit on files. Would we have to add --preserve=mode to copy that?

Also, how will this interact with #245 (Switch from cp to rsync with fallback to cp)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you please check out #415? I'd like to come up with the minimal set of flags for cp and rsync.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Stealthii made these changes (which remains me I should credit him as an author in the commits..), I believe this was to fix issues with running as a non-privileged user. @Stealthii can you comment here / have a look at the above PRs :D

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cognifloyd I've commented on #415 with a proposed change that should preserve all relevant attributes besides ownership. #415 (comment)

Once verified and if it becomes part of that PR, we can drop the change here and have this PR focus solely on the enablement of non-root runtimes.

@@ -100,7 +100,7 @@ spec:
{{- end }}
ports:
- protocol: TCP
port: {{ eq (get .Values.st2web.env "ST2WEB_HTTPS" | toString) "1" | ternary 443 80 }}
port: {{ eq (get .Values.st2web.env "ST2WEB_HTTPS" | toString) "1" | ternary 8443 8080 }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need this to be in values to support older docker images.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annoyingly in #401 I also make changes here:

@@ -99,8 +107,10 @@ spec:
   {{- end }}
   {{- end }}
   ports:
-  - protocol: TCP
-    port: {{ eq (get .Values.st2web.env "ST2WEB_HTTPS" | toString) "1" | ternary 443 80 }}
+  - name: st2web
+    protocol: TCP
+    port: {{ if and .Values.st2.tls.enabled .Values.st2web.tls.enabled -}}8443{{- else -}}8080{{- end }}
+    targetPort: {{ if and .Values.st2.tls.enabled .Values.st2web.tls.enabled -}}8443{{- else -}}8080{{- end }}
 
 {{ if .Values.st2chatops.enabled -}}
 ---

It would probably be easier in that PR to make changes to ensure backwards compatability

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I've updated this in both PR to check to see if ((($.Values.st2web.securityContext).runAsUser) is defined and set to a non 0 value (i.e. running as non-root) and if so return the non-privileged ports, otherwise it returns the old privileged ports

Comment on lines +293 to +307
podSecurityContext:
runAsNonRoot: true
securityContext:
runAsUser: 1000
allowPrivilegeEscalation: false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my cluster, I use drop capabilities, re-adding them only for the deployments that need it. So I have:

#podSecurityContext:
securityContext:
  capabilities:
    drop: ["ALL"]
st2actionrunner:
  #podSecurityContext:
  securityContext:
    capabilities:
      drop: ["ALL"]
      add:
        - chown
        - fowner # chmod
        - fsetid
        - setfcap
        - setgid
        - setuid
        - dac_override # allows root to bypass "discretionary access control" (file rwx permission checks)
        - kill
        - net_raw # ping
        - audit_write # sudo
        - setpcap
st2web:
  #podSecurityContext:
  securityContext:
    capabilities:
      drop: ["ALL"]
      add:
        - chown
        - setgid
        - setuid
        - dac_override # allows root to bypass "discretionary access control" (file rwx permission checks)
        - net_bind_service # bind privileged ports 80 or 443

So, that's a slightly different approach than just running as a non-root user. In particular, if st2web is not running as root, and is using unprivileged ports, then the config could be somewhat simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for st2web, the fact I don't bind privileged ports, and also I ensured any required files are chown by nginx in the Docker image removes any need for this capabilities?

For st2actionrunner do we have any documentation that points towards what st2actionrunner needs each of those capabilities for? I'm wondering if we can remove any of them by making changes elsewhere, w/o breaking compatibility ofc.

@cognifloyd cognifloyd added this to the v1.2.0 milestone Apr 13, 2024
@jk464 jk464 force-pushed the feature/non-root branch 2 times, most recently from f7ba491 to 73ed0a2 Compare May 8, 2024 14:19
@pull-request-size pull-request-size bot added size/L PR that changes 100-499 lines. Requires some effort to review. and removed size/M PR that changes 30-99 lines. Good size to review. labels May 8, 2024
@jk464 jk464 requested a review from cognifloyd May 8, 2024 14:28
jk464 and others added 2 commits May 8, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature security size/L PR that changes 100-499 lines. Requires some effort to review. status:feedback needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants