Conversation
c150db0 to
233df5b
Compare
|
|
This pr is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Signed-off-by: Moritz Johner <beller.moritz@googlemail.com>
Signed-off-by: Moritz Johner <beller.moritz@googlemail.com>
Signed-off-by: Moritz Johner <beller.moritz@googlemail.com>
Signed-off-by: Moritz Johner <beller.moritz@googlemail.com>
Signed-off-by: Moritz Johner <beller.moritz@googlemail.com>
Signed-off-by: Moritz Johner <beller.moritz@googlemail.com>
Signed-off-by: Moritz Johner <beller.moritz@googlemail.com>
Signed-off-by: Moritz Johner <beller.moritz@googlemail.com>
Signed-off-by: Moritz Johner <beller.moritz@googlemail.com>
Signed-off-by: Moritz Johner <beller.moritz@googlemail.com>
Signed-off-by: Moritz Johner <beller.moritz@googlemail.com>
Signed-off-by: Moritz Johner <beller.moritz@googlemail.com>
| ) | ||
|
|
||
| const ( | ||
| errSecretStoreNotReady = "%s %q is not ready" |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical
| } | ||
|
|
||
| if meta.Spec.SecretType == "" { | ||
| meta.Spec.SecretType = "String" |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical
| // Declares metadata information for pushing secrets to AWS Secret Store. | ||
| const ( | ||
| SecretPushFormatKey = "secretPushFormat" | ||
| SecretPushFormatString = "string" |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical
| const ( | ||
| SecretPushFormatKey = "secretPushFormat" | ||
| SecretPushFormatString = "string" | ||
| SecretPushFormatBinary = "binary" |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical
| } | ||
|
|
||
| if meta.Spec.SecretPushFormat == "" { | ||
| meta.Spec.SecretPushFormat = SecretPushFormatBinary |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical
| // Format with goimports/gofmt | ||
| formattedMain, err := formatGoCode(mainContent) | ||
| if err != nil { | ||
| log.Printf("Warning: Failed to format main.go for %s: %v", config.Provider.Name, err) |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 23 days ago
In general, to avoid log forgery when logging user-controlled data, sanitize or encode that data before logging: for plain-text logs, strip newline (\n) and carriage return (\r) characters (and optionally other control characters) so an attacker cannot break or inject additional log lines. Then log the sanitized value instead of the raw user input.
For this specific code, the only problematic sink in the shown snippet is the usage of config.Provider.Name in log messages, notably on line 197: log.Printf("Warning: Failed to format main.go for %s: %v", config.Provider.Name, err). We should introduce a small helper that takes a string and returns a sanitized version with \n and \r removed (using strings.ReplaceAll per the recommendation), and then use that helper when interpolating config.Provider.Name into log messages. This preserves the semantics—provider names will still appear as-is for normal values—while mitigating the injection vector.
Concretely:
- Add a function, e.g.
sanitizeLogString(s string) string, near the bottom ofproviders/v2/hack/generate-provider-main.go(or anywhere appropriate in that file) that removes\nand\rusingstrings.ReplaceAll. - Update the log call around line 197 to pass
sanitizeLogString(config.Provider.Name)instead ofconfig.Provider.Name. - Optionally, for consistency and additional safety, apply the same helper to the other log calls that include
config.Provider.Namein this file (lines 191 and 214), since they share the same tainted source. This doesn’t require new imports becausestringsis already imported.
No new external dependencies are needed; the standard library strings package is already in use.
| @@ -188,13 +188,13 @@ | ||
| // Generate main.go | ||
| mainContent, err := executeTemplate(mainTemplate, templateData) | ||
| if err != nil { | ||
| log.Fatalf("Failed to generate main.go for %s: %v", config.Provider.Name, err) | ||
| log.Fatalf("Failed to generate main.go for %s: %v", sanitizeLogString(config.Provider.Name), err) | ||
| } | ||
|
|
||
| // Format with goimports/gofmt | ||
| formattedMain, err := formatGoCode(mainContent) | ||
| if err != nil { | ||
| log.Printf("Warning: Failed to format main.go for %s: %v", config.Provider.Name, err) | ||
| log.Printf("Warning: Failed to format main.go for %s: %v", sanitizeLogString(config.Provider.Name), err) | ||
| formattedMain = mainContent // Use unformatted if formatting fails | ||
| } | ||
|
|
||
| @@ -211,7 +206,7 @@ | ||
| // Generate Dockerfile | ||
| dockerContent, err := executeTemplate(dockerfileTemplate, templateData) | ||
| if err != nil { | ||
| log.Fatalf("Failed to generate Dockerfile for %s: %v", config.Provider.Name, err) | ||
| log.Fatalf("Failed to generate Dockerfile for %s: %v", sanitizeLogString(config.Provider.Name), err) | ||
| } | ||
|
|
||
| dockerPath := filepath.Join(providerDir, "Dockerfile") | ||
| @@ -284,6 +279,14 @@ | ||
| return &config, nil | ||
| } | ||
|
|
||
| // sanitizeLogString removes line breaks from strings before logging to prevent | ||
| // log forgery or confusion caused by user-controlled values. | ||
| func sanitizeLogString(s string) string { | ||
| s = strings.ReplaceAll(s, "\n", "") | ||
| s = strings.ReplaceAll(s, "\r", "") | ||
| return s | ||
| } | ||
|
|
||
| func loadTemplate(name string) (*template.Template, error) { | ||
| content, err := embeddedFS.ReadFile(name) | ||
| if err != nil { |
| // Format with goimports/gofmt | ||
| formattedMain, err := formatGoCode(mainContent) | ||
| if err != nil { | ||
| log.Printf("Warning: Failed to format main.go for %s: %v", config.Provider.Name, err) |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Signed-off-by: Moritz Johner <beller.moritz@googlemail.com>
Signed-off-by: Moritz Johner <beller.moritz@googlemail.com>
Signed-off-by: Moritz Johner <beller.moritz@googlemail.com>
|


Summary
Overview
The v2 provider enables out-of-process providers using gRPC, allowing a single provider codebase to expose multiple APIs (e.g., AWS SecretsManager, ParameterStore, ECR, STS) without requiring modifications to existing v1 provider implementations.
Open Issues
ADR Deliverables
main.goFlow Diagram
graph TB subgraph "ESO Controller (In-Process)" A[ExternalSecret Controller] -->|"GetProviderSecretData()"| B[Client Manager] B -->|"Check storeRef.kind == Provider"| C{Is v2 Provider?} C -->|Yes| D[Create gRPC Client] C -->|No| E[Use v1 Provider In-Process] D --> F[V2ClientWrapper<br/>v1→v2 Adapter] F -->|"Implements<br/>esv1.SecretsClient"| G[gRPC Client] end subgraph "gRPC Communication" G -->|"GetSecretRequest<br/>{ProviderRef, RemoteRef}"| H[mTLS Connection] end subgraph "Provider Server (Out-of-Process)" H --> I[AdapterServer<br/>v2→v1 Adapter] I -->|"1. Parse ProviderRef<br/>(apiVersion + kind)"| J{Resolve GVK} J -->|"SecretsManager"| K[AWS v1 Provider] J -->|"ParameterStore"| K J -->|"ECRAuthToken"| K J -->|"STSSessionToken"| K K -->|"2. Fetch CR<br/>(e.g., SecretsManager)"| M[v1.SyntheticStore] M -->|"4. Call v1 provider"| N[provider.NewClient] N -->|"5. Get secret"| O[AWS API] O -->|"6. Return secret data"| H end style A fill:#e1f5ff style F fill:#ffe1f5 style I fill:#fff5e1 style K fill:#e1ffe11. Client-Side: v2 → v1 Adapter (In-Process)
How ExternalSecret Controller Uses gRPC Clients
In
externalsecret_controller_secret.go, the reconciler uses the Client Manager to obtain provider clients:Client Manager: Creating gRPC Clients
When a
SecretStoreRefhaskind: Provider, the manager creates a gRPC client:The
getV2ProviderClientmethod:ProviderresourceV2ClientWrapper(the v2→v1 adapter)V2ClientWrapper: Implementing v1.SecretsClient
The wrapper adapts the gRPC
v2.Providerinterface to the v1SecretsClientinterface:gRPC Client: Making RPC Calls
The gRPC client converts v1 types to protobuf and makes RPC calls:
2. Multiple APIs via ProviderReference Mapping
Separate CRDs for Each AWS Service
The AWS v2 provider exposes separate Kubernetes Custom Resources for different services:
Example:
SecretsManagerCRD:Future expansion will include
ParameterStore,ECRAuthToken,STSSessionToken, etc., all served by the same gRPC server process.3. Server-Side: v1 → v2 Adapter (Out-of-Process)
AdapterServer: Mapping ProviderRef to v1 Clients
The gRPC server uses
AdapterServerto map incomingProviderReference(apiVersion + kind) to v1 provider implementations:Resolving Provider from ProviderReference
The server resolves the v1 provider based on GVK:
GetSecret RPC Handler
The server receives GetSecret requests and delegates to v1 providers:
AWS Provider Main: Single Process, Multiple APIs
The AWS provider's main function sets up the mapping:
To add ParameterStore, you'd simply extend the mapping:
And update the
specMapperto handle the new Kind.Example manifest