Secret management for Platform API#2228
Conversation
|
Naduni Pamudika seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces end-to-end encrypted secrets management across the platform. It adds Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (1)
platform-api/src/internal/repository/secret.go (1)
269-292: ⚡ Quick winConsider deduplicating artifact references in
FindRefs.The query may return duplicate artifact entries if the same artifact references a secret via both artifact-level rows (
gateway_id='') and gateway-specific rows. This could affect UI display or delete-protection logic.Adding
DISTINCTor grouping would prevent duplicates:Suggested fix
func (r *SecretRepo) FindRefs(orgID, handle string) ([]model.SecretReference, error) { query := r.db.Rebind(` - SELECT art.handle, art.name, art.kind + SELECT DISTINCT art.handle, art.name, art.kind FROM artifact_secret_refs asr JOIN artifacts art ON art.uuid = asr.artifact_uuid WHERE asr.organization_id = ? AND asr.secret_handle = ? `)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/src/internal/repository/secret.go` around lines 269 - 292, The FindRefs method in SecretRepo may return duplicate artifact entries when the same artifact references a secret through multiple rows. Add the DISTINCT keyword to the SELECT statement in the query to eliminate duplicate artifact handles, names, and kinds that could be returned from the join between artifact_secret_refs and artifacts tables.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gateway/gateway-controller/pkg/secrets/service.go`:
- Around line 349-367: The UpsertFromPlatform method has a race condition where
the SecretExists check followed by UpdateSecret or SaveSecret can fail under
concurrent sync runs. Replace the check-then-write pattern by attempting to
update the secret first, then handle the case where the secret is not found by
calling SaveSecret instead. Remove the SecretExists call and rely on the storage
layer's UpdateSecret response or error to determine whether to proceed with
SaveSecret, making the operation atomic and resilient to concurrent access.
In `@platform-api/src/internal/handler/gateway_internal.go`:
- Around line 858-865: Instead of logging the decryption error and continuing to
return a partial response, modify the error handling in the includeValues block
to fail the entire request when h.secretService.Decrypt() returns an error. When
a decryption failure occurs for any secret, return an error from the handler
function immediately so the caller can retry the bulk sync operation
deterministically, rather than receiving a 200 response with incomplete secret
values.
In `@platform-api/src/internal/handler/gateway_secret_integration_test.go`:
- Around line 568-579: The test in the loop currently returns early if the
deprecated secret "dep-iv" is found and correctly asserts it lacks a value
field, but if the loop completes without finding "dep-iv", the test passes
silently. This weakens the assertion because the test should require that
"dep-iv" is present in the response. Add a boolean flag before the loop
(initialized to false) to track whether "dep-iv" was found, set it to true when
the item is matched, and after the loop completes, add an assertion that fails
the test if the flag is still false, ensuring "dep-iv" must be present in the
response before checking its value field.
- Around line 71-98: Add error checks for all SQL execution calls in the test
setup that currently lack them. For each db.Exec() and sqlDB.Exec() call
(including the PRAGMA foreign_keys statement at the beginning, and the INSERT
operations for the organizations table, gateways table, and gateway_tokens
table), capture the error return value and assert it is nil using t.Fatalf with
a descriptive message. This applies to the setup operations in lines 71, 87, 91,
and 96 of the diff, as well as similar unchecked db.Exec calls elsewhere in the
test file, ensuring all SQL mutations are validated before proceeding with test
execution.
In `@platform-api/src/internal/handler/secret_integration_test.go`:
- Around line 650-651: Replace the strict string equality checks for error
assertions at the two locations (comparing decryptErr.Error() against "secret is
deprecated") with sentinel error checks instead. Define a sentinel error
variable for the "secret is deprecated" error at the package level, then update
both test assertions to use errors.Is() or check for error type/wrapped error
equality rather than comparing the exact error message text. This approach keeps
tests stable when error message text is modified in the future.
- Around line 390-411: The test setup code is ignoring errors from critical
operations like os.ReadFile, db.Exec for schema and data insertion, and
vault.NewInHouseVault. Replace all blank error identifiers (the underscores in
constructs like schema, _ := os.ReadFile and v, _ := vault.NewInHouseVault) with
proper error handling by capturing the error and checking it immediately after
each operation. Use t.Fatalf or similar to fail the test if any setup step
errors occur. Apply this consistent error-checking pattern to all setup blocks
mentioned in the comment (starting at lines 390, 588, 668, 711, 750, 786, and
822) to ensure test failures are due to actual test logic issues rather than
silent setup failures.
In `@platform-api/src/internal/handler/secret.go`:
- Around line 114-115: The secretService.List method call in the handler is
missing the projectId filter parameter that is exposed in the API contract.
Extract the projectId from the incoming request (similar to how orgID, limit,
offset, and updatedAfter are being extracted), then pass it as an additional
parameter to the h.secretService.List call to align the implementation with the
API contract expectations.
- Line 226: The line using c.JSON(http.StatusNoContent, nil) is incorrect
because HTTP 204 No Content responses must not include a response body. Replace
this call with c.Status(http.StatusNoContent) to return only the status code
without writing any body content to the response.
- Around line 93-101: In the query parameter validation blocks for limit and
offset (where c.Query("limit") and c.Query("offset") are being parsed), the
current code silently falls back to default values when parsing fails or values
are out of bounds. For the limit parameter, add an upper bound check to ensure
the converted value does not exceed the documented maximum of 100, placing this
check alongside the existing v > 0 condition. For the offset parameter, add a
reasonable upper bound check as well to prevent excessively large offset values.
Instead of silently ignoring invalid values, consider adding explicit validation
logic that either rejects out-of-bounds values or caps them to their maximum
allowed values before assigning them to the limit and offset variables.
In `@platform-api/src/internal/repository/secret_test.go`:
- Around line 488-520: The test
TestSecretRepo_FindRefs_DeduplicatesAcrossGateways inserts three
artifact_secret_refs rows (one with no gateway and two with different gateways)
but only verifies that len(refs) is greater than zero, which does not actually
test deduplication. Either replace the len(refs) == 0 check with an explicit
assertion that len(refs) == 1 to verify the rows are properly deduplicated to a
single result, or rename the test function to reflect that it only verifies refs
are returned without testing deduplication behavior.
- Around line 626-634: Replace all instances where database transaction and
query operations ignore returned errors by capturing the error values and
asserting they are nil in test setup and verification paths. Specifically, for
db.Begin() calls (around lines 626 and 632), upsertArtifactSecretRefs function
calls, tx.Commit() calls, and query/scan operations (around lines 637 and 642),
change the blank identifier underscore to actual error variables and add
assertions using require.NoError or similar test assertion methods. Apply this
error checking pattern consistently across all occurrences mentioned in lines
626, 632, 637, 642, 664, 671, 699, 705, 730, 733, 772, and 775 to ensure test
reliability.
In `@platform-api/src/internal/service/llm.go`:
- Around line 328-334: The code is discarding the error returned by
marshalUpstreamForValidation when validating secret placeholders in the upstream
config. Capture the error from the marshalUpstreamForValidation call instead of
using the blank identifier, then either return the error early or log it before
proceeding to ValidateSecretRefs. This prevents validation from silently passing
when marshaling fails. Apply this fix to both the Create method (around line
328-334) and the Update method (around line 498-504) where
marshalUpstreamForValidation is called.
In `@platform-api/src/internal/service/secret_service.go`:
- Around line 191-196: The error returned by the repo.Exists() call on line 192
is being discarded with an underscore, which means database errors will be
silently ignored and treated as if the secret is missing. Capture the error as
the second return value from repo.Exists() instead of discarding it, then check
if the error is not nil and return the error to the caller so database failures
are properly surfaced rather than being masked as missing secrets.
In `@platform-api/src/resources/openapi.yaml`:
- Around line 7287-7299: The OpenAPI specification for the GET /secrets endpoint
is missing documentation for the 400 Bad Request response that occurs when the
updatedAfter parameter has an invalid format. Add a '400' response entry to the
responses section following the same pattern as the existing error responses
(using $ref to reference '`#/components/responses/BadRequest`'), positioning it
logically before the 401 response to document that invalid query parameter
validation is performed by the handler.
In `@portals/ai-workspace/src/apis/secretApis.ts`:
- Around line 79-84: The generateSecretHandle function can return an empty
string if the providerId and fieldName parameters normalize away completely
through the chaining operations. Add a guard check after the final normalization
operation in generateSecretHandle that validates the result is not empty, and
throw an appropriate error if the normalized handle is empty to prevent invalid
empty handles from being passed to downstream API calls.
In `@portals/ai-workspace/src/contexts/llmProvider/LLMProvidersContext.tsx`:
- Around line 246-260: In the updatesPayload construction within the
LLMProvidersContext component, the url property inside upstream.main is
defaulting to an empty string when not provided in the update payload. This
causes unintended URL overrides when callers only update the auth value. Instead
of defaulting to an empty string, preserve the existing URL from the current
provider state by using the current provider's upstream.main.url as the fallback
value when the update payload does not include a URL. This ensures that partial
updates (such as auth-only changes) do not inadvertently clear the URL field.
---
Nitpick comments:
In `@platform-api/src/internal/repository/secret.go`:
- Around line 269-292: The FindRefs method in SecretRepo may return duplicate
artifact entries when the same artifact references a secret through multiple
rows. Add the DISTINCT keyword to the SELECT statement in the query to eliminate
duplicate artifact handles, names, and kinds that could be returned from the
join between artifact_secret_refs and artifacts tables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78e1eb6c-f40b-4e38-8a5d-88289fb79269
📒 Files selected for processing (44)
gateway/build-manifest.yamlgateway/gateway-controller/pkg/controlplane/client.gogateway/gateway-controller/pkg/controlplane/sync_secrets.gogateway/gateway-controller/pkg/secrets/service.gogateway/gateway-controller/pkg/utils/api_utils.goplatform-api/src/cmd/cmdplatform-api/src/config/config.goplatform-api/src/config/default_config.goplatform-api/src/internal/constants/error.goplatform-api/src/internal/database/schema.postgres.sqlplatform-api/src/internal/database/schema.sqlite.sqlplatform-api/src/internal/dto/secret.goplatform-api/src/internal/handler/gateway_internal.goplatform-api/src/internal/handler/gateway_secret_integration_test.goplatform-api/src/internal/handler/llm.goplatform-api/src/internal/handler/secret.goplatform-api/src/internal/handler/secret_integration_test.goplatform-api/src/internal/model/secret.goplatform-api/src/internal/repository/api.goplatform-api/src/internal/repository/artifact_refs.goplatform-api/src/internal/repository/deployment.goplatform-api/src/internal/repository/interfaces.goplatform-api/src/internal/repository/llm.goplatform-api/src/internal/repository/mcp.goplatform-api/src/internal/repository/secret.goplatform-api/src/internal/repository/secret_test.goplatform-api/src/internal/server/server.goplatform-api/src/internal/service/gateway_internal.goplatform-api/src/internal/service/llm.goplatform-api/src/internal/service/llm_secret_validation_test.goplatform-api/src/internal/service/secret_service.goplatform-api/src/internal/service/secret_service_test.goplatform-api/src/internal/vault/vault.goplatform-api/src/mainplatform-api/src/resources/openapi.yamlportals/ai-workspace/configs/config-platform-api-template.tomlportals/ai-workspace/configs/config-platform-api.tomlportals/ai-workspace/configs/config.tomlportals/ai-workspace/docker-compose.yamlportals/ai-workspace/src/apis/secretApis.tsportals/ai-workspace/src/auth/permissions.tsportals/ai-workspace/src/config.env.tsportals/ai-workspace/src/contexts/llmProvider/LLMProvidersContext.tsxportals/ai-workspace/src/pages/appShell/appShellPages/externalServers/ExternalServersNew.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gateway/gateway-controller/pkg/secrets/service.go`:
- Around line 355-363: The fallback logic after the failed UpdateSecret call
does not handle the race condition where another concurrent worker inserts the
same secret between the NotFoundError check and the SaveSecret call. Capture the
error returned from s.storage.SaveSecret and check if it is an "already exists"
error using an appropriate storage error checking function (similar to
storage.IsNotFoundError). If the error indicates the secret already exists,
return nil since another worker has successfully inserted it and the upsert is
complete. Only return the error if it is something other than an "already
exists" condition. This ensures the operation remains idempotent under
concurrent sync scenarios.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 494da115-346f-4e3a-9a00-8b215d5d3637
📒 Files selected for processing (11)
gateway/gateway-controller/pkg/secrets/service.goplatform-api/src/internal/handler/gateway_internal.goplatform-api/src/internal/handler/gateway_secret_integration_test.goplatform-api/src/internal/handler/secret.goplatform-api/src/internal/repository/secret.goplatform-api/src/internal/repository/secret_test.goplatform-api/src/internal/service/llm.goplatform-api/src/internal/service/secret_service.goplatform-api/src/resources/openapi.yamlportals/ai-workspace/src/apis/secretApis.tsportals/ai-workspace/src/contexts/llmProvider/LLMProvidersContext.tsx
✅ Files skipped from review due to trivial changes (1)
- portals/ai-workspace/src/apis/secretApis.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- platform-api/src/internal/handler/gateway_internal.go
- portals/ai-workspace/src/contexts/llmProvider/LLMProvidersContext.tsx
- platform-api/src/internal/service/llm.go
- platform-api/src/internal/handler/secret.go
- platform-api/src/internal/repository/secret_test.go
- platform-api/src/internal/repository/secret.go
- platform-api/src/internal/service/secret_service.go
- platform-api/src/resources/openapi.yaml
ebcb919 to
b34f435
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform-api/src/internal/repository/secret.go`:
- Around line 253-313: The FindRefsAndSoftDelete function has a race condition
where a concurrent transaction can insert a reference between the check for refs
and the deprecation update. Add a SELECT FOR UPDATE query on the secrets table
before querying for references to lock the secret row and prevent other
transactions from modifying it during the check. Execute this lock query against
the transaction (tx) after beginning it and before running the refsQuery,
selecting the secret by organization_id and handle, and ensure the query
succeeds and the secret exists (similar to the affected rows check on the
deprecation).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2136fdc-ce26-4e3d-9b93-0fdbee9a6164
📒 Files selected for processing (6)
platform-api/src/internal/handler/secret_integration_test.goplatform-api/src/internal/repository/interfaces.goplatform-api/src/internal/repository/secret.goplatform-api/src/internal/repository/secret_test.goplatform-api/src/internal/service/secret_service.goplatform-api/src/internal/service/secret_service_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- platform-api/src/internal/handler/secret_integration_test.go
- platform-api/src/internal/service/secret_service.go
- platform-api/src/internal/service/secret_service_test.go
e7ad901 to
6300e14
Compare
45047f7 to
f9b7aec
Compare
d42e601 to
fe2566d
Compare
f94232f to
13d3b4f
Compare
Purpose
Introduces an end-to-end Secrets Management feature for the WSO2 API Platform. Currently, sensitive credentials (API keys, tokens, certificates) are stored in plain text inside artifact configuration blobs. This creates security risks: credentials are exposed in the database, audit logs, and API responses. There is also no lifecycle management (rotation, deprecation, delete protection) and no way for gateway controllers to efficiently sync only the secrets they need.
Goals
InHouseVault)POST/GET/PUT/DELETE /api/v1/secrets) for org-scoped secret management{{ secret "handle" }}placeholders instead of plain-text credentialsGET /api/internal/v1/secrets) that returns only the secrets referenced by a specific gateway's deployed artifacts, with optional decrypted values for bulk startup syncApproach
Storage: A new
secretstable storesciphertext(AES-GCM-256 BLOB), asha256:hash for change detection,value_scope(alwaysORG_SHARED),status(ACTIVE/DEPRECATED), andprovider(IN_BUILT). Secrets are never physically deleted —DELETEsetsstatus = DEPRECATED.Reference tracking: A new
artifact_secret_refstable with composite PK(organization_id, artifact_uuid, secret_handle, gateway_id)tracks every{{ secret "..." }}placeholder across all artifact types. Two row types coexist:gateway_id = ''— artifact-level rows written on artifact create/update (used for delete protection — blocks deletion even of undeployed drafts)gateway_id = <uuid>— deployment-snapshot rows written atomically inside theSetCurrentWithDetailstransaction on deploy; cleared on undeploy/archiveDelete protection:
FindRefsqueriesartifact_secret_refsby(org_id, secret_handle)— a single indexed lookup replacing the previousLIKE-based scans across multiple tables.Gateway sync:
GetSecretHandlesByGatewayqueriesartifact_secret_refs WHERE org = ? AND gateway_id = ?(indexed).ListByHandlesfilters tovalue_scope = 'ORG_SHARED'only. The?includeValues=trueparameter enables startup bulk fetch (decrypts all secrets server-side), avoiding N round trips for/valuecalls.Validation:
ValidateSecretRefsis called at artifact create/update time; rejects configs that reference non-existent or DEPRECATED secret handles.See https://docs.google.com/document/d/1I-ls5QUvDpGLmYPe39ai_i3AEL_awv8vQbi83jOnoRg/edit?tab=t.w3lefdjelsk5#heading=h.qxwb86l2rlwz for the full feature spec including database DDL, API shapes, flow diagrams, and gateway sync sequence.
User stories
PUT /api/v1/secrets/{handle}and all gateways will automatically pick up the new value on the next sync cycle (detected via hash change).GET /api/internal/v1/secretswith my API key and receive only the secrets referenced by my deployed artifacts, with decrypted values available for bulk startup sync.Documentation
https://docs.google.com/document/d/1I-ls5QUvDpGLmYPe39ai_i3AEL_awv8vQbi83jOnoRg/edit?tab=t.w3lefdjelsk5#heading=h.qxwb86l2rlwz — full feature specification including API contracts, database schema, encryption design, gateway sync protocol, and flow diagrams.
Automation tests
Unit tests
src/internal/repository/secret_test.go— 26 tests coveringSecretRepoCRUD,Listpagination,updatedAfterfilter,ListByHandleswith scope filter,FindRefs(artifact-level and gateway-level rows),extractSecretHandles(regex, deduplication),upsertArtifactSecretRefs(insert on create, replace on update, clear when no secrets),upsertDeploymentSecretRefs(deploy, undeploy, isolation from artifact-level rows)src/internal/service/secret_service_test.go— 21 tests coveringCreate(value scope alwaysORG_SHARED, type defaulting, encryption failure propagation),List(pagination shape),Get/Update,Delete(blocked by artifact-level ref, blocked by deployment ref, success, soft-delete contract),ValidateSecretRefs(missing handle, deduplication),Decrypt(DEPRECATED rejection)src/internal/service/llm_secret_validation_test.go— 4 tests covering LLM provider create/update with valid and invalid{{ secret "..." }}placeholdersIntegration tests
src/internal/handler/secret_integration_test.go— 19 handler-level tests using real SQLite + AES-GCM vault + Gin router. Covers the full public API (/api/v1/secrets): create (201, 409 duplicate, 401 no auth), list (pagination object shape,?limit/?offset,?updatedAfterfilter, invalid timestamp → 400), get (200 metadata-only, 404 not found), update (200 new value returned), delete (204 unreferenced, 409 with references array, 404 not found), cross-org isolation (org B sees empty list, org B gets 404 for org A secret), soft-delete verification (DB row retained withstatus=DEPRECATED,Decryptreturns "secret is deprecated"), andValidateSecretRefsrejection of DEPRECATED handle references.src/internal/handler/gateway_secret_integration_test.go— 18 handler-level tests for the internal GW API (/api/internal/v1/secrets). Covers: authentication (no key → 401, invalid key → 401), list scoping (only gateway's deployed secrets returned,?updatedAfterfilter, invalid timestamp → 400, novaluefield by default), multi-gateway isolation (GW-A secret not visible to GW-B, shared handle visible to both), undeploy removes secrets from list, deduplication of same handle across multiple artifacts,GET /:id/value(200 plaintext, 200 after rotation, 404 not found),?includeValues=truebulk fetch (ACTIVE secrets includevalue, DEPRECATED secrets omitvalue, empty list when no deployments,hashpresent alongsidevalue).Test scenario traceability: https://docs.google.com/spreadsheets/d/1FfXxjNbNh1SgjgxabPG8VcKLkITfqQTKwH61VVmfDkk/edit?gid=750717062#gid=750717062 — 85 test cases with
Test ResultandAutomated Testcolumns. 64 PASS (automated), 21 MANUAL (gateway controller sync behaviour, process startup, and AI Workspace frontend orchestration flows).Security checks
Notes:
ciphertextcolumn stores binary blobs only — plaintext is never written to the databasePLATFORM_SECRET_ENCRYPTION_KEY) is injected via environment variable; an ephemeral key is auto-generated (with a warning log) if unset — operators must set a persistent key for productionPOST) and rotation time (PUT); allGETresponses omit thevaluefieldSamples
N/A — this is a platform infrastructure feature with no standalone samples.
Related PRs
N/A
Test environment
schema.postgres.sql)go test ./src/internal/...— all packages pass