Skip to content

Add securityContext of ReadOnlyRootFilesystem to steps #1885

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Add securityContext of ReadOnlyRootFilesystem to steps #1885

wants to merge 2 commits into from

Conversation

hasanawad94
Copy link
Contributor

@hasanawad94 hasanawad94 commented May 13, 2025 •
edited
Loading

Changes

Explicitly set readOnlyRootFilesystem to true for taskruns according to security best practice.

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Set a kind label on this PR
  • Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

Set the securityContext configuration for the Git, Waiter, Bundle, and ImageProcessing containers to readOnlyRootFilesystem: true.
Set the securityContext configuration for the Buildah, Buildpacks, Buildkit, and multiarch-native-buildah strategies to readOnlyRootFilesystem: true.

@openshift-ci openshift-ci bot added the release-note Label for when a PR has specified a release note label May 13, 2025
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 13, 2025
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 13, 2025
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented May 13, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign apoorvajagtap for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@hasanawad94
Copy link
Contributor Author

Fixing tests after adding the default value

@hasanawad94
Copy link
Contributor Author

/retest

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented May 13, 2025

@hasanawad94: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@sayan-biswas
Copy link

/ok-to-test

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented May 13, 2025

@sayan-biswas: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@hasanawad94 hasanawad94 force-pushed the mount-security-context branch 2 times, most recently from 0ae6b68 to e9878b0 Compare May 15, 2025 13:16
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 15, 2025
SaschaSchwarze0
SaschaSchwarze0 reviewed May 16, 2025
View reviewed changes
@hasanawad94 hasanawad94 force-pushed the mount-security-context branch 2 times, most recently from fefbd3e to ecd8d58 Compare May 18, 2025 11:48
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 18, 2025
@hasanawad94 hasanawad94 force-pushed the mount-security-context branch from ecd8d58 to e411479 Compare May 18, 2025 14:54
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 18, 2025
@hasanawad94
Copy link
Contributor Author

Currently working on Image processing part

@hasanawad94 hasanawad94 force-pushed the mount-security-context branch 6 times, most recently from 67357d6 to 5c69e96 Compare May 20, 2025 09:30
@hasanawad94 hasanawad94 changed the title add securityContext of ReadOnlyRootFilesystem to steps Add securityContext of ReadOnlyRootFilesystem to steps May 20, 2025
@hasanawad94 hasanawad94 force-pushed the mount-security-context branch 13 times, most recently from c95b664 to 6118cea Compare July 14, 2025 08:31
@hasanawad94 hasanawad94 force-pushed the mount-security-context branch 8 times, most recently from 0d21d84 to 7f6168c Compare July 22, 2025 11:43
@hasanawad94
Add ReadOnlyRootFilesystem securityContext to build steps
8af9ecd 
Set the root filesystem to read-only for all build and buildstrategy
containers as a security best practice.

To support this, steps that require write access now explicitly mount
`emptyDir` volumes for paths like `/tmp` `/home`.

A new `AppendWriteableVolumes` function centralizes the setup for volume
mounting , using idempotent helpers (`ensureVolume`, `ensureVolumeMount`)
to prevent duplicate entries.

The writeable home directory for the steps can be configured using
`WRITABLE_HOME_DIR`. Default is value is `/writable-home`

Signed-off-by: Hasan Awad <[email protected]>
@hasanawad94 hasanawad94 force-pushed the mount-security-context branch from 7f6168c to 8af9ecd Compare July 22, 2025 12:31
@hasanawad94
Copy link
Contributor Author

@SaschaSchwarze0 You think we can get this into v0.17 as a better security practice ?
Summary of changes :

  • Home directory volume has been added to each container (not shared)
  • Volume for trivy to work with
  • Volumes for buildstrategies to use instead of writing to the rootfs (tmp,home)

@hasanawad94
Copy link
Contributor Author

@SaschaSchwarze0 I want to break this pr into smaller parts to make the review easier

@hasanawad94 hasanawad94 marked this pull request as draft July 30, 2025 14:42
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 30, 2025
@hasanawad94 hasanawad94 mentioned this pull request Jul 31, 2025
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HeavyWombat HeavyWombat Awaiting requested review from HeavyWombat

@adambkaplan adambkaplan Awaiting requested review from adambkaplan

@SaschaSchwarze0 SaschaSchwarze0 Awaiting requested review from SaschaSchwarze0

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Label for when a PR has specified a release note size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Shipwright Overview
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hasanawad94 @sayan-biswas @SaschaSchwarze0