fix: use SignJwt instead of GenerateAccessToken for Vault GCP Workload Identity auth#5647
Conversation
29f8352 to
55bb4f7
Compare
55bb4f7 to
4fbd554
Compare
|
@Skarlso can you review this one? Sorry for tagging you xD |
|
I'll get to this eventually. :D Sorry for the wait time. :) |
|
@SamuelMolling Can you please take care of the linter and unit test errors? :) Thanks. |
|
And the sonar issues? :D Please 🙇 |
|
@Skarlso can you run again? just to check |
| GetObjectMeta() *metav1.ObjectMeta | ||
| GetTypeMeta() *metav1.TypeMeta | ||
| GetKind() string | ||
| GetName() string |
There was a problem hiding this comment.
Why is this needed is there is already GetObjectMeta?
Also, definitely, please do not update our main interface for all stores. :)
|
Why are all the provider go mods updated? |
795b284 to
459cf79
Compare
|
Hey @Skarlso, The ~100 files are mostly (93) go.mod/go.sum dependency updates across the workspace modules. This happened because the fix required adding a replace directive in providers/v1/vault/go.mod to use the local GCP provider, which triggered go mod tidy to update all module dependencies. The actual fix involves only 7 source files (auth.go, workload_identity.go, auth_gcp.go, client.go, tests, and docs). All tests and builds pass. |
|
@moolen Hey hai hello. How do we deal with the GCP stuff? :) Should we extract that into util? We don't want an artificial dependency on the GCP module, because that would break this provider. If someone wants the provider, it would mean they would have to build GCP as well. That's not desired. |
|
@moolen any return? |
|
@Skarlso Is there any other way for us to find this out? |
|
Okay, so... I don't think we like that. :D If something is shared between one or two modules it should be extracted and put into the runtime section or something like that. Two modules should not co-depend. Do you think you can do that? |
WalkthroughAdds a shared GCP auth package (runtime/gcp/auth) with token-source and signed-JWT utilities, refactors Secret Manager to delegate to it, extends Vault with GCP IAM JWT login, moves/adds comprehensive auth tests, updates client structs, and applies widespread Go module dependency bumps. Changes
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (47)
📒 Files selected for processing (3)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2026-01-09T19:14:48.246ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (5)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
732daa2 to
0612d77
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
runtime/gcp/auth/workload_identity.go (2)
202-206: Consider using an HTTP client with a timeout.Using
http.DefaultClientwithout a timeout could cause the request to hang indefinitely if the STS endpoint is unresponsive. Consider passing a client with a configured timeout, similar to hownewMetadataClient(line 331-333) creates a client with a 5-second timeout.🔎 Suggested approach
- idBindToken, err := w.idBindTokenGenerator.Generate(ctx, http.DefaultClient, resp.Status.Token, idPool, idProvider) + httpClient := &http.Client{Timeout: 30 * time.Second} + idBindToken, err := w.idBindTokenGenerator.Generate(ctx, httpClient, resp.Status.Token, idPool, idProvider)Alternatively, consider injecting the HTTP client via the
workloadIdentitystruct for better testability.
404-406: Consider including response body in error for debugging.When the STS endpoint returns a non-200 status, the error message only includes the status code. Including a truncated response body could aid debugging token exchange failures.
🔎 Optional improvement
if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("could not get idbindtoken token, status: %v", resp.StatusCode) + defer func() { _ = resp.Body.Close() }() + body, _ := io.ReadAll(io.LimitReader(resp.Body, 256)) + return nil, fmt.Errorf("could not get idbindtoken token, status: %v, body: %s", resp.StatusCode, string(body)) }runtime/gcp/go.mod (1)
3-3: Go version mismatch with root and most modules.This module specifies
go 1.25.1while the rootgo.modand most other modules usego 1.25.5. Note that it is aligned with its parentruntime/go.mod(also1.25.1). Consider aligning to1.25.5for consistency across the codebase if a uniform version is preferred.runtime/gcp/auth/workload_identity_federation.go (1)
346-368: Consider refactoring error accumulation pattern.The pattern of prepending a static error then checking
len(errs) > 1works but is unconventional. A clearer approach would be to accumulate errors normally and prepend the prefix only when returning.🔎 Suggested refactor
func validateExternalAccountConfig(config *externalaccount.Config, wif *esv1.GCPWorkloadIdentityFederation) error { var errs []error - errs = append(errs, fmt.Errorf("invalid %s config", externalAccountCredentialType)) if config.Audience == "" { errs = append(errs, fmt.Errorf("audience is empty")) } if config.ServiceAccountImpersonationURL != "" && !serviceAccountImpersonationURLRegex.MatchString(config.ServiceAccountImpersonationURL) { errs = append(errs, fmt.Errorf("service_account_impersonation_url \"%s\" does not have expected value", config.ServiceAccountImpersonationURL)) } if config.TokenURL != workloadIdentityTokenURL { errs = append(errs, fmt.Errorf("token_url \"%s\" must match %s", config.TokenURL, workloadIdentityTokenURL)) } if config.CredentialSource != nil { errs = append(errs, validateCredConfigCredentialSource(config.CredentialSource, wif)...) } - if len(errs) > 1 { + if len(errs) > 0 { + errs = append([]error{fmt.Errorf("invalid %s config", externalAccountCredentialType)}, errs...) return errors.Join(errs...) } return nil }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
providers/v1/gcp/go.sumis excluded by!**/*.sumproviders/v1/vault/go.sumis excluded by!**/*.sumruntime/gcp/go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
go.modproviders/v1/gcp/go.modproviders/v1/gcp/secretmanager/auth.goproviders/v1/vault/auth_gcp.goproviders/v1/vault/client.goproviders/v1/vault/go.modruntime/gcp/auth/auth.goruntime/gcp/auth/workload_identity.goruntime/gcp/auth/workload_identity_federation.goruntime/gcp/go.mod
🧰 Additional context used
🧬 Code graph analysis (4)
runtime/gcp/auth/auth.go (1)
providers/v1/gcp/secretmanager/auth.go (2)
NewTokenSource(36-38)GenerateSignedJWTForVault(42-44)
providers/v1/vault/auth_gcp.go (2)
providers/v1/gcp/secretmanager/auth.go (2)
NewTokenSource(36-38)GenerateSignedJWTForVault(42-44)runtime/gcp/auth/auth.go (2)
NewTokenSource(43-73)GenerateSignedJWTForVault(78-93)
providers/v1/vault/client.go (1)
providers/v1/vault/util/vault.go (4)
Client(51-61)Auth(31-33)Logical(42-47)Token(36-39)
runtime/gcp/auth/workload_identity.go (1)
runtime/gcp/auth/auth.go (1)
CloudPlatformRole(35-35)
🔇 Additional comments (25)
go.mod (1)
493-493: LGTM!The replace directive follows the established pattern for local module references and correctly wires the new
runtime/gcpmodule into the dependency graph.runtime/gcp/auth/workload_identity.go (2)
269-286: Potential for double IAM client creation.The call to
TokenSource(line 271) may internally create and close an IAM client when a GCP service account annotation is present (see lines 217-223). Then this method creates another IAM client at line 280. While this works correctly, it results in two separate IAM client round-trips.This is acceptable if code reuse is prioritized over efficiency, but worth noting for future optimization if this path becomes performance-sensitive.
1-47: Well-structured implementation with proper interfaces.The file introduces clean abstractions (
IamClient,MetadataClient) that enable testability and separation of concerns. The token exchange flow is correctly implemented with proper error handling and metrics instrumentation.providers/v1/vault/client.go (1)
41-53: LGTM!The new
gcpSignedJWTfield is appropriately scoped as private and well-documented. It correctly stores the signed JWT between the setup phase (where it's generated) and the login phase (where it's used).providers/v1/gcp/secretmanager/auth.go (1)
34-44: LGTM!Clean refactoring that delegates to the shared
runtime/gcp/authmodule. The wrapper functions maintain the existing API surface while enabling code reuse across providers.runtime/gcp/go.mod (1)
98-101: LGTM!The replace directives correctly reference the local
apisandruntimemodules using appropriate relative paths for this module's location in the directory structure.providers/v1/vault/go.mod (2)
62-62: LGTM!The new indirect dependency on
runtime/gcpcorrectly reflects the Vault provider's use of the shared GCP authentication logic.
177-178: LGTM!The replace directive correctly wires the local
runtime/gcpmodule for development, following the same pattern used by other providers.providers/v1/vault/auth_gcp.go (4)
67-70: LGTM!Good early return pattern that separates the signed JWT flow from the traditional GCP auth methods, making the code flow clearer and easier to follow.
145-166: Correct signed JWT workflow for Workload Identity.The implementation properly generates a signed JWT with the required claims (sub, aud, exp) and stores it for later use during Vault login. This fixes the original issue where an access token was incorrectly used instead of a signed JWT.
214-241: Well-implemented direct JWT login.The
loginWithSignedJWTmethod correctly POSTs to the Vault GCP auth endpoint with the role and JWT, validates the response, and sets the client token. The error handling and logging are appropriate.
128-143: LGTM!The service account key auth correctly delegates to the shared
gcpauth.NewTokenSourceand follows the existing pattern of setting the access token via environment variable.providers/v1/gcp/go.mod (2)
57-57: LGTM!The new dependency on
runtime/gcpcorrectly reflects the refactoring where GCP authentication logic is now shared via the runtime module.
132-133: LGTM!The replace directive correctly wires the local
runtime/gcpmodule, enabling the GCP provider to use the shared authentication logic.runtime/gcp/auth/workload_identity_federation.go (7)
1-38: LGTM!License header and imports are well-organized. The dependencies are appropriate for the GCP workload identity federation functionality.
40-127: LGTM!Type definitions are well-structured. Good documentation on
credentialsFilenoting its source from the oauth2 library.
143-171: LGTM!The TokenSource method correctly validates that exactly one credential source is provided and enforces the audience requirement for
serviceAccountRefandawsSecurityCredentials. The mutual exclusivity check is well-implemented.
246-252: Good security consideration.The check preventing use of the operator's auto-mounted service account token is an important security measure. This ensures users must explicitly configure service account references rather than inadvertently using elevated operator permissions.
309-342: LGTM!AWS security credentials handling correctly validates required fields (
aws_access_key_idandaws_secret_access_key) while allowing optionalaws_session_token. Namespace scoping is properly handled for cluster-scoped stores.
370-385: Good security practice blocking executable credential sources.Restricting the use of executables is the right call from a security standpoint since their behavior cannot be validated at configuration time.
406-428: LGTM!Token reader implementations are clean. The
SubjectTokenmethod appropriately validates that the requested audience and token type match expected values before generating the token. Metrics observation is correctly placed.runtime/gcp/auth/auth.go (4)
17-31: LGTM!Good package documentation explaining the module's purpose. Imports are appropriate for the authentication orchestration functionality.
75-93: LGTM!The
GenerateSignedJWTForVaultfunction properly validates the required workload identity configuration and handles the lifecycle correctly. Since a JWT is returned as a string value, there are no concerns about the deferred close affecting the result.
95-114: LGTM!The
serviceAccountTokenSourcehelper follows the established fallback pattern (returningnil, nilwhen not configured) and correctly uses Google's JWT config for service account authentication.
49-61: Remove concern about TokenSource lifecycle dependency.The
workloadIdentity.TokenSource()method returnsoauth2.StaticTokenSource, which wraps only a static token value (anoauth2.Tokenstruct containing the access token string). The returnedTokenSourceis completely independent from theworkloadIdentityobject and does not hold any references to its resources. ClosingworkloadIdentityafter the method returns does not affect the usability of the returnedStaticTokenSource.
- Remove stored context from refreshableTokenSource struct - Use context.Background() with 30s timeout for each refresh operation - This ensures token refresh works independently of original request context - Addresses review feedback about artificial context limitation Signed-off-by: Samuel Molling <samuelmolling@gmail.com>
8afd578 to
761c2fb
Compare
|
@SamuelMolling I'm so sorry, you were right in the first run, after talking with Jakob, I do agree that the auth is too granular. Can you please move it back to runtime/gcp and then an auth package? After that, I think this will be okay to merge finally. :) |
|
I just realised, why are we creating another module? runtime package is already shared..... |
|
Crap. I should have realised this 🤦 I'm going to try and fix this PR. |
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
e2e/go.mod (1)
70-254: Fix k8s.io/client-go version mismatch: v1.5.2 is severely outdated.The e2e/go.mod file has a critical version inconsistency. Line 75 requires
k8s.io/client-go v1.5.2(from Kubernetes 1.5, released 2016–2017), while the replace block at line 15 maps it tov0.34.0and all other k8s.io packages are pinned tov0.34.1(Kubernetes 1.34, 2024). This creates an incompatible dependency state and leaves the client exposed to ~8 years of unpatched security vulnerabilities. Update the require statement to align with v0.34.0 or v0.34.1 to match the rest of the Kubernetes ecosystem dependencies.providers/v1/vault/go.mod (1)
13-13: Remove unused direct dependency on GCP provider module from go.mod.The Vault provider's
go.modlists a direct dependency ongithub.com/external-secrets/external-secrets/providers/v1/gcp, but vault provider code does not import or use it. The vault provider correctly imports and usesgithub.com/external-secrets/external-secrets/runtime/gcp/authfor GCP authentication instead (seeauth_gcp.go). Remove the unusedproviders/v1/gcpdependency fromproviders/v1/vault/go.modto align with the shared auth package architecture.
🤖 Fix all issues with AI agents
In `@generators/v1/grafana/go.mod`:
- Around line 71-76: Revert the incidental indirect dependency version bumps in
the Grafana generator module by removing or restoring the updated indirect
entries (e.g., golang.org/x/net, golang.org/x/oauth2, golang.org/x/sync,
golang.org/x/sys, golang.org/x/term, golang.org/x/text) in
generators/v1/grafana/go.mod so the module’s go.mod matches the versions
required by its direct dependencies; if the Vault change legitimately requires
newer versions across shared modules, move these intentional upgrades into a
separate dependency-maintenance PR and run `go mod tidy` to regenerate clean
indirect entries only after confirming direct-module needs.
♻️ Duplicate comments (8)
providers/v1/kubernetes/go.mod (1)
79-88: Consistent dependency bumps across modules.These indirect dependency version bumps are identical to those in other provider modules and align with the broader module graph updates in this PR.
providers/v1/conjur/go.mod (1)
97-106: Consistent dependency bumps across modules.These indirect dependency version bumps are identical to those in other provider modules and align with the broader module graph updates in this PR.
generators/v1/sts/go.mod (1)
90-99: Consistent dependency bumps across modules.These indirect dependency version bumps are identical to those in other provider modules and align with the broader module graph updates in this PR.
providers/v1/beyondtrust/go.mod (1)
85-94: Consistent dependency bumps across modules.These indirect dependency version bumps are identical to those in other provider modules and align with the broader module graph updates in this PR.
providers/v1/senhasegura/go.mod (1)
54-62: Consider separating dependency updates from the functional fix.Similar to the IBM provider, these indirect dependency bumps appear unrelated to the senhasegura provider's functionality or the Vault GCP authentication fix. This increases the PR's footprint without clear necessity.
generators/v1/ecr/go.mod (1)
75-83: Consider separating dependency updates from the functional fix.The ECR generator module is receiving indirect dependency updates unrelated to its AWS ECR functionality or the Vault GCP authentication fix.
providers/v1/doppler/go.mod (1)
79-88: Consider separating dependency updates from the functional fix.The Doppler provider is receiving indirect dependency updates unrelated to its functionality or the Vault GCP authentication fix.
generators/v1/webhook/go.mod (1)
77-86: Consider separating dependency updates from the functional fix.The webhook generator is receiving indirect dependency updates unrelated to its functionality or the Vault GCP authentication fix. This is consistent with the pattern observed across all reviewed modules.
Recommendation: Consider verifying which modules genuinely need these dependency updates for the Vault GCP authentication functionality, and defer the remaining updates to a separate dependency maintenance PR. This would:
- Reduce review complexity
- Minimize merge conflicts
- Make the functional fix easier to backport if needed
- Separate concerns between feature changes and dependency maintenance
🧹 Nitpick comments (1)
providers/v1/ibm/go.mod (1)
91-100: Separate transitive dependency updates from the functional fix.These indirect dependency bumps (golang.org/x/*, google.golang.org/protobuf, gomodules.xyz/jsonpatch) are transitive updates pulled by direct dependencies like k8s.io/apimachinery and sigs.k8s.io/controller-runtime. They're unrelated to the IBM provider's functionality or the stated PR objective (fixing Vault GCP Workload Identity authentication).
Mixing widespread transitive dependency updates with a focused bug fix increases review complexity and merge risk. These updates should be deferred to a separate maintenance PR to keep the change history clear.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (49)
e2e/go.sumis excluded by!**/*.sumgenerators/v1/acr/go.sumis excluded by!**/*.sumgenerators/v1/cloudsmith/go.sumis excluded by!**/*.sumgenerators/v1/ecr/go.sumis excluded by!**/*.sumgenerators/v1/gcr/go.sumis excluded by!**/*.sumgenerators/v1/grafana/go.sumis excluded by!**/*.sumgenerators/v1/quay/go.sumis excluded by!**/*.sumgenerators/v1/sts/go.sumis excluded by!**/*.sumgenerators/v1/vault/go.sumis excluded by!**/*.sumgenerators/v1/webhook/go.sumis excluded by!**/*.sumproviders/v1/akeyless/go.sumis excluded by!**/*.sumproviders/v1/alibaba/go.sumis excluded by!**/*.sumproviders/v1/aws/go.sumis excluded by!**/*.sumproviders/v1/azure/go.sumis excluded by!**/*.sumproviders/v1/barbican/go.sumis excluded by!**/*.sumproviders/v1/beyondtrust/go.sumis excluded by!**/*.sumproviders/v1/bitwarden/go.sumis excluded by!**/*.sumproviders/v1/chef/go.sumis excluded by!**/*.sumproviders/v1/cloudru/go.sumis excluded by!**/*.sumproviders/v1/conjur/go.sumis excluded by!**/*.sumproviders/v1/delinea/go.sumis excluded by!**/*.sumproviders/v1/device42/go.sumis excluded by!**/*.sumproviders/v1/doppler/go.sumis excluded by!**/*.sumproviders/v1/dvls/go.sumis excluded by!**/*.sumproviders/v1/fake/go.sumis excluded by!**/*.sumproviders/v1/fortanix/go.sumis excluded by!**/*.sumproviders/v1/gcp/go.sumis excluded by!**/*.sumproviders/v1/github/go.sumis excluded by!**/*.sumproviders/v1/gitlab/go.sumis excluded by!**/*.sumproviders/v1/ibm/go.sumis excluded by!**/*.sumproviders/v1/infisical/go.sumis excluded by!**/*.sumproviders/v1/keepersecurity/go.sumis excluded by!**/*.sumproviders/v1/kubernetes/go.sumis excluded by!**/*.sumproviders/v1/ngrok/go.sumis excluded by!**/*.sumproviders/v1/onboardbase/go.sumis excluded by!**/*.sumproviders/v1/onepassword/go.sumis excluded by!**/*.sumproviders/v1/onepasswordsdk/go.sumis excluded by!**/*.sumproviders/v1/oracle/go.sumis excluded by!**/*.sumproviders/v1/passbolt/go.sumis excluded by!**/*.sumproviders/v1/passworddepot/go.sumis excluded by!**/*.sumproviders/v1/previder/go.sumis excluded by!**/*.sumproviders/v1/pulumi/go.sumis excluded by!**/*.sumproviders/v1/scaleway/go.sumis excluded by!**/*.sumproviders/v1/secretserver/go.sumis excluded by!**/*.sumproviders/v1/senhasegura/go.sumis excluded by!**/*.sumproviders/v1/vault/go.sumis excluded by!**/*.sumproviders/v1/volcengine/go.sumis excluded by!**/*.sumproviders/v1/webhook/go.sumis excluded by!**/*.sumproviders/v1/yandex/go.sumis excluded by!**/*.sum
📒 Files selected for processing (49)
e2e/go.modgenerators/v1/acr/go.modgenerators/v1/cloudsmith/go.modgenerators/v1/ecr/go.modgenerators/v1/gcr/go.modgenerators/v1/grafana/go.modgenerators/v1/quay/go.modgenerators/v1/sts/go.modgenerators/v1/vault/go.modgenerators/v1/webhook/go.modproviders/v1/akeyless/go.modproviders/v1/alibaba/go.modproviders/v1/aws/go.modproviders/v1/azure/go.modproviders/v1/barbican/go.modproviders/v1/beyondtrust/go.modproviders/v1/bitwarden/go.modproviders/v1/chef/go.modproviders/v1/cloudru/go.modproviders/v1/conjur/go.modproviders/v1/delinea/go.modproviders/v1/device42/go.modproviders/v1/doppler/go.modproviders/v1/dvls/go.modproviders/v1/fake/go.modproviders/v1/fortanix/go.modproviders/v1/gcp/go.modproviders/v1/github/go.modproviders/v1/gitlab/go.modproviders/v1/ibm/go.modproviders/v1/infisical/go.modproviders/v1/keepersecurity/go.modproviders/v1/kubernetes/go.modproviders/v1/ngrok/go.modproviders/v1/onboardbase/go.modproviders/v1/onepassword/go.modproviders/v1/onepasswordsdk/go.modproviders/v1/oracle/go.modproviders/v1/passbolt/go.modproviders/v1/passworddepot/go.modproviders/v1/previder/go.modproviders/v1/pulumi/go.modproviders/v1/scaleway/go.modproviders/v1/secretserver/go.modproviders/v1/senhasegura/go.modproviders/v1/vault/go.modproviders/v1/volcengine/go.modproviders/v1/webhook/go.modproviders/v1/yandex/go.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- generators/v1/gcr/go.mod
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-09T19:14:48.246Z
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:14:48.246Z
Learning: In external-secrets CRDs, new providers are added to the GA v1 schema only; v1beta1 is maintained for backward compatibility and does not receive new features (e.g., DVLS should be v1-only).
Applied to files:
providers/v1/github/go.modproviders/v1/gcp/go.mod
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze project (go, autobuild)
🔇 Additional comments (37)
providers/v1/scaleway/go.mod (1)
76-82: Routine indirect dependency updates — no concerns with the version bumps themselves.These minor and patch version updates to golang.org/x packages and google.golang.org/protobuf are standard maintenance changes. The updates propagating to the Scaleway provider from Vault+GCP changes align with the architectural concern already noted in the PR discussion about provider module boundaries.
Also applies to: 85-85
providers/v1/delinea/go.mod (1)
90-96: This review comment is based on an incorrect understanding of the PR's actual objective.The commit message is "remove all the go mod changes and let's see what breaks"—this PR is testing the removal of go.mod changes, not adding unnecessary dependency updates. The Delinea provider's dependency versions are identical to all other providers in the workspace due to the replace directives in the root go.mod, not because of unrelated changes. The original comment's concerns about isolating dependency updates into a separate PR are misdirected since the PR itself is investigating the impact of removing these changes.
Likely an incorrect or invalid review comment.
providers/v1/volcengine/go.mod (1)
79-88: These indirect dependencies are legitimate transitive dependencies from shared workspace packages, not unnecessary scope expansion.The golang.org/x/*, google.golang.org/protobuf, and gomodules.xyz/jsonpatch updates are pulled transitively through volcengine's shared dependencies (particularly k8s.io packages v0.34.1 and controller-runtime v0.22.3). The identical versions appearing across vault and volcengine providers confirms these are workspace-level dependency synchronization, not arbitrary scope creep.
That said, while the updates are justified:
- Verify these versions are free from security vulnerabilities
- Confirm volcengine functionality has been regression tested (given the transitive update chain touches core k8s client-go and controller-runtime)
providers/v1/infisical/go.mod (1)
17-17: Infisical's indirect dependency updates are legitimate cascades from shared modules.The Infisical provider's go.mod changes are not "unintentional side effects" or "unrelated updates." Both the Infisical and Vault providers depend on identical shared modules (
apisandruntime), which are local replacements in the root go.mod. When these shared modules update their transitive dependencies, all dependent providers must resolve those same versions—this is standard Go module behavior in monorepos. The identical versions across providers (e.g.,cloud.google.com/go/auth v0.18.0in both Vault and Infisical) confirm this cascade rather than independent updates.Isolating dependency updates in separate PRs may not be practical here since
go mod tidywill automatically propagate shared dependency changes across all modules. Consider documenting the shared module dependency relationship if it's not obvious to reviewers.providers/v1/keepersecurity/go.mod (1)
72-81: All Go module versions verified exist and are secure. No known CVEs apply to the specified versions:
golang.org/x/crypto v0.46.0,golang.org/x/net v0.48.0, andgoogle.golang.org/protobuf v1.36.11are all patched against known vulnerabilities.- The cascading updates across unrelated providers (keepersecurity affected by vault→gcp changes) is expected behavior from
go mod tidyand confirms the workspace-wide dependency coupling already noted in the PR discussion.providers/v1/gitlab/go.mod (1)
83-96: Verified: All indirect dependency versions are valid with no critical security concerns.These indirect dependency updates have been verified:
- golang.org/x/crypto v0.46.0: No vulnerabilities (SSH CVEs fixed in v0.45.0)
- google.golang.org/grpc v1.78.0: Only minor breaking change (rejects unbracketed colons in URLs on Go 1.26+), unlikely to impact this provider
- golang.org/x/oauth2 v0.34.0: Version is after the v0.27.0 fix for CVE-2025-22868, no vulnerability exposure
The 6-minor-version gRPC jump is safe to proceed with.
providers/v1/fortanix/go.mod (1)
76-85: Maintainer is already testing necessity of these updates.These golang.org/x dependency updates are transitive dependencies from Kubernetes packages (k8s.io/apimachinery, controller-runtime, etc.) and represent normal Go module resolution, not workspace misconfiguration.
Your scope concern is valid. However, the recent commit "remove all the go mod changes and let's see what breaks" (f1d2c74) shows that you're already testing whether these updates are necessary—which is the right approach. The previous versions were:
- golang.org/x/crypto: v0.43.0 → v0.46.0
- golang.org/x/net: v0.46.0 → v0.48.0
- golang.org/x/oauth2: v0.32.0 → v0.34.0
- golang.org/x/sync: v0.17.0 → v0.19.0
- golang.org/x/sys: v0.37.0 → v0.39.0
- golang.org/x/term: v0.36.0 → v0.38.0
- golang.org/x/text: v0.30.0 → v0.32.0
- google.golang.org/protobuf: v1.36.10 → v1.36.11
Keep monitoring the test results to determine if these updates can be excluded or should remain.
providers/v1/passbolt/go.mod (1)
79-88: These dependency versions are secure and do not pose security risks.All verified versions (golang.org/x/crypto v0.46.0, golang.org/x/net v0.48.0, golang.org/x/oauth2 v0.34.0, golang.org/x/sync v0.19.0, golang.org/x/sys v0.39.0, golang.org/x/term v0.38.0, golang.org/x/text v0.32.0, golang.org/x/time v0.14.0, and google.golang.org/protobuf v1.36.11) have no unpatched CVEs. Recent SSH vulnerabilities in golang.org/x/crypto (CVE-2025-58181, CVE-2025-47914) were fixed in v0.45.0 and earlier, and protobuf's CVE-2024-24786 was fixed in v1.33.0.
However, the architectural question remains valid: confirm whether the passbolt provider should have its own go.mod file, especially if this PR's primary objective is the Vault GCP authentication fix.
providers/v1/barbican/go.mod (1)
56-64: Verify these dependency versions align with shared module versions.These indirect dependencies are legitimate transitive deps from the
runtimemodule that barbican depends on. However, they show version divergence from theruntimeandapismodules:
apis: golang.org/x/net v0.38.0, oauth2 v0.27.0, protobuf v1.36.5runtime: golang.org/x/net v0.43.0, oauth2 v0.30.0, protobuf v1.36.8barbican: golang.org/x/net v0.48.0, oauth2 v0.34.0, protobuf v1.36.11This suggests separate
go mod tidyoperations. Consider synchronizing versions across shared modules to avoid unnecessary divergence in the dependency tree.Please verify:
- Whether this version divergence is intentional or should be consolidated
- That these versions are free from security vulnerabilities
providers/v1/azure/go.mod (1)
23-23: Rungo mod tidyto ensure consistency with go.sum.The golang.org/x/crypto v0.46.0 dependency is valid and exists (confirmed across other modules in the codebase). However, the Azure provider's go.sum is missing the h1 hash for v0.46.0—it only contains the .mod hash. This file should be tidied to include the complete hash entry.
The broader dependency updates across all 34 provider modules are due to the monorepo structure with replace directives in the root go.mod, which causes transitive updates to propagate to all providers when the dependency graph changes.
providers/v1/previder/go.mod (2)
1-11: "Previder" is not a typo—it is the correct name of a legitimate external secret vault service (Previder Secret Vault). This provider integration is stable, well-documented, and properly registered throughout the codebase. The module name, package structure, and external dependencygithub.com/previder/vault-cliare all correct. No changes are needed.Likely an incorrect or invalid review comment.
54-62: Dependency versions verified as secure.Verification confirms that all indirect dependencies in lines 54-62 are free from known security vulnerabilities:
golang.org/x/net v0.48.0— secure (includes fixes for CVE-2024-45338 and CVE-2025-22872)golang.org/x/oauth2 v0.34.0— secure (above minimum fixed version for CVE-2025-22868)google.golang.org/protobuf v1.36.11— secure (above minimum fixed version for CVE-2024-24786)- Other golang.org/x packages — no known vulnerabilities
providers/v1/device42/go.mod (1)
72-81: LGTM: Routine indirect dependency updates.The golang.org/x/* and protobuf version bumps are consistent with the broader dependency refresh across the PR. These transitive updates maintain alignment with the codebase's updated dependency graph.
providers/v1/onepassword/go.mod (1)
82-91: LGTM: Consistent dependency version alignment.The indirect dependency bumps match the versions observed across other modules in this PR, maintaining consistency in the dependency graph.
providers/v1/dvls/go.mod (1)
68-76: LGTM: Dependency updates align with PR-wide refresh.The updated indirect dependencies maintain version consistency with other modules. The partial update set suggests this module's other transitive dependencies were already current.
generators/v1/cloudsmith/go.mod (1)
72-81: LGTM: Standard transitive dependency maintenance.The golang.org/x/* and protobuf updates are consistent with the dependency refresh pattern across all modules in this PR.
providers/v1/onboardbase/go.mod (1)
76-85: LGTM: Dependency version updates complete the PR-wide alignment.The indirect dependency bumps maintain consistency with all other reviewed modules, ensuring a unified dependency graph across the codebase.
providers/v1/beyondtrust/go.mod (1)
1-110: Address the architectural concern about provider-to-provider dependencies.Based on the PR discussion, the widespread go.mod updates across ~93 dependency files stem from introducing a replace directive in providers/v1/vault/go.mod that creates a dependency from the Vault provider to the GCP provider module. This creates an undesired provider-to-provider co-dependency.
As discussed with the maintainers, the recommended approach is to move the shared GCP authentication logic to a common location (e.g., runtime/gcp with an auth package) rather than having providers depend on each other. This refactoring should resolve the need for these widespread module updates and establish a cleaner architectural boundary.
Have the linter issues, unit test failures, and Sonar issues mentioned in the PR discussion been resolved? Has the auth logic been moved to runtime/gcp as requested by the maintainers?
providers/v1/aws/go.mod (1)
97-106: Indirect dependency versions are current and secure.The bumped indirect dependencies (golang.org/x/crypto v0.46.0, golang.org/x/net v0.48.0, golang.org/x/oauth2 v0.34.0, and google.golang.org/protobuf v1.36.11) are the latest stable versions available as of December 2025 with no known security vulnerabilities. These versions represent routine maintenance updates to incorporate the latest security patches and improvements.
providers/v1/oracle/go.mod (1)
82-91: LGTM: Consistent indirect dependency updates.The dependency version bumps align with the coordinated update across the repository's Go modules. No functional changes to this provider module.
providers/v1/passworddepot/go.mod (1)
72-81: LGTM: Indirect dependency version alignment.The golang.org/x/* and protobuf version bumps are part of the repository-wide dependency graph update. No changes to direct dependencies or module behavior.
providers/v1/github/go.mod (2)
62-70: LGTM: Indirect dependency updates.The remaining indirect dependency version bumps are consistent with the repository-wide update.
11-11: golang.org/x/crypto is correctly promoted to a direct dependency.The package is imported in
providers/v1/github/client.go(golang.org/x/crypto/nacl/box), confirming this promotion to the direct require block is intentional and necessary.providers/v1/webhook/go.mod (1)
76-85: LGTM: Consistent dependency version bumps.The indirect dependency updates match the pattern across all reviewed provider modules. No functional changes to the webhook provider.
providers/v1/bitwarden/go.mod (1)
75-84: Approve indirect dependency version bumps.The golang.org/x/* and protobuf dependency updates are consistent with the broader dependency graph alignment across the repository. These are indirect dependencies updated transitively and are current releases (v0.46.0, v0.48.0, v1.36.11 are the latest published versions with no tracked security vulnerabilities).
providers/v1/chef/go.mod (1)
76-85: Architectural refactoring has been completed; reviewed provider modules are unaffected.The requested refactoring to move GCP authentication logic into runtime/gcp has been successfully implemented. Vault's auth_gcp.go now correctly imports from
runtime/gcp/authrather than from the provider module, and the runtime/gcp/auth package contains the complete authentication implementation.The reviewed provider modules (chef, secretserver, ngrok, pulumi, alibaba) have no problematic provider-to-provider cross-dependencies. Their indirect dependency updates for golang.org/x/* and google.golang.org/protobuf are standard transitive version bumps cascading from their legitimate direct dependencies and the updated runtime module. These updates are necessary and safe.
providers/v1/cloudru/go.mod (2)
13-13: Verify compatibility of gRPC v1.78.0 dial target formatting in cloudru provider.The gRPC v1.78.0 update includes a breaking change that rejects dial targets with unbracketed colons in hostnames when running on Go 1.26+. This affects any code using
grpc.Dial()or resolver results with malformed host:port syntax or unbracketed IPv6 addresses.Audit the cloudru provider for dial target construction to ensure:
- IPv6 addresses are properly bracketed (e.g.,
[::1]:50051)- Hostnames don't contain unescaped colons
- All target URLs follow proper host:port syntax
Run
go testwith v1.78.0 on Go 1.26+ to verify no runtime failures occur.
70-81: Dependency versions verified as valid and secure.The cloudru provider's indirect dependencies across golang.org/x/* and google.golang.org/* packages have been confirmed. Key packages (golang.org/x/crypto v0.46.0, google.golang.org/protobuf v1.36.11, golang.org/x/sys v0.39.0) are legitimate tagged releases with no unfixed security vulnerabilities. This is consistent with the broader dependency alignment in this PR.
providers/v1/vault/go.mod (1)
28-158: Dependencies are valid and secure; revise verification approach.The updated dependencies in the Vault provider module are valid versions with no known security vulnerabilities. However, the verification commands in the original comment are inaccurate—
go list -m -versionsis not a valid Go command.For future verification, use standard approaches:
go mod tidyto validate dependencies resolve correctly- Review Go security advisories at go.dev/security for tracked CVEs
- Use tools like
govulncheck ./...to scan for known vulnerabilities in the module's dependenciesThe sampled critical packages are secure:
golang.org/x/crypto v0.46.0— no known vulnerabilitiesgolang.org/x/net v0.48.0— no known vulnerabilitiesgoogle.golang.org/grpc v1.78.0— no known advisoriesLikely an incorrect or invalid review comment.
providers/v1/akeyless/go.mod (1)
19-126: Dependency versions are valid and current with no known security vulnerabilities.All sampled critical packages have been verified:
- golang.org/x/crypto v0.46.0: Safe (earlier CVEs fixed in v0.35.0 and v0.45.0)
- google.golang.org/grpc v1.78.0: Safe (HTTP/2 DoS CVE-2023-44487 fixed in prior versions)
- golang.org/x/net v0.48.0: Safe (normal tagged release with prior fixes)
- google.golang.org/api v0.260.0: Safe (released Jan 14, 2026)
- Go 1.25.5: Official release (published December 2, 2025)
These indirect dependency updates from the GCP/Kubernetes ecosystem are legitimate and compatible with the specified Go version.
generators/v1/vault/go.mod (1)
20-169: The Vault generator module's indirect dependencies are valid and secure. Verification confirms that all checked versions—including google.golang.org/protobuf v1.36.11, google.golang.org/grpc v1.78.0, and golang.org/x/crypto v0.46.0—are current releases with no known CVEs or security vulnerabilities. These versions include patches for previously identified issues (protobuf DoS fix in v1.33.0, grpc HTTP/2 fixes in v1.56+, and crypto SSH fixes in v0.45.0+).providers/v1/gcp/go.mod (3)
31-103: Large number of indirect dependency updates require validation.Numerous indirect dependencies have been updated, including security-sensitive packages like
golang.org/x/crypto(v0.43.0 → v0.46.0) andgolang.org/x/net(v0.46.0 → v0.48.0). According to the PR discussion, these updates resulted from adding a replace directive in the Vault provider's go.mod.Please ensure:
- The transitive dependency updates don't introduce breaking changes or incompatibilities
- All tests pass with the updated dependency graph
- Security-sensitive packages (crypto, net) are reviewed for relevant security advisories
Based on PR comments, you mentioned that tests and builds pass. Please confirm that the full test suite has been run with these dependency updates.
116-119: Replace directives are appropriate for workspace structure.The replace directives correctly reference the local workspace modules using relative paths, which is standard practice for Go workspaces.
Note: The architectural concern raised in PR discussion about cross-provider dependencies (Vault depending on GCP) would manifest in
providers/v1/vault/go.mod, not in this file. This GCP provider's go.mod remains clean and doesn't create any problematic dependencies.
16-20: Dependency versions verified as valid and secure.All updated versions have been validated:
- golang.org/x/oauth2 v0.34.0 — valid, no known vulnerabilities (prior CVE-2025-22868 fixed in 0.27.0)
- google.golang.org/api v0.260.0 — valid, stable
- google.golang.org/grpc v1.78.0 — valid, no known vulnerabilities
- google.golang.org/protobuf v1.36.11 — valid, no known vulnerabilities
These versions are safe to use.
generators/v1/quay/go.mod (1)
72-81: Remove this review comment—the premise is not supported by the code.The quay generator does not depend on the vault provider, and these golang.org/x/* versions are transitive dependencies from the shared kubernetes ecosystem (k8s.io/client-go, sigs.k8s.io/controller-runtime), not a result of cross-provider coupling. Both modules independently require these versions.
Additionally, the claim that "~93 dependency files were updated" is factually incorrect—the repository contains 58 total go.mod files, not 93.
Likely an incorrect or invalid review comment.
providers/v1/fake/go.mod (1)
77-83: No action required — all indirect dependency versions are safe and current.The versions in the go.mod file (golang.org/x/{crypto,net,oauth2,sync,sys,term,text} and google.golang.org/protobuf) are all above any patched versions for known vulnerabilities. Recent security advisories (CVE-2025-22869, CVE-2025-22868, CVE-2025-22872, CVE-2024-45338, and protobuf DoS issues) were fixed in earlier versions, and these versions are unaffected.
generators/v1/acr/go.mod (1)
106-112: Verify dependency versions and security advisories.These indirect dependency bumps (golang.org/x/crypto v0.46.0, golang.org/x/net v0.48.0, golang.org/x/oauth2 v0.34.0, golang.org/x/sync v0.19.0, golang.org/x/sys v0.39.0, golang.org/x/term v0.38.0, golang.org/x/text v0.32.0, google.golang.org/protobuf v1.36.11) are confirmed to be present at the specified lines. Ensure these versions are valid and free from known vulnerabilities by checking the Go vulnerability database and verifying against the latest available versions.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
|
Okay, this is utterly broken. I will clean this up. |
|
Okay, so here is the PR #5828 If it goes green you need to just overwrite everything in this branch with that content and force push a completely new one over this. Or I can attribute you in that Pr. Sorry it got so wild in the end. I should have realised things earlier. 🤦 |
|
Oh nooooo. This will have to update all the modules because of the runtime module update and |
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
|
|
/ok-to-test sha=dcea3eced3c644c6f72810bc324ab234c0589c66 |
|
hm, did something change? |
|
Actually, the e2e failure is unrelated. |
|
Ain't it a rollercoaster? |
|
Will you work on deps updates separately or did i get things wrong ? |
|
Depds update needs to happen because of the runtime update I'm afraid. Because of GCP we have new dependencies, that will bleed out into the other modules because of the |
|
I lost some packages here xD |
|
@SamuelMolling What do you mean? I thought they are now updated correctly because of the runtime module update. :) |
| @@ -0,0 +1,132 @@ | |||
| /* | |||
There was a problem hiding this comment.
we should not use code shared between two providers into the runtime module. this module is intended for things that are shared across most of , if not all providers.
There was a problem hiding this comment.
The fact that this PR is touching every single dependency for me is a red flag.
I would think it would be more appropriate to create a new common/gcp/v1 module and add all of this stuff there - then have gcp/vault to import from common/gcp/v1.
There was a problem hiding this comment.
yes - this would probably mean first introducing a PR with a copy of the methods we want to move - and then introduce a PR that removes the (now duplicate) code and uses the shared logic appropriately 😅
|
Until we have proper tagging, it's okay if the created module is referenced by commit sha. It should live under I think Jakob also said something akin to that. So I believe this would be the right path. ( I knew I felt bad here with this. :D ) Thanks peeps for pointing it out. Alright. Let's cut this up. First, create a PR with the shared code in the module. Then, create a PR for the two vault and gcp using the same thing. And that should be it then. Sorry about this back-and-forth if you don't wish to do that, I totally understand and I will take over. I appreciate your patience on this @SamuelMolling! |
Hey @Skarlso, I’m a bit swamped this week and also next week 😅 |
|
Yah, no worries, I'll do this and break this up for you using your code. :) Thank you so much for sticking with this. :) 🙇 |
|
I will do this during my Friday office hours this week. :) |
Description
This PR fixes the Vault GCP IAM authentication when using Workload Identity by generating a properly signed JWT instead of using an OAuth2 access token.
Problem
When using Vault's GCP auth method with Workload Identity, the External Secrets Operator was passing an OAuth2 access token instead of a signed JWT, causing the error:
Solution
SignJwtmethod toIamClientinterfaceSignedJWTForVaultmethod to generate JWT with correct claims (sub, aud, exp)GenerateSignedJWTForVaultpublic function in auth.goauth_gcp.goto use signed JWT for Workload IdentityloginWithSignedJWTmethod to authenticate directly with Vault APIChanges
providers/v1/gcp/secretmanager/auth.go- Added public function to generate signed JWT for Vaultproviders/v1/gcp/secretmanager/workload_identity.go- Added SignJwt method and JWT generation logicproviders/v1/vault/auth_gcp.go- Modified to use signed JWT and added login methodproviders/v1/vault/client.go- Added gcpSignedJWT field to store the JWTTesting
Tested successfully with:
Related Issues
Fixes the JWT parsing error when using Vault GCP auth with Workload Identity.
This PR fixes Vault GCP IAM authentication when using Workload Identity by generating and using a properly signed JWT (via SignJwt) instead of an OAuth2 access token, resolving the error "unable to parse signed JWT: go-jose/go-jose: compact JWS format must have three parts."
Key changes
Notes