feat: improve Helm chart #12691

fernandezcuesta · 2025-06-25T11:41:32Z

Add extraInitContainers to celery+django deployments.
Add extraEnv to all deployments
Remove existing volume logic in favor of agnostic extraVolumes and extraVolumeMounts
Add the ability to deploy secrets as regular (non-hooked) resources due to issues found with ArgoCD
Make some secret references optional (secret might not be created)
Fix optional secret mounts + reference
Update bitnami chart reference (OCI)
Bump up redis chart

Reasoning behind the logic change for volumes, with the existing behaviour we cannot mount projected volumes (e.g. a secret mount where we want the key names being renamed) or per-container volumeMounts (e.g. nginx emptyDir when readOnlyRootFs is enforced).

- Add extraInitContainers to celery+django deployments. - Add extraEnv to all deployments - Remove existing volume logic in favor of agnostic extraVolumes and extraVolumeMounts - Fix optional secret mounts + reference - Update bitnami chart reference (OCI) - Bump up redis chart

dryrunsecurity · 2025-06-25T11:43:10Z

This pull request reveals multiple security vulnerabilities in the DefectDojo Helm chart, including weak TLS configuration, potential arbitrary configuration and environment variable injection, optional critical secret keys, and potential information disclosure in logs, which could compromise the application's security and data integrity if exploited.

Weak TLS Configuration by Default in helm/defectdojo/templates/configmap.yaml

Vulnerability	Weak TLS Configuration by Default
Description	The Helm chart configures the Celery broker connection to Redis with `ssl_cert_reqs=optional` by default when TLS is enabled. This setting means that the client will not verify the server's certificate, making the connection vulnerable to Man-in-the-Middle (MitM) attacks. While the user can override this setting, the insecure default poses a risk.

django-DefectDojo/helm/defectdojo/templates/configmap.yaml

Lines 29 to 35 in 3f502ad

    
           DD_CELERY_BROKER_USER: '' 
        
           DD_CELERY_BROKER_HOST: {{ if eq .Values.celery.broker "redis" }}{{ template "redis.hostname" . }}{{ end }} 
        
           DD_CELERY_BROKER_PORT: '{{ if eq .Values.celery.broker "redis" }}{{- if ( hasKey .Values.redis "master" ) -}}{{ .Values.redis.master.service.ports.redis }}{{ else }}6379{{ end }}{{- end -}}' 
        
           DD_CELERY_BROKER_PARAMS: '{{ .Values.celery.brokerParams | default $defaultBrokerParams }}' 
        
           DD_CELERY_BROKER_PATH: '{{ .Values.celery.path | default "//" }}' 
        
           DD_CELERY_LOG_LEVEL: {{ .Values.celery.logLevel }} 
        
           DD_CELERY_WORKER_POOL_TYPE: {{ .Values.celery.worker.appSettings.poolType | default "solo" }}

Arbitrary Configuration Injection in helm/defectdojo/templates/configmap.yaml

Vulnerability	Arbitrary Configuration Injection
Description	The Helm chart allows arbitrary key-value pairs to be injected into the main ConfigMap via the `.Values.extraConfigs` parameter. The content is directly rendered using `toYaml` without validation, meaning an operator could inject malicious or overriding configuration values, potentially compromising the application's security. For example, an attacker could override `DD_SECRET_KEY` or `DD_CREDENTIAL_AES_256_KEY` if they have control over the Helm chart values.

django-DefectDojo/helm/defectdojo/templates/configmap.yaml

Lines 55 to 60 in 3f502ad

    
           {{- if  .Values.django.uwsgi.certificates.enabled }} 
        
             REQUESTS_CA_BUNDLE: {{ .Values.django.uwsgi.certificates.certMountPath }}{{ .Values.django.uwsgi.certificates.certFileName }} 
        
           {{- end }} 
        
           {{- with .Values.extraConfigs }} 
        
             {{- toYaml . | nindent 2 }} 
        
           {{- end }}

Arbitrary Environment Variable Injection in Initializer in helm/defectdojo/templates/initializer-job.yaml

Vulnerability	Arbitrary Environment Variable Injection in Initializer
Description	The `initializer-job.yaml` template uses `toYaml` on `.Values.initializer.extraEnv`, allowing arbitrary environment variables to be injected into the initializer job's environment. This job executes Django database migrations (`manage.py migrate`). An attacker capable of controlling Helm values could inject malicious environment variables, potentially overriding critical database connection parameters (e.g., host, port, user, database name).

django-DefectDojo/helm/defectdojo/templates/initializer-job.yaml

Lines 145 to 161 in 3f502ad

    
               name: {{ $fullName }} 
        
           - secretRef: 
        
               name: {{ $fullName }} 
        
               optional: true 
        
           - secretRef: 
        
               name: {{ $fullName }}-extrasecrets 
        
               optional: true 
        
           env: 
        
           - name: DD_DATABASE_PASSWORD 
        
             valueFrom: 
        
               secretKeyRef: 
        
                 name: {{ .Values.postgresql.auth.existingSecret }} 
        
                 key: {{ .Values.postgresql.auth.secretKeys.userPasswordKey }} 
        
           {{- with .Values.initializer.extraEnv }} 
        
           {{- toYaml . | nindent 8 }} 
        
           {{- end }} 
        
           resources:

Optional Critical Secret Keys in helm/defectdojo/templates/django-deployment.yaml

Vulnerability	Optional Critical Secret Keys
Description	The Helm chart for DefectDojo allows `DD_SECRET_KEY` and `DD_CREDENTIAL_AES_256_KEY` to be marked as `optional: true` when sourced from Kubernetes secrets. This means that the application pods can start even if these critical security keys are not provided. In a Django application, `SECRET_KEY` is fundamental for cryptographic operations like session management and CSRF protection. Similarly, `DD_CREDENTIAL_AES_256_KEY` is crucial for encrypting sensitive credentials. Allowing these to be optional creates a severe security risk, as the application may fall back to insecure defaults or operate without essential security mechanisms, leading to compromised data integrity, confidentiality, and user authentication.

django-DefectDojo/helm/defectdojo/templates/django-deployment.yaml

Lines 198 to 219 in 3f502ad

    
               secretKeyRef: 
        
                 name: {{ $fullName }} 
        
                 key: DD_SECRET_KEY 
        
                 optional: true 
        
           - name: DD_CREDENTIAL_AES_256_KEY 
        
             valueFrom: 
        
               secretKeyRef: 
        
                 name: {{ $fullName }} 
        
                 key: DD_CREDENTIAL_AES_256_KEY 
        
                 optional: true 
        
           - name: DD_SESSION_COOKIE_SECURE 
        
             value: {{- if or .Values.django.ingress.activateTLS .Values.django.nginx.tls.enabled }} "True" {{- else }} "False" {{- end }} 
        
           - name: DD_CSRF_COOKIE_SECURE 
        
             value: {{- if or .Values.django.ingress.activateTLS .Values.django.nginx.tls.enabled }} "True" {{- else }} "False" {{- end }} 
        
           {{- with .Values.extraEnv }} 
        
             {{- . | toYaml | nindent 8 }} 
        
           {{- end }} 
        
           {{- with .Values.django.uwsgi.extraEnv }} 
        
             {{- . | toYaml | nindent 8 }} 
        
           {{- end }} 
        
           {{- if .Values.django.uwsgi.livenessProbe.enabled }} 
        
           livenessProbe:

Information Disclosure in Logs in .github/workflows/k8s-tests.yml

Vulnerability	Information Disclosure in Logs
Description	The GitHub Actions workflow in `.github/workflows/k8s-tests.yml` has been modified to output logs from the `uwsgi` container when a login screen check fails. This occurs when the HTTP status code is not 200. The `kubectl logs` command is executed with `--tail=30` and targets the `uwsgi` container specifically. While the logs are limited to the last 30 lines, `uwsgi` can, under certain failure conditions (e.g., unhandled exceptions, misconfigurations), output sensitive information such as stack traces, internal paths, or environment variable details. If the repository is public, these logs are publicly accessible, potentially aiding an attacker in understanding the application's internal structure and identifying further vulnerabilities.

django-DefectDojo/.github/workflows/k8s-tests.yml

Lines 132 to 146 in 3f502ad

    
                               --max-time 20 \ 
        
                               --head \ 
        
                               --header "Host: $DD_HOSTNAME" \ 
        
                               "http://${DJANGO_IP}/login?next=/") 
        
                          echo $OUT 
        
                          CR=$(echo $OUT | egrep "^HTTP" | cut -d' ' -f2) 
        
                          echo $CR 
        
                          if [[ $CR -ne 200 ]]; then 
        
                             echo $RETRY 
        
                             if [[ $RETRY -gt 2 ]]; then 
        
                               kubectl get pods 
        
                               echo $(kubectl logs --tail=30 -l defectdojo.org/component=django -c uwsgi) 
        
                               echo "ERROR: cannot display login screen; got HTTP code $CR" 
        
                               exit 1 
        
                             else

All finding details can be found in the DryRun Security Dashboard.

github-actions · 2025-06-25T18:20:11Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

valentijnscholten · 2025-06-25T19:49:26Z

@fernandezcuesta Thanks for the PR, we'll ask some helm/k8s experienced devs to look at it. In the mean time could you look at the conflicts? Also this seems to be a bigger change, could you look at basing it against the dev branch

github-actions · 2025-06-26T06:28:52Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

fernandezcuesta · 2025-06-26T06:30:47Z

@valentijnscholten done, thanks for having a look at it! I also changed the base to dev branch.

kiblik

I will need more time for the rest (this is not my final review).

kiblik · 2025-06-27T14:25:52Z

helm/defectdojo/Chart.lock

-  repository: https://charts.bitnami.com/bitnami
-  version: 16.7.0
+  repository: oci://registry-1.docker.io/bitnamicharts
+  version: 16.7.13


TBH, this is a bit confusing for me. I'm using this chart, and 16.7.13 is automatically selected in the final template even though it is not pinned here. I want to look at this more.

That was autogenerated after a helm dependency update IIRC but can roll it back

helm/defectdojo/Chart.yaml

helm/defectdojo/values.yaml

mtesauro

Approved

kiblik

Sorry for the delay, I had been out for a couple of weeks, plus your PR is changing quite a lot of variables.

This is changing quite critical settings. It should be mentioned in the release notes.

But thank you for this work. I was planning to add some adjustments that you used, but never had enough time.

helm/defectdojo/templates/celery-beat-deployment.yaml

helm/defectdojo/values.yaml

kiblik · 2025-08-12T17:51:11Z

helm/defectdojo/templates/configmap.yaml

@@ -28,7 +28,7 @@ data:
  DD_CELERY_BROKER_USER: ''
  DD_CELERY_BROKER_HOST: {{ if eq .Values.celery.broker "redis" }}{{ template "redis.hostname" . }}{{ end }}
  DD_CELERY_BROKER_PORT: '{{ if eq .Values.celery.broker "redis" }}{{- if ( hasKey .Values.redis "master" ) -}}{{ .Values.redis.master.service.ports.redis }}{{ else }}6379{{ end }}{{- end -}}'
-  DD_CELERY_BROKER_PARAMS: '{{ if eq .Values.celery.broker "redis" }}{{- if .Values.redis.transportEncryption.enabled -}}{{ .Values.redis.transportEncryption.params | default "ssl_cert_reqs=optional" }}{{ end }}{{ end }}'
+  DD_CELERY_BROKER_PARAMS: '{{ if eq .Values.celery.broker "redis" }}{{- if .Values.redis.tls.enabled -}}{{ .Values.celery.brokerParams | default "ssl_cert_reqs=optional" }}{{ end }}{{ end }}'


I'm not against moving params to .Values.celery.brokerParams, but they are becoming more general. They should not be used only if .Values.redis.tls.enabled == true

OK I see your point, changing that!

kiblik · 2025-08-12T18:17:58Z

helm/defectdojo/templates/initializer-job.yaml

+            name: {{ $fullName }}-extrasecrets
+            optional: true


I understand why this might look like a typo, but it is not. This definition is right.

{{ $fullName }} contains DD_ADMIN_PASSWORD, DD_CREDENTIAL_AES_256_KEY and DD_SECRET_KEY which are necesseary for init of application.
Values from {{ $fullName }}-extrasecrets are usually not needed for initialization. But we might add them.

Suggested change

name: {{ $fullName }}-extrasecrets

optional: true

name: {{ $fullName }}

- secretRef:

name: {{ $fullName }}-extrasecrets

optional: true

or feel free to move values from {{ $fullName }} to

- name: DD_XXX valueFrom: secretKeyRef: name: {{ $fullName }} key: DD_XXX optional: true

It might make more sense to do it the other way around (drop separate loading of DD_CREDENTIAL_AES_256_KEY and DD_SECRET_KEY from all deployments and use there):

- secretRef: name: {{ $fullName }}

On the other hand, DD_ADMIN_PASSWORD is not needed in deployments (only in the initializer). So it might be against the least privilege principle.

Btw, it is probably the reason why the test is failing. DD_ADMIN_PASSWORD was not provided to the initializer, so it used a random password.

kiblik · 2025-08-12T18:25:28Z

helm/defectdojo/values.yaml

+  # Additional environment variables injected to the initializer job pods.
+  extraEnv: []
+  # Array of additional volume mount points for the initializer job pods.
+  extraVolumeMounts: []


You probably wanted to edit also

{{- range .Values.initializer.extraVolumes }} - name: userconfig-{{ .name }} {{ .type }}: {{- if (eq .type "configMap") }} name: {{ .name }} {{- else if (eq .type "secret") }} secretName: {{ .name }} {{- else if (eq .type "hostPath") }} type: {{ .pathType | default "Directory" }} path: {{ .hostPath }} {{- end }} {{- end }}

and

{{- range .Values.initializer.extraVolumes }} - name: userconfig-{{ .name }} readOnly: true mountPath: {{ .path }} subPath: {{ .subPath }} {{- end }}

in helm/defectdojo/templates/initializer-job.yaml

kiblik · 2025-08-12T18:49:43Z

helm/defectdojo/templates/celery-worker-deployment.yaml

-          type: {{ .pathType | default "Directory" }}
-          path: {{ .hostPath }}
-          {{- end }}
+      {{- with .Values.celery.beat.extraVolumes }}


Probably copy-paste issue

Suggested change

{{- with .Values.celery.beat.extraVolumes }}

{{- with .Values.celery.worker.extraVolumes }}

kiblik · 2025-08-12T18:52:46Z

helm/defectdojo/templates/celery-beat-deployment.yaml

    {{- end }}
-{{- if .Values.celery.annotations }}
+  {{- with .Values.celery.worker.annotations }}


Some copy-paste typo probably

Suggested change

{{- with .Values.celery.worker.annotations }}

{{- with .Values.celery.beat.annotations }}

Plus, this might be the issue in some cases. Until now, .Values.celery.worker.annotations was used to annotate pods created from this
deployment. Annotating deployment with exactly the same annotations might be problematic for somebody.
In the initializer, there are annotations and jobAnnotations. What about a similar approach?

kiblik · 2025-08-12T18:53:30Z

helm/defectdojo/values.yaml

+    # @type: array<map>
+    extraVolumes: []
+    # Enable liveness probe for Celery worker containers.
+    livenessProbe: {}


Same as beat

kiblik · 2025-08-12T19:02:24Z

helm/defectdojo/values.yaml

-  # To use an external Redis instance, set enabled to false and uncomment
-  # the line below:
-  # redisServer: myrediscluster


Why dropping this?

Moved to the comment in L473:

# To use an external Redis instance, switch enabled to false and set the address in .Values.celery.brokerHost

kiblik · 2025-08-12T19:03:29Z

helm/defectdojo/templates/celery-worker-deployment.yaml

@@ -13,12 +13,10 @@ metadata:
    {{- with .Values.extraLabels }}
      {{- toYaml . | nindent 4 }}
    {{- end }}
-{{- if .Values.celery.annotations }}
+  {{- with .Values.celery.worker.annotations }}


Same comment as beat

kiblik · 2025-08-13T17:40:11Z

Btw, this fix #7961

github-actions · 2025-08-18T14:31:07Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2025-08-21T10:56:44Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions · 2025-08-21T11:51:24Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

fernandezcuesta · 2025-08-21T12:04:41Z

@kiblik Thanks for your detailed review and suggestions. I tried to go through all, but have some doubts about one of them (see inline comments).

fernandezcuesta requested review from Maffooch and mtesauro as code owners June 25, 2025 11:41

github-actions bot added the helm label Jun 25, 2025

fernandezcuesta changed the title ~~**Summary:**~~ feat: improve Helm chart Jun 25, 2025

chore: add livenessProbe entries for celery

475ed17

Maffooch changed the base branch from master to bugfix June 25, 2025 17:47

github-actions bot added the conflicts-detected label Jun 25, 2025

fix: reference to removed field

6b18b96

fernandezcuesta changed the base branch from bugfix to dev June 26, 2025 06:28

fix: conflict

4165ca8

github-actions bot removed the conflicts-detected label Jun 26, 2025

fernandezcuesta added 4 commits June 26, 2025 08:32

chore: add reference to upstream chart

cbf8778

fix: missing default values from upstream chart used in templates

67d6348

chore: rephrase

b34e833

feat: allow deploy secret as regular non-hooked resources

330a127

Maffooch requested a review from rossops June 26, 2025 16:07

kiblik requested changes Jun 27, 2025

View reviewed changes

valentijnscholten added this to the 2.49.0 milestone Jun 29, 2025

fernandezcuesta added 3 commits June 30, 2025 09:19

fix: review

b474297

chore: restore Chart.lock

5c2f804

chore: update chart.lock

fba0cd0

rossops approved these changes Jul 7, 2025

View reviewed changes

fernandezcuesta requested a review from kiblik July 23, 2025 11:49

chore: wrap services url

108e7b4

valentijnscholten modified the milestones: 2.49.0, 2.50.0 Aug 4, 2025

mtesauro approved these changes Aug 11, 2025

View reviewed changes

kiblik requested changes Aug 12, 2025

View reviewed changes

github-actions bot added the conflicts-detected label Aug 18, 2025

Merge branch 'DefectDojo:master' into feat/improve-helm-chart

55958c4

github-actions bot added docker docs unittests ui parser labels Aug 21, 2025

Merge branch 'dev' into feat/improve-helm-chart

7bd3273

github-actions bot removed the conflicts-detected label Aug 21, 2025

github-actions bot added conflicts-detected and removed docker docs unittests ui parser labels Aug 21, 2025

fix: PR review suggestions

6794694

github-actions bot removed the conflicts-detected label Aug 21, 2025

chore: mount extraVolumes in initContainers too

3f502ad

fernandezcuesta requested a review from kiblik August 21, 2025 12:05

	{{- with .Values.celery.beat.extraVolumes }}
	{{- with .Values.celery.worker.extraVolumes }}

	{{- with .Values.celery.worker.annotations }}
	{{- with .Values.celery.beat.annotations }}

feat: improve Helm chart #12691

Are you sure you want to change the base?

feat: improve Helm chart #12691

Conversation

fernandezcuesta commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dryrunsecurity bot commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 25, 2025

Uh oh!

valentijnscholten commented Jun 25, 2025

Uh oh!

github-actions bot commented Jun 26, 2025

Uh oh!

fernandezcuesta commented Jun 26, 2025

Uh oh!

kiblik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtesauro left a comment

Choose a reason for hiding this comment

Uh oh!

kiblik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiblik commented Aug 13, 2025

Uh oh!

github-actions bot commented Aug 18, 2025

Uh oh!

github-actions bot commented Aug 21, 2025

Uh oh!

github-actions bot commented Aug 21, 2025

Uh oh!

fernandezcuesta commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

fernandezcuesta commented Jun 25, 2025 •

edited

Loading

dryrunsecurity bot commented Jun 25, 2025 •

edited

Loading

fernandezcuesta commented Aug 21, 2025 •

edited

Loading