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

Simple security context options (Kubernetes) #2550

Merged
merged 27 commits into from
Nov 26, 2023

Conversation

zc-devs
Copy link
Contributor

@zc-devs zc-devs commented Oct 8, 2023

Part of #2545

Adds security context options: privileged, runAsUser, runAsGroup, runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation.


Pipeline:

steps:
  test:
    image: alpine
    commands:
      - echo Hello from $(whoami)
    backend_options:
      kubernetes:
        securityContext:
          runAsUser: 999

Log:

{"level":"trace","Security context":{"runAsUser":999},"time":"2023-10-08T15:14:39Z","caller":"/src/pipeline/backend/kubernetes/pod.go:200","message":"Security context that will be used for containers"}

Pod:

spec:
  containers:
    - name: wp-01hc7qqcf5q5gqd86yjytb37q3-0-step-0
      image: alpine
      securityContext:
        privileged: false
        runAsUser: 999

Step output:

+ echo Hello from $(whoami)
Hello from
whoami: unknown uid 999

@zc-devs
Copy link
Contributor Author

zc-devs commented Oct 8, 2023

@dominic-p, could you test it and maybe write some docs?

@zc-devs zc-devs force-pushed the 2545-k8s-security-context branch from 3e9089f to d6f06ec Compare October 8, 2023 15:41
@dominic-p
Copy link
Contributor

Thanks for working on this! Yes, I'd be happy to test and write up some docs. Two questions:

  1. Could you point me in the right direction for testing? I'm not a go developer, so I'm not sure what the right approach would be. I'm currently running Woodpecker via the Helm chart. If there's an image published for this PR, I should be able to reference it, but it looks like the CI hasn't run yet.

  2. I didn't see fsGroup listed in the securityContext struct definitions. I believe that will be needed in addition to runAsGroup to make sure the mounted volume gets appropriate permissions.

@qwerty287
Copy link
Contributor

@dominic-p I approved the pipeline to run now.
You should be able to get a docker image from this pr using the tag pull_2550

@codecov-commenter
Copy link

codecov-commenter commented Oct 9, 2023

Codecov Report

Attention: 76 lines in your changes are missing coverage. Please review.

Comparison is base (111a0b4) 34.03% compared to head (ff7b45f) 33.88%.
Report is 3 commits behind head on main.

Files Patch % Lines
pipeline/backend/kubernetes/pod.go 0.00% 51 Missing ⚠️
pipeline/frontend/yaml/compiler/convert.go 47.05% 16 Missing and 2 partials ⚠️
pipeline/backend/kubernetes/kubernetes.go 0.00% 4 Missing ⚠️
server/forge/gitea/gitea.go 0.00% 1 Missing ⚠️
server/forge/github/github.go 0.00% 1 Missing ⚠️
server/forge/gitlab/gitlab.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2550      +/-   ##
==========================================
- Coverage   34.03%   33.88%   -0.16%     
==========================================
  Files         217      217              
  Lines       13825    13890      +65     
==========================================
+ Hits         4706     4707       +1     
- Misses       8746     8809      +63     
- Partials      373      374       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dominic-p
Copy link
Contributor

@qwerty287 thanks for doing that. I was able to test the PR. Here's what I'm seeing:

  1. The securityContext is being set at the container level (i.e. spec.containers[0].securityContext). That may be useful for some circumstances, but I believe a more common approach (and more useful in general) is to set the securityContext at the Pod level (i.e. spec.securityContext). Maybe both could be supported? The YAML could look like this:
    backend_options:
      kubernetes:
        securityContext:
          runAsNonRoot: true
          runAsUser: 1000
          runAsGroup: 1000
          fsGroup: 1000
        containerSecurityContext:
          runAsUser: 1000

That said, my vote would be to just support setting the Pod level securityContext since these Pods will typically (always?) have just one container.

  1. We're still missing the fsGroup setting which makes sense because I don't believe that's supported at the container level. But, if we switch to the Pod level fsGroup should also be added as a supported option.

@pat-s
Copy link
Contributor

pat-s commented Oct 9, 2023

I wonder how much we want to bake into the backend_options or actually port these options to the helm chart?
Most of the options in these PR are usually set for all pods of an instance and shouldn't be necessary to define on the step level?

@zc-devs
Copy link
Contributor Author

zc-devs commented Oct 9, 2023

@dominic-p, you're right 1, 2. I did it yesterday, but faced issue with stucking pipeline. I'm figuring it out now.

I made one security context:

type SecurityContext struct {
	// Pod Security Context
	RunAsUser          *int64  `yaml:"runAsUser,omitempty"`
	RunAsGroup         *int64  `yaml:"runAsGroup,omitempty"`
	RunAsNonRoot       *bool   `yaml:"runAsNonRoot,omitempty"`
	SupplementalGroups []int64 `yaml:"supplementalGroups,omitempty"`
	FSGroup            *int64  `yaml:"fsGroup,omitempty"`
	// Container Security Context
	Privileged               *bool `yaml:"privileged,omitempty"`
	ReadOnlyRootFilesystem   *bool `yaml:"readOnlyRootFilesystem,omitempty"`
	AllowPrivilegeEscalation *bool `yaml:"allowPrivilegeEscalation,omitempty"`
}

@pat-s, why should it be ported to the helm chart?

Most of the options are usually set for all pods of an instance and shouldn't be necessary to define on the step level?

Do you mean, I should add WOODPECKER_BACKEND_K8S_POD_SECURITY_* options? I agree. Then step level options could be treated as exclusion: for example, we set runAsNonRoot=true instance-wide, but for some step/pod/container we make exclusion in step config.

I wonder how much we want to bake into the backend_options?

¯_(ツ)_/¯ Probably, all Kube spec 🤣 Seriously, at least I plan to add seccomp and AppArmor. @dominic-p, what of simple SC options (above) do you need and what we can get rid of (except privileged)?

@zc-devs zc-devs force-pushed the 2545-k8s-security-context branch from 1e682fa to caf8db3 Compare October 9, 2023 11:34
@zc-devs
Copy link
Contributor Author

zc-devs commented Oct 9, 2023

Added supplementalGroups and fsGroup.

Pipeline:

steps:
  test:
    image: alpine
    commands:
      - echo Hello from $(whoami)
    backend_options:
      kubernetes:
        securityContext:
          runAsUser: 999
          readOnlyRootFilesystem: true

Pod:

kind: Pod
spec:
  containers:
    - name: wp-01hca2t4zh0r9xsqpbze5ryqww-0-step-0
      image: alpine
      securityContext:
        privileged: false
        readOnlyRootFilesystem: true
  securityContext:
    runAsUser: 999

@pat-s
Copy link
Contributor

pat-s commented Oct 9, 2023

Do you mean, I should add WOODPECKER_BACKEND_K8S_POD_SECURITY_* options? I agree. Then step level options could be treated as exclusion: for example, we set runAsNonRoot=true instance-wide, but for some step/pod/container we make exclusion in step config.

Yes. I think in 99% of all cases you want to have these options set on the instance level and not on the step level. Individual overrides are always good to have though.

I think we should add all options at the same time, otherwise people will start to use this notation at first as there are no global options available.

¯_(ツ)_/¯ Probably, all Kube spec 🤣 Seriously, at least I plan to add seccomp and AppArmor. @dominic-p, what of simple SC options (above) do you need and what we can get rid of (except privileged)?

Fair enough. Again, having them on the step level is certainly not bad for granular config.
Yet I also wonder how we might deal with overrides that are actually not wanted. E.g. an admin might want to control if users can override security specific settings on the step level.
Maybe we need an extra var to control for this?

@dominic-p
Copy link
Contributor

@zc-devs for my use case I just need runAsNonRoot, runAsUser, runAsGroup, and fsGroup (although runAsUser may imply runAsNonRoot I'm not sure on that).

I would hesitate to combine Pod and container level config in the same object just because as a user I wouldn't be sure which one I was setting. That said, it's probably mostly a matter of style/preference in a single container Pod.

As for global options in the helm chart, I agree with @zc-devs. If I have multiple steps running in different containers, it's very unlikely that they are all going to need to run as the same UID/GID, for instance. I would still need to configure/override some of the settings at the step level.

@zc-devs
Copy link
Contributor Author

zc-devs commented Oct 10, 2023

  • Deleted unnecessary options
  • Added pipeline schema
  • Made runAsNonRoot agent-wide
  • Don't set securityContext if all fields aren't set

Test with WOODPECKER_BACKEND_K8S_SECCTX_NONROOT: 'true'

  1. Pipeline:
skip_clone: true
steps:
  test:
    image: alpine
    commands:
      - echo Hello from $(whoami)

Error:

Error: container has runAsNonRoot and image will run as root (container: wp-01hcd9bzzxydxr5fes0fhq97rz-0-step-0)
  1. Pipeline:
skip_clone: true
steps:
  test:
    image: alpine
    commands:
      - echo Hello from $(whoami)
    backend_options:
      kubernetes:
        securityContext:
          runAsUser: 999
          runAsGroup: 999

Pod:

kind: Pod
spec:
  containers:
    - name: wp-01hcd7tjy79ctkkx3g3hg8hae2-0-step-0
      image: alpine
  securityContext:
    runAsUser: 999
    runAsGroup: 999
    runAsNonRoot: true
  1. Pipeline:
skip_clone: true
steps:
  test:
    image: alpine
    commands:
      - echo Hello from $(whoami)
    backend_options:
      kubernetes:
        securityContext:
          runAsUser: 999
          runAsGroup: 999
          privileged: true

Pod:

kind: Pod
spec:
  containers:
    - name: wp-01hcd83q7be5ymh89k5accn3k6-0-step-0
      image: alpine
      securityContext:
        privileged: true
  securityContext:
    runAsUser: 999
    runAsGroup: 999
    runAsNonRoot: true

@zc-devs
Copy link
Contributor Author

zc-devs commented Oct 10, 2023

I would hesitate to combine Pod and container level config in the same object just because as a user I wouldn't be sure which one I was setting. That said, it's probably mostly a matter of style/preference in a single container Pod.

It's abstraction here which doesn't have to match 1:1 with Kubernetes. Also as a step is a pod with only one container now, there is no reason to make things more complex.

have these options set on the instance level

If I have multiple steps running in different containers, it's very unlikely that they are all going to need to run as the same UID/GID

Agree, it doesn't make sense to have user and group options agent-wide. Consider this, I added only runAsNonRoot in the agent configuration.

admin might want to control if users can override security specific settings on the step level

Then Pod Security Admission will help, I think.

@dominic-p
Copy link
Contributor

@zc-devs that all makes sense to me. If we run the CI again, I can smoke test the revisions on my cluster.

Once everyone's settled on the semantics for the config I can submit a PR with some docs.

@qwerty287 qwerty287 added enhancement improve existing features backend/kubernetes labels Oct 11, 2023
@zc-devs zc-devs mentioned this pull request Oct 17, 2023
@zc-devs
Copy link
Contributor Author

zc-devs commented Oct 25, 2023

@pat-s, could you add build_pr_images label here?

…ontext

# Conflicts:
#	pipeline/schema/schema.json
@pat-s pat-s added the build_pr_images If set, the CI will build images for this PR and push to Dockerhub label Oct 25, 2023
@pat-s
Copy link
Contributor

pat-s commented Nov 6, 2023

@zc-devs Your PR images are finally ready!

@zc-devs
Copy link
Contributor Author

zc-devs commented Nov 6, 2023

Thanks.
@dominic-p, could you test it (woodpeckerci/woodpecker-server:pull_2735, woodpeckerci/woodpecker-agent:pull_2735)?

@dominic-p
Copy link
Contributor

Sorry for the radio silence here. I've been stuck working on a pretty consuming project the last couple weeks. That's wrapping up now, so I should be able to test this in the next day or two.

@dominic-p
Copy link
Contributor

Ok, I was finally able to test this tonight. It looks good to me.

I'm still not able to run my particular pipeline as we're still missing AppArmor functionality, but the security context setting seem to be working as expected.

Thanks so much for your work on this. Did you want me to take a stab at some documentation on this for the website?

@zc-devs
Copy link
Contributor Author

zc-devs commented Nov 14, 2023

Did you want me to take a stab at some documentation on this for the website?

Yeah, that would be great!

dominic-p added a commit to dominic-p/woodpecker that referenced this pull request Nov 14, 2023
The new docs explain how to use the recently introduced kubernetes backend option to set the security context for pipeline steps.

See woodpecker-ci#2550
@woodpecker-bot
Copy link
Collaborator

woodpecker-bot commented Nov 17, 2023

Deployment of preview was successful: https://woodpecker-ci-woodpecker-pr-2550.surge.sh

@zc-devs
Copy link
Contributor Author

zc-devs commented Nov 24, 2023

@qwerty287, could you review again?

Copy link
Contributor

@qwerty287 qwerty287 left a comment

Choose a reason for hiding this comment

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

Yes, looks good so far. Didn't test. And as I wrote already on other PR: I can't really say something about kubernetes-specific stuff

@qwerty287 qwerty287 added this to the 2.1.0 milestone Nov 25, 2023
@qwerty287
Copy link
Contributor

I'm merging now. If there are still issues we can fix them later

@qwerty287 qwerty287 merged commit 3adb98b into woodpecker-ci:main Nov 26, 2023
6 of 8 checks passed
qwerty287 added a commit that referenced this pull request Nov 26, 2023
The new docs explain how to use the recently introduced kubernetes
backend option to set the security context for pipeline steps.

See #2550

---------

Co-authored-by: qwerty287 <80460567+qwerty287@users.noreply.github.com>
@woodpecker-bot woodpecker-bot mentioned this pull request Nov 26, 2023
1 task
@zc-devs zc-devs deleted the 2545-k8s-security-context branch November 26, 2023 15:20
6543 pushed a commit that referenced this pull request Dec 26, 2023
## [2.1.0](https://github.com/woodpecker-ci/woodpecker/releases/tag/2.1.0)
- 2023-12-26

### ✨ Features

- Add pull request closed event
[[#2684](#2684)]
- Add depends_on support for steps
[[#2771](#2771)]
- gitlab: support nested repos
[[#2981](#2981)]
- Support go plugins for forges and agent backends
[[#2751](#2751)]

### 📈 Enhancement

- Show default branch on top
[[#3019](#3019)]
- Support more addon types
[[#2984](#2984)]
- Hide PR tab if PRs are disabled
[[#3004](#3004)]
- Switch to ULID
[[#2986](#2986)]
- Ignore pipelines without config
[[#2949](#2949)]
- Link labels to input and select
[[#2974](#2974)]
- Register Agent with hostname
[[#2936](#2936)]
- Update slogan & logo
[[#2962](#2962)]
- Improve error handling when activating a repository
[[#2965](#2965)]
- Add check for storage where repo/org name is empty
[[#2968](#2968)]
- Update pipeline icons
[[#2783](#2783)]
- Kubernetes refactor
[[#2794](#2794)]
- Export changed files via builtin environment variables
[[#2935](#2935)]
- Show secrets from org and global level
[[#2873](#2873)]
- Only update pipelineStatus in one place
[[#2952](#2952)]
- Rename `engine` to `backend`
[[#2950](#2950)]
- Add linting for `log.Fatal()`
[[#2946](#2946)]
- Remove separate root path config
[[#2943](#2943)]
- init CI_COMMIT_TAG if commit ref is a tag
[[#2934](#2934)]
- Update go module path for major version 2
[[#2905](#2905)]
- Unify date/time dependencies
[[#2891](#2891)]
- Add linting for `any`
[[#2893](#2893)]
- Fix vite deprecations
[[#2885](#2885)]
- Migrate to Xormigrate
[[#2711](#2711)]
- Simple security context options (Kubernetes)
[[#2550](#2550)]
- Changes PullRequest Index to ForgeRemoteID type
[[#2823](#2823)]

### 🐛 Bug Fixes

- Hide queue visualization if nothing to show
[[#3003](#3003)]
- fix and lint swagger file
[[#3007](#3007)]
- Fix IPv6 host aliases for kubernetes
[[#2992](#2992)]
- Fix cli lint throwing error on warnings
[[#2995](#2995)]
- Fix static file caching
[[#2975](#2975)]
- Gitea driver: ignore GetOrg error if we get a valid user.
[[#2967](#2967)]
- feat(k8s): Add a port name to service definition
[[#2933](#2933)]
- Fix error container overflow
[[#2957](#2957)]
- ignore some errors on repairAllRepos
[[#2792](#2792)]
- Allow to restart pipelines that has warnings
[[#2939](#2939)]
- Fix skipped pipelines model
[[#2923](#2923)]
- fix: Add `backend_options` to service linter entry
[[#2930](#2930)]
- Fix flags added multiple times
[[#2914](#2914)]
- Fix schema validation with array syntax for clone and services
[[#2920](#2920)]
- Fix prometheus docs
[[#2919](#2919)]
- Fix podman agent container in v2
[[#2897](#2897)]
- Fix bitbucket org fetching
[[#2874](#2874)]
- Only deploy docs on `main`
[[#2892](#2892)]
- Fix pipeline-related environment
[[#2876](#2876)]
- Fix version check partially
[[#2871](#2871)]
- Fix unregistering agents when using agent tokens
[[#2870](#2870)]

### 📚 Documentation

- [Awesome Woodpecker] added yet another autoscaler
[[#3011](#3011)]
- Add cookbook blog and improve docs
[[#3002](#3002)]
- Replace multi-pipelines with workflows on docs frontpage
[[#2990](#2990)]
- Update README badges
[[#2956](#2956)]
- Update 20-kubernetes.md
[[#2927](#2927)]
- Add release documentation to CONTRIBUTING
[[#2917](#2917)]
- Add nix-attic plugin to the index
[[#2889](#2889)]
- Add usage with Tunnelmole to docs
[[#2881](#2881)]
- Improve code blocks in docs
[[#2879](#2879)]
- Add a blog post
[[#2877](#2877)]
- Add documentation on Kubernetes securityContext
[[#2822](#2822)]
- Add default page to categories
[[#2869](#2869)]
- Use same format for Github docs as used for the other forges
[[#2866](#2866)]

### Misc

- chore(deps): update dependency isomorphic-dompurify to v2
[[#3001](#3001)]
- fix(deps): update dependency @intlify/unplugin-vue-i18n to v2
[[#2998](#2998)]
- Fix go in gitpod
[[#2973](#2973)]
- fix(deps): update module google.golang.org/grpc to v1.60.1
[[#2969](#2969)]
- chore(deps): update docker.io/alpine docker tag to v3.19
[[#2970](#2970)]
- Fix broken gated repos
[[#2959](#2959)]
- fix(deps): update golang (packages)
[[#2958](#2958)]
- Update docker.io/techknowlogick/xgo Docker tag to go-1.21.5
[[#2926](#2926)]
- Update docker.io/golang Docker tag to v1.21.5
[[#2925](#2925)]
- Lock file maintenance
[[#2910](#2910)]
- Update web npm deps non-major
[[#2909](#2909)]
- Update docs npm deps non-major
[[#2908](#2908)]
- Update golang (packages)
[[#2904](#2904)]
- Update module github.com/google/go-github/v56 to v57
[[#2899](#2899)]
- Update dependency marked to v11
[[#2898](#2898)]
- Update dependency vite-svg-loader to v5
[[#2837](#2837)]
- Update golang (packages)
[[#2894](#2894)]
- Update web npm deps non-major
[[#2895](#2895)]
- Update web npm deps non-major
[[#2884](#2884)]
- Update docker.io/woodpeckerci/plugin-docker-buildx Docker tag to
v2.2.1 [[#2883](#2883)]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend/kubernetes build_pr_images If set, the CI will build images for this PR and push to Dockerhub enhancement improve existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants