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

feat: add SETUID and SETGID capabilities for gitlab runner container security context #116

Merged
merged 25 commits into from
Aug 26, 2024

Conversation

ericwyles
Copy link
Contributor

@ericwyles ericwyles commented Aug 14, 2024

Description

Add SETUID capability for gitlab runner container security context to allow running buildah inside a container from the runners.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

Copy link
Contributor

@Racer159 Racer159 left a comment

Choose a reason for hiding this comment

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

I think this change should be fine but can you add a docs note to configuration.md to mention needing this sysctl setting? https://github.com/defenseunicorns/uds-rke2-image-builder/pull/81/files#diff-8c6b38593d0267b1fcfe569ffd1ab064b64bdecc6293bc967cded5f4b9c8bc13R15
(ideally in an admonition to call attention to it)

Copy link
Contributor

@Racer159 Racer159 left a comment

Choose a reason for hiding this comment

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

Small tweak since it is only applicable to buildah and tools like it

docs/configuration.md Outdated Show resolved Hide resolved
ericwyles and others added 2 commits August 14, 2024 12:37
Co-authored-by: Wayne Starr <Racer159@users.noreply.github.com>
@ericwyles ericwyles requested a review from a team as a code owner August 14, 2024 21:44
@bburky
Copy link
Member

bburky commented Aug 14, 2024

CAP_SETUID is basically equivalent to root in the container. I understand there's a need for this for container builds, but I don't love seeing this be the default config for GitLab.

Do we support multiple configurations of gitlab runner groups (so this could be an optional config)? Do we support/suggest using dedicated nodes for gitlab runners (so that compromising CI doesn't compromise the whole cluster)?

Have we made any progress on using ephemeral EC2/VM-based runners? Let people have root in their CI (and use real docker build), if you've isolated them with a VM or dedicated EC2 instance.

@bburky
Copy link
Member

bburky commented Aug 14, 2024

I guess DISABLE_RESTRICTCAPABILITIES_EXEMPTION is disabled by default. But it does end up enabled for all runners.

@Racer159
Copy link
Contributor

CAP_SETUID is basically equivalent to root in the container. I understand there's a need for this for container builds, but I don't love seeing this be the default config for GitLab.

Do we support multiple configurations of gitlab runner groups (so this could be an optional config)? Do we support/suggest using dedicated nodes for gitlab runners (so that compromising CI doesn't compromise the whole cluster)?

Have we made any progress on using ephemeral EC2/VM-based runners? Let people have root in their CI (and use real docker build), if you've isolated them with a VM or dedicated EC2 instance.

We do support running gitlab runners on their own cluster (you can give it a different gitlab URL and registration token) and it is an optional config (though defaulted) as written - we can undefault it or remove that part of the PR to only partially support Buildah in the runner (buildah can run without this but flags warnings and I would expect some container build operations would fail though haven't dug that far yet).

Not yet on fleeting runners though it is something still on the docket to setup for envs that can use it (really just AWS rn)

@Racer159
Copy link
Contributor

Opened a draft PR for the fleeting plugin: #117 - looks like it is out of beta now too cc @oates / @zachariahmiller

@zachariahmiller
Copy link
Contributor

CAP_SETUID is basically equivalent to root in the container. I understand there's a need for this for container builds, but I don't love seeing this be the default config for GitLab.
Do we support multiple configurations of gitlab runner groups (so this could be an optional config)? Do we support/suggest using dedicated nodes for gitlab runners (so that compromising CI doesn't compromise the whole cluster)?
Have we made any progress on using ephemeral EC2/VM-based runners? Let people have root in their CI (and use real docker build), if you've isolated them with a VM or dedicated EC2 instance.

We do support running gitlab runners on their own cluster (you can give it a different gitlab URL and registration token) and it is an optional config (though defaulted) as written - we can undefault it or remove that part of the PR to only partially support Buildah in the runner (buildah can run without this but flags warnings and I would expect some container build operations would fail though haven't dug that far yet).

Not yet on fleeting runners though it is something still on the docket to setup for envs that can use it (really just AWS rn)

Unless something has drastically changed recently it fails without SETUID and honestly i thought SETGUID in rootless. IIRC its on the mount, buildah bud and buildah unshare

@ericwyles ericwyles marked this pull request as draft August 23, 2024 18:19
@ericwyles ericwyles marked this pull request as ready for review August 26, 2024 22:02
@ericwyles ericwyles changed the title feat: add SETUID capability for gitlab runner container security context feat: add SETUID and SETGID capabilities for gitlab runner container security context Aug 26, 2024
tests/journey/pipeline-run.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@Racer159 Racer159 left a comment

Choose a reason for hiding this comment

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

lgtm assuming tests pass

@ericwyles ericwyles merged commit 6609aa0 into main Aug 26, 2024
13 checks passed
Racer159 pushed a commit that referenced this pull request Aug 30, 2024
🤖 I have created a release *beep* *boop*
---


##
[17.2.1-uds.0](v17.1.0-uds.1...v17.2.1-uds.0)
(2024-08-30)


### Features

* add SETUID and SETGID capabilities for gitlab runner container
security context
([#116](#116))
([6609aa0](6609aa0))


### Miscellaneous

* cleanup tests to match pattern in other repos
([#112](#112))
([e7c2d33](e7c2d33))
* **deps:** update gitlab runner package dependencies
([#108](#108))
([beaec62](beaec62))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants