Implement API key policy #389
Conversation
WalkthroughThis pull request adds complete API key management functionality to the gateway system, including key generation endpoints, persistent storage with SQLite schema, in-memory caching, key validation services, and an APIKeyAuthentication policy for request authentication. The feature spans public API handlers, internal validation APIs, storage layers, and policy implementations. Changes
Sequence Diagram(s)sequenceDiagram
actor Developer
participant PublicAPI as Public API<br/>(POST /api-key)
participant Handler as GenerateAPIKey<br/>Handler
participant Model as APIKey<br/>Model
participant DB as SQLite<br/>Storage
participant Cache as ConfigStore<br/>(In-Memory)
Developer->>PublicAPI: POST /apis/{name}/{version}/api-key
PublicAPI->>Handler: GenerateAPIKey(name, version)
Handler->>Handler: Validate API exists
Handler->>Model: GenerateAPIKey(name, version)
Model->>Model: Generate gw_ prefixed<br/>32-byte hex key
Model-->>Handler: Return APIKey
Handler->>DB: SaveAPIKey(apiKey)
alt Unique Constraint Violation
DB-->>Handler: ErrConflict
Handler->>Model: GenerateAPIKey (retry)
Model-->>Handler: Return new APIKey
Handler->>DB: SaveAPIKey(newKey)
end
DB-->>Handler: Success
Handler->>Cache: StoreAPIKey(apiKey)
Cache-->>Handler: Success
Handler-->>PublicAPI: 201 Created<br/>(name, version, key, created_at)
PublicAPI-->>Developer: Return API Key
sequenceDiagram
actor Client
participant PolicyEngine as Policy<br/>Engine
participant Policy as APIKeyPolicy<br/>(OnRequest)
participant InternalAPI as Internal API<br/>(validate-apikey)
participant Validator as APIKeyValidator
participant Cache as ConfigStore<br/>(In-Memory)
participant DB as SQLite<br/>Storage
Client->>PolicyEngine: HTTP Request<br/>(with API key header/query)
PolicyEngine->>Policy: OnRequest(ctx, params)
Policy->>Policy: Extract API key<br/>from header/query
Policy->>Policy: Strip optional<br/>value-prefix
Policy->>InternalAPI: POST /validate-apikey<br/>(apiName, apiVersion, apiKey)
InternalAPI->>Validator: ValidateAPIKey(name, version, key)
Validator->>Policy: Extract prefix (gw_)
Validator->>Cache: GetAPIKeyByKey(key)
alt Found in Cache
Cache-->>Validator: Return APIKey
Validator->>Validator: Check IsValid()<br/>(active & not expired)
else Not in Cache
Validator->>DB: GetAPIKeyByKey(key)
DB-->>Validator: Return APIKey (or not found)
Validator->>Validator: Check IsValid()
end
Validator-->>InternalAPI: (isValid: bool)
InternalAPI-->>Policy: 200 OK (isValid)
alt isValid == true
Policy->>Policy: Set auth metadata
Policy-->>PolicyEngine: Forward to upstream
else isValid == false
Policy-->>PolicyEngine: 401 Unauthorized
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
gateway/gateway-controller/pkg/storage/gateway-controller-db.sql (1)
86-117: API key table schema looks correct; idx_api_key is slightly redundantThe
api_keystable schema aligns withmodels.APIKeyandAPIKeyStatus, and the foreign key todeployments(name, version)withON DELETE CASCADEis appropriate. The additional indexes on(api_name, api_version),status, andexpires_atcover the expected query patterns.One minor optimization:
api_key TEXT NOT NULL UNIQUEalready creates a unique index onapi_key, soidx_api_keyis technically redundant and could be dropped to save a bit of space and write overhead if desired.gateway/gateway-controller/api/openapi.yaml (1)
379-454: New API key endpoint spec matches implementation; consider reusable schemaThe
POST /apis/{name}/{version}/api-keypath, response fields, and examples are consistent with the handler andmodels.GenerateAPIKey(32-byte random hex withgw_prefix,created_atas RFC3339,expires_atnullable). To keep the spec DRY and help generated clients, you may want to move this 201 response body into a named schema undercomponents.schemasand reference it here, similar toAPICreateResponse.gateway/gateway-controller/pkg/storage/interface.go (1)
108-136: API key operations fit well into the storage abstractionThe added API key methods have clear semantics (conflict vs not-found) and match the
models.APIKeyand SQLite implementation. For larger backends or testing, you might eventually factor these into a dedicated sub-interface (e.g.,APIKeyStore) embedded inStorage, but that can wait until there are multiple implementations.gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
1442-1532: GenerateAPIKey handler logic is sound; clarify behavior in memory-only modeThe flow (existence check, secure key generation, single retry on
storage.ErrConflict, and 201 response) looks correct, and logging only the key ID avoids leaking the secret.Two small follow-ups you might consider:
- In memory-only mode (
s.db == nil), generated keys are not persisted anywhere; if validation later depends on storage, it may be worth either rejecting the call with a clear error or emitting a strong warning so operators know keys are ephemeral.- For consistency with other endpoints, you could later switch the 201 body from
gin.Hto a generated response type referenced from the OpenAPI spec.Nothing blocking from a correctness standpoint.
gateway/gateway-controller/pkg/models/api_key.go (1)
30-95: APIKey model and generator are correct and secureThe model, status enum, and
GenerateAPIKeyimplementation (UUID ID + 32 bytes fromcrypto/randhex-encoded withgw_prefix) align with the DB schema and OpenAPI description.IsValid/IsExpiredalso behave as expected.If you want to avoid magic numbers/strings, consider promoting the 32-byte length and
"gw_"prefix to exported or package-level constants so they can be shared with docs and tests.gateway/gateway-controller/pkg/storage/sqlite.go (1)
743-927: API key CRUD implementation is correct; consider hashing keys and hardening constraint checksThe API key storage methods (
SaveAPIKey,GetAPIKeyByKey,GetAPIKeysByAPI,UpdateAPIKey,DeleteAPIKey) are implemented consistently with the interface and schema: they handle nullableexpires_at, wrapErrConflict/ErrNotFoundappropriately, and avoid logging the raw key.Two non-blocking improvements to consider:
- At-rest protection: API keys are currently stored in plaintext. If you don’t need to re-display the key after creation, you could store a cryptographic hash (e.g., SHA-256) instead and have the validator compare hashes. This significantly reduces the impact of a DB compromise, at the cost of a slightly more complex validation path.
- Constraint detection robustness:
isAPIKeyUniqueConstraintError(and the existing helpers) rely on matching exact SQLite error strings. In a future cleanup, you might centralize this logic by asserting on thesqlite3.Errortype/code instead of string comparison, to make it more resilient across driver or SQLite version differences.Functionally, the current code is fine; these are hardening/refinement opportunities.
Also applies to: 1043-1048
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
gateway/gateway-controller/pkg/api/generated/generated.gois excluded by!**/generated/**
📒 Files selected for processing (6)
gateway/gateway-controller/api/openapi.yaml(1 hunks)gateway/gateway-controller/pkg/api/handlers/handlers.go(2 hunks)gateway/gateway-controller/pkg/models/api_key.go(1 hunks)gateway/gateway-controller/pkg/storage/gateway-controller-db.sql(1 hunks)gateway/gateway-controller/pkg/storage/interface.go(1 hunks)gateway/gateway-controller/pkg/storage/sqlite.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
gateway/gateway-controller/pkg/storage/interface.go (1)
gateway/gateway-controller/pkg/models/api_key.go (1)
APIKey(40-49)
gateway/gateway-controller/pkg/models/api_key.go (1)
kubernetes/gateway-operator/api/v1/apiconfig_spec.go (1)
APIVersion(51-51)
gateway/gateway-controller/pkg/storage/sqlite.go (2)
gateway/gateway-controller/pkg/models/api_key.go (1)
APIKey(40-49)gateway/gateway-controller/pkg/storage/errors.go (2)
ErrConflict(29-29)ErrNotFound(26-26)
gateway/gateway-controller/pkg/api/handlers/handlers.go (4)
gateway/gateway-controller/pkg/models/api_key.go (2)
GenerateAPIKey(52-77)APIKey(40-49)gateway/gateway-controller/pkg/api/middleware/correlation.go (1)
GetLogger(70-77)gateway/gateway-controller/pkg/api/generated/generated.go (2)
ErrorResponse(364-371)Error(59-59)gateway/gateway-controller/pkg/storage/errors.go (1)
ErrConflict(29-29)
🔇 Additional comments (1)
gateway/gateway-controller/pkg/storage/sqlite.go (1)
151-183: Schema migration to version 4 correctly introduces api_keysThe migration from schema version 3 adds
api_keyswith the expected columns and foreign key todeployments(name, version), builds the relevant indexes, and bumpsuser_versionto 4. This is consistent with the embeddedgateway-controller-db.sqlschema and provides a clean upgrade path for existing databases.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gateway/gateway-controller/cmd/controller/main.go (1)
73-96: Avoid wiring internal API key validation with a nil storage backendIn non‑persistent mode,
cfg.IsPersistentMode()is false, sodbstays nil, but you still constructinternalAPIServerand itsAPIKeyValidator, which then callsv.db.GetAPIKeyByKey(...)at line 92 ofapikey_validator.go. With a nilstorage.Storageinterface, this will panic at runtime when the internal/validate/{apikey}endpoint is hit.You should ensure the internal validation path never runs against a nil backend, e.g. by either:
- Guarding registration here (lines 257-260):
- // Initialize and register internal API server - internalAPIServer := internalapihandler.NewInternalAPIServer(configStore, db, log) - internalapi.RegisterHandlers(router, internalAPIServer) + // Initialize and register internal API server only when persistent storage is configured + if cfg.IsPersistentMode() && db != nil { + internalAPIServer := internalapihandler.NewInternalAPIServer(configStore, db, log) + internalapi.RegisterHandlers(router, internalAPIServer) + } else { + log.Info("Skipping internal API key validation endpoints (persistent storage not configured)") + }
- Or (preferably in addition) adding a nil‑backend guard inside
APIKeyValidator.validateGatewayAPIKeyso any other future callers are also protected.
🧹 Nitpick comments (1)
gateway/gateway-controller/pkg/internalapi/services/apikey_validator.go (1)
50-72: Clarify behavior for management/dev portal prefixes and unknown prefixesRight now:
"gw_"keys are fully implemented."mgt_"and"dev_"keys always return(false, error)with “not implemented”, which the handler maps to HTTP 500.- Unknown prefixes log a warning and return
(false, nil), so the handler returns 200 withisValid=false.That behavior is acceptable as long as:
- No callers are yet relying on
"mgt_"or"dev_"prefixes, and- Returning 500 for those prefixes is the intended temporary contract.
It would help future maintainers to be explicit in a comment or TODO near
ValidateAPIKeythat only gateway (gw_) keys are currently supported and that management/dev validation will be wired later, so clients don’t accidentally depend on undefined behavior.Also applies to: 122-158
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
gateway/gateway-controller/pkg/internalapi/generated/generated.gois excluded by!**/generated/**
📒 Files selected for processing (6)
gateway/gateway-controller/Makefile(1 hunks)gateway/gateway-controller/api/gateway-controller-internal-api.yaml(1 hunks)gateway/gateway-controller/cmd/controller/main.go(3 hunks)gateway/gateway-controller/oapi-codegen-internal-api.yaml(1 hunks)gateway/gateway-controller/pkg/internalapi/handlers/gateway_internal_handlers.go(1 hunks)gateway/gateway-controller/pkg/internalapi/services/apikey_validator.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-06T11:57:08.456Z
Learnt from: BLasan
Repo: wso2/api-platform PR: 140
File: gateway/gateway-controller/pkg/utils/api_deployment.go:88-105
Timestamp: 2025-12-06T11:57:08.456Z
Learning: In the gateway-controller Go codebase (file: gateway/gateway-controller/pkg/utils/api_deployment.go), only the async/websub API kind is currently supported. async/websocket and async/sse API kinds are not yet implemented.
Applied to files:
gateway/gateway-controller/oapi-codegen-internal-api.yamlgateway/gateway-controller/cmd/controller/main.go
🧬 Code graph analysis (3)
gateway/gateway-controller/pkg/internalapi/services/apikey_validator.go (2)
gateway/gateway-controller/pkg/storage/memory.go (1)
ConfigStore(30-36)gateway/gateway-controller/pkg/storage/interface.go (1)
Storage(64-168)
gateway/gateway-controller/cmd/controller/main.go (3)
gateway/gateway-controller/pkg/internalapi/handlers/gateway_internal_handlers.go (1)
NewInternalAPIServer(43-54)gateway/gateway-controller/pkg/internalapi/generated/generated.go (1)
RegisterHandlers(112-114)gateway/gateway-controller/pkg/api/generated/generated.go (1)
RegisterHandlers(1203-1205)
gateway/gateway-controller/pkg/internalapi/handlers/gateway_internal_handlers.go (4)
gateway/gateway-controller/pkg/storage/interface.go (1)
Storage(64-168)gateway/gateway-controller/pkg/internalapi/services/apikey_validator.go (2)
APIKeyValidator(31-35)NewAPIKeyValidator(38-48)gateway/gateway-controller/pkg/api/middleware/correlation.go (1)
GetLogger(70-77)gateway/gateway-controller/pkg/internalapi/generated/generated.go (3)
ErrorResponse(28-35)ApiKeyValidationResponse(22-25)ValidationError(38-44)
🪛 Checkov (3.2.334)
gateway/gateway-controller/api/gateway-controller-internal-api.yaml
[high] 1-120: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-120: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 103-109: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (3)
gateway/gateway-controller/Makefile (1)
39-43: Internal API codegen wiring looks consistentThe additional generate step for the internal API mirrors the existing public API codegen and is straightforward. No issues from a build perspective.
gateway/gateway-controller/oapi-codegen-internal-api.yaml (1)
1-9: Codegen config is aligned with internal API usagePackage name, output path, and gin-server/models settings match how the internal server is wired; this looks good as a starting config.
gateway/gateway-controller/pkg/internalapi/handlers/gateway_internal_handlers.go (1)
34-40: Internal handler wiring, logging, and validation look solidThe
InternalAPIServersetup andValidateApiKeyhandler are well-structured:
- Uses the correlation-aware logger and masks API keys before logging.
- Performs basic input validation and returns structured
ErrorResponsewithValidationErrorentries.- Checks API existence via the config store before delegating to
APIKeyValidator.- Maps validator outcome cleanly to
200(valid/invalid) vs4xx/5xxfor bad input and internal errors.No changes needed here beyond ensuring the underlying
APIKeyValidatorbehaves safely when storage is unavailable (addressed separately).Also applies to: 56-112, 114-148
gateway/gateway-controller/api/gateway-controller-internal-api.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
gateway/gateway-controller/pkg/storage/gateway-controller-db.sql (1)
105-133: Solid schema design with appropriate constraints and indexes.The table structure aligns well with the
APIKeymodel and includes proper foreign key relationships with cascade delete. The status CHECK constraint and indexes support the expected query patterns.One minor observation: the
idx_api_keyindex on line 130 may be redundant since theUNIQUEconstraint onapi_key(line 111) already creates an implicit index in SQLite. This won't cause issues but adds slight overhead during writes.Consider removing the redundant index:
-- Indexes for API key lookups -CREATE INDEX IF NOT EXISTS idx_api_key ON api_keys(api_key); CREATE INDEX IF NOT EXISTS idx_api_key_api ON api_keys(api_name, api_version); CREATE INDEX IF NOT EXISTS idx_api_key_status ON api_keys(status); CREATE INDEX IF NOT EXISTS idx_api_key_expiry ON api_keys(expires_at);gateway/gateway-controller/pkg/models/api_key.go (1)
79-95: Validation methods are correct.The logic properly handles the nullable
ExpiresAtfield and the status check. Minor observation:IsValidcould callIsExpired()to reduce duplication, but the current implementation is clear and avoids an extra function call.func (ak *APIKey) IsValid() bool { if ak.Status != APIKeyStatusActive { return false } - - if ak.ExpiresAt != nil && time.Now().After(*ak.ExpiresAt) { - return false - } - - return true + return !ak.IsExpired() }gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
1972-1980: UseapiKey.ExpiresAtinstead of hardcodednilfor future-proofing.The response hardcodes
expires_at: nilinstead of usingapiKey.ExpiresAt. While currently equivalent (sinceGenerateAPIKeysetsExpiresAttonil), this will need updating if expiration support is added later.// Return success response + var expiresAt *string + if apiKey.ExpiresAt != nil { + formatted := apiKey.ExpiresAt.Format(time.RFC3339) + expiresAt = &formatted + } c.JSON(http.StatusCreated, gin.H{ "status": "success", "message": "API key generated successfully", "name": apiKey.APIName, "version": apiKey.APIVersion, "api_key": apiKey.APIKey, "created_at": apiKey.CreatedAt.Format(time.RFC3339), - "expires_at": nil, // No expiration by default + "expires_at": expiresAt, })gateway/gateway-controller/api/gateway-controller-internal-api.yaml (2)
1-16: Consider adding security definitions for the internal API.Static analysis flags missing
securitySchemesandsecuritydefinitions (CKV_OPENAPI_4, CKV_OPENAPI_5). Even for internal APIs, it's good practice to document the expected authentication mechanism (e.g., mTLS, internal token, network-level security) so tooling and documentation accurately reflect the protection in place.Example addition to
components:securitySchemes: internalAuth: type: apiKey in: header name: X-Internal-Token description: Internal service authentication token security: - internalAuth: []If the API relies purely on network-level security (e.g., service mesh), consider adding a comment in the description explaining this.
114-118: Optional: AddmaxItemsto theerrorsarray.Static analysis (CKV_OPENAPI_21) suggests adding a maximum bound to arrays. Consider adding a reasonable limit:
errors: type: array description: Detailed validation errors + maxItems: 100 items: $ref: "#/components/schemas/ValidationError"gateway/gateway-controller/pkg/internalapi/handlers/gateway_internal_handlers.go (1)
70-96: Redundant empty check for API key.The empty check for
apikeyat lines 71-78 is duplicated invalidateInputParams(lines 151-156). Consider removing the duplicate and relying solely onvalidateInputParamsfor consistency.Apply this diff to consolidate validation:
apikey := req.ApiKey - if apikey == "" { - log.Warn("API key is missing in request body") - c.JSON(http.StatusBadRequest, internalapi.ErrorResponse{ - Status: "error", - Message: "API key cannot be empty", - }) - return - } log.Info("Validating API key", zap.String("apiName", name), zap.String("apiVersion", version), zap.String("apiKey", maskApiKey(apikey)), ) // Validate input parameters validationErrors := s.validateInputParams(name, version, apikey)gateway/gateway-controller/pkg/internalapi/services/apikey_validator.go (1)
140-176: Consider returning an error for unimplemented validation paths.Both
validateManagementPortalAPIKeyandvalidateDevPortalAPIKeyreturnfalse, nil, which is indistinguishable from a key that simply doesn't exist. Consider returning an error to make the "not implemented" state explicit to callers:func (v *APIKeyValidator) validateManagementPortalAPIKey(apiName, apiVersion, apiKey string) (bool, error) { // ... v.logger.Warn("Management portal API key validation not yet implemented") - return false, nil + return false, fmt.Errorf("management portal API key validation not yet implemented") }This allows the handler to return a more informative error (e.g., 501 Not Implemented) rather than treating it as an invalid key.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
gateway/gateway-controller/pkg/api/generated/generated.gois excluded by!**/generated/**gateway/gateway-controller/pkg/internalapi/generated/generated.gois excluded by!**/generated/**
📒 Files selected for processing (13)
gateway/gateway-controller/Makefile(1 hunks)gateway/gateway-controller/api/gateway-controller-internal-api.yaml(1 hunks)gateway/gateway-controller/api/openapi.yaml(1 hunks)gateway/gateway-controller/cmd/controller/main.go(3 hunks)gateway/gateway-controller/oapi-codegen-internal-api.yaml(1 hunks)gateway/gateway-controller/pkg/api/handlers/handlers.go(3 hunks)gateway/gateway-controller/pkg/internalapi/handlers/gateway_internal_handlers.go(1 hunks)gateway/gateway-controller/pkg/internalapi/services/apikey_validator.go(1 hunks)gateway/gateway-controller/pkg/models/api_key.go(1 hunks)gateway/gateway-controller/pkg/storage/gateway-controller-db.sql(1 hunks)gateway/gateway-controller/pkg/storage/interface.go(1 hunks)gateway/gateway-controller/pkg/storage/memory.go(3 hunks)gateway/gateway-controller/pkg/storage/sqlite.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- gateway/gateway-controller/cmd/controller/main.go
- gateway/gateway-controller/api/openapi.yaml
- gateway/gateway-controller/oapi-codegen-internal-api.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-18T10:07:26.115Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 160
File: platform-api/src/internal/utils/api.go:1403-1434
Timestamp: 2025-11-18T10:07:26.115Z
Learning: In the wso2/api-platform repository's OpenAPI validation endpoint response (ValidateOpenAPIRequest/OpenAPIValidationResponse), backend service names are not required in the API DTO because the frontend only displays the backend URLs if they exist. The Name field can be left empty when parsing OpenAPI/Swagger definitions.
Applied to files:
gateway/gateway-controller/api/gateway-controller-internal-api.yaml
🧬 Code graph analysis (6)
gateway/gateway-controller/pkg/models/api_key.go (1)
kubernetes/gateway-operator/api/v1/apiconfig_spec.go (1)
APIVersion(51-51)
gateway/gateway-controller/pkg/api/handlers/handlers.go (2)
gateway/gateway-controller/pkg/models/api_key.go (2)
APIKey(40-49)GenerateAPIKey(52-77)gateway/gateway-controller/pkg/storage/errors.go (1)
ErrConflict(29-29)
gateway/gateway-controller/pkg/internalapi/services/apikey_validator.go (3)
gateway/gateway-controller/pkg/storage/memory.go (1)
ConfigStore(30-44)gateway/gateway-controller/pkg/storage/interface.go (1)
Storage(64-208)gateway/gateway-controller/pkg/models/api_key.go (1)
APIKey(40-49)
gateway/gateway-controller/pkg/storage/interface.go (1)
gateway/gateway-controller/pkg/models/api_key.go (1)
APIKey(40-49)
gateway/gateway-controller/pkg/storage/sqlite.go (2)
gateway/gateway-controller/pkg/models/api_key.go (1)
APIKey(40-49)gateway/gateway-controller/pkg/storage/errors.go (2)
ErrConflict(29-29)ErrNotFound(26-26)
gateway/gateway-controller/pkg/storage/memory.go (3)
gateway/gateway-controller/pkg/models/api_key.go (1)
APIKey(40-49)gateway/gateway-controller/pkg/storage/errors.go (2)
ErrConflict(29-29)ErrNotFound(26-26)kubernetes/gateway-operator/api/v1/apiconfig_spec.go (1)
APIVersion(51-51)
🪛 Checkov (3.2.334)
gateway/gateway-controller/api/gateway-controller-internal-api.yaml
[high] 1-132: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-132: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 115-121: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (15)
gateway/gateway-controller/Makefile (1)
42-43: LGTM!The internal API code generation step follows the same pattern as the primary API generation, maintaining consistency in the build process.
gateway/gateway-controller/pkg/storage/interface.go (1)
148-175: LGTM!The interface additions follow the established patterns in the codebase, with clear documentation about atomicity requirements and error semantics. The method signatures are consistent with the
APIKeymodel and support the full lifecycle management of API keys.gateway/gateway-controller/pkg/models/api_key.go (1)
51-77: LGTM! Secure key generation implementation.Using
crypto/randwith 32 bytes (256 bits) provides excellent entropy for API key generation. Thegw_prefix aids key identification. The function properly handles errors from the random number generator.gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
551-583: Verify whether API key deletion is actually redundant due to cascade behavior.The review assumes the
api_keystable hasON DELETE CASCADEconfigured with a foreign key todeployments(name, version). However, I was unable to locate the database schema definition or migration files in the codebase to confirm this foreign key relationship exists.While the manual deletion logic at lines 553-574 is clearly present and calls
GetAPIKeysByAPIfollowed byDeleteAPIKeyfor each key, without verifying the actual FK constraint configuration, it's uncertain whether this is truly redundant. To proceed:
- Confirm the FK constraint: Locate and review the database schema or migration files to verify if
api_keys.api_nameandapi_keys.api_versionactually referencedeployments(name, version)withON DELETE CASCADEconfigured.- If confirmed as redundant: The suggested refactor to remove lines 553-574 is valid, as the cascade would handle cleanup automatically.
- If no cascade exists: The manual deletion is necessary and should be retained.
gateway/gateway-controller/api/gateway-controller-internal-api.yaml (1)
18-74: Endpoint design looks good.The endpoint correctly accepts the API key in the request body rather than the URL path, which avoids leaking secrets in logs and proxies. The response structure with explicit error codes (400, 404, 500) is appropriate.
gateway/gateway-controller/pkg/storage/sqlite.go (2)
1010-1046: API key storage implementation looks correct.The
SaveAPIKeymethod properly handles unique constraint violations and maps them toErrConflict, following the established pattern in this file.
1048-1136: API key retrieval methods are well-implemented.Both
GetAPIKeyByKeyandGetAPIKeysByAPIcorrectly handle the nullableexpires_atfield usingsql.NullTimeand follow the existing error handling patterns.gateway/gateway-controller/pkg/internalapi/handlers/gateway_internal_handlers.go (2)
161-167: Good API key masking implementation.The
maskApiKeyfunction properly obscures the middle portion of API keys for logging while preserving enough characters for debugging. This follows security best practices.
56-131: Handler implementation looks solid.The
ValidateApiKeymethod correctly:
- Extracts correlation-aware logger from context
- Validates inputs before processing
- Checks API existence before key validation
- Returns appropriate HTTP status codes for different error conditions
- Logs validation results with masked API keys
gateway/gateway-controller/pkg/storage/memory.go (4)
394-416: Consider defensive copying inStoreAPIKey.The same
*models.APIKeypointer is stored in bothapiKeysandapiKeysByAPImaps. If a caller mutates theAPIKeyafter storing, it affects both lookups. This matches existing patterns in the file (e.g.,StoredConfig), but worth noting for awareness.
431-446: Good defensive copying pattern.
GetAPIKeysByAPIcorrectly returns a copy of the slice to prevent external modification of the internal slice structure. Note that the*models.APIKeypointers within still reference the same objects.
448-479:RemoveAPIKeyimplementation is correct.The slice removal logic using
append(apiKeys[:i], apiKeys[i+1:]...)correctly removes the element and the cleanup of empty slices prevents memory accumulation.
40-57: In-memory API key storage initialization looks good.The new maps are properly initialized in
NewConfigStoreand the field additions follow the existing structure.gateway/gateway-controller/pkg/internalapi/services/apikey_validator.go (2)
84-138: Past review concern addressed: nil check for storage backend.The
validateGatewayAPIKeyfunction now properly checksv.store.GetAPIKeyByKeyfirst, then falls back tov.db.GetAPIKeyByKeyonly ifv.db != nil(line 100). This prevents the nil pointer dereference in memory-only mode.
50-73: API key routing logic is clear and well-structured.The prefix-based routing using
switchis clean. TheextractAPIKeyPrefixhelper correctly handles the underscore delimiter and normalizes to lowercase.
thivindu
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
gateway/gateway-controller/pkg/storage/sqlite.go (1)
187-187: Fix error message version number.The error message still references "version 4" but should say "version 5" since this migration is upgrading to version 5.
Apply this diff:
- return fmt.Errorf("failed to migrate schema to version 4 (api_keys): %w", err) + return fmt.Errorf("failed to migrate schema to version 5 (api_keys): %w", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
gateway/gateway-controller/pkg/api/handlers/handlers.go(3 hunks)gateway/gateway-controller/pkg/storage/sqlite.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
gateway/gateway-controller/pkg/api/handlers/handlers.go (3)
gateway/gateway-controller/pkg/models/api_key.go (2)
APIKey(40-49)GenerateAPIKey(52-77)gateway/gateway-controller/pkg/api/middleware/correlation.go (1)
GetLogger(70-77)gateway/gateway-controller/pkg/storage/errors.go (1)
ErrConflict(29-29)
gateway/gateway-controller/pkg/storage/sqlite.go (2)
gateway/gateway-controller/pkg/models/api_key.go (1)
APIKey(40-49)gateway/gateway-controller/pkg/storage/errors.go (2)
ErrConflict(29-29)ErrNotFound(26-26)
🔇 Additional comments (3)
gateway/gateway-controller/pkg/api/handlers/handlers.go (3)
551-583: LGTM! API key cleanup properly integrated into API deletion.The implementation correctly handles cleanup of associated API keys during API deletion, with appropriate separation of database and in-memory store operations. The best-effort error handling (logging warnings/errors but continuing) is appropriate for cascade deletion scenarios.
1953-1970: Rollback logic correctly addresses previous review feedback.The implementation now properly handles ConfigStore failures by rolling back the database save (lines 1959-1964), maintaining consistency between the database and in-memory store. This directly resolves the data inconsistency issue raised in the previous review.
1877-1987: Well-implemented API key generation with proper error handling and security.The handler correctly:
- Validates API existence before generating keys
- Uses cryptographically secure key generation (32 random bytes)
- Handles rare key collisions with retry logic
- Maintains data consistency via rollback on ConfigStore failures
- Returns the generated key only once (expected for key generation endpoints)
- Logs operations without exposing the key value (line 1975 logs key_id only)
No duplicate GenerateAPIKey function declaration exists in the file.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
gateway/gateway-controller/pkg/storage/gateway-controller-db.sql (1)
105-133: LGTM with one minor note on index redundancy.The table schema and constraints are well-designed. The
ON DELETE CASCADEensures API keys are cleaned up when the parent deployment is removed, and the status check constraint maintains data integrity.Note that
idx_api_keyon line 130 is technically redundant since theUNIQUEconstraint onapi_key(line 111) already creates an implicit index in SQLite. This doesn't cause harm but adds minor overhead during writes.gateway/gateway-controller/cmd/controller/main.go (1)
7-7: Consider grouping imports by convention.The
internalapihandlerimport is placed between standard library imports. Go convention groups imports as: standard library, external packages, then internal/project packages.import ( "context" "flag" "fmt" - internalapihandler "github.com/wso2/api-platform/gateway/gateway-controller/pkg/internalapi/handlers" "net/http" "os" "os/signal" "syscall" "time" "github.com/gin-gonic/gin" api "github.com/wso2/api-platform/gateway/gateway-controller/pkg/api/generated" "github.com/wso2/api-platform/gateway/gateway-controller/pkg/api/handlers" "github.com/wso2/api-platform/gateway/gateway-controller/pkg/api/middleware" "github.com/wso2/api-platform/gateway/gateway-controller/pkg/config" "github.com/wso2/api-platform/gateway/gateway-controller/pkg/controlplane" internalapi "github.com/wso2/api-platform/gateway/gateway-controller/pkg/internalapi/generated" + internalapihandler "github.com/wso2/api-platform/gateway/gateway-controller/pkg/internalapi/handlers" "github.com/wso2/api-platform/gateway/gateway-controller/pkg/logger"gateway/gateway-controller/api/gateway-controller-internal-api.yaml (2)
1-16: Consider documenting internal API security expectations.Static analysis flags missing
securitySchemesandsecuritydefinitions (CKV_OPENAPI_4/5). While internal APIs often rely on infrastructure-level protection (mTLS, service mesh, network isolation), explicitly documenting the security model helps with:
- API consumers understanding access requirements
- Static analysis tooling compliance
- Security audit documentation
If this API relies on network-level security, consider adding documentation:
components: securitySchemes: InternalNetworkOnly: type: http scheme: bearer description: | This API is intended for internal use only and should be accessed within the cluster network. No authentication token is required when accessed from authorized internal services. security: - InternalNetworkOnly: []Alternatively, if no authentication is intentional, add a comment in the description explaining the security model.
114-118: Consider adding maxItems to the errors array.Static analysis (CKV_OPENAPI_21) flags unbounded arrays. Adding a reasonable upper bound improves API contract clarity and can help with input validation.
errors: type: array description: Detailed validation errors + maxItems: 100 items: $ref: "#/components/schemas/ValidationError"gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
1917-1945: Consider retry strategy for future resilience.The current implementation retries API key generation once on collision (lines 1926-1944). While acceptable given the extremely low probability of collision with 32-byte random keys (2^256 keyspace), you might consider:
- Adding a configurable retry count for defense in depth
- Logging collision events to monitor for implementation issues
This is an optional enhancement and not a current concern.
gateway/gateway-controller/pkg/internalapi/handlers/gateway_internal_handlers.go (2)
70-70: Use idiomatic variable naming.The variable
apikeyshould follow Go naming conventions and be renamed toapiKeyfor consistency with the rest of the codebase.Apply this diff:
- apikey := req.ApiKey - if apikey == "" { + apiKey := req.ApiKey + if apiKey == "" { log.Warn("API key is missing in request body") c.JSON(http.StatusBadRequest, internalapi.ErrorResponse{ Status: "error", Message: "API key cannot be empty", }) return } log.Info("Validating API key", zap.String("apiName", name), zap.String("apiVersion", version), - zap.String("apiKey", maskApiKey(apikey)), + zap.String("apiKey", maskApiKey(apiKey)), ) // Validate input parameters - validationErrors := s.validateInputParams(name, version, apikey) + validationErrors := s.validateInputParams(name, version, apiKey)And update the validator call:
// Validate the API key using the new validation service - isValid, err := s.keyValidator.ValidateAPIKey(name, version, apikey) + isValid, err := s.keyValidator.ValidateAPIKey(name, version, apiKey)
71-78: Consider removing redundant API key validation.The API key is validated for emptiness twice:
- Lines 71-78: Early check after binding the request body
- Lines 151-156: Within
validateInputParamsSince
validateInputParamsalready checks all three parameters (name, version, apiKey), you could remove the early check at lines 71-78 and let the unified validation handle it. This would simplify the logic while maintaining the same validation coverage.Also applies to: 86-96, 151-156
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
gateway/gateway-controller/pkg/api/generated/generated.gois excluded by!**/generated/**gateway/gateway-controller/pkg/internalapi/generated/generated.gois excluded by!**/generated/**
📒 Files selected for processing (13)
gateway/gateway-controller/Makefile(1 hunks)gateway/gateway-controller/api/gateway-controller-internal-api.yaml(1 hunks)gateway/gateway-controller/api/openapi.yaml(1 hunks)gateway/gateway-controller/cmd/controller/main.go(3 hunks)gateway/gateway-controller/oapi-codegen-internal-api.yaml(1 hunks)gateway/gateway-controller/pkg/api/handlers/handlers.go(3 hunks)gateway/gateway-controller/pkg/internalapi/handlers/gateway_internal_handlers.go(1 hunks)gateway/gateway-controller/pkg/internalapi/services/apikey_validator.go(1 hunks)gateway/gateway-controller/pkg/models/api_key.go(1 hunks)gateway/gateway-controller/pkg/storage/gateway-controller-db.sql(1 hunks)gateway/gateway-controller/pkg/storage/interface.go(1 hunks)gateway/gateway-controller/pkg/storage/memory.go(3 hunks)gateway/gateway-controller/pkg/storage/sqlite.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- gateway/gateway-controller/oapi-codegen-internal-api.yaml
- gateway/gateway-controller/Makefile
- gateway/gateway-controller/pkg/storage/sqlite.go
- gateway/gateway-controller/api/openapi.yaml
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-06T11:57:08.456Z
Learnt from: BLasan
Repo: wso2/api-platform PR: 140
File: gateway/gateway-controller/pkg/utils/api_deployment.go:88-105
Timestamp: 2025-12-06T11:57:08.456Z
Learning: In the gateway-controller Go codebase (file: gateway/gateway-controller/pkg/utils/api_deployment.go), only the async/websub API kind is currently supported. async/websocket and async/sse API kinds are not yet implemented.
Applied to files:
gateway/gateway-controller/cmd/controller/main.go
📚 Learning: 2025-11-12T08:52:52.909Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 142
File: platform-api/src/resources/openapi.yaml:905-969
Timestamp: 2025-11-12T08:52:52.909Z
Learning: In the wso2/api-platform repository, the team follows an API-first development approach where OpenAPI specs may document planned features before backend implementation is complete, allowing frontend development to proceed against the intended API contract without requiring changes later.
Applied to files:
gateway/gateway-controller/pkg/internalapi/services/apikey_validator.gogateway/gateway-controller/pkg/api/handlers/handlers.gogateway/gateway-controller/api/gateway-controller-internal-api.yaml
📚 Learning: 2025-11-18T10:07:26.115Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 160
File: platform-api/src/internal/utils/api.go:1403-1434
Timestamp: 2025-11-18T10:07:26.115Z
Learning: In the wso2/api-platform repository's OpenAPI validation endpoint response (ValidateOpenAPIRequest/OpenAPIValidationResponse), backend service names are not required in the API DTO because the frontend only displays the backend URLs if they exist. The Name field can be left empty when parsing OpenAPI/Swagger definitions.
Applied to files:
gateway/gateway-controller/api/gateway-controller-internal-api.yaml
🧬 Code graph analysis (4)
gateway/gateway-controller/cmd/controller/main.go (2)
gateway/gateway-controller/pkg/internalapi/handlers/gateway_internal_handlers.go (1)
NewInternalAPIServer(43-54)gateway/gateway-controller/pkg/internalapi/generated/generated.go (1)
RegisterHandlers(112-114)
gateway/gateway-controller/pkg/internalapi/services/apikey_validator.go (3)
gateway/gateway-controller/pkg/storage/memory.go (1)
ConfigStore(30-44)gateway/gateway-controller/pkg/storage/interface.go (1)
Storage(64-208)gateway/gateway-controller/pkg/models/api_key.go (1)
APIKey(40-49)
gateway/gateway-controller/pkg/api/handlers/handlers.go (4)
gateway/gateway-controller/pkg/api/generated/generated.go (2)
Error(59-59)ErrorResponse(442-449)gateway/gateway-controller/pkg/models/api_key.go (2)
APIKey(40-49)GenerateAPIKey(52-77)gateway/gateway-controller/pkg/api/middleware/correlation.go (1)
GetLogger(70-77)gateway/gateway-controller/pkg/storage/errors.go (1)
ErrConflict(29-29)
gateway/gateway-controller/pkg/models/api_key.go (1)
kubernetes/gateway-operator/api/v1/apiconfig_spec.go (1)
APIVersion(51-51)
🪛 Checkov (3.2.334)
gateway/gateway-controller/api/gateway-controller-internal-api.yaml
[high] 1-132: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-132: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 115-121: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (10)
gateway/gateway-controller/pkg/models/api_key.go (2)
51-77: LGTM! Secure key generation implementation.Good practices observed:
- Uses
crypto/randfor cryptographically secure random bytes- 256 bits of entropy provides excellent security
- The
gw_prefix aids key identification and prevents accidental misuse of other credentials- Error handling for random byte generation is appropriate
79-95: LGTM! Clear validation semantics.The distinction between
IsValid()(checks both status and expiry) andIsExpired()(checks only time-based expiry) is appropriate. This allows callers to distinguish between explicitly revoked keys and naturally expired ones.gateway/gateway-controller/pkg/storage/interface.go (1)
148-175: LGTM! Well-documented interface additions.The new API key management methods follow the established patterns in this interface:
- Clear documentation of expected behavior
- Consistent error semantics (returns error if not found)
- Atomic operation guarantees documented
The method set covers the necessary CRUD operations plus the specific lookup patterns needed for validation and management.
gateway/gateway-controller/api/gateway-controller-internal-api.yaml (1)
18-74: LGTM! Well-designed validation endpoint.Good security practices:
- API key is passed in request body, not URL path (prevents logging/caching exposure)
minLength: 8provides basic input validationadditionalProperties: falseprevents unexpected fields- POST method is semantically appropriate for validation with sensitive data
The error response coverage (400, 404, 500) is comprehensive for this use case.
gateway/gateway-controller/cmd/controller/main.go (1)
260-262: No action needed—nil storage is already handled gracefully.The
InternalAPIServercorrectly handles the nildbcase in memory-only mode. TheAPIKeyValidator.ValidateAPIKeymethod explicitly checksif v.db != nil(line 96 in apikey_validator.go) before attempting persistent storage lookups. When running without persistent storage, it falls back to the in-memoryConfigStoreand returnsfalseif the API key is not found, preventing any nil pointer dereferences.gateway/gateway-controller/pkg/api/handlers/handlers.go (2)
553-585: LGTM - API key cleanup logic is well-structured.The implementation properly handles API key deletion during API removal with:
- Correct ordering (database first, then ConfigStore)
- Graceful handling of retrieval failures
- Detailed per-key logging for troubleshooting
- Appropriate use of warnings rather than blocking API deletion if key cleanup encounters issues
1884-1993: Well-structured API key generation handler.The implementation demonstrates good practices:
- Correlation-aware logging for traceability
- Proper validation before generation
- Collision handling with retry logic
- Rollback mechanism for consistency (confirmed as addressed in past review)
- Appropriate error responses at each failure point
gateway/gateway-controller/pkg/storage/memory.go (2)
40-57: LGTM - Efficient dual-index structure for API keys.The storage design provides optimal lookup performance:
apiKeysmap enables O(1) validation by key valueapiKeysByAPImap enables O(1) retrieval of all keys for an API- Proper initialization in the constructor
- Existing mutex provides concurrency safety
389-501: Excellent implementation of API key storage methods.All methods demonstrate solid engineering practices:
StoreAPIKey:
- Nil validation prevents invalid entries
- Conflict detection before storage
- Maintains both indexes consistently
GetAPIKeysByAPI:
- Returns empty slice instead of nil (better API ergonomics)
- Defensive copy prevents external modifications
RemoveAPIKey & RemoveAPIKeysByAPI:
- Proper cleanup of both indexes
- Empty slice cleanup prevents memory leaks
- Graceful handling of missing keys
The implementation is thread-safe, efficient, and maintains index consistency.
gateway/gateway-controller/pkg/internalapi/services/apikey_validator.go (1)
80-134: LGTM - Robust validation with proper nil handling.The
validateGatewayAPIKeyimplementation is well-designed:
- Memory-first lookup (lines 90-95) for performance
- Graceful DB fallback (lines 96-108) when available, with proper nil check for
v.db(addressed in past review)- Dual-layer defense handles both memory-only and persistent modes
- Proper API association validation (lines 116-118) prevents key reuse across APIs
- IsValid() check (line 121) centralizes status and expiration logic
- Detailed debug logging (lines 123-131) aids troubleshooting
The past review concern about nil
v.dbpanics has been confirmed as addressed.
thivindu
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
gateway/gateway-controller/pkg/storage/gateway-controller-db.sql (1)
130-130: Redundant index onapi_keycolumn.The
UNIQUEconstraint onapi_key(line 111) already creates an implicit unique index in SQLite. The explicitidx_api_keyindex is redundant and adds minor overhead during inserts/updates.Consider removing this line:
-CREATE INDEX IF NOT EXISTS idx_api_key ON api_keys(api_key);gateway/gateway-controller/api/gateway-controller-internal-api.yaml (2)
1-17: Consider documenting the internal API's security model.Static analysis (CKV_OPENAPI_4/5) flags missing
securitySchemesandsecuritydefinitions. While this is an internal API not exposed to external clients, documenting how it's protected (e.g., network isolation, mTLS, internal service mesh) in the description or via a security scheme would satisfy tooling checks and improve operational clarity.info: title: Gateway Controller Internal API description: | Internal REST API for Gateway Controller operations that are not exposed to external clients. This API provides internal functionality for validation, monitoring, and administration purposes within the Gateway Controller ecosystem. + + **Security**: This API is intended for internal service-to-service communication only. + Access should be restricted via network policies or service mesh configurations. version: 1.0.0
114-118: Consider addingmaxItemsto the errors array.Per CKV_OPENAPI_21, unbounded arrays may pose concerns. Adding a reasonable upper bound improves the schema's defensive posture.
errors: type: array description: Detailed validation errors + maxItems: 100 items: $ref: "#/components/schemas/ValidationError"gateway/gateway-controller/pkg/internalapi/handlers/gateway_internal_handlers.go (1)
151-156: Redundant empty API key check invalidateInputParams.The
apikeyparameter is already validated at lines 71-78 beforevalidateInputParamsis called. This check will never trigger.Consider removing this redundant validation or restructuring so all validation happens in one place.
func (s *InternalAPIServer) validateInputParams(name, version, apikey string) []internalapi.ValidationError { var validationErrors []internalapi.ValidationError if name == "" { validationErrors = append(validationErrors, internalapi.ValidationError{ Field: stringPtr("name"), Message: stringPtr("API name cannot be empty"), }) } if version == "" { validationErrors = append(validationErrors, internalapi.ValidationError{ Field: stringPtr("version"), Message: stringPtr("API version cannot be empty"), }) } - if apikey == "" { - validationErrors = append(validationErrors, internalapi.ValidationError{ - Field: stringPtr("apikey"), - Message: stringPtr("API key cannot be empty"), - }) - } - return validationErrors }gateway/policies/api-key-auth/v1.0.0/apikey.go (3)
254-258: Consider reusing HTTP client for better performance.Creating a new
http.Clientper request adds overhead. While acceptable for now, consider using a package-level client or pooling for high-throughput scenarios.+var httpClient = &http.Client{ + Timeout: DefaultHTTPTimeout, +} + // validateGatewayAPIKey validates API keys with "gw_" prefix against the gateway controller database func validateGatewayAPIKey(apiName, apiVersion, apiKey string) bool { // ... existing code ... - // Create HTTP client with timeout - client := &http.Client{ - Timeout: DefaultHTTPTimeout, - } + client := httpClient // ... rest of function ... }
306-314: Implement environment variable support as indicated by TODO.The hardcoded base URL limits deployment flexibility. The commented code shows the intended implementation — consider uncommenting and enabling it.
+import "os" + // getGatewayControllerBaseURL returns the gateway controller base URL // This can be overridden via environment variables or configuration func getGatewayControllerBaseURL() string { - // In a real implementation, this could check environment variables: - // if baseURL := os.Getenv("GATEWAY_CONTROLLER_BASE_URL"); baseURL != "" { - // return baseURL - // } + if baseURL := os.Getenv("GATEWAY_CONTROLLER_BASE_URL"); baseURL != "" { + return baseURL + } return DefaultGatewayControllerBaseURL }
316-340: Stub implementations silently reject valid keys withmgt_anddev_prefixes.These functions always return
false, meaning any API key withmgt_ordev_prefix will fail validation without any indication to the caller. This could cause confusion during integration.Consider either:
- Removing the switch cases for unsupported prefixes (so they fall through to "unknown prefix")
- Adding logging to indicate unimplemented validation
- Returning an error that can propagate to the response
Would you like me to suggest an implementation that provides clearer feedback for unimplemented key types?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
gateway/gateway-controller/pkg/api/generated/generated.gois excluded by!**/generated/**gateway/gateway-controller/pkg/internalapi/generated/generated.gois excluded by!**/generated/**
📒 Files selected for processing (18)
docs/gateway/policies/apikey-authentication.md(1 hunks)gateway/gateway-controller/Makefile(1 hunks)gateway/gateway-controller/api/gateway-controller-internal-api.yaml(1 hunks)gateway/gateway-controller/api/openapi.yaml(1 hunks)gateway/gateway-controller/cmd/controller/main.go(3 hunks)gateway/gateway-controller/default-policies/api-key-auth-v1.0.yaml(1 hunks)gateway/gateway-controller/oapi-codegen-internal-api.yaml(1 hunks)gateway/gateway-controller/pkg/api/handlers/handlers.go(3 hunks)gateway/gateway-controller/pkg/internalapi/handlers/gateway_internal_handlers.go(1 hunks)gateway/gateway-controller/pkg/internalapi/services/apikey_validator.go(1 hunks)gateway/gateway-controller/pkg/models/api_key.go(1 hunks)gateway/gateway-controller/pkg/storage/gateway-controller-db.sql(1 hunks)gateway/gateway-controller/pkg/storage/interface.go(1 hunks)gateway/gateway-controller/pkg/storage/memory.go(3 hunks)gateway/gateway-controller/pkg/storage/sqlite.go(3 hunks)gateway/policies/api-key-auth/v1.0.0/apikey.go(1 hunks)gateway/policies/api-key-auth/v1.0.0/go.mod(1 hunks)gateway/policies/api-key-auth/v1.0.0/policy-definition.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- gateway/gateway-controller/oapi-codegen-internal-api.yaml
- gateway/gateway-controller/api/openapi.yaml
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-12T08:52:52.909Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 142
File: platform-api/src/resources/openapi.yaml:905-969
Timestamp: 2025-11-12T08:52:52.909Z
Learning: In the wso2/api-platform repository, the team follows an API-first development approach where OpenAPI specs may document planned features before backend implementation is complete, allowing frontend development to proceed against the intended API contract without requiring changes later.
Applied to files:
gateway/gateway-controller/api/gateway-controller-internal-api.yamlgateway/gateway-controller/pkg/api/handlers/handlers.gogateway/gateway-controller/pkg/storage/sqlite.gogateway/gateway-controller/pkg/internalapi/services/apikey_validator.go
📚 Learning: 2025-11-18T10:07:26.115Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 160
File: platform-api/src/internal/utils/api.go:1403-1434
Timestamp: 2025-11-18T10:07:26.115Z
Learning: In the wso2/api-platform repository's OpenAPI validation endpoint response (ValidateOpenAPIRequest/OpenAPIValidationResponse), backend service names are not required in the API DTO because the frontend only displays the backend URLs if they exist. The Name field can be left empty when parsing OpenAPI/Swagger definitions.
Applied to files:
gateway/gateway-controller/api/gateway-controller-internal-api.yaml
📚 Learning: 2025-12-06T11:57:08.456Z
Learnt from: BLasan
Repo: wso2/api-platform PR: 140
File: gateway/gateway-controller/pkg/utils/api_deployment.go:88-105
Timestamp: 2025-12-06T11:57:08.456Z
Learning: In the gateway-controller Go codebase (file: gateway/gateway-controller/pkg/utils/api_deployment.go), only the async/websub API kind is currently supported. async/websocket and async/sse API kinds are not yet implemented.
Applied to files:
gateway/gateway-controller/cmd/controller/main.go
🧬 Code graph analysis (4)
gateway/gateway-controller/pkg/models/api_key.go (1)
kubernetes/gateway-operator/api/v1/apiconfig_spec.go (1)
APIVersion(51-51)
gateway/gateway-controller/cmd/controller/main.go (3)
gateway/gateway-controller/pkg/internalapi/handlers/gateway_internal_handlers.go (1)
NewInternalAPIServer(43-54)gateway/gateway-controller/pkg/internalapi/generated/generated.go (1)
RegisterHandlers(112-114)gateway/gateway-controller/pkg/api/generated/generated.go (1)
RegisterHandlers(1785-1787)
gateway/gateway-controller/pkg/storage/sqlite.go (2)
gateway/gateway-controller/pkg/models/api_key.go (1)
APIKey(40-49)gateway/gateway-controller/pkg/storage/errors.go (2)
ErrConflict(29-29)ErrNotFound(26-26)
gateway/gateway-controller/pkg/storage/interface.go (1)
gateway/gateway-controller/pkg/models/api_key.go (1)
APIKey(40-49)
🪛 Checkov (3.2.334)
gateway/gateway-controller/api/gateway-controller-internal-api.yaml
[high] 1-132: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-132: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 115-121: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🪛 LanguageTool
docs/gateway/policies/apikey-authentication.md
[grammar] ~65-~65: Use a hyphen to join words.
Context: ... policy configuration only requires user provided parameters when attaching to an...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (31)
gateway/gateway-controller/pkg/storage/gateway-controller-db.sql (1)
105-136: Well-structured schema for API key management.The table design with proper foreign key relationship to deployments, appropriate status check constraint, and nullable
expires_atfor optional expiration is solid. The ON DELETE CASCADE ensures keys are cleaned up when APIs are removed.gateway/gateway-controller/Makefile (1)
42-43: LGTM!The internal API code generation follows the same pattern as the public API, using a consistent
oapi-codegenversion and proper configuration file structure.gateway/gateway-controller/cmd/controller/main.go (2)
260-262: Verify internal API exposure is intentional.The internal API handlers are registered on the same router/port as the public API, making endpoints like
/api/internal/v1/apis/{name}/{version}/validate-apikeypublicly accessible. If these are meant for internal service-to-service communication only, consider:
- Binding internal API to a separate port
- Adding authentication/authorization middleware for internal endpoints
- Using network-level isolation (e.g., internal-only listener)
Is exposing the internal API on the public port intentional? If not:
// Option: Create separate internal router and server internalRouter := gin.New() internalRouter.Use(middleware.InternalAuthMiddleware()) // Add auth internalapi.RegisterHandlers(internalRouter, internalAPIServer) // Start on separate port
7-7: LGTM on imports.The new imports for internal API handler and generated code are correctly structured and follow the existing import patterns.
Also applies to: 20-20
gateway/policies/api-key-auth/v1.0.0/policy-definition.yaml (2)
1-50: Well-documented policy definition.The policy definition is comprehensive with clear descriptions for each parameter, proper enum constraints for the
infield, and sensible validation lengths. The description clearly explains header vs. query behavior and prefix stripping functionality.
12-15: Remove non-standardrequired: true/falsefrom individual property definitions.Lines 14, 25, and 35 use
required: true/falsewithin property definitions, which deviates from standard JSON Schema and the pattern used throughout the codebase (basic-auth, jwt-auth, modify-headers policies). The correctrequiredarray is already present at lines 44-46. Remove the inlinerequireddeclarations and rely only on the object-levelrequiredarray to specify mandatory fields.Also applies to: 23-26, 33-35
docs/gateway/policies/apikey-authentication.md (1)
1-115: Comprehensive documentation with practical examples.The documentation covers key features, configuration schema, and provides varied examples (header, Bearer prefix, query parameter, custom header). This aligns well with the policy definition.
gateway/gateway-controller/pkg/models/api_key.go (2)
52-77: Solid API key generation implementation.Good security practices: using
crypto/randfor cryptographic randomness, 32 bytes (256 bits) of entropy, and a clear "gw_" prefix for identification. The UUID for record ID is appropriate for database keying.
79-95: LGTM on validation methods.
IsValid()andIsExpired()correctly handle the nullableExpiresAtfield. The independenttime.Now()calls in each method are standard practice and acceptable for this use case.gateway/gateway-controller/pkg/storage/interface.go (1)
148-175: Well-designed interface extension for API key management.The new methods follow the established patterns in this interface with consistent documentation, error semantics, and naming conventions. The method signatures properly align with the
models.APIKeystruct and support the full CRUD lifecycle.gateway/gateway-controller/pkg/api/handlers/handlers.go (2)
553-585: API key cleanup on API deletion is well-implemented.The cleanup logic properly:
- Retrieves all API keys for the deleted API
- Deletes each from the database with individual error logging
- Removes keys from the ConfigStore
- Continues processing even if individual key deletions fail (resilient)
The warn-level logging for retrieval failures and debug-level for successful deletions is appropriate.
1883-1993: GenerateAPIKey handler is well-structured with proper error handling.The implementation correctly:
- Validates API existence before key generation
- Handles the rare collision case with a single retry (appropriate for 32-byte random keys)
- Maintains consistency between DB and ConfigStore with rollback on failure
- Returns appropriate HTTP status codes (404 for not found, 500 for errors, 201 for success)
gateway/gateway-controller/pkg/storage/sqlite.go (5)
1011-1047: SaveAPIKey implementation follows established patterns.The method correctly handles unique constraint violations by mapping them to
ErrConflict, and the success log appropriately omits the sensitive API key value.
1049-1137: API key retrieval methods are correctly implemented.Both
GetAPIKeyByKeyandGetAPIKeysByAPIproperly handle the nullableexpires_atfield usingsql.NullTimeand follow established error handling patterns.
1139-1195: Update and delete operations follow security best practices.
UpdateAPIKeycorrectly updates only mutable fields (status, updated_at, expires_at) whileDeleteAPIKeymasks the key in logs (key[:min(8, len(key))]+"***") to prevent sensitive data exposure.
1311-1316: Helper function for constraint error detection is consistent with existing patterns.The
isAPIKeyUniqueConstraintErrorfunction follows the same pattern asisUniqueConstraintErrorandisCertificateUniqueConstraintErrorin this file.
175-207: Schema migration to version 5 is correctly implemented.The migration properly creates the
api_keystable with:
- Appropriate constraints (UNIQUE on api_key, CHECK on status values)
- Foreign key referencing
deployments(name, version)with CASCADE delete (both columns are TEXT NOT NULL with UNIQUE constraint)- Useful indexes for common query patterns (by key, by API, by status, by expiry)
- Correct version progression (check version 4 → set version 5)
gateway/gateway-controller/default-policies/api-key-auth-v1.0.yaml (1)
1-50: Well-documented API key authentication policy definition.The policy manifest provides:
- Clear parameter descriptions for
key,in, andvalue-prefix- Appropriate validation constraints (minLength/maxLength)
- Support for flexible key extraction (header or query)
- Optional prefix stripping for Authorization header patterns
This aligns well with the API key validation flow implemented across the PR.
gateway/gateway-controller/pkg/storage/memory.go (5)
40-57: In-memory API key storage is well-designed with dual indexing.The dual-map approach (
apiKeysfor O(1) key lookup,apiKeysByAPIfor listing by API) is appropriate for the access patterns. Initialization inNewConfigStoreensures maps are ready for use.
394-416: StoreAPIKey correctly enforces uniqueness and maintains both indexes.The method properly checks for conflicts before storing and updates both the primary key map and the API-specific index atomically under the lock.
431-446: Returning a copy in GetAPIKeysByAPI is a good defensive practice.This prevents callers from accidentally modifying the internal slice, maintaining data integrity.
448-479: RemoveAPIKey properly maintains both indexes and cleans up empty entries.The cleanup of empty slices from
apiKeysByAPI(lines 472-475) prevents memory leaks from accumulating empty map entries over time.
481-500: RemoveAPIKeysByAPI efficiently handles bulk removal.The method correctly iterates through the API's keys to remove them from the primary map before cleaning up the API-specific entry. Returning
nilwhen no keys exist is appropriate behavior.gateway/gateway-controller/pkg/internalapi/handlers/gateway_internal_handlers.go (3)
34-54: LGTM!The struct and constructor are well-structured with proper dependency injection. The
APIKeyValidatoris created inline during construction, keeping the initialization clean.
56-131: LGTM!The handler follows a clean validation flow with proper error handling and logging. The correlation-aware logger, structured error responses, and masking of API keys in logs are good practices.
161-172: LGTM!The
maskApiKeyfunction properly handles short keys by fully masking them and reveals only the first and last 4 characters for longer keys. ThestringPtrhelper is a standard Go pattern.gateway/gateway-controller/pkg/internalapi/services/apikey_validator.go (2)
50-69: LGTM!The prefix-based routing design is clean and extensible for adding new key types in the future. Logging unknown prefixes as warnings is appropriate.
80-134: LGTM!The validation logic correctly implements a tiered lookup strategy: in-memory store first, then persistent storage fallback when available. The nil check for
v.dbat line 96 properly handles non-persistent mode.gateway/policies/api-key-auth/v1.0.0/apikey.go (3)
131-164: LGTM!The success and failure handlers properly set metadata and return appropriate responses. The 401 response with JSON body follows standard authentication failure patterns.
182-230: LGTM!The helper functions are well-implemented:
extractQueryParamproperly handles URL decoding and parsingstripPrefixcorrectly performs case-insensitive prefix matchingextractAPIKeyPrefixfollows the same pattern as the server-side implementation
278-303: LGTM!The response handling covers all expected status codes appropriately. Treating network errors and unexpected statuses as validation failures is the correct security posture.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
gateway/policies/api-key-auth/v1.0.0/apikey.go (4)
88-105: Clarify handling ofin/header name and prefix stripping semanticsThe overall flow in
OnRequestis solid, but there are a few edge behaviors worth tightening up:
locationvalues other than"header"or"query"currently fall through and end up as “missing API key”. It may be clearer to treat an unsupportedinas a configuration error and fail fast (similar to the'key'/'in'validation) rather than as if the client omitted the key.ctx.Headers.Get(strings.ToLower(keyName))assumes the underlying header store is case-normalized and supports lower-case lookups; if it is keyed by canonical names, this could miss headers likeX-API-Key. Consider either usingkeyNameas provided, or confirming thatHeaders.Getis case-insensitive.stripPrefixreturns an empty string when the prefix does not match at all, which turns “value present but with wrong prefix” into “missing API key”. If you want “strip only when present”, consider returning the original value on non-match instead.A possible tweak for the prefix helper:
-func stripPrefix(value, prefix string) string { - // Do exact case-insensitive prefix matching - if len(value) >= len(prefix) && strings.EqualFold(value[:len(prefix)], prefix) { - return value[len(prefix):] - } - - // No matching prefix found, return empty string - return "" -} +func stripPrefix(value, prefix string) string { + if len(prefix) == 0 { + return value + } + if len(value) >= len(prefix) && strings.EqualFold(value[:len(prefix)], prefix) { + return value[len(prefix):] + } + // No matching prefix; keep original so it can still be validated + return value +}…and then treat an empty string after stripping as “missing” as you already do.
Also applies to: 110-118, 125-133, 231-241
186-200: Prefix-based dispatch is clear; consider explicit handling for unsupported prefixesThe prefix extraction and dispatch (
gw_/mgt_/dev_) is clean and handles case variations correctly. Returningfalsefor unknown/no-prefix keys is a sensible default.If you expect additional key types in future, you might want to:
- Add a small metric/log for the
defaultbranch invalidateAPIKeyto help detect unexpected prefixes.- Optionally differentiate “unsupported prefix” vs “invalid key” internally (while still returning 401 to the client).
Also applies to: 243-250
202-229: Use standard URL parsing instead of manual split for query parameter extraction
extractQueryParamwill work for the common case but is reimplementing URL parsing (manualPathUnescape,Spliton"?", etc.), which can be brittle around edge cases.You can simplify and harden this using the stdlib parser:
-func extractQueryParam(path, param string) string { - // Parse the URL-encoded path - decodedPath, err := url.PathUnescape(path) - if err != nil { - return "" - } - - // Split the path into components - parts := strings.Split(decodedPath, "?") - if len(parts) != 2 { - return "" - } - - // Parse the query string - queryString := parts[1] - values, err := url.ParseQuery(queryString) - if err != nil { - return "" - } - - // Get the first value of the specified parameter - if value, ok := values[param]; ok && len(value) > 0 { - return value[0] - } - return "" -} +func extractQueryParam(path, param string) string { + u, err := url.ParseRequestURI(path) + if err != nil { + return "" + } + return u.Query().Get(param) +}This should handle encoding and edge cases more consistently.
252-319: Gateway validation flow is safe (fail-closed); consider config override and observabilityThe gateway-controller validation call is generally well-structured and, importantly, fails closed (any network/parse/5xx issue -> invalid key), which is correct for auth.
Two improvements to consider:
- Configurability:
getGatewayControllerBaseURLcurrently hardcodesDefaultGatewayControllerBaseURL. Enabling an env/config override now (instead of leaving it as a commented TODO) would make non-default deployments and local dev simpler.- Observability: All failure paths just return
false; without logs/metrics it will be hard to distinguish “invalid key” from “controller down/buggy”. Even a coarse-grained log/metric on non-200 responses and network errors would help operations without changing external behavior.Also applies to: 321-329
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gateway/policies/api-key-auth/v1.0.0/apikey.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
gateway/policies/api-key-auth/v1.0.0/apikey.go (3)
sdk/gateway/policy/v1alpha/interface.go (4)
PolicyMetadata(5-8)ProcessingMode(55-67)HeaderModeProcess(77-77)HeaderModeSkip(74-74)sdk/gateway/policy/v1alpha/context.go (3)
RequestContext(50-71)ResponseContext(74-95)Body(4-16)sdk/gateway/policy/v1alpha/action.go (3)
RequestAction(4-7)UpstreamRequestModifications(16-23)ImmediateResponse(31-35)
🔇 Additional comments (2)
gateway/policies/api-key-auth/v1.0.0/apikey.go (2)
32-40: Singleton policy and processing mode look appropriateUsing a shared
http.Clientwith a reasonable timeout and declaring header-only request processing is a good fit for this policy; no issues here from a concurrency or integration perspective.Also applies to: 58-67, 69-86
151-159: Auth success and response hook behavior are consistentSetting
auth.success/auth.methodmetadata and returning an emptyUpstreamRequestModifications, plus a no-opOnResponse, is consistent with a request-only auth policy and should integrate cleanly with the engine.Also applies to: 161-164
| // handleAuthFailure handles authentication failure | ||
| func (p *APIKeyPolicy) handleAuthFailure(ctx *policy.RequestContext, reason string) policy.RequestAction { | ||
| // Set metadata indicating failed authentication | ||
| ctx.Metadata[MetadataKeyAuthSuccess] = false | ||
| ctx.Metadata[MetadataKeyAuthMethod] = "api-key" | ||
|
|
||
| // Return 401 Unauthorized response | ||
| headers := map[string]string{ | ||
| "content-type": "application/json", | ||
| } | ||
|
|
||
| body := fmt.Sprintf(`{"error": "Unauthorized", "message": "Valid API key required - %s"}`, reason) | ||
|
|
||
| return policy.ImmediateResponse{ | ||
| StatusCode: 401, | ||
| Headers: headers, | ||
| Body: []byte(body), | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all calls to handleAuthFailure to see what reasons are passed
rg "handleAuthFailure" gateway/policies/api-key-auth/v1.0.0/ -A 2 -B 2Repository: wso2/api-platform
Length of output: 2777
🏁 Script executed:
# Also check the full file to understand the context better
cat -n gateway/policies/api-key-auth/v1.0.0/apikey.go | head -200Repository: wso2/api-platform
Length of output: 7516
Avoid exposing internal configuration details in error messages
handleAuthFailure currently passes internal reasons (e.g., "missing or invalid 'key' configuration", "missing or invalid 'in' configuration", and "API name or version not found") directly into the client-visible message field. This leaks configuration structure and internal implementation details to callers, which is undesirable for security hardening.
Consider:
- Returning a generic message like
"Valid API key required"to clients regardless of the specific failure reason. - Logging the detailed
reason(and relevant metadata) server-side instead of including it in the response body.
For example:
-func (p *APIKeyPolicy) handleAuthFailure(ctx *policy.RequestContext, reason string) policy.RequestAction {
+func (p *APIKeyPolicy) handleAuthFailure(ctx *policy.RequestContext, reason string) policy.RequestAction {
// Set metadata indicating failed authentication
ctx.Metadata[MetadataKeyAuthSuccess] = false
ctx.Metadata[MetadataKeyAuthMethod] = "api-key"
- // Return 401 Unauthorized response
+ // TODO: log `reason` at debug/info level with route metadata.
+
+ // Return 401 Unauthorized response with generic message
headers := map[string]string{
"content-type": "application/json",
}
- body := fmt.Sprintf(`{"error": "Unauthorized", "message": "Valid API key required - %s"}`, reason)
+ body := `{"error": "Unauthorized", "message": "Valid API key required"}`📝 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.
| // handleAuthFailure handles authentication failure | |
| func (p *APIKeyPolicy) handleAuthFailure(ctx *policy.RequestContext, reason string) policy.RequestAction { | |
| // Set metadata indicating failed authentication | |
| ctx.Metadata[MetadataKeyAuthSuccess] = false | |
| ctx.Metadata[MetadataKeyAuthMethod] = "api-key" | |
| // Return 401 Unauthorized response | |
| headers := map[string]string{ | |
| "content-type": "application/json", | |
| } | |
| body := fmt.Sprintf(`{"error": "Unauthorized", "message": "Valid API key required - %s"}`, reason) | |
| return policy.ImmediateResponse{ | |
| StatusCode: 401, | |
| Headers: headers, | |
| Body: []byte(body), | |
| } | |
| } | |
| // handleAuthFailure handles authentication failure | |
| func (p *APIKeyPolicy) handleAuthFailure(ctx *policy.RequestContext, reason string) policy.RequestAction { | |
| // Set metadata indicating failed authentication | |
| ctx.Metadata[MetadataKeyAuthSuccess] = false | |
| ctx.Metadata[MetadataKeyAuthMethod] = "api-key" | |
| // TODO: log `reason` at debug/info level with route metadata. | |
| // Return 401 Unauthorized response with generic message | |
| headers := map[string]string{ | |
| "content-type": "application/json", | |
| } | |
| body := `{"error": "Unauthorized", "message": "Valid API key required"}` | |
| return policy.ImmediateResponse{ | |
| StatusCode: 401, | |
| Headers: headers, | |
| Body: []byte(body), | |
| } | |
| } |
🤖 Prompt for AI Agents
In gateway/policies/api-key-auth/v1.0.0/apikey.go around lines 166 to 184, the
function currently embeds internal failure reasons in the client-facing JSON
message which leaks configuration details; change it to always return a generic
message like "Valid API key required" in the response body while moving the
detailed reason into a server-side log (or retain it only in ctx.Metadata) —
keep the Metadata flags (AuthSuccess/AuthMethod) as-is, log ctx.Metadata and the
detailed reason via the policy logger (or fmt) before returning the
ImmediateResponse, and ensure the response headers/body no longer include the
detailed reason.
There was a problem hiding this comment.
The reason can be "missing API key", "invalid API key" or other generic strings. And the key and in configurations are not internal and they are user provided. These do not expose internal configurations.
There was a problem hiding this comment.
@thivindu, you're right that most of the messages are generic and appropriate. Messages like "missing API key" and "invalid API key" are perfectly fine for end-users.
However, looking at the code more carefully, there are a couple of messages that reference configuration parameter names:
- Line 92:
"missing or invalid 'key' configuration" - Line 96:
"missing or invalid 'in' configuration"
These occur when the policy itself is misconfigured (i.e., when the API developer hasn't properly configured the policy parameters). While you're correct that the schema is public and these aren't "secret" internal details, OWASP recommends to "respond with generic error messages - avoid revealing details of the failure unnecessarily".
The bigger consideration here is less about security and more about API design: when the policy is misconfigured, should end-users making API calls see a 401 Unauthorized with details about missing policy configuration? This seems more like an internal server error (500) scenario since it indicates a deployment/configuration issue rather than an authentication problem.
That said, if you're comfortable with the current approach and believe it provides helpful debugging information without meaningful security risk, I respect that decision. This is more of a best practice consideration than a critical vulnerability, especially since the policy schema is publicly documented.
| // validateManagementPortalAPIKey validates API keys with "mgt_" prefix against the management portal | ||
| func (p *APIKeyPolicy) validateManagementPortalAPIKey(apiName, apiVersion, apiKey string) bool { | ||
| // TODO: Implement management portal API key validation | ||
| // This should make an HTTP request to the management portal's API key validation endpoint | ||
| // Example implementation: | ||
| // 1. Use p.httpClient to make HTTP request | ||
| // 2. Make POST request to management portal: POST /api/v1/validate-key | ||
| // 3. Send payload: {"apiName": apiName, "apiVersion": apiVersion, "apiKey": apiKey} | ||
| // 4. Parse response and return validation result | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
mgt_/dev_ key paths are stubbed and always fail
Right now validateManagementPortalAPIKey and validateDevPortalAPIKey are TODO implementations that always return false, while validateAPIKey already routes "mgt" and "dev" prefixes here.
If management/developer portal API keys are not yet issued, this is acceptable but should be:
- Clearly documented in the policy docs / configuration (e.g., “only
gw_keys are currently supported”). - Guarded (optionally) by config/feature flags so it’s obvious why these prefixes always fail.
Once those portals start issuing keys, these stubs will need real implementations wired to their validation endpoints.
Also applies to: 344-355
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
gateway/policies/api-key-auth/v1.0.0/apikey.go (1)
329-353:mgt_/dev_paths still always fail (TODO stubs).
This was already raised earlier; just ensure docs/config make it explicit which prefixes are supported in this release.
🧹 Nitpick comments (5)
gateway/gateway-controller/api/gateway-controller-internal-api.yaml (1)
1-130: Consider documenting the internal API's security model.The internal API specification is well-structured, and the past security concern (API key in URL path) has been properly addressed by moving it to the request body. However, the OpenAPI document lacks security scheme definitions, which triggers static analysis warnings (CKV_OPENAPI_4/5).
While this is an internal API likely protected by network isolation or infrastructure-level security (mTLS, firewall rules), explicitly documenting the security model in the OpenAPI spec provides benefits:
- Improves tooling support and code generation
- Makes security expectations clear to developers
- Helps with compliance and security audits
Consider adding a security schemes section if applicable:
components: securitySchemes: internalMTLS: type: mutualTLS description: Internal mTLS authentication (enforced at infrastructure level) # ... existing schemas # Then reference in the operation: security: - internalMTLS: []If security is purely handled outside the application layer (e.g., Kubernetes network policies), you can document this in the API description and ignore the static analysis warnings.
The missing
maxItemson the errors array (line 115) is a minor style issue and can be safely ignored or set to a reasonable limit like 50 if desired.gateway/gateway-controller/pkg/storage/sqlite.go (1)
1311-1316: Prefer typed sqlite3 constraint checks overerr.Error()string matching.
String matching is brittle across driver versions/locales. Since you already depend ongithub.com/mattn/go-sqlite3, useerrors.Asintosqlite3.Error(checking constraint/extended codes) and optionally fall back to message matching.+import ( + ... + sqlite3 "github.com/mattn/go-sqlite3" +) + func isAPIKeyUniqueConstraintError(err error) bool { - return err != nil && - (err.Error() == "UNIQUE constraint failed: api_keys.api_key" || - err.Error() == "UNIQUE constraint failed: api_keys.id") + if err == nil { + return false + } + var se sqlite3.Error + if errors.As(err, &se) { + // ErrConstraintUnique / ErrConstraintPrimaryKey depending on which UNIQUE/PK hit + return se.ExtendedCode == sqlite3.ErrConstraintUnique || + se.ExtendedCode == sqlite3.ErrConstraintPrimaryKey + } + return err.Error() == "UNIQUE constraint failed: api_keys.api_key" || + err.Error() == "UNIQUE constraint failed: api_keys.id" }gateway/gateway-controller/pkg/storage/memory.go (1)
419-430: *Returning the cached models.APIKey pointer allows callers to mutate cache state without locks.
If callers might modify fields (status/expiry), consider returning a shallow copy (or documenting immutability expectations).gateway/gateway-controller/pkg/internalapi/handlers/gateway_internal_handlers.go (1)
56-131: Validation flow is solid; consider using a single validation path (avoid double-checking empty apiKey).
You checkapikey == ""before callingvalidateInputParams, which checks it again—can simplify to one source of truth.gateway/policies/api-key-auth/v1.0.0/apikey.go (1)
32-67: HTTP client timeout is a good default; keep it configurable if possible.
If this policy runs in varied network setups, consider reading timeout/baseURL from env/config (you already hint at it).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
gateway/gateway-controller/pkg/api/generated/generated.gois excluded by!**/generated/**gateway/gateway-controller/pkg/internalapi/generated/generated.gois excluded by!**/generated/**
📒 Files selected for processing (19)
docs/gateway/policies/apikey-authentication.md(1 hunks)gateway/gateway-controller/Makefile(1 hunks)gateway/gateway-controller/api/gateway-controller-internal-api.yaml(1 hunks)gateway/gateway-controller/api/openapi.yaml(1 hunks)gateway/gateway-controller/cmd/controller/main.go(3 hunks)gateway/gateway-controller/default-policies/APIKeyAuthentication-v1.0.0.yaml(1 hunks)gateway/gateway-controller/oapi-codegen-internal-api.yaml(1 hunks)gateway/gateway-controller/pkg/api/handlers/handlers.go(3 hunks)gateway/gateway-controller/pkg/internalapi/handlers/gateway_internal_handlers.go(1 hunks)gateway/gateway-controller/pkg/internalapi/services/apikey_validator.go(1 hunks)gateway/gateway-controller/pkg/models/api_key.go(1 hunks)gateway/gateway-controller/pkg/storage/gateway-controller-db.sql(1 hunks)gateway/gateway-controller/pkg/storage/interface.go(1 hunks)gateway/gateway-controller/pkg/storage/memory.go(3 hunks)gateway/gateway-controller/pkg/storage/sqlite.go(3 hunks)gateway/policies/api-key-auth/v1.0.0/apikey.go(1 hunks)gateway/policies/api-key-auth/v1.0.0/go.mod(1 hunks)gateway/policies/api-key-auth/v1.0.0/policy-definition.yaml(1 hunks)gateway/policies/policy-manifest.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- gateway/gateway-controller/oapi-codegen-internal-api.yaml
- gateway/policies/api-key-auth/v1.0.0/go.mod
- gateway/gateway-controller/pkg/models/api_key.go
- gateway/gateway-controller/Makefile
- gateway/gateway-controller/pkg/internalapi/services/apikey_validator.go
- gateway/gateway-controller/cmd/controller/main.go
- gateway/gateway-controller/pkg/storage/gateway-controller-db.sql
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-12T04:05:38.530Z
Learnt from: Thenujan-Nagaratnam
Repo: wso2/api-platform PR: 385
File: gateway/policies/url-guardrail/v0.1.0/urlguardrail.go:261-281
Timestamp: 2025-12-12T04:05:38.530Z
Learning: In the api-platform repository, when adding new guardrail policies, security enhancements that are not present in existing/old policies are deferred to future versions to maintain consistency across policy implementations.
Applied to files:
gateway/policies/policy-manifest.yaml
📚 Learning: 2025-11-12T08:52:52.909Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 142
File: platform-api/src/resources/openapi.yaml:905-969
Timestamp: 2025-11-12T08:52:52.909Z
Learning: In the wso2/api-platform repository, the team follows an API-first development approach where OpenAPI specs may document planned features before backend implementation is complete, allowing frontend development to proceed against the intended API contract without requiring changes later.
Applied to files:
gateway/gateway-controller/pkg/storage/sqlite.gogateway/gateway-controller/pkg/api/handlers/handlers.gogateway/gateway-controller/api/gateway-controller-internal-api.yaml
📚 Learning: 2025-12-06T11:57:08.456Z
Learnt from: BLasan
Repo: wso2/api-platform PR: 140
File: gateway/gateway-controller/pkg/utils/api_deployment.go:88-105
Timestamp: 2025-12-06T11:57:08.456Z
Learning: In the gateway-controller Go codebase (file: gateway/gateway-controller/pkg/utils/api_deployment.go), only the async/websub API kind is currently supported. async/websocket and async/sse API kinds are not yet implemented.
Applied to files:
gateway/gateway-controller/pkg/api/handlers/handlers.go
📚 Learning: 2025-11-18T10:07:26.115Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 160
File: platform-api/src/internal/utils/api.go:1403-1434
Timestamp: 2025-11-18T10:07:26.115Z
Learning: In the wso2/api-platform repository's OpenAPI validation endpoint response (ValidateOpenAPIRequest/OpenAPIValidationResponse), backend service names are not required in the API DTO because the frontend only displays the backend URLs if they exist. The Name field can be left empty when parsing OpenAPI/Swagger definitions.
Applied to files:
gateway/gateway-controller/api/gateway-controller-internal-api.yaml
🧬 Code graph analysis (4)
gateway/gateway-controller/pkg/storage/memory.go (2)
gateway/gateway-controller/pkg/models/api_key.go (1)
APIKey(40-49)gateway/gateway-controller/pkg/storage/errors.go (2)
ErrConflict(29-29)ErrNotFound(26-26)
gateway/gateway-controller/pkg/storage/sqlite.go (2)
gateway/gateway-controller/pkg/models/api_key.go (1)
APIKey(40-49)gateway/gateway-controller/pkg/storage/errors.go (2)
ErrConflict(29-29)ErrNotFound(26-26)
gateway/gateway-controller/pkg/storage/interface.go (1)
gateway/gateway-controller/pkg/models/api_key.go (1)
APIKey(40-49)
gateway/gateway-controller/pkg/api/handlers/handlers.go (5)
gateway/gateway-controller/pkg/api/generated/generated.go (2)
Error(53-53)ErrorResponse(440-447)gateway/gateway-controller/pkg/models/api_key.go (2)
APIKey(40-49)GenerateAPIKey(52-77)gateway/gateway-controller/pkg/api/middleware/correlation.go (1)
GetLogger(70-77)gateway/gateway-controller/pkg/internalapi/generated/generated.go (1)
ErrorResponse(34-41)gateway/gateway-controller/pkg/storage/errors.go (1)
ErrConflict(29-29)
🪛 Checkov (3.2.334)
gateway/gateway-controller/api/gateway-controller-internal-api.yaml
[high] 1-132: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-132: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 115-121: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🪛 LanguageTool
docs/gateway/policies/apikey-authentication.md
[grammar] ~65-~65: Use a hyphen to join words.
Context: ... policy configuration only requires user provided parameters when attaching to an...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (11)
gateway/gateway-controller/pkg/api/handlers/handlers.go (2)
1943-2052: LGTM! Solid API key generation implementation.The handler correctly implements the complete key generation flow with:
- API existence validation before key generation
- Cryptographically secure random key generation (32-byte hex with
gw_prefix)- Collision detection with single retry (appropriate for the extremely low probability of collision with 32-byte random keys)
- Atomic dual-write pattern: DB persistence followed by ConfigStore with rollback on failure
- Comprehensive error handling and correlation-aware logging
The rollback mechanism on ConfigStore failure (lines 2024-2029) properly maintains consistency between storage layers, as noted in the past review.
573-605: LGTM! Proper API key cleanup on API deletion.The implementation correctly handles API key lifecycle when an API is deleted:
- Retrieves all associated API keys from the database
- Deletes each key from both persistent storage and in-memory ConfigStore
- Uses appropriate logging levels (warn for retrieval failures, error for deletion failures, debug for successful deletions)
- Best-effort cleanup approach ensures API deletion proceeds even if some key deletions fail
The database foreign key with
ON DELETE CASCADEprovides additional safety for data consistency.gateway/policies/policy-manifest.yaml (1)
58-61: LGTM! Policy registration is correct.The APIKeyAuthentication policy entry follows the established manifest format and is properly positioned in the policy list.
gateway/gateway-controller/default-policies/APIKeyAuthentication-v1.0.0.yaml (1)
1-47: LGTM! Policy definition is well-structured.The APIKeyAuthentication policy definition correctly specifies:
- Clear parameter schema with appropriate constraints (minLength: 1, maxLength: 128 for key name)
- Proper enum restriction for the
infield (header/query)- Reasonable validation limits for value-prefix (1-64 characters)
- Empty systemParameters indicating user-provided configuration only
The structure aligns with other policy definitions in the codebase.
docs/gateway/policies/apikey-authentication.md (1)
1-115: LGTM! Comprehensive policy documentation.The documentation effectively covers:
- Clear feature introduction and capabilities
- Complete configuration schema with parameter descriptions
- Practical examples covering common use cases (header-based, Authorization with Bearer prefix, query parameter, custom headers)
- Important notes about key management and validation flow
The examples are well-chosen and demonstrate the flexibility of the policy configuration.
gateway/policies/api-key-auth/v1.0.0/policy-definition.yaml (1)
1-47: LGTM! Policy definition matches the default policy configuration.The policy definition is consistent with the version in
gateway/gateway-controller/default-policies/and properly structured for API key authentication.gateway/gateway-controller/api/openapi.yaml (1)
379-453: LGTM! API endpoint properly defined.The new API key generation endpoint is well-designed:
- Uses POST method (appropriate for resource creation)
- Clear path parameters for API identification
- Comprehensive response schema including the generated key (necessary for one-time retrieval), metadata, and expiration information
- Proper error responses for not found (404) and internal errors (500)
- Good documentation explaining the key format (32-byte hex with
gw_prefix)Returning the API key in the response is the correct behavior for a generation endpoint, as clients must receive the key value exactly once at creation time.
gateway/gateway-controller/pkg/storage/interface.go (1)
148-175: LGTM! Storage interface properly extended.The new API key management methods are well-designed:
- Complete CRUD operations: Save, Get (by key and by API), Update, Delete
- Clear documentation explaining each method's purpose and usage
- Consistent with existing Storage interface patterns
- Appropriate method signatures for the use cases (validation, listing, lifecycle management)
- Atomicity and thread-safety requirements documented
The interface additions enable proper persistence and retrieval of API keys across different storage backends.
gateway/gateway-controller/pkg/storage/sqlite.go (1)
175-207: Migration to schema v5 looks consistent now (version + PRAGMA + logs).
Good to seePRAGMA user_version = 5aligned withversion = 5and the v5 log message.gateway/gateway-controller/pkg/storage/memory.go (1)
41-58: ConfigStore API-key maps: initialization + locking strategy look correct.
RWMutex coverage matches the new maps’ access patterns.gateway/gateway-controller/pkg/internalapi/handlers/gateway_internal_handlers.go (1)
42-54: Constructor wiring is clean (store/db/logger + validator).
Nice separation by delegating actual validation toservices.APIKeyValidator.
| // validateInputParams validates the input parameters and returns structured validation errors | ||
| func (s *InternalAPIServer) validateInputParams(name, version, apikey string) []internalapi.ValidationError { | ||
| var validationErrors []internalapi.ValidationError | ||
|
|
||
| if name == "" { | ||
| validationErrors = append(validationErrors, internalapi.ValidationError{ | ||
| Field: stringPtr("name"), | ||
| Message: stringPtr("API name cannot be empty"), | ||
| }) | ||
| } | ||
|
|
||
| if version == "" { | ||
| validationErrors = append(validationErrors, internalapi.ValidationError{ | ||
| Field: stringPtr("version"), | ||
| Message: stringPtr("API version cannot be empty"), | ||
| }) | ||
| } | ||
|
|
||
| if apikey == "" { | ||
| validationErrors = append(validationErrors, internalapi.ValidationError{ | ||
| Field: stringPtr("apikey"), | ||
| Message: stringPtr("API key cannot be empty"), | ||
| }) | ||
| } | ||
|
|
||
| return validationErrors | ||
| } |
There was a problem hiding this comment.
ValidationError field name likely should be apiKey, not apikey.
Clients will typically key off the JSON/property name; apikey is inconsistent with ApiKey / "apiKey" used elsewhere in this handler.
func (s *InternalAPIServer) validateInputParams(name, version, apikey string) []internalapi.ValidationError {
@@
if apikey == "" {
validationErrors = append(validationErrors, internalapi.ValidationError{
- Field: stringPtr("apikey"),
+ Field: stringPtr("apiKey"),
Message: stringPtr("API key cannot be empty"),
})
}📝 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.
| // validateInputParams validates the input parameters and returns structured validation errors | |
| func (s *InternalAPIServer) validateInputParams(name, version, apikey string) []internalapi.ValidationError { | |
| var validationErrors []internalapi.ValidationError | |
| if name == "" { | |
| validationErrors = append(validationErrors, internalapi.ValidationError{ | |
| Field: stringPtr("name"), | |
| Message: stringPtr("API name cannot be empty"), | |
| }) | |
| } | |
| if version == "" { | |
| validationErrors = append(validationErrors, internalapi.ValidationError{ | |
| Field: stringPtr("version"), | |
| Message: stringPtr("API version cannot be empty"), | |
| }) | |
| } | |
| if apikey == "" { | |
| validationErrors = append(validationErrors, internalapi.ValidationError{ | |
| Field: stringPtr("apikey"), | |
| Message: stringPtr("API key cannot be empty"), | |
| }) | |
| } | |
| return validationErrors | |
| } | |
| // validateInputParams validates the input parameters and returns structured validation errors | |
| func (s *InternalAPIServer) validateInputParams(name, version, apikey string) []internalapi.ValidationError { | |
| var validationErrors []internalapi.ValidationError | |
| if name == "" { | |
| validationErrors = append(validationErrors, internalapi.ValidationError{ | |
| Field: stringPtr("name"), | |
| Message: stringPtr("API name cannot be empty"), | |
| }) | |
| } | |
| if version == "" { | |
| validationErrors = append(validationErrors, internalapi.ValidationError{ | |
| Field: stringPtr("version"), | |
| Message: stringPtr("API version cannot be empty"), | |
| }) | |
| } | |
| if apikey == "" { | |
| validationErrors = append(validationErrors, internalapi.ValidationError{ | |
| Field: stringPtr("apiKey"), | |
| Message: stringPtr("API key cannot be empty"), | |
| }) | |
| } | |
| return validationErrors | |
| } |
🤖 Prompt for AI Agents
gateway/gateway-controller/pkg/internalapi/handlers/gateway_internal_handlers.go
around lines 133 to 159: the ValidationError for the API key uses "apikey"
(lowercase) but the rest of the code and clients expect "apiKey"; update the
Field value for that validation error to "apiKey" to match the canonical
JSON/property name used elsewhere, and run a quick search to confirm no other
instances use the incorrect casing.
| // StoreAPIKey stores an API key in the in-memory cache | ||
| func (cs *ConfigStore) StoreAPIKey(apiKey *models.APIKey) error { | ||
| if apiKey == nil { | ||
| return fmt.Errorf("API key cannot be nil") | ||
| } | ||
|
|
||
| cs.mu.Lock() | ||
| defer cs.mu.Unlock() | ||
|
|
||
| // Check if API key already exists | ||
| if _, exists := cs.apiKeys[apiKey.APIKey]; exists { | ||
| return ErrConflict | ||
| } | ||
|
|
||
| // Store by API key value | ||
| cs.apiKeys[apiKey.APIKey] = apiKey | ||
|
|
||
| // Store by API name:version | ||
| cacheKey := cs.getAPIKeyCacheKey(apiKey.APIName, apiKey.APIVersion) | ||
| cs.apiKeysByAPI[cacheKey] = append(cs.apiKeysByAPI[cacheKey], apiKey) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Reject empty apiKey.APIKey (and ideally name/version) to avoid poisoning the cache.
Right now StoreAPIKey only checks apiKey != nil, so APIKey: "" becomes a valid map key.
func (cs *ConfigStore) StoreAPIKey(apiKey *models.APIKey) error {
if apiKey == nil {
return fmt.Errorf("API key cannot be nil")
}
+ if strings.TrimSpace(apiKey.APIKey) == "" {
+ return fmt.Errorf("API key value cannot be empty")
+ }
+ if strings.TrimSpace(apiKey.APIName) == "" || strings.TrimSpace(apiKey.APIVersion) == "" {
+ return fmt.Errorf("API name/version cannot be empty")
+ }
@@
}🤖 Prompt for AI Agents
In gateway/gateway-controller/pkg/storage/memory.go around lines 395 to 417, the
method currently only checks apiKey != nil and allows an empty apiKey.APIKey
(and potentially empty APIName/APIVersion) which can poison the maps; add
validation after the nil check to reject empty or whitespace-only apiKey.APIKey
(and ideally APIName and APIVersion) and return a descriptive error (e.g.,
fmt.Errorf("API key cannot be empty") or a shared ErrInvalid) before acquiring
the lock so empty strings are never used as map keys; ensure you trim input
whitespace, perform the check prior to computing cacheKey, and keep existing
conflict logic unchanged.
| // OnRequest performs API Key Authentication | ||
| func (p *APIKeyPolicy) OnRequest(ctx *policy.RequestContext, params map[string]interface{}) policy.RequestAction { | ||
| // Get configuration parameters | ||
| keyName, ok := params["key"].(string) | ||
| if !ok || keyName == "" { | ||
| return p.handleAuthFailure(ctx, "missing or invalid 'key' configuration") | ||
| } | ||
| location, ok := params["in"].(string) | ||
| if !ok || location == "" { | ||
| return p.handleAuthFailure(ctx, "missing or invalid 'in' configuration") | ||
| } | ||
|
|
||
| var valuePrefix string | ||
| if valuePrefixRaw, ok := params["value-prefix"]; ok { | ||
| if vp, ok := valuePrefixRaw.(string); ok { | ||
| valuePrefix = vp | ||
| } | ||
| } | ||
|
|
||
| // Extract API key based on location | ||
| var providedKey string | ||
|
|
||
| if location == "header" { | ||
| // Check header (case-insensitive) | ||
| if headerValues := ctx.Headers.Get(strings.ToLower(keyName)); len(headerValues) > 0 { | ||
| providedKey = headerValues[0] | ||
| } | ||
| } else if location == "query" { | ||
| // Extract query parameters from the full path | ||
| providedKey = extractQueryParam(ctx.Path, keyName) | ||
| } | ||
|
|
||
| // If no API key provided | ||
| if providedKey == "" { | ||
| return p.handleAuthFailure(ctx, "missing API key") | ||
| } | ||
|
|
||
| // Strip prefix if configured | ||
| if valuePrefix != "" { | ||
| providedKey = stripPrefix(providedKey, valuePrefix) | ||
| // If after stripping prefix, the key is empty, treat as missing | ||
| if providedKey == "" { | ||
| return p.handleAuthFailure(ctx, "missing API key") | ||
| } | ||
| } | ||
|
|
||
| apiName := ctx.APIName | ||
| apiVersion := ctx.APIVersion | ||
|
|
||
| if apiName == "" || apiVersion == "" { | ||
| return p.handleAuthFailure(ctx, "API name or version not found") | ||
| } | ||
|
|
||
| // API key was provided - validate it using external validation | ||
| isValid := p.validateAPIKey(apiName, apiVersion, providedKey) | ||
| if !isValid { | ||
| return p.handleAuthFailure(ctx, "invalid API key") | ||
| } | ||
|
|
||
| // Authentication successful | ||
| return p.handleAuthSuccess(ctx) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*context*.go" -o -name "*policy*.go" | grep -v vendor | grep -v node_modules | head -20Repository: wso2/api-platform
Length of output: 740
Header lookup will fail due to case mismatch.
Headers are stored in canonical form (e.g., "X-Api-Key", "Content-Type") from Go's http.Header, but ctx.Headers.Get(strings.ToLower(keyName)) performs a case-sensitive lookup for lowercase keys. This will always fail. Either normalize the header key to canonical form (using Go's http.CanonicalHeaderKey), implement case-insensitive lookup in the Headers.Get() method, or accept the key name in canonical form from configuration.
🤖 Prompt for AI Agents
In gateway/policies/api-key-auth/v1.0.0/apikey.go around lines 86 to 147, the
header lookup uses strings.ToLower(keyName) which doesn't match Go's canonical
header keys and will fail; replace the lowercase lookup with a canonicalized
header key (use http.CanonicalHeaderKey(keyName)) before calling
ctx.Headers.Get, or alternatively perform a case-insensitive search over header
names and pick the first match; ensure the chosen approach compiles by importing
the net/http package if needed.
| // handleAuthFailure handles authentication failure | ||
| func (p *APIKeyPolicy) handleAuthFailure(ctx *policy.RequestContext, reason string) policy.RequestAction { | ||
| // Set metadata indicating failed authentication | ||
| ctx.Metadata[MetadataKeyAuthSuccess] = false | ||
| ctx.Metadata[MetadataKeyAuthMethod] = "api-key" | ||
|
|
||
| // Return 401 Unauthorized response | ||
| headers := map[string]string{ | ||
| "content-type": "application/json", | ||
| } | ||
|
|
||
| body := fmt.Sprintf(`{"error": "Unauthorized", "message": "Valid API key required - %s"}`, reason) | ||
|
|
||
| return policy.ImmediateResponse{ | ||
| StatusCode: 401, | ||
| Headers: headers, | ||
| Body: []byte(body), | ||
| } | ||
| } |
There was a problem hiding this comment.
Don’t build JSON via fmt.Sprintf (can produce invalid JSON); marshal a struct instead.
func (p *APIKeyPolicy) handleAuthFailure(ctx *policy.RequestContext, reason string) policy.RequestAction {
@@
- body := fmt.Sprintf(`{"error": "Unauthorized", "message": "Valid API key required - %s"}`, reason)
+ payload, _ := json.Marshal(map[string]string{
+ "error": "Unauthorized",
+ "message": "Valid API key required - " + reason,
+ })
@@
return policy.ImmediateResponse{
StatusCode: 401,
Headers: headers,
- Body: []byte(body),
+ Body: payload,
}
}🤖 Prompt for AI Agents
In gateway/policies/api-key-auth/v1.0.0/apikey.go around lines 164 to 182, the
handler currently builds the JSON response with fmt.Sprintf which can produce
invalid JSON; replace that with marshaling a small struct (e.g., type resp
struct { Error string `json:"error"`; Message string `json:"message"` }) and
call json.Marshal to produce Body bytes, set the same content-type header, and
return the ImmediateResponse; ensure you handle json.Marshal errors (log or
fallback to a safe hard-coded JSON like {"error":"Unauthorized","message":"Valid
API key required"}), preserving the 401 status and metadata assignments.
1 participant
Purpose
APIs and API resources need to be secured using API keys. This PR adds the functionality to generate and validate API keys for an API from the gateway controller using the API key policy.
Related to: #355
Approach
This pull request introduces API Key Authentication support to the gateway, including a new policy, OpenAPI documentation, endpoint implementations, and supporting infrastructure. The changes enable secure API access using pre-shared API keys, provide mechanisms for key generation and validation, and ensure proper key lifecycle management.
API Key Authentication Policy and Documentation:
APIKeyAuthenticationpolicy (v1.0.0) with configurable parameters for extracting and validating API keys from headers or query parameters, including optional prefix stripping and context enrichment. Documentation and configuration schema are provided in both the policy YAML and user documentation. [1] [2]API Key Generation and Lifecycle Management:
/apis/{name}/{version}/api-keyto generate unique, random API keys for APIs, with detailed response schema and error handling. The endpoint saves keys in both persistent storage and the in-memory config store, with rollback on failure. [1] [2]Internal API for Key Validation:
gateway-controller-internal-api.yaml) and code generation config for internal REST endpoints, notably for API key validation. This includes request/response schemas and error models, and the internal server is registered and initialized in the controller. [1] [2] [3] [4] [5] [6]Supporting Codebase Updates:
These changes collectively enable robust API key authentication and lifecycle management for APIs managed by the gateway.
Summary by CodeRabbit
New Features
✏️ Tip: You can customize this high-level summary in your review settings.