Implement secret management for gateway controller #751
Conversation
WalkthroughAdds AES‑GCM encryption providers and key management, a ProviderManager and SecretService with full CRUD and storage, a PolicyResolver to inject decrypted secret values into policies, config/validation for encryption/secrets, DB/schema and SQLite storage methods, API handlers/OpenAPI for /secrets, Dockerfile asset copy, and integration tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as API Client
participant Handler as Secret Handler
participant Service as Secret Service
participant Manager as Provider Manager
participant Provider as AES‑GCM Provider
participant Storage as SQLite Storage
Client->>Handler: POST /secrets (plaintext)
Handler->>Service: CreateSecret(params)
Service->>Manager: Encrypt(plaintext)
Manager->>Provider: Encrypt(plaintext)
Provider->>Provider: generate nonce & produce ciphertext
Provider-->>Manager: EncryptedPayload
Manager-->>Service: EncryptedPayload
Service->>Storage: SaveSecret(model with ciphertext)
Storage-->>Service: Saved
Service-->>Handler: Secret (plaintext in response)
Handler-->>Client: 201 Created
sequenceDiagram
participant APIServer as API Server
participant Resolver as PolicyResolver
participant SecretSvc as Secret Service
participant Manager as Provider Manager
participant Provider as AES‑GCM Provider
APIServer->>Resolver: ResolvePolicies(apiConfig)
Resolver->>Resolver: Identify resolve paths in schemas
Resolver->>SecretSvc: Get(secretHandle)
SecretSvc->>Manager: Decrypt(encrypted payload)
Manager->>Provider: Decrypt(ciphertext)
Provider->>Provider: extract nonce & decrypt
Provider-->>Manager: plaintext
Manager-->>SecretSvc: plaintext
SecretSvc-->>Resolver: plaintext value
Resolver->>Resolver: Substitute plaintext into policy params
Resolver-->>APIServer: Resolved apiConfig
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
gateway/gateway-controller/pkg/api/handlers/policy_ordering_test.go (1)
29-49: Use a properly initialized SecretService stub to prevent nil panics when secret resolution is tested.
&secrets.SecretService{}leaves all fields (storage, providerManager, logger) nil. TheGet()method immediately callss.logger.Info()at line 161 without nil-guards, which will panic if secret resolution is triggered in future x-resolve tests. Either use theNewSecretsService()constructor with a nop logger and in-memory storage, or create a lightweight test stub with safe defaults.gateway/gateway-controller/api/openapi.yaml (1)
3632-3880: Align secret size limits and value-return semantics across schemas.There are conflicting limits and descriptions:
SecretConfigData.valueis capped at 8192 and says “never returned,” whileSecretCreateRequest/SecretUpdateRequestallow 10240 and responses clearly include the value.SecretListResponsealso claims “metadata” but the schema is a string list. This is an API contract inconsistency.✅ Minimal alignment proposal (example)
- value: - type: string - description: Secret value (stored securely and never returned in API responses) - minLength: 1 - maxLength: 8192 + value: + type: string + description: Secret value (stored securely; returned only on create/get, never in list responses) + minLength: 1 + maxLength: 8192- maxLength: 10240 + maxLength: 8192- maxLength: 10240 + maxLength: 8192gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
92-123:secretServiceandpolicyResolverare never assigned inNewAPIServer.The constructor accepts
secretServiceandpolicyResolverparameters (lines 92-93) but they are never assigned to theserverstruct. This will cause nil pointer panics when secret endpoints or policy resolution are invoked.🐛 Proposed fix
server := &APIServer{ store: store, db: db, snapshotManager: snapshotManager, policyManager: policyManager, policyDefinitions: policyDefinitions, parser: config.NewParser(), validator: validator, logger: logger, deploymentService: deploymentService, mcpDeploymentService: utils.NewMCPDeploymentService(store, db, snapshotManager), llmDeploymentService: utils.NewLLMDeploymentService(store, db, snapshotManager, templateDefinitions, deploymentService, &systemConfig.GatewayController.Router), apiKeyService: utils.NewAPIKeyService(store, db, apiKeyXDSManager), apiKeyXDSManager: apiKeyXDSManager, controlPlaneClient: controlPlaneClient, routerConfig: &systemConfig.GatewayController.Router, httpClient: &http.Client{Timeout: 10 * time.Second}, systemConfig: systemConfig, + secretService: secretService, + policyResolver: policyResolver, }
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/api/openapi.yaml`:
- Around line 1703-1735: The OpenAPI spec uses the "secrets" tag for the
/secrets endpoints (see operationId listSecrets and the POST create secret) but
lacks a corresponding tag definition; add a "secrets" entry to the global tags
list (with a short name "secrets" and a brief description like "Operations for
managing secrets") so the tag is declared for the spec and clients/docs can
display it correctly.
In `@gateway/gateway-controller/Dockerfile`:
- Around line 80-82: Remove the baked-in test keys by deleting the COPY
aesgcm-keys /app/aesgcm-keys instruction from the Dockerfile and update
build/runtime guidance to require providing key files via mounted volumes or
external secret stores; update documentation and examples to show using the
configuration/env var (e.g.,
GATEWAY_GATEWAY_CONTROLLER_ENCRYPTION_PROVIDERS_0_KEYS_0_FILEPATH=/mnt/secrets/key.bin)
to point to the mounted key path, and add a note in deployment docs that keys
must be mounted or supplied externally (and not included in image layers) for
production.
In `@gateway/gateway-controller/pkg/api/handlers/handlers.go`:
- Around line 2266-2270: The call to s.secretService.CreateSecret builds a
secrets.SecretParams without the CorrelationID, so update the struct literal
passed to CreateSecret to include the correlation ID (e.g., CorrelationID:
correlationID or CorrelationID: log.CorrelationID()) so service logging/tracing
receives it; locate the call to s.secretService.CreateSecret and add the
CorrelationID field to the secrets.SecretParams being constructed.
- Around line 2389-2421: UpdateSecret currently ignores the path parameter id
and relies only on the request body; modify UpdateSecret to parse the incoming
body to extract the secret handle/name (same place UpdateAPI validates id vs
body) and verify it matches the id path parameter before calling
s.secretService.UpdateSecret; if they differ return a 400 error indicating
path/body mismatch, otherwise proceed as before (ensure the validation
references the id parameter and the parsed metadata.name from the request body
and keep the existing logging and error handling).
- Around line 2286-2291: The CreateSecret response currently includes
secret.Value in the c.JSON response (the c.JSON call that returns id, value,
created_at, updated_at); remove the plaintext secret value from that response so
it only returns the secret handle/ID and timestamps (e.g., "id", "created_at",
"updated_at") and do not expose secret.Value. If you need to signal success to
callers, return a creation-only token or confirmation flag instead of the full
secret, and ensure any retrieval of the secret is done via a separate secure
fetch endpoint.
In `@gateway/gateway-controller/pkg/encryption/aesgcm/keymgmt.go`:
- Around line 31-60: NewKeyManager currently silently overwrites duplicate
KeyConfig.Version entries when populating km.keys; update NewKeyManager to
detect duplicate versions before calling km.loadKey (or immediately after) and
return an error if a version already exists to fail fast. Specifically, in
NewKeyManager, check km.keys[config.Version] (or track seen versions) for an
existing key and return fmt.Errorf("duplicate key version: %s", config.Version)
if found; keep existing behavior for setting km.primaryKey and km.primaryVersion
when i==0 and log as before.
In `@gateway/gateway-controller/pkg/encryption/aesgcm/provider.go`:
- Around line 52-60: The Encrypt method may dereference a nil key because
p.keyManager.GetPrimaryKey() is used without checking for nil; update
AESGCMProvider.Encrypt to validate the returned key from
keyManager.GetPrimaryKey() (check if key == nil) before accessing key.Data and
return a clear error (e.g., "no primary key available") so callers receive a
controlled error instead of a panic; reference the AESGCMProvider.Encrypt
function and the keyManager.GetPrimaryKey()/key.Data usage when making the
change.
In `@gateway/gateway-controller/pkg/resolver/policy_resolver.go`:
- Around line 299-333: The resolveSecretTemplates function can panic when
pr.secretsService is nil; add a nil check at the start of resolveSecretTemplates
(or before any call to pr.secretsService.Get) and return a clear validation
error (e.g., "secrets service not configured") instead of attempting to call
Get; ensure you still handle templates with no secret service by returning an
error immediately and do not attempt to decrypt, referencing
resolveSecretTemplates, pr.secretsService, and the Get/decryptedSecret.Value
usage so reviewers can locate the change.
- Around line 220-223: The current error handling after calling
resolvedConfig.Configuration.Spec.FromAPIConfigData(apiData) swallows the
original error by appending an empty config.ValidationError; change it to
capture and propagate the actual error details by creating a
config.ValidationError (or appropriate error wrapper) that includes the returned
err message and context (e.g., source API data or field), and append that
populated ValidationError to the errors slice so logs and API responses surface
the root cause; update any callers or tests expecting the previous empty
ValidationError shape if necessary.
In `@gateway/gateway-controller/pkg/secrets/service.go`:
- Around line 258-260: In the if !exists error branch, fix the typo and
terminology by replacing the message fmt.Errorf("secret with id '%s' does not
exists", handle) with a grammatically correct and consistent message such as
fmt.Errorf("secret with handle '%s' does not exist", handle); ensure the
placeholder still uses the existing handle variable and preserve the fmt.Errorf
usage.
- Around line 17-20: Add validation in CreateSecret to enforce the MaxSecretSize
limit: before encrypting or storing the secret, check the byte length of
secretConfig.Spec.Value and return a clear error (e.g., ErrSecretTooLarge or
fmt.Errorf) if it exceeds MaxSecretSize. Reference the MaxSecretSize constant
and the CreateSecret function and ensure the check occurs prior to any
encryption or memory-heavy operations on secretConfig.Spec.Value so oversized
secrets are rejected early.
In `@gateway/gateway-controller/pkg/storage/interface.go`:
- Around line 247-255: The interface methods DeleteSecret and SecretExists
currently use the parameter name id string but implementations and callers use
handle (user-provided identifier) and query the Secret.handle column; update the
interface signatures to use handle string instead of id string and adjust their
doc comments to reference "handle" to keep names consistent with implementations
(e.g., the SQLite methods that accept handle string). Ensure only the parameter
name changes (not types) so existing callers compile.
In `@gateway/gateway-controller/pkg/storage/sqlite.go`:
- Around line 1689-1713: In GetSecrets on SQLiteStorage, ensure the sql.Rows are
properly closed and any iteration error is checked: after s.db.Query(query)
defer rows.Close(), iterate as before, then after the loop check if err :=
rows.Err(); err != nil and return fmt.Errorf("failed during rows iteration: %w",
err) if non-nil; keep the existing error handling for Scan and the final return
of ids.
- Around line 1645-1687: The SaveSecret implementation currently does a
pre-check using SecretExists which creates a TOCTOU race; remove the pre-check
and rely on the DB INSERT to enforce uniqueness, then map unique-constraint DB
errors to ErrConflict (add a helper like isSecretUniqueConstraintError that
detects "UNIQUE constraint failed: secrets.handle" and "UNIQUE constraint
failed: secrets.id") inside SaveSecret so when s.db.Exec returns a
unique-constraint error you return fmt.Errorf("%w: secret with id '%s' already
exists", ErrConflict, secret.Handle) and otherwise log/return the original
error; keep the existing logging and field names (secret.Handle,
secret.Provider, secret.KeyVersion) and ensure SecretExists is no longer called
from SaveSecret.
In `@gateway/it/features/health.feature`:
- Around line 27-34: Feature uses the step "the healthy response status code
should be 200" but no step definition is registered; add a step definition that
either registers that exact phrase or maps it to the existing generic step in
assert_steps.go ("the response status code should be (\\d+)"). Implement a new
step binding (e.g., in the same steps file as other assertions) that calls the
existing assertion helper used by the generic step so both feature lines
(gateway health and router ready) validate status 200 without duplicating logic.
In `@gateway/it/features/secrets.feature`:
- Around line 282-289: Update the scenario title to match the actual limit
asserted in the test: change "Create secret with oversized value exceeding 10KB
limit" to "Create secret with oversized value exceeding 8192 characters (8KB)
limit" so the description aligns with the asserted error message "Secret value
must be at most 8192 characters"; ensure the scenario name in the feature file
and any related documentation use 8192 characters / 8KB consistently.
- Around line 484-502: The test assertion in the "Update non-existent secret
returns not found" scenario is checking for the brittle string "does not
exists"; update the expectation to match the API contract by asserting the
response body contains the canonical message "Secret not found." (ensure the
JSON "status" remains "error" and the status code expectation stays 404 in the
same scenario).
- Around line 80-86: The post-delete retrieval uses the wrong secret ID
("wso2-openai-keye") so the test doesn't confirm deletion; update the retrieval
step to use the correct ID "wso2-openai-key" (the same ID used in the delete
step) so the scenario checks for a 404 after deletion (look for the
Given/When/Then lines referencing "wso2-openai-keye" and replace it with
"wso2-openai-key").
🧹 Nitpick comments (8)
gateway/gateway-controller/pkg/config/secret_validator.go (1)
35-36: UnusedsupportedKindsfield.The
supportedKindsfield is initialized but never used. Line 81 hardcodes"Secret"instead of checking againstsupportedKinds. Either use the field or remove it for consistency withsupportedTypes.♻️ Option 1: Use the field
// Validate kind -if config.Kind != "Secret" { +if !slices.Contains(v.supportedKinds, config.Kind) { errors = append(errors, ValidationError{ Field: "kind", - Message: "Unsupported configuration kind (only 'Secret' is supported)", + Message: fmt.Sprintf("Unsupported configuration kind (supported kinds: %s)", strings.Join(v.supportedKinds, ", ")), }) }♻️ Option 2: Remove the unused field
type SecretValidator struct { // urlFriendlyNameRegex matches URL-safe characters for secret display names urlFriendlyNameRegex *regexp.Regexp - - // supportedKinds defines supported Secret kinds - supportedKinds []string // supportedTypes defines supported secret types supportedTypes []string } func NewSecretValidator() *SecretValidator { return &SecretValidator{ urlFriendlyNameRegex: regexp.MustCompile(`^[a-zA-Z0-9\-_. ]+$`), - supportedKinds: []string{"Secret"}, supportedTypes: []string{"default"}, } }Also applies to: 46-46, 80-86
gateway/gateway-controller/pkg/config/config.go (2)
308-323: Add validation for encryption providers and keys.Consider validating provider type, non-empty key lists, unique key versions, and non-empty file paths in
Config.Validate()to fail fast on misconfiguration.
505-517: Default encryption key path is shipped in Docker images but relies on filesystem availability.The key file is properly copied into the Docker image (
COPY aesgcm-keys /app/aesgcm-keys), so the default config works for standard deployments. However, the initialization is conditional only on whether providers are configured—not on whether key files exist. If anyone runs the binary outside Docker or with a custom config that includes the default provider configuration but lacks the key files, startup will fail withlog.Fatal.Consider making encryption providers opt-in with empty defaults, or document that key files must be present when the default provider config is used in non-Docker deployments.
gateway/gateway-controller/pkg/encryption/aesgcm/provider.go (1)
99-102: Ciphertext length validation is incomplete.The check validates
len(payload.Ciphertext) < NonceSize, but AES-GCM also requires at least the authentication tag (16 bytes). A ciphertext shorter thanNonceSize + gcm.Overhead()would fail duringgcm.Open()anyway, but an explicit check provides clearer error messages.♻️ Suggested improvement
- // Validate ciphertext length (must be at least nonce size + tag size) - if len(payload.Ciphertext) < NonceSize { - return nil, fmt.Errorf("ciphertext too short: %d bytes", len(payload.Ciphertext)) + // Validate ciphertext length (must be at least nonce size + tag size) + // AES-GCM tag is 16 bytes + minLength := NonceSize + 16 + if len(payload.Ciphertext) < minLength { + return nil, fmt.Errorf("ciphertext too short: %d bytes (minimum %d)", len(payload.Ciphertext), minLength) }gateway/gateway-controller/pkg/storage/interface.go (1)
236-239: Doc comment/parameter naming inconsistency.The
GetSecretmethod's doc says "retrieves a secret by ID" but the parameter is namedhandle. Looking at the Secret model (which has bothIDandHandlefields), this could cause confusion. The doc should clarify that this retrieves byHandle(user-provided identifier), not the internal UUIDID.📝 Suggested doc fix
- // GetSecret retrieves a secret by ID. + // GetSecret retrieves a secret by handle. // - // Returns error if the secret does not exist. + // Returns error if the secret with the given handle does not exist. GetSecret(handle string) (*models.Secret, error)gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
2315-2318: Redundant loop when copying slice.The loop copies elements from
idstosecretsListone by one, but since both are[]string, you can simplify this.♻️ Simplified code
- secretsList := make([]string, 0, len(ids)) - for _, id := range ids { - secretsList = append(secretsList, id) - } + secretsList := idsOr if a copy is intentionally needed:
secretsList := make([]string, len(ids)) copy(secretsList, ids)gateway/gateway-controller/pkg/secrets/service.go (1)
102-111: Minor:time.Now().UTC()called twice.Both
CreatedAtandUpdatedAtshould have the same timestamp for a new secret. Consider storing the result in a variable.♻️ Suggested improvement
+ now := time.Now().UTC() // Create secret model secret := &models.Secret{ ID: generateUUID(), Handle: handle, Value: "", // Don't store plaintext Provider: payload.Provider, KeyVersion: payload.KeyVersion, Ciphertext: []byte(ciphertext), - CreatedAt: time.Now().UTC(), - UpdatedAt: time.Now().UTC(), + CreatedAt: now, + UpdatedAt: now, }gateway/gateway-controller/pkg/encryption/manager.go (1)
103-107: Typo in comment: "curruption" → "corruption".✏️ Fix typo
// Find the provider that can decrypt this payload // If the provider name matches, use it to decrypt // If no match, return error indicating no provider found - // If decryption fails, return error indicating curruption or invalid data + // If decryption fails, return error indicating corruption or invalid data
| // UpdateSecret handles PUT /secrets/{id} | ||
| func (s *APIServer) UpdateSecret(c *gin.Context, id string) { | ||
| log := middleware.GetLogger(c, s.logger) | ||
|
|
||
| // Read request body | ||
| body, err := io.ReadAll(c.Request.Body) | ||
| if err != nil { | ||
| log.Error("Failed to read request body", zap.Error(err)) | ||
| c.JSON(http.StatusBadRequest, api.ErrorResponse{ | ||
| Status: "error", | ||
| Message: "Failed to read request body", | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| // Get correlation ID from context | ||
| correlationID := middleware.GetCorrelationID(c) | ||
|
|
||
| // Delegate to service which parses/validates/encrypt and persists | ||
| secret, err := s.secretService.UpdateSecret(secrets.SecretParams{ | ||
| Data: body, | ||
| ContentType: c.GetHeader("Content-Type"), | ||
| Logger: log, | ||
| }) | ||
| if err != nil { | ||
| log.Error("Failed to encrypt Secret", zap.Error(err)) | ||
| if strings.Contains(err.Error(), "does not exist") { | ||
| c.JSON(http.StatusNotFound, api.ErrorResponse{Status: "error", Message: err.Error()}) | ||
| } else { | ||
| c.JSON(http.StatusBadRequest, api.ErrorResponse{Status: "error", Message: err.Error()}) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
UpdateSecret ignores the id path parameter.
The handler accepts id as a path parameter but never uses it. Instead, it relies solely on the metadata.name from the request body. This could lead to confusion where a PUT to /secrets/foo with a body containing metadata.name: bar would update bar instead of foo. Consider validating that the path id matches the body's handle, similar to UpdateAPI (lines 546-560).
🐛 Proposed fix
func (s *APIServer) UpdateSecret(c *gin.Context, id string) {
log := middleware.GetLogger(c, s.logger)
// Read request body
body, err := io.ReadAll(c.Request.Body)
if err != nil {
log.Error("Failed to read request body", zap.Error(err))
c.JSON(http.StatusBadRequest, api.ErrorResponse{
Status: "error",
Message: "Failed to read request body",
})
return
}
+ // Parse to validate handle matches path parameter
+ var secretConfig api.SecretConfiguration
+ if err := s.parser.Parse(body, c.GetHeader("Content-Type"), &secretConfig); err == nil {
+ if secretConfig.Metadata.Name != "" && secretConfig.Metadata.Name != id {
+ log.Warn("Handle mismatch between path and body",
+ zap.String("path_id", id),
+ zap.String("body_handle", secretConfig.Metadata.Name))
+ c.JSON(http.StatusBadRequest, api.ErrorResponse{
+ Status: "error",
+ Message: fmt.Sprintf("Handle mismatch: path has '%s' but body metadata.name has '%s'", id, secretConfig.Metadata.Name),
+ })
+ return
+ }
+ }
+
// Get correlation ID from context
correlationID := middleware.GetCorrelationID(c)🤖 Prompt for AI Agents
In `@gateway/gateway-controller/pkg/api/handlers/handlers.go` around lines 2389 -
2421, UpdateSecret currently ignores the path parameter id and relies only on
the request body; modify UpdateSecret to parse the incoming body to extract the
secret handle/name (same place UpdateAPI validates id vs body) and verify it
matches the id path parameter before calling s.secretService.UpdateSecret; if
they differ return a 400 error indicating path/body mismatch, otherwise proceed
as before (ensure the validation references the id parameter and the parsed
metadata.name from the request body and keep the existing logging and error
handling).
| Scenario: Create secret with oversized value exceeding 10KB limit | ||
| Given I authenticate using basic auth as "admin" | ||
| When I create a secret with oversized value | ||
| Then the response status code should be 400 | ||
| And the response should be valid JSON | ||
| And the JSON response field "status" should be "error" | ||
| And the response body should contain "Secret value must be at most 8192 characters" | ||
|
|
There was a problem hiding this comment.
Clarify the oversized-value limit in the scenario text.
The scenario title says “10KB,” but the asserted message is 8192 characters (8KB). Align the description with the actual limit.
✏️ Suggested wording fix
- Scenario: Create secret with oversized value exceeding 10KB limit
+ Scenario: Create secret with oversized value exceeding 8KB limit📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Scenario: Create secret with oversized value exceeding 10KB limit | |
| Given I authenticate using basic auth as "admin" | |
| When I create a secret with oversized value | |
| Then the response status code should be 400 | |
| And the response should be valid JSON | |
| And the JSON response field "status" should be "error" | |
| And the response body should contain "Secret value must be at most 8192 characters" | |
| Scenario: Create secret with oversized value exceeding 8KB limit | |
| Given I authenticate using basic auth as "admin" | |
| When I create a secret with oversized value | |
| Then the response status code should be 400 | |
| And the response should be valid JSON | |
| And the JSON response field "status" should be "error" | |
| And the response body should contain "Secret value must be at most 8192 characters" |
🤖 Prompt for AI Agents
In `@gateway/it/features/secrets.feature` around lines 282 - 289, Update the
scenario title to match the actual limit asserted in the test: change "Create
secret with oversized value exceeding 10KB limit" to "Create secret with
oversized value exceeding 8192 characters (8KB) limit" so the description aligns
with the asserted error message "Secret value must be at most 8192 characters";
ensure the scenario name in the feature file and any related documentation use
8192 characters / 8KB consistently.
| Scenario: Update non-existent secret returns not found | ||
| Given I authenticate using basic auth as "admin" | ||
| When I update the secret "non-existent-update-secret-abc" with: | ||
| """ | ||
| apiVersion: gateway.api-platform.wso2.com/v1alpha1 | ||
| kind: Secret | ||
| metadata: | ||
| name: non-existent-update-secret-abc | ||
| spec: | ||
| displayName: Non-existent Secret | ||
| description: Attempting to update non-existent secret | ||
| type: default | ||
| value: new-value | ||
| """ | ||
| Then the response status code should be 404 | ||
| And the response should be valid JSON | ||
| And the JSON response field "status" should be "error" | ||
| And the response body should contain "does not exists" | ||
|
|
There was a problem hiding this comment.
Align the not-found error text with the API contract.
The spec examples use “Secret not found.” This assertion uses “does not exists,” which risks brittle tests.
✏️ Suggested fix
- And the response body should contain "does not exists"
+ And the response body should contain "not found"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Scenario: Update non-existent secret returns not found | |
| Given I authenticate using basic auth as "admin" | |
| When I update the secret "non-existent-update-secret-abc" with: | |
| """ | |
| apiVersion: gateway.api-platform.wso2.com/v1alpha1 | |
| kind: Secret | |
| metadata: | |
| name: non-existent-update-secret-abc | |
| spec: | |
| displayName: Non-existent Secret | |
| description: Attempting to update non-existent secret | |
| type: default | |
| value: new-value | |
| """ | |
| Then the response status code should be 404 | |
| And the response should be valid JSON | |
| And the JSON response field "status" should be "error" | |
| And the response body should contain "does not exists" | |
| Scenario: Update non-existent secret returns not found | |
| Given I authenticate using basic auth as "admin" | |
| When I update the secret "non-existent-update-secret-abc" with: | |
| """ | |
| apiVersion: gateway.api-platform.wso2.com/v1alpha1 | |
| kind: Secret | |
| metadata: | |
| name: non-existent-update-secret-abc | |
| spec: | |
| displayName: Non-existent Secret | |
| description: Attempting to update non-existent secret | |
| type: default | |
| value: new-value | |
| """ | |
| Then the response status code should be 404 | |
| And the response should be valid JSON | |
| And the JSON response field "status" should be "error" | |
| And the response body should contain "not found" |
🤖 Prompt for AI Agents
In `@gateway/it/features/secrets.feature` around lines 484 - 502, The test
assertion in the "Update non-existent secret returns not found" scenario is
checking for the brittle string "does not exists"; update the expectation to
match the API contract by asserting the response body contains the canonical
message "Secret not found." (ensure the JSON "status" remains "error" and the
status code expectation stays 404 in the same scenario).
nimsara66
force-pushed
the
policy-secrets-pr
branch
from
29fdac6 to
6b5d6c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gateway/gateway-controller/pkg/storage/gateway-controller-db.sql (1)
146-168: Add secrets table creation to version 5 schema migration.The
secretstable (defined in the SQL file) is missing from the version 5 migration insqlite.go. Existing databases upgrading from v4 to v5 will not create this table, causing runtime failures when code attempts to use SaveSecret, GetSecrets, and other secrets operations. New installations will work correctly since they load the full schema from the SQL file initially.Add the secrets table and
idx_secrets_updated_atindex to theif version == 4migration block in theinitSchema()function before settingPRAGMA user_version = 5.gateway/gateway-controller/cmd/controller/main.go (1)
122-179: Fail fast or disable/secretswhen encryption isn’t configured.
secretsServicestays nil when no encryption providers are configured (or in memory-only mode), yet the API server always registers/secretsendpoints and the policy resolver depends on it. This will panic at runtime. Consider failing startup with a clear message or wiring a disabled service/conditional routes.🛠️ Example fail-fast guard
// Initialize encryption providers for secret management var encryptionProviderManager *encryption.ProviderManager var secretsService *secrets.SecretService + + if len(cfg.GatewayController.Encryption.Providers) == 0 { + log.Fatal("Secret management requires at least one encryption provider to be configured") + } + if !cfg.IsPersistentMode() { + log.Fatal("Secret management requires persistent storage") + }Also applies to: 364-365
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/api/openapi.yaml`:
- Around line 3688-3691: SecretConfigData.value is documented as "never
returned" but SecretResponse and examples currently include plaintext; remove
value from the SecretResponse schema and from all response examples (and any
response example objects under the SecretResponse component) so responses never
expose secret values, and instead mark SecretConfigData.value as writeOnly: true
(or keep it only in request schemas) so it can be accepted on create/update but
never emitted; update any related refs/usages and example blocks (including the
other occurrence at the 3832-3847 region) to reflect the removal.
In `@gateway/gateway-controller/pkg/api/handlers/handlers.go`:
- Around line 2249-2268: The handlers call s.secretService (e.g., CreateSecret)
without guarding for nil which causes panics when the encryption
provider/service is not configured; add a nil-check at the start of CreateSecret
that returns HTTP 503 (Service Unavailable) with an api.ErrorResponse and a
descriptive message when s.secretService == nil, and apply the same guard to the
other secret handlers (Get, Update, Delete, List) so they consistently return
503 instead of dereferencing a nil service.
In `@gateway/gateway-controller/pkg/encryption/aesgcm/provider.go`:
- Around line 99-102: The length check on payload.Ciphertext is incomplete:
update the validation in the provider (where payload.Ciphertext and NonceSize
are used) to ensure the ciphertext length is at least NonceSize plus the GCM
authentication tag size (e.g., TagSize or 16 bytes) by checking
len(payload.Ciphertext) >= NonceSize + TagSize, and update the error message to
reflect the required minimum (nonce + tag) when returning the fmt.Errorf about
"ciphertext too short".
In `@gateway/gateway-controller/pkg/encryption/manager.go`:
- Around line 95-101: The Decrypt method on ProviderManager dereferences payload
(payload.Provider, payload.KeyVersion) before checking for nil; add an early
nil-guard at the start of ProviderManager.Decrypt that returns a clear error
(e.g., fmt.Errorf or errors.New) when payload == nil, and log the error via
m.logger before returning so callers don't panic when passing a nil
*EncryptedPayload; update any tests that call ProviderManager.Decrypt to assert
the new error path if needed.
In `@gateway/gateway-controller/pkg/resolver/policy_resolver.go`:
- Around line 118-125: ResolvePolicies currently unconditionally calls
apiConfig.Configuration.Spec.AsAPIConfigData(), which fails for async/webhook
configs; update ResolvePolicies to first detect the API kind (e.g., inspect
apiConfig.Configuration.Spec.Type or check for a non-nil Rest-specific field)
and only call AsAPIConfigData for REST APIs, skipping policy resolution for
other kinds (or add a separate webhook/async handling branch) so that non-Rest
configs do not produce validation errors and are not skipped incorrectly; locate
the logic around ResolvePolicies and apiConfig.Configuration.Spec and implement
the guard/alternate path there.
In `@gateway/gateway-controller/pkg/secrets/service.go`:
- Around line 59-63: The code reads secretConfig.Metadata.Name into handle
before checking the parse error; move the extraction of handle (from
secretConfig.Metadata.Name) to after verifying err == nil (i.e., after the
s.parser.Parse call and its error check), or alternatively check err first and
only access secretConfig.Metadata.Name when parsing succeeded so handle cannot
be empty/invalid; update the block around s.parser.Parse, the err check, and the
handle assignment accordingly.
- Around line 224-229: In UpdateSecret the code reads secretConfig.Metadata.Name
into handle before checking the Parse error; move the extraction of handle (the
access to secretConfig.Metadata.Name) to after verifying err == nil so you only
use secretConfig when Parse succeeds, i.e. call s.parser.Parse(params.Data,
params.ContentType, &secretConfig), check if err != nil and return the wrapped
error, and only then set handle from secretConfig.Metadata.Name; reference
s.parser.Parse and secretConfig.Metadata.Name in UpdateSecret to locate the fix.
♻️ Duplicate comments (16)
gateway/gateway-controller/pkg/storage/interface.go (1)
247-255: Interface parameter names inconsistent with implementation.The
DeleteSecretandSecretExistsmethods declare parameters asid string, but the SQLite implementations and theSecretmodel usehandleas the user-provided identifier. TheGetSecretmethod correctly useshandle string.gateway/it/features/secrets.feature (3)
84-86: Fix the typo in the post-delete retrieval ID.Line 85 uses
"wso2-openai-keye"instead of"wso2-openai-key", so this test doesn't validate that the deleted secret returns 404.
282-288: Scenario title mismatches the actual limit.The scenario says "10KB limit" but the asserted error message is "8192 characters" (8KB).
498-501: Error message text may be inconsistent with API contract.The assertion checks for
"does not exists"which has a grammatical error and may not match the actual API response format.gateway/gateway-controller/pkg/encryption/aesgcm/provider.go (1)
52-60: Potential nil pointer dereference ifGetPrimaryKey()returns nil.
Encryptcallsp.keyManager.GetPrimaryKey()without checking for nil before accessingkey.Data. WhileHealthCheckvalidates this,Encryptcould be called independently.gateway/gateway-controller/pkg/storage/sqlite.go (2)
1645-1687: TOCTOU race condition in SaveSecret.The
SecretExists+INSERTpattern creates a race where concurrent creates can bypass the existence check and hit the unique constraint, surfacing as 500 errors instead of proper 409 conflict responses. Other methods in this file (SaveConfig, SaveCertificate) use constraint-error-mapping which is race-safe.
1689-1713: Missingdefer rows.Close()androws.Err()check in GetSecrets.The result set isn't closed, which can leak database resources. Also missing the post-iteration error check.
gateway/gateway-controller/pkg/encryption/aesgcm/keymgmt.go (1)
41-48: Duplicate key versions are silently overwritten.If two
KeyConfigentries share the sameVersion, the later entry overwrites the former without warning, potentially breaking decryption of secrets encrypted with the overwritten key.gateway/gateway-controller/pkg/secrets/service.go (2)
17-20:MaxSecretSizeconstant is defined but never enforced.The constant suggests a 10KB limit, but no validation checks the size of
secretConfig.Spec.Valuebefore encryption. Large secrets could consume excessive memory.
258-260: Typo in error message and inconsistent terminology.The error message
"does not exists"has a grammatical error and uses "id" while the variable ishandle.gateway/gateway-controller/pkg/api/handlers/handlers.go (3)
2267-2272: Propagate correlation ID intoSecretParams.The correlation ID is read but not passed to the secret service, so service logs/tracing lose it.
🛠️ Suggested fix
secret, err := s.secretService.CreateSecret(secrets.SecretParams{ - Data: body, - ContentType: c.GetHeader("Content-Type"), - Logger: log, + Data: body, + ContentType: c.GetHeader("Content-Type"), + CorrelationID: correlationID, + Logger: log, })secret, err := s.secretService.UpdateSecret(secrets.SecretParams{ - Data: body, - ContentType: c.GetHeader("Content-Type"), - Logger: log, + Data: body, + ContentType: c.GetHeader("Content-Type"), + CorrelationID: correlationID, + Logger: log, })Also applies to: 2409-2414
2287-2292: Do not return plaintext secret values in API responses.Line 2289/2385/2432 returns
secret.Valuein plaintext. This is a critical security exposure—return only opaque IDs and timestamps.🛠️ Suggested fix (apply to Create/Get/Update responses)
c.JSON(http.StatusCreated, gin.H{ "id": secret.Handle, - "value": secret.Value, "created_at": secret.CreatedAt, "updated_at": secret.UpdatedAt, })Also applies to: 2382-2387, 2430-2434
2391-2414: Validate pathidmatches body handle inUpdateSecret.The handler ignores the
idpath parameter and relies only on the body. This can update the wrong secret (e.g.,/secrets/foowith bodymetadata.name: bar).🛠️ Suggested fix
// Read request body body, err := io.ReadAll(c.Request.Body) if err != nil { ... } + + // Parse to validate handle matches path parameter + var secretConfig api.SecretConfiguration + if err := s.parser.Parse(body, c.GetHeader("Content-Type"), &secretConfig); err == nil { + if secretConfig.Metadata.Name != "" && secretConfig.Metadata.Name != id { + log.Warn("Handle mismatch between path and body", + zap.String("path_id", id), + zap.String("body_handle", secretConfig.Metadata.Name)) + c.JSON(http.StatusBadRequest, api.ErrorResponse{ + Status: "error", + Message: fmt.Sprintf("Handle mismatch: path has '%s' but body metadata.name has '%s'", id, secretConfig.Metadata.Name), + }) + return + } + }gateway/gateway-controller/api/openapi.yaml (1)
3965-3981: Declare thesecretstag in the global tags list.The
/secretsoperations use thesecretstag, but it’s not defined undertags:.🛠️ Suggested addition
tags: - name: System description: System health and status endpoints + - name: secrets + description: Manage encrypted secrets for gateway configuration - name: API Management description: CRUD operations for API configurationsgateway/gateway-controller/pkg/resolver/policy_resolver.go (2)
220-223: Include error details when rebuilding the spec fails.Appending an empty
ValidationErrorhides the root cause.🛠️ Suggested fix
err = resolvedConfig.Configuration.Spec.FromAPIConfigData(apiData) if err != nil { - errors = append(errors, config.ValidationError{}) + errors = append(errors, config.ValidationError{ + Field: "spec", + Message: fmt.Sprintf("Failed to rebuild API spec after policy resolution: %v", err), + }) }
302-320: Guard against nilsecretsServicein template resolution.If
secretsServiceisn’t configured, any$secret{}template will panic.🛠️ Suggested fix
func (pr *PolicyResolver) resolveSecretTemplates(templateStr string) (string, error) { + if pr.secretsService == nil { + return "", fmt.Errorf("secrets service is not configured") + } // Pattern to match $secret{key} secretPattern := `\$secret\{([^}]+)\}`
🧹 Nitpick comments (5)
gateway/gateway-controller/pkg/config/config.go (2)
62-63: Add validation for encryption provider config.
New config types introduce provider/key settings, butValidate()doesn’t check provider type, key list, or empty file paths. A fail-fast validation step would prevent opaque runtime errors.Also applies to: 308-323
505-517: Static default encryption key in version control poses a security risk for production.The hardcoded key path
./aesgcm-keys/default-aesgcm256-v1.binpoints to a default key checked into the repository. While file existence is validated at startup and the path is configurable viaGATEWAY_*environment variables or config file overrides, using an identical key across all deployments—especially one committed to version control—weakens security in production. Consider removing or replacing the default key with a per-instance provisioned key, and document the requirement to override via configuration in production deployments.gateway/gateway-controller/pkg/storage/interface.go (1)
231-234: Return type mismatch with method semantics.
GetSecrets()returns[]string(handles only), which is inconsistent with other "GetAll" methods in this interface that return full model objects. Consider renaming toGetSecretHandles()orListSecretHandles()for clarity, or return[]*models.Secretfor consistency withGetAllConfigs(),GetAllLLMProviderTemplates(), etc.gateway/gateway-controller/pkg/storage/sqlite.go (1)
1734-1736: Useerrors.Isfor error comparison.Direct comparison
err == sql.ErrNoRowsshould useerrors.Is(err, sql.ErrNoRows)for consistency with other methods in this file and to handle wrapped errors correctly.♻️ Proposed fix
- if err == sql.ErrNoRows { + if errors.Is(err, sql.ErrNoRows) { return nil, fmt.Errorf("%w: id=%s", ErrNotFound, handle) }gateway/gateway-controller/pkg/encryption/aesgcm/keymgmt.go (1)
102-105: Consider zeroing key data on manager shutdown.The
Key.Databyte slice holds sensitive cryptographic material in memory. For defense-in-depth, consider adding a method to securely zero the key data when the manager is no longer needed (though Go's GC makes this less critical than in languages with manual memory management).
| value: | ||
| type: string | ||
| description: Secret value (stored securely and never returned in API responses) | ||
| minLength: 1 |
There was a problem hiding this comment.
Align secret value documentation with the response schema.
SecretConfigData.value says it’s never returned, but SecretResponse includes value and examples show plaintext. Decide on the intended behavior and update the schema/examples accordingly (prefer removing value from responses for security).
Also applies to: 3832-3847
🤖 Prompt for AI Agents
In `@gateway/gateway-controller/api/openapi.yaml` around lines 3688 - 3691,
SecretConfigData.value is documented as "never returned" but SecretResponse and
examples currently include plaintext; remove value from the SecretResponse
schema and from all response examples (and any response example objects under
the SecretResponse component) so responses never expose secret values, and
instead mark SecretConfigData.value as writeOnly: true (or keep it only in
request schemas) so it can be accepted on create/update but never emitted;
update any related refs/usages and example blocks (including the other
occurrence at the 3832-3847 region) to reflect the removal.
| // Decrypt decrypts the payload using the provider chain | ||
| // It tries to match the provider by name from the payload metadata | ||
| func (m *ProviderManager) Decrypt(payload *EncryptedPayload) ([]byte, error) { | ||
| m.logger.Debug("Decrypting payload", | ||
| zap.String("provider", payload.Provider), | ||
| zap.String("key_version", payload.KeyVersion), | ||
| ) |
There was a problem hiding this comment.
Guard against nil payloads in Decrypt.
payload is dereferenced on Line 98 before any nil check, so a nil input will panic. Add an early guard and return a clear error.
🛠️ Suggested fix
func (m *ProviderManager) Decrypt(payload *EncryptedPayload) ([]byte, error) {
+ if payload == nil {
+ return nil, fmt.Errorf("payload is nil")
+ }
m.logger.Debug("Decrypting payload",
zap.String("provider", payload.Provider),
zap.String("key_version", payload.KeyVersion),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Decrypt decrypts the payload using the provider chain | |
| // It tries to match the provider by name from the payload metadata | |
| func (m *ProviderManager) Decrypt(payload *EncryptedPayload) ([]byte, error) { | |
| m.logger.Debug("Decrypting payload", | |
| zap.String("provider", payload.Provider), | |
| zap.String("key_version", payload.KeyVersion), | |
| ) | |
| // Decrypt decrypts the payload using the provider chain | |
| // It tries to match the provider by name from the payload metadata | |
| func (m *ProviderManager) Decrypt(payload *EncryptedPayload) ([]byte, error) { | |
| if payload == nil { | |
| return nil, fmt.Errorf("payload is nil") | |
| } | |
| m.logger.Debug("Decrypting payload", | |
| zap.String("provider", payload.Provider), | |
| zap.String("key_version", payload.KeyVersion), | |
| ) |
🤖 Prompt for AI Agents
In `@gateway/gateway-controller/pkg/encryption/manager.go` around lines 95 - 101,
The Decrypt method on ProviderManager dereferences payload (payload.Provider,
payload.KeyVersion) before checking for nil; add an early nil-guard at the start
of ProviderManager.Decrypt that returns a clear error (e.g., fmt.Errorf or
errors.New) when payload == nil, and log the error via m.logger before returning
so callers don't panic when passing a nil *EncryptedPayload; update any tests
that call ProviderManager.Decrypt to assert the new error path if needed.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/api/openapi.yaml`:
- Around line 3688-3693: The OpenAPI schema for SecretConfigData.value currently
sets maxLength: 8192 causing a mismatch with the 10KB limit enforced elsewhere;
update the OpenAPI definition (symbol: SecretConfigData.value in openapi.yaml)
to set maxLength: 10240 to match the 10KB constraint and ensure any related
examples/validators reference the same 10240 byte/character limit so client-side
validation aligns with server rules.
In `@gateway/gateway-controller/pkg/config/secret_validator.go`:
- Around line 76-82: The validation currently reports the wrong field name: when
checking config.ApiVersion against
api.SecretConfigurationApiVersionGatewayApiPlatformWso2Comv1alpha1, change the
ValidationError Field from "version" to "apiVersion" so the error references the
actual schema key; update the ValidationError created in the ApiVersion check
(where config.ApiVersion is compared) to set Field: "apiVersion" and keep the
existing Message text.
In `@gateway/gateway-controller/pkg/storage/sqlite.go`:
- Around line 1664-1666: The error messages use the word "id" but these paths
operate on handles; update all messages to reference "handle" instead. Replace
occurrences like fmt.Errorf("%w: secret with id '%s' already exists",
ErrConflict, secret.Handle) with wording that says "secret with handle '%s'
already exists" (using ErrConflict and secret.Handle), and make the same wording
change for the other similar error/return strings referenced (around the UNIQUE
constraint check "UNIQUE constraint failed: secrets.handle" and the other
occurrences flagged at 1737-1739, 1788-1790, 1819-1821, 1830-1831) so all
secret-related errors consistently say "handle" instead of "id".
♻️ Duplicate comments (3)
gateway/gateway-controller/pkg/storage/interface.go (1)
225-255: Docs should referencehandle, notid.The method parameters are
handle, but comments still say “ID”. Align wording to avoid confusion.🔧 Proposed fix
- // Returns an error if a secret with the same ID already exists. + // Returns an error if a secret with the same handle already exists. @@ - // GetSecret retrieves a secret by ID. + // GetSecret retrieves a secret by handle. @@ - // DeleteSecret permanently removes a secret. + // DeleteSecret permanently removes a secret by handle. @@ - // SecretExists checks if a secret with the given handle exists. + // SecretExists checks if a secret with the given handle exists.gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
2416-2459:UpdateSecretstill ignores theidpath parameter.The handler accepts
idas a path parameter but only validates against the existing secret, not against the body'smetadata.name. A PUT to/secrets/foowith body containingmetadata.name: barwould check iffooexists, then store with the body's handle, leading to confusing behavior. Validate that the pathidmatches the body's handle, similar toUpdateAPI(lines 548-561).🐛 Proposed fix
func (s *APIServer) UpdateSecret(c *gin.Context, id string) { log := middleware.GetLogger(c, s.logger) // Read request body body, err := io.ReadAll(c.Request.Body) if err != nil { log.Error("Failed to read request body", zap.Error(err)) c.JSON(http.StatusBadRequest, api.ErrorResponse{ Status: "error", Message: "Failed to read request body", }) return } + // Parse to validate handle matches path parameter + var secretConfig api.SecretConfiguration + if err := s.parser.Parse(body, c.GetHeader("Content-Type"), &secretConfig); err == nil { + if secretConfig.Metadata.Name != "" && secretConfig.Metadata.Name != id { + log.Warn("Handle mismatch between path and body", + zap.String("path_id", id), + zap.String("body_handle", secretConfig.Metadata.Name)) + c.JSON(http.StatusBadRequest, api.ErrorResponse{ + Status: "error", + Message: fmt.Sprintf("Handle mismatch: path has '%s' but body metadata.name has '%s'", id, secretConfig.Metadata.Name), + }) + return + } + } + // Get correlation ID from context correlationID := middleware.GetCorrelationID(c)gateway/gateway-controller/pkg/secrets/service.go (1)
17-20:MaxSecretSizeconstant is defined but never enforced.The constant suggests a 10KB limit on secret values, but no validation checks the size of the secret value before encryption. Large secrets could consume excessive memory during encryption or storage.
🐛 Proposed enforcement in CreateSecret
func (s *SecretService) CreateSecret(params SecretParams) (*models.Secret, error) { var secretConfig api.SecretConfiguration // Parse configuration err := s.parser.Parse(params.Data, params.ContentType, &secretConfig) if err != nil { return nil, fmt.Errorf("failed to parse configuration: %w", err) } handle := secretConfig.Metadata.Name + // Validate secret size + if len(secretConfig.Spec.Value) > MaxSecretSize { + return nil, fmt.Errorf("secret value exceeds maximum size of %d bytes", MaxSecretSize) + } + // Validate configuration
🧹 Nitpick comments (5)
gateway/gateway-controller/api/openapi.yaml (1)
3789-3831: Clarify which request schema is authoritative for secrets.
SecretCreateRequestandSecretUpdateRequestare defined but/secretsPOST/PUT useSecretConfiguration. Consider either referencing these schemas in the endpoints or removing them to avoid confusion for clients and SDK generators.gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
1655-1672: Policy resolution error handling looks reasonable but consider logging correlation ID.The error aggregation and fallback to original config on resolution failure is appropriate. However, consider adding a correlation ID to the error log for traceability since this runs in the context of API creation/update operations.
gateway/gateway-controller/pkg/encryption/manager.go (1)
165-170: Add nil guard inMarshalPayloadfor defensive coding.
MarshalPayloaddereferencespayload.Ciphertextwithout a nil check. While callers should ensure non-nil payloads, a guard would prevent panics from programming errors.♻️ Proposed fix
func MarshalPayload(payload *EncryptedPayload) string { + if payload == nil { + return "" + } encoded := base64.StdEncoding.EncodeToString(payload.Ciphertext) return fmt.Sprintf("enc:%s:v1:%s:%s", payload.Provider, payload.KeyVersion, encoded) }gateway/gateway-controller/pkg/resolver/policy_resolver.go (1)
310-317: Consider pre-compiling the regex pattern.The
secretPatternregex is compiled on every call toresolveSecretTemplates. For APIs with many policies containing secret templates, this adds unnecessary overhead.♻️ Proposed fix - move regex to package level
+var secretPatternRe = regexp.MustCompile(`\$secret\{([^}]+)\}`) + func (pr *PolicyResolver) resolveSecretTemplates(templateStr string) (string, error) { if pr.secretsService == nil { return "", fmt.Errorf("secret service is not initialized properly") } - // Pattern to match $secret{key} - secretPattern := `\$secret\{([^}]+)\}` - re := regexp.MustCompile(secretPattern) - var resolveErr error - resolved := re.ReplaceAllStringFunc(templateStr, func(match string) string { + resolved := secretPatternRe.ReplaceAllStringFunc(templateStr, func(match string) string { // Extract the secret key from $secret{key} - matches := re.FindStringSubmatch(match) + matches := secretPatternRe.FindStringSubmatch(match)gateway/gateway-controller/pkg/secrets/service.go (1)
252-259: Error message terminology is slightly inconsistent.Line 258 uses
"secret configuration not found: id=%s"but the variable ishandle. Consider using consistent terminology.✏️ Proposed fix
if !exists { - return nil, fmt.Errorf("secret configuration not found: id=%s", handle) + return nil, fmt.Errorf("secret with handle '%s' not found", handle) }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/api/openapi.yaml`:
- Around line 1707-1711: The OpenAPI docs for operationId listSecrets claim the
endpoint returns secret metadata but SecretListResponse.secrets currently
contains only string IDs; either change SecretListResponse to return full
metadata objects (e.g., SecretMetadata with name, createdAt, labels, etc.) and
update the schema reference used by listSecrets, or update the listSecrets
description to state it returns only secret identifiers. Locate and modify the
schema named SecretListResponse and the path operation listSecrets (and the
analogous definitions at the other occurrence) so the response schema and the
description are consistent.
- Around line 1740-1748: The OpenAPI spec currently mixes two incompatible
request contracts for secret create/update: the schema refs under requestBody
point to SecretConfiguration while the examples and separate schemas
SecretCreateRequest/SecretUpdateRequest expect a simple {id, value} payload;
choose one contract and make the spec consistent across all occurrences (e.g.,
update the requestBody refs to use `#/components/schemas/SecretCreateRequest` and
SecretUpdateRequest and align examples, or remove
SecretCreateRequest/SecretUpdateRequest and change examples to match
SecretConfiguration). Update every instance mentioned (including the refs around
lines matching 1740–1748, 1771–1776, 1905–1913, 3789–3831) so the
requestBody.schema, examples, and error examples all reference the same schema
symbol (SecretConfiguration OR SecretCreateRequest/SecretUpdateRequest) and
remove any unused schemas to avoid codegen mismatches.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/api/openapi.yaml`:
- Around line 1764-1822: Update the OpenAPI error response examples to match the
ErrorResponse schema: replace the top-level "error" field with "status" and
remove any "correlation_id" entries in each example (e.g., examples named
missing-id, invalid-value, unauthorized, duplicate, encryption-error and all
other error examples referencing ErrorResponse). Keep the "message" field as-is
and ensure each example conforms to the ErrorResponse shape (status, message,
errors) used elsewhere; apply this change consistently for all error response
blocks that reference the ErrorResponse schema.
| '400': | ||
| description: Bad request - missing or invalid fields | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/ErrorResponse' | ||
| examples: | ||
| missing-id: | ||
| summary: Missing required field | ||
| value: | ||
| error: "bad_request" | ||
| message: "Missing required field: id" | ||
| correlation_id: "req-abc-123" | ||
| invalid-value: | ||
| summary: Invalid value | ||
| value: | ||
| error: "bad_request" | ||
| message: "Secret value too large (max 10KB)" | ||
| correlation_id: "req-def-456" | ||
| '401': | ||
| description: Unauthorized - authentication required | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/ErrorResponse' | ||
| examples: | ||
| unauthorized: | ||
| summary: Authentication missing | ||
| value: | ||
| error: "unauthorized" | ||
| message: "Authentication required" | ||
| correlation_id: "req-ghi-789" | ||
| '409': | ||
| description: Conflict - secret with this ID already exists | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/ErrorResponse' | ||
| examples: | ||
| duplicate: | ||
| summary: Duplicate ID | ||
| value: | ||
| error: "conflict" | ||
| message: "Secret already exists" | ||
| correlation_id: "req-jkl-012" | ||
| '500': | ||
| description: Internal server error - encryption failed | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/ErrorResponse' | ||
| examples: | ||
| encryption-error: | ||
| summary: Encryption failure | ||
| value: | ||
| error: "internal_error" | ||
| message: "Failed to encrypt secret" | ||
| correlation_id: "req-mno-345" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the ErrorResponse schema definition
rg -n -A 15 'ErrorResponse:' gateway/gateway-controller/api/openapi.yaml | head -50Repository: wso2/api-platform
Length of output: 542
🏁 Script executed:
#!/bin/bash
# Check all the error response examples mentioned
rg -n -C 3 'error.*:.*bad_request|unauthorized|conflict|internal_error' gateway/gateway-controller/api/openapi.yamlRepository: wso2/api-platform
Length of output: 2283
🏁 Script executed:
#!/bin/bash
# Search for correlation_id mentions in the file
rg -n 'correlation_id|correlationId' gateway/gateway-controller/api/openapi.yaml | head -20Repository: wso2/api-platform
Length of output: 610
Fix error response examples to match ErrorResponse schema definition.
The error response examples use error and correlation_id fields, but the ErrorResponse schema (line 3001+) only defines status, message, and errors. Examples across all error responses (lines 1764–1822 and additional instances at 1872–1894, 1930–1966, 1987–2004) must be updated to use status instead of error and remove correlation_id unless the schema is extended to include this field.
🤖 Prompt for AI Agents
In `@gateway/gateway-controller/api/openapi.yaml` around lines 1764 - 1822, Update
the OpenAPI error response examples to match the ErrorResponse schema: replace
the top-level "error" field with "status" and remove any "correlation_id"
entries in each example (e.g., examples named missing-id, invalid-value,
unauthorized, duplicate, encryption-error and all other error examples
referencing ErrorResponse). Keep the "message" field as-is and ensure each
example conforms to the ErrorResponse shape (status, message, errors) used
elsewhere; apply this change consistently for all error response blocks that
reference the ErrorResponse schema.
nimsara66
force-pushed
the
policy-secrets-pr
branch
from
4c8db3b to
aab8b60
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/pkg/api/handlers/handlers.go`:
- Around line 1656-1673: The code calls s.policyResolver.ResolvePolicies(cfg)
without guarding for a nil policyResolver, which can panic; before calling
ResolvePolicies, check if s.policyResolver == nil and if so log a warning via
s.logger (including cfg.ID) and set apiCfg = &cfg.Configuration as the fallback,
otherwise call resolvedCfg, validationErrors :=
s.policyResolver.ResolvePolicies(cfg) and proceed with the existing
validationErrors handling; update references to
s.policyResolver.ResolvePolicies, apiCfg, cfg and s.logger.Error so the nil-path
mirrors the current error fallback behavior.
In `@gateway/gateway-controller/pkg/config/config.go`:
- Around line 526-538: Config.Validate currently doesn't check encryption
settings and the repo ships a hard-coded AES-GCM key; update Config.Validate to
validate EncryptionConfig by ensuring EncryptionConfig.Providers is non-empty,
each ProviderConfig.Providers/ProviderConfig.Keys is non-empty, the provider
Type is one of the supported set (e.g., "aesgcm"), and each
EncryptionKeyConfig.FilePath points to an existing, readable file (return a
validation error if missing or unreadable). Also add a runtime mode check so
non-development deployments must not use the shipped default key path
(./aesgcm-keys/default-aesgcm256-v1.bin) and must explicitly provide key files;
fail validation if the default path is present in non-dev mode. After changes,
rebuild images per guidelines (cd gateway && make build-local).
In `@gateway/gateway-controller/pkg/secrets/service.go`:
- Around line 270-289: The error message uses "id=%s" but the variable is named
handle; update the fmt.Errorf call returned after s.storage.SecretExists(handle)
to use "handle=%s" for consistency (locate the call that returns
fmt.Errorf("secret configuration not found: id=%s", handle) and change the
format string to "secret configuration not found: handle=%s").
In `@gateway/it/features/secrets.feature`:
- Around line 98-151: The scenario "List all secrets returns metadata without
sensitive values" currently lacks negative assertions; update the scenario
(around the "When I list all secrets" / "Then the response status code should be
200" steps) to assert that the response body does NOT contain any secret values
(e.g., "super-secret-value-1", "super-secret-value-2", "super-secret-value-3")
and does not expose a "value" field for the listed items (ensure the step(s)
following "When I list all secrets" verify absence of those strings/fields).
♻️ Duplicate comments (6)
gateway/gateway-controller/api/openapi.yaml (2)
3720-3750: Security consideration: SecretResponse returns decrypted secret values.The
SecretResponseschema returns the plaintextvaluefield. While this appears intentional for the secret retrieval use case, consider whether this is the desired behavior from a security standpoint. Many secret management systems return values only on create or require separate authenticated calls for retrieval.If returning values is intended, this is fine. Otherwise, consider marking
valueaswriteOnly: trueinSecretConfigDataand removing it fromSecretResponse.
1928-1934: Fix error response example to matchErrorResponseschema.The error example uses
errorandcorrelation_idfields, but theErrorResponseschema (lines 2932-2949) only definesstatus,message, anderrors. This inconsistency can cause confusion in API documentation and client implementations.📝 Suggested fix
not-found: summary: Secret does not exist value: - error: "not_found" - message: "secret configuration not found" - correlation_id: "req-bcd-890" + status: "error" + message: "secret configuration not found"gateway/gateway-controller/pkg/config/secret_validator.go (1)
76-81: UseapiVersionin the validation error field.
The schema field isapiVersion, but the error reportsversion.🔧 Proposed fix
- Field: "version", + Field: "apiVersion",gateway/gateway-controller/pkg/storage/sqlite.go (1)
1731-1733: Use “handle” terminology in secret errors/messages.
These paths operate on secret handles, not IDs.🔧 Proposed fix
- return fmt.Errorf("%w: secret with id '%s' already exists", ErrConflict, secret.Handle) + return fmt.Errorf("%w: secret with handle '%s' already exists", ErrConflict, secret.Handle) @@ - return nil, fmt.Errorf("secret %w: id=%s", ErrNotFound, handle) + return nil, fmt.Errorf("secret %w: handle=%s", ErrNotFound, handle) @@ - return fmt.Errorf("secret %w: id=%s", ErrNotFound, secret.Handle) + return fmt.Errorf("secret %w: handle=%s", ErrNotFound, secret.Handle) @@ - return fmt.Errorf("secret %w: id=%s", ErrNotFound, handle) + return fmt.Errorf("secret %w: handle=%s", ErrNotFound, handle) @@ -// SecretExists checks if a secret with the given ID exists +// SecretExists checks if a secret with the given handle existsAlso applies to: 1804-1806, 1855-1857, 1886-1888, 1897-1898
gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
2417-2460:UpdateSecretdoesn't validate pathidmatches bodymetadata.name.The handler passes the path
idto the service, but if the request body contains a differentmetadata.name, the service will check for conflicts but won't prevent the update. This could lead to confusion wherePUT /secrets/foowith body containingmetadata.name: barwould updatefoo(or fail ifbarexists). Consider validating consistency similar toUpdateAPI(lines 549-563).🐛 Proposed fix
func (s *APIServer) UpdateSecret(c *gin.Context, id string) { log := middleware.GetLogger(c, s.logger) // Read request body body, err := io.ReadAll(c.Request.Body) if err != nil { log.Error("Failed to read request body", zap.Error(err)) c.JSON(http.StatusBadRequest, api.ErrorResponse{ Status: "error", Message: "Failed to read request body", }) return } + // Parse to validate handle matches path parameter + var secretConfig api.SecretConfiguration + if err := s.parser.Parse(body, c.GetHeader("Content-Type"), &secretConfig); err == nil { + if secretConfig.Metadata.Name != "" && secretConfig.Metadata.Name != id { + log.Warn("Handle mismatch between path and body", + zap.String("path_id", id), + zap.String("body_handle", secretConfig.Metadata.Name)) + c.JSON(http.StatusBadRequest, api.ErrorResponse{ + Status: "error", + Message: fmt.Sprintf("Handle mismatch: path has '%s' but body metadata.name has '%s'", id, secretConfig.Metadata.Name), + }) + return + } + } + // Get correlation ID from context correlationID := middleware.GetCorrelationID(c)gateway/gateway-controller/pkg/secrets/service.go (1)
35-38:MaxSecretSizeconstant is defined but never enforced.The constant suggests a 10KB limit on secret values, but no validation checks the size of
secretConfig.Spec.Valuebefore encryption inCreateSecretorUpdateSecret. Large secrets could consume excessive memory during encryption or storage.🐛 Proposed enforcement in CreateSecret and UpdateSecret
Add this validation after parsing and before encryption in both methods:
// Validate configuration + // Check secret size limit + if len(secretConfig.Spec.Value) > MaxSecretSize { + return nil, fmt.Errorf("secret value exceeds maximum size of %d bytes", MaxSecretSize) + } + validationErrors := s.validator.Validate(&secretConfig)
🧹 Nitpick comments (3)
gateway/gateway-controller/pkg/storage/sqlite.go (1)
1754-1763: Simplify therows.Close()defer (or log close errors).
The current wrapper ignores the close error and adds noise.♻️ Proposed fix
- defer func(rows *sql.Rows) { - err := rows.Close() - if err != nil { - - } - }(rows) + defer rows.Close()gateway/it/features/secrets.feature (1)
542-589: Consider cleanup steps to keep the suite isolated.
This scenario creates two secrets but never deletes them.♻️ Suggested cleanup
Then the response status code should be 409 And the response should be valid JSON And the JSON response field "status" should be "error" And the response body should contain "already exists" + + Given I authenticate using basic auth as "admin" + When I delete the secret "update-conflict-secret-1" + Then the response status code should be 200 + Given I authenticate using basic auth as "admin" + When I delete the secret "update-conflict-secret-2" + Then the response status code should be 200gateway/gateway-controller/cmd/controller/main.go (1)
1-45: Reminder: Rebuild Docker images after gateway changes.As per the coding guidelines, when modifying code in gateway components, rebuild Docker images using:
cd gateway && make build-localBased on coding guidelines for
gateway/**/*.{go,yaml,yml,Dockerfile}.
| required: | ||
| - action | ||
| - name | ||
| resolve: |
There was a problem hiding this comment.
Have to update gateway controller repo and release new version
3 participants
Purpose
The platform gateway currently lacks built-in support for secret management and runtime secret resolution. This pull request introduces a Gateway REST API for managing secrets, along with an annotation-based mechanism to securely resolve those secrets at runtime.
Related discussions
#654
#673
Summary by CodeRabbit
New Features
Validation
Tests
Other
✏️ Tip: You can customize this high-level summary in your review settings.