Fix: ESO template crash when Kubernetes secret data is nil by ma-ble · Pull Request #3537 · external-secrets/external-secrets

ma-ble · 2024-05-29T12:56:27Z

Problem Statement

If you create a PushSecret that references a secret without a data key and uses it in the template, the eso pod crashes.

Related Issue

Fixes #3294

Proposed Changes

Add nil pointer check before assigning a value.

Checklist

I have read the contribution guidelines
All commits are signed with git commit --signoff
My changes have reasonable test coverage
All tests pass with make test
I ensured my PR is ready for review with make reviewable

Signed-off-by: MathiasBleimhofer <mathias.bleimhofer@deutschebahn.com>

…ret-data-nil

moolen · 2024-05-29T18:49:56Z

/ok-to-test sha=79c3e36302df107822745e42161561ace8b9a79c

moolen · 2024-05-29T18:53:46Z

pkg/template/v2/template.go

 		secret.Labels[k] = val
 	case esapi.TemplateTargetData:
-		secret.Data[k] = []byte(val)
+		if secret.Data != nil {


Shouldn't we create an empty map before we set the key to prevent the panic?
Otherwise we're not applying the value to the target secret

I'm not entirely sure about the use case, but similar to externalsecret, you can place the check in front of the template engine and then create an empty map. I adjusted the PR accordingly.

fix: revert: check if secret.Data is nil before assigning a value

…template values to the secret Signed-off-by: MathiasBleimhofer <mathias.bleimhofer@deutschebahn.com>

…ret-data-nil

moolen

Hey, i re-read the referenced issue #3294. Looking at stack traces and context provided there i suggest to change the function which modifies the Data/Labels/Annotations here and add a guard which creates a map if needed:

external-secrets/pkg/template/v2/template.go

Lines 72 to 82 in c3bf442

    
           func applyToTarget(k, val string, target esapi.TemplateTarget, secret *corev1.Secret) { 
        
           	switch target { 
        
           	case esapi.TemplateTargetAnnotations: 
        
           		secret.Annotations[k] = val 
        
           	case esapi.TemplateTargetLabels: 
        
           		secret.Labels[k] = val 
        
           	case esapi.TemplateTargetData: 
        
           		secret.Data[k] = []byte(val) 
        
           	default: 
        
           	} 
        
           }

Not only could the .Data be nil, also Annotations or Labels could be nil.

…ield in the secret is nil Signed-off-by: MathiasBleimhofer <mathias.bleimhofer@deutschebahn.com>

…ret-data-nil

sonarqubecloud · 2024-06-03T09:51:45Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

moolen

LGTM! Thanks for the tests as well 🙇

This PR contains the following updates: | Package | Update | Change | |---|---|---| | [external-secrets](https://github.com/external-secrets/external-secrets) | patch | `0.9.18` -> `0.9.19` | --- ### Release Notes <details> <summary>external-secrets/external-secrets (external-secrets)</summary> ### [`v0.9.19`](https://github.com/external-secrets/external-secrets/releases/tag/v0.9.19) [Compare Source](external-secrets/external-secrets@v0.9.18...v0.9.19) Image: `ghcr.io/external-secrets/external-secrets:v0.9.19` Image: `ghcr.io/external-secrets/external-secrets:v0.9.19-ubi` Image: `ghcr.io/external-secrets/external-secrets:v0.9.19-ubi-boringssl` #### What's Changed - bump 0.9.18 by [@gusfcarvalho](https://github.com/gusfcarvalho) in external-secrets/external-secrets#3492 - Fix flaky ES controller test by [@shuheiktgw](https://github.com/shuheiktgw) in external-secrets/external-secrets#3493 - Raise error when unknown key specified in template by [@shuheiktgw](https://github.com/shuheiktgw) in external-secrets/external-secrets#3480 - Update ci.yml with ppc64le arch support by [@IdanAdar](https://github.com/IdanAdar) in external-secrets/external-secrets#3500 - chore(deps): bump regex from 2024.5.10 to 2024.5.15 in /hack/api-docs by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3512 - chore(deps): bump zipp from 3.18.1 to 3.18.2 in /hack/api-docs by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3511 - chore(deps): bump platformdirs from 4.2.1 to 4.2.2 in /hack/api-docs by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3510 - chore(deps): bump golang from `6d71b7c` to `5c56bd4` in /e2e by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3508 - chore(deps): bump github/codeql-action from 3.25.4 to 3.25.5 by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3507 - chore(deps): bump codecov/codecov-action from 4.3.1 to 4.4.0 by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3506 - chore(deps): bump actions/checkout from 4.1.5 to 4.1.6 by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3505 - chore(deps): bump golangci/golangci-lint-action from 5.3.0 to 6.0.1 by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3504 - chore(deps): bump golang from `2a88224` to `f1fe698` by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3503 - chore(deps): bump mkdocs-material from 9.5.22 to 9.5.23 in /hack/api-docs by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3509 - chore: update dependencies by [@eso-service-account-app](https://github.com/eso-service-account-app) in external-secrets/external-secrets#3513 - ppc64le additions by [@IdanAdar](https://github.com/IdanAdar) in external-secrets/external-secrets#3535 - chore(deps): bump ubi8/ubi-minimal from `2fa47fa` to `f729a7f` by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3523 - doc(BitWarden): extends the liveness timeout by [@acolombier](https://github.com/acolombier) in external-secrets/external-secrets#3542 - chore(deps): bump mkdocs-material from 9.5.23 to 9.5.24 in /hack/api-docs by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3526 - chore(deps): bump zipp from 3.18.2 to 3.19.0 in /hack/api-docs by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3527 - chore(deps): bump requests from 2.31.0 to 2.32.2 in /hack/api-docs by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3528 - chore(deps): bump codecov/codecov-action from 4.4.0 to 4.4.1 by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3530 - chore(deps): bump aquasecurity/trivy-action from 0.20.0 to 0.21.0 by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3531 - chore(deps): bump github/codeql-action from 3.25.5 to 3.25.6 by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3532 - chore(deps): bump ubi8/ubi-minimal from `f729a7f` to `9e458f4` by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3543 - chore(deps): bump golang from `f1fe698` to `b8ded51` by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3522 - chore(deps): bump alpine from 3.19.1 to 3.20.0 in /e2e by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3525 - chore(deps): bump watchdog from 4.0.0 to 4.0.1 in /hack/api-docs by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3529 - chore(deps): bump alpine from 3.19 to 3.20 in /hack/api-docs by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3533 - chore: update dependencies by [@eso-service-account-app](https://github.com/eso-service-account-app) in external-secrets/external-secrets#3534 - Fix: ESO template crash when Kubernetes secret data is nil by [@ma-ble](https://github.com/ma-ble) in external-secrets/external-secrets#3537 - chore(deps): bump alpine from `c5b1261` to `77726ef` by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3524 - Add CA-Bundle to pemToPkcs12 output by [@WatcherWhale](https://github.com/WatcherWhale) in external-secrets/external-secrets#3494 - chore(deps): bump docker/login-action from 3.1.0 to 3.2.0 by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3544 - chore(deps): bump github/codeql-action from 3.25.6 to 3.25.7 by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3545 - chore(deps): bump certifi from 2024.2.2 to 2024.6.2 in /hack/api-docs by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3546 - chore(deps): bump mkdocs-material from 9.5.24 to 9.5.25 in /hack/api-docs by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3547 - chore(deps): bump zipp from 3.19.0 to 3.19.1 in /hack/api-docs by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3549 - chore(deps): bump requests from 2.32.2 to 2.32.3 in /hack/api-docs by [@dependabot](https://github.com/dependabot) in external-secrets/external-secrets#3548 - chore: update dependencies by [@eso-service-account-app](https://github.com/eso-service-account-app) in external-secrets/external-secrets#3550 #### New Contributors - [@acolombier](https://github.com/acolombier) made their first contribution in external-secrets/external-secrets#3542 - [@ma-ble](https://github.com/ma-ble) made their first contribution in external-secrets/external-secrets#3537 **Full Changelog**: external-secrets/external-secrets@v0.9.18...v0.9.19 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://git.internal/nrdufour/home-ops/pulls/471 Co-authored-by: Renovate <renovate@ptinem.casa> Co-committed-by: Renovate <renovate@ptinem.casa>

…secrets#3537) * fix: check if secret.Data is nil before assigning a value Signed-off-by: MathiasBleimhofer <mathias.bleimhofer@deutschebahn.com> Signed-off-by: Bude8 <henryblee8@gmail.com>

…secrets#3537) * fix: check if secret.Data is nil before assigning a value Signed-off-by: MathiasBleimhofer <mathias.bleimhofer@deutschebahn.com> Signed-off-by: Bill Hamilton <bill.hamilton@delinea.com>

ma-ble and others added 2 commits May 10, 2024 14:21

fix: check if secret.Data is nil before assigning a value

42b70e7

Signed-off-by: MathiasBleimhofer <mathias.bleimhofer@deutschebahn.com>

Merge branch 'external-secrets:main' into fix/template-crash-when-sec…

79c3e36

…ret-data-nil

ma-ble requested a review from a team as a code owner May 29, 2024 12:56

ma-ble requested a review from moolen May 29, 2024 12:56

moolen reviewed May 29, 2024

View reviewed changes

ma-ble and others added 3 commits May 31, 2024 14:00

--signoff

c29c46d

fix: revert: check if secret.Data is nil before assigning a value

fix: create empty map for secret data if secret data is nil to apply …

bc002cd

…template values to the secret Signed-off-by: MathiasBleimhofer <mathias.bleimhofer@deutschebahn.com>

Merge branch 'external-secrets:main' into fix/template-crash-when-sec…

f193086

…ret-data-nil

moolen reviewed May 31, 2024

View reviewed changes

ma-ble and others added 2 commits June 3, 2024 11:50

fix: add guard for data, annotations and labels when the respective f…

4c2075f

…ield in the secret is nil Signed-off-by: MathiasBleimhofer <mathias.bleimhofer@deutschebahn.com>

Merge branch 'external-secrets:main' into fix/template-crash-when-sec…

e3ca135

…ret-data-nil

moolen approved these changes Jun 3, 2024

View reviewed changes

moolen merged commit 30e1887 into external-secrets:main Jun 3, 2024

renovate bot mentioned this pull request Jul 15, 2024

Update all gomod dependencies blesswinsamuel/infra-base#45

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix: ESO template crash when Kubernetes secret data is nil #3537

Fix: ESO template crash when Kubernetes secret data is nil #3537
moolen merged 7 commits intoexternal-secrets:mainfrom
ma-ble:fix/template-crash-when-secret-data-nil

ma-ble commented May 29, 2024

Uh oh!

moolen commented May 29, 2024

Uh oh!

moolen May 29, 2024

Uh oh!

ma-ble May 31, 2024

Uh oh!

moolen left a comment

Uh oh!

sonarqubecloud bot commented Jun 3, 2024

Uh oh!

moolen left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	func applyToTarget(k, val string, target esapi.TemplateTarget, secret *corev1.Secret) {
	switch target {
	case esapi.TemplateTargetAnnotations:
	secret.Annotations[k] = val
	case esapi.TemplateTargetLabels:
	secret.Labels[k] = val
	case esapi.TemplateTargetData:
	secret.Data[k] = []byte(val)
	default:
	}
	}

Uh oh!

Conversation

ma-ble commented May 29, 2024

Problem Statement

Related Issue

Proposed Changes

Checklist

Uh oh!

moolen commented May 29, 2024

Uh oh!

moolen May 29, 2024

Choose a reason for hiding this comment

Uh oh!

ma-ble May 31, 2024

Choose a reason for hiding this comment

Uh oh!

moolen left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 3, 2024

Quality Gate passed

Uh oh!

moolen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants