Skip to content

fix: do not validate auth if auth is not defined#4962

Merged
Skarlso merged 1 commit intoexternal-secrets:mainfrom
Skarlso:fix-nil-panic-in-vault
Jul 2, 2025
Merged

fix: do not validate auth if auth is not defined#4962
Skarlso merged 1 commit intoexternal-secrets:mainfrom
Skarlso:fix-nil-panic-in-vault

Conversation

@Skarlso
Copy link
Copy Markdown
Contributor

@Skarlso Skarlso commented Jun 27, 2025 •
edited
Loading

Problem Statement

What is the problem you're trying to solve?

Related Issue

Fixes this nil panic:

k8s.io/apimachinery/pkg/util/runtime.logPanic
	/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.33.1/pkg/util/runtime/runtime.go:142
sigs.k8s.io/controller-runtime/pkg/webhook/admission.(*Webhook).Handle.func1
	/home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.21.0/pkg/webhook/admission/webhook.go:163
runtime.gopanic
	/opt/hostedtoolcache/go/1.24.4/x64/src/runtime/panic.go:792
runtime.panicmem
	/opt/hostedtoolcache/go/1.24.4/x64/src/runtime/panic.go:262
runtime.sigpanic
	/opt/hostedtoolcache/go/1.24.4/x64/src/runtime/signal_unix.go:925
github.com/external-secrets/external-secrets/pkg/provider/vault.(*Provider).ValidateStore
	/home/runner/work/external-secrets/external-secrets/pkg/provider/vault/validate.go:65
github.com/external-secrets/external-secrets/apis/externalsecrets/v1.validateStore
	/home/runner/work/external-secrets/external-secrets/apis/externalsecrets/v1/secretstore_validator.go:72
github.com/external-secrets/external-secrets/apis/externalsecrets/v1.(*GenericStoreValidator).ValidateCreate
	/home/runner/work/external-secrets/external-secrets/apis/externalsecrets/v1/secretstore_validator.go:42
sigs.k8s.io/controller-runtime/pkg/webhook/admission.(*validatorForType).Handle
	/home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.21.0/pkg/webhook/admission/validator_custom.go:94
sigs.k8s.io/controller-runtime/pkg/webhook/admission.(*Webhook).Handle
	/home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.21.0/pkg/webhook/admission/webhook.go:181
sigs.k8s.io/controller-runtime/pkg/webhook/admission.(*Webhook).ServeHTTP
	/home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.21.0/pkg/webhook/admission/http.go:119
sigs.k8s.io/controller-runtime/pkg/webhook/internal/metrics.InstrumentedHook.InstrumentHandlerInFlight.func1
	/home/runner/go/pkg/mod/github.com/prometheus/client_golang@v1.22.0/prometheus/promhttp/instrument_server.go:60
net/http.HandlerFunc.ServeHTTP
	/opt/hostedtoolcache/go/1.24.4/x64/src/net/http/server.go:2294
github.com/prometheus/client_golang/prometheus/promhttp.InstrumentHandlerCounter.func1
	/home/runner/go/pkg/mod/github.com/prometheus/client_golang@v1.22.0/prometheus/promhttp/instrument_server.go:147
net/http.HandlerFunc.ServeHTTP
	/opt/hostedtoolcache/go/1.24.4/x64/src/net/http/server.go:2294
github.com/prometheus/client_golang/prometheus/promhttp.InstrumentHandlerDuration.func2
	/home/runner/go/pkg/mod/github.com/prometheus/client_golang@v1.22.0/prometheus/promhttp/instrument_server.go:109
net/http.HandlerFunc.ServeHTTP
	/opt/hostedtoolcache/go/1.24.4/x64/src/net/http/server.go:2294
net/http.(*ServeMux).ServeHTTP
	/opt/hostedtoolcache/go/1.24.4/x64/src/net/http/server.go:2822
net/http.serverHandler.ServeHTTP
	/opt/hostedtoolcache/go/1.24.4/x64/src/net/http/server.go:3301
net/http.(*conn).serve
	/opt/hostedtoolcache/go/1.24.4/x64/src/net/http/server.go:2102

Proposed Changes

How do you like to solve the issue and why?

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

@Skarlso Skarlso requested a review from a team as a code owner June 27, 2025 10:15
@Skarlso Skarlso requested a review from moolen June 27, 2025 10:15
@gusfcarvalho
Copy link
Copy Markdown
Member

gusfcarvalho commented Jun 27, 2025

I believe we should also add minProperties=1 maxProperties=2 to auth block in vault 🤔 . as one can only set the vaultNamespace + any other auth type in there.

@gusfcarvalho
Copy link
Copy Markdown
Member

gusfcarvalho commented Jun 27, 2025

Also, there are a couple of other providers that could benefit the same validation... oracle is one of them IIRC 🤔

@Skarlso
Copy link
Copy Markdown
Contributor Author

Skarlso commented Jun 28, 2025

Sure. I can add that.

@Skarlso
fix: do not validate auth if auth is not defined
13ccb00 
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
@Skarlso Skarlso force-pushed the fix-nil-panic-in-vault branch from ec9f4c3 to 13ccb00 Compare July 1, 2025 10:14
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Jul 1, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@Skarlso
Copy link
Copy Markdown
Contributor Author

Skarlso commented Jul 1, 2025

@gusfcarvalho I can't unless I restructure because Auth has a Namespace in it that can be given together with another authentication method.

@Skarlso Skarlso merged commit cf4796d into external-secrets:main Jul 2, 2025
20 checks passed
alliseeisgold pushed a commit to alliseeisgold/external-secrets that referenced this pull request Jul 10, 2025
@Skarlso
fix: do not validate auth if auth is not defined (external-secrets#4962)
fd34d40 
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Signed-off-by: asrormirzoev <asrormirzoev@yandex-team.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@moolen moolen moolen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Skarlso @gusfcarvalho @moolen