charts: Remove ClusterRoleBinding default#4551
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements a security improvement by changing the default ClusterRoleBinding behavior in the Headlamp Helm chart. The chart now disables ClusterRoleBinding creation by default instead of creating one with cluster-admin permissions, reducing the attack surface for new deployments.
Changes:
- Changed default ClusterRoleBinding creation from enabled with cluster-admin to disabled with empty clusterRoleName
- Added pre-upgrade hook to automatically clean up old
headlamp-adminClusterRoleBinding during upgrades - Updated ClusterRoleBinding template to remove
-adminsuffix from resource name - Updated documentation, tests, and NOTES.txt to reflect new security-focused defaults
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| charts/headlamp/values.yaml | Changed clusterRoleBinding.create default to false and clusterRoleName to empty string; fixed typo in comment |
| charts/headlamp/tests/test_cases/clusterrolebinding-enabled.yaml | Added new test case for explicitly enabling ClusterRoleBinding with "view" role |
| charts/headlamp/tests/expected_templates/*.yaml | Updated 19 test expected templates to remove ClusterRoleBinding resources (reflecting new disabled default) |
| charts/headlamp/tests/expected_templates/clusterrolebinding-enabled.yaml | Added new expected template showing ClusterRoleBinding with "view" role and pre-upgrade hook resources |
| charts/headlamp/templates/pre-upgrade-cleanup.yaml | Added new pre-upgrade hook with Job, ServiceAccount, ClusterRole, and ClusterRoleBinding to remove old headlamp-admin ClusterRoleBinding |
| charts/headlamp/templates/clusterrolebinding.yaml | Removed -admin suffix from ClusterRoleBinding name |
| charts/headlamp/templates/NOTES.txt | Updated installation instructions to guide users on manually creating admin ClusterRoleBinding |
| charts/headlamp/README.md | Added upgrade section explaining security improvements and automatic migration; updated parameter documentation |
| charts/headlamp/Chart.yaml | Added changelog annotations documenting the breaking change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ff53355 to
87d5180
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6a8e342 to
bd0586c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I'm pretty happy with this now... I'll sleep on it, and have another look. What do you think? @skoeva @ashu8912 @cacarico To see the difference between the two PRs... In short the changes are:
diff --git a/charts/headlamp/Chart.yaml b/charts/headlamp/Chart.yaml
index 43c9a29ab..2717c0bd4 100644
--- a/charts/headlamp/Chart.yaml
+++ b/charts/headlamp/Chart.yaml
@@ -30,7 +30,7 @@ annotations:
artifacthub.io/license: Apache-2.0
artifacthub.io/changes: |
- kind: changed
- description: The default ClusterRoleBinding is no longer created by default (ClusterRoleBinding creation is now disabled)
+ description: The default ClusterRoleBinding is no longer created by default (ClusterRoleBinding creation is now disabled). Users who want a ClusterRoleBinding must now explicitly enable it and provide a clusterRoleName value, as the default clusterRoleName is now empty.
- kind: added
description: Pre-upgrade hook automatically removes old ClusterRoleBinding during upgrades
artifacthub.io/screenshots: |
diff --git a/charts/headlamp/README.md b/charts/headlamp/README.md
index c443fdd44..14b594c53 100644
--- a/charts/headlamp/README.md
+++ b/charts/headlamp/README.md
@@ -61,7 +61,7 @@ Starting from version 0.39.0, the chart implements enhanced security by default:
- **Removed Permissions**: The default ClusterRoleBinding creation has been disabled (`create: false`), removing the previous `cluster-admin` binding.
-**Automatic Migration**: When `clusterRoleBinding.create` is enabled, a pre-upgrade hook removes the old `headlamp-admin` ClusterRoleBinding during upgrades to help migrate to the new security configuration. If you have disabled this option or manage ClusterRoleBindings manually, you must remove any existing `headlamp-admin` ClusterRoleBinding yourself.
+**Automatic Migration**: A pre-upgrade hook automatically removes the old `headlamp-admin` ClusterRoleBinding during upgrades to help migrate to the new security configuration. The hook only removes ClusterRoleBindings that were created by Helm, preserving any user-created resources with the same name.
## Configurationdiff --git a/charts/headlamp/templates/pre-upgrade-cleanup.yaml b/charts/headlamp/templates/pre-upgrade-cleanup.yaml
index 2ed70db6b..d30178b17 100644
--- a/charts/headlamp/templates/pre-upgrade-cleanup.yaml
+++ b/charts/headlamp/templates/pre-upgrade-cleanup.yaml
@@ -1,4 +1,3 @@
-{{- if .Values.clusterRoleBinding.create -}}
---
apiVersion: v1
kind: ServiceAccount
@@ -113,11 +112,16 @@ spec:
memory: 128Mi
securityContext:
- {{- toYaml .Values.podSecurityContext | nindent 10 }}
+ allowPrivilegeEscalation: false
+ runAsNonRoot: true
+ capabilities:
+ drop:
+ - ALL
+ seccompProfile:
+ type: RuntimeDefault
volumeMounts:
- name: tmp
mountPath: /tmp
volumes:
- name: tmp
- emptyDir: {}
-{{- end }}
+ emptyDir: {} |
Changed default ClusterRoleBinding creation from create: true to
create: false and removed default cluster-admin role.
Added pre-upgrade hook to automatically clean up the old
headlamp-admin ClusterRoleBinding during upgrades.
Updated ClusterRoleBinding name from {fullname}-admin to
{fullname} and added required field validation.
Updated documentation (README, NOTES.txt, Chart.yaml) to
reflect the security changes.
Added comprehensive tests for the pre-upgrade hook functionality.
c86b81e to
097b3b4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ashu8912, illume The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
continued on from PR by @cacarico
Summary
This PR addresses security and usability issues in the Helm chart's default configuration. The chart now disables ClusterRoleBinding creation by default instead of creating one with cluster-admin permissions, reducing the attack surface for new deployments. Additionally it removes ones previously created by the chart.
Related Issue
Fixes #4435
Changes
Steps to Test