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 <[email protected]>

…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 <[email protected]>

…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 <[email protected]>

…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 🙇

…secrets#3537) * fix: check if secret.Data is nil before assigning a value Signed-off-by: MathiasBleimhofer <[email protected]> Signed-off-by: Bude8 <[email protected]>

…secrets#3537) * fix: check if secret.Data is nil before assigning a value Signed-off-by: MathiasBleimhofer <[email protected]> Signed-off-by: Bill Hamilton <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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!

Comments

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!

Comments

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