Implement API key hashing #644
Conversation
WalkthroughAdds configurable API key hashing (SHA256, bcrypt, Argon2id) with validation and defaults; propagates hashing config into API server and APIKeyService; converts storage to per-API, ID-keyed maps; updates interfaces and revocation/lookup call sites; and adds extensive hashing and storage tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIServer
participant APIKeyService
participant HashingConfig
participant Storage
Client->>APIServer: Create API Key request
APIServer->>APIKeyService: GenerateKey(request)
APIKeyService->>APIKeyService: generate plain key + id
APIKeyService->>HashingConfig: GetHashingConfig()
APIKeyService->>APIKeyService: hashAPIKey(plain, algorithm)
APIKeyService->>Storage: Save(hashedKey, id, apiId)
Storage-->>APIKeyService: OK
APIKeyService-->>APIServer: return PlainAPIKey (client-visible)
APIServer-->>Client: Response with plain key
sequenceDiagram
participant Client
participant APIServer
participant APIKeyService
participant HashingConfig
participant Storage
Client->>APIServer: Request using API Key (value)
APIServer->>APIKeyService: ValidateAPIKey(providedKey)
APIKeyService->>APIKeyService: parseAPIKey(providedKey) -> (id, plain)
APIKeyService->>Storage: GetAPIKeyByID(id)
Storage-->>APIKeyService: storedHashedKey
APIKeyService->>HashingConfig: GetHashingConfig()
APIKeyService->>APIKeyService: compareAPIKeys(plain, storedHashedKey)
alt match
APIKeyService-->>APIServer: Valid
else mismatch
APIKeyService-->>APIServer: Invalid
end
APIServer-->>Client: Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @gateway/gateway-controller/pkg/config/config.go:
- Around line 1105-1106: The function comment is incorrect: it says
validateControlPlaneConfig but the function is validateAPIKeyHashingConfig;
update the comment above the function to accurately describe
validateAPIKeyHashingConfig (e.g., "// validateAPIKeyHashingConfig validates the
API key hashing configuration") so the comment name and description match the
function validateAPIKeyHashingConfig.
- Around line 1118-1134: Validation currently checks
c.GatewayController.APIKeyHashing.Algorithm case-insensitively but leaves the
original casing, causing downstream exact matches (e.g., in hashAPIKey) to fail;
after the loop confirms isValidAlgorithm, normalize the stored value by
assigning c.GatewayController.APIKeyHashing.Algorithm =
strings.ToLower(c.GatewayController.APIKeyHashing.Algorithm) so later code using
exact string comparisons against constants.HashingAlgorithm* will match.
In @gateway/gateway-controller/pkg/storage/apikey_store.go:
- Around line 219-222: The update path for api keys is inconsistent: when
existingKeyID != "" but existingKeyID != apiKey.ID you must remove the old keyed
entry to avoid stale data; modify the branch that currently does
s.apiKeysByAPI[apiKey.APIId][existingKeyID] = apiKey so that if existingKeyID !=
apiKey.ID you delete the old entries (remove s.apiKeys[existingKeyID] and delete
s.apiKeysByAPI[apiKey.APIId][existingKeyID]) before inserting the new apiKey
under apiKey.ID, then set s.apiKeys[apiKey.ID] = apiKey and
s.apiKeysByAPI[apiKey.APIId][apiKey.ID] = apiKey (preserving behavior when IDs
match).
In @gateway/gateway-controller/pkg/storage/memory.go:
- Around line 484-503: When updating an existing API key (existingKeyID != ""),
ensure the same ID is used in both maps to avoid inconsistency: set apiKey.ID to
existingKeyID (or otherwise use existingKeyID as the canonical key) before
storing into cs.apiKeys and cs.apiKeysByAPI, or if you prefer to adopt a new ID,
remove the old entry from cs.apiKeysByAPI[apiKey.APIId][existingKeyID] and
cs.apiKeys[existingKeyID] first and then insert the apiKey under apiKey.ID into
both cs.apiKeys and cs.apiKeysByAPI[apiKey.APIId]; update or remove any
conflicting entries to keep both maps consistent.
In @gateway/gateway-controller/pkg/utils/api_key.go:
- Around line 1148-1150: The function comment for hashAPIKeyWithSHA256 is
corrupted; replace the malformed text with a clear, concise Go doc comment
describing what the function does and what it returns (e.g.,
"hashAPIKeyWithSHA256 securely hashes an API key using SHA-256 with a random
salt and returns the salted hash to be stored in the database and used by the
policy engine"). Ensure the comment starts with the function name and uses
proper grammar and punctuation so it conforms to Go doc conventions.
- Around line 1380-1388: SetHashingConfig currently writes s.hashingConfig
without synchronization causing a data race when other goroutines read it (e.g.,
in hashAPIKey); change the backing field hashingConfig to an
atomic.Pointer[config.APIKeyHashingConfig] (import sync/atomic), update
SetHashingConfig to call Store, update GetHashingConfig and every place that
reads hashingConfig (notably hashAPIKey) to call Load() and handle nil safely,
and ensure types are converted appropriately when loading/storing so all
concurrent reads/writes are race-free.
In @sdk/gateway/policy/v1alpha/api_key.go:
- Around line 119-123: When an existing API key with the same name is detected,
ensure both maps are consistent: if existingKeyID != apiKey.ID remove the stale
entry from aks.apiKeys (delete aks.apiKeys[existingKeyID]) and replace the key
in aks.apiKeysByAPI[apiId] so that both maps store the record under the new
apiKey.ID (i.e., delete aks.apiKeysByAPI[apiId][existingKeyID] then set
aks.apiKeys[apiKey.ID] = apiKey and aks.apiKeysByAPI[apiId][apiKey.ID] =
apiKey); keep the same update path when IDs match (existingKeyID == apiKey.ID).
🧹 Nitpick comments (6)
gateway/gateway-controller/pkg/models/api_key.go (1)
36-49: LGTM! PlainAPIKey field is correctly configured as transient.The
PlainAPIKeyfield is properly marked withjson:"-"anddb:"-"tags to ensure it's never persisted or serialized. The comment on Line 39 helpfully clarifies thatAPIKeystores the hashed value.However,
PlainAPIKeyis set during key generation (api_key.go:798, 978) and used in response building (api_key.go:837-845) but is not explicitly cleared after the response is returned to the caller. While Go's garbage collector will eventually clean up the object, explicitly clearing the field—e.g.,apiKey.PlainAPIKey = ""after buildAPIKeyResponse—would be a good defensive practice to minimize the window sensitive data remains in memory.gateway/gateway-controller/pkg/apikeyxds/apikey_manager.go (1)
150-156: Consider extractingMaskAPIKeyto avoid duplication.This method duplicates
MaskAPIKeyinapikey_snapshot.go(lines 272-277). Consider extracting to a shared utility to reduce maintenance burden.gateway/gateway-controller/pkg/config/api_key_hashing_test.go (1)
46-51: Consider asserting the defaulted algorithm value.This test expects no error when algorithm is empty, but doesn't verify that
config.GatewayController.APIKeyHashing.Algorithmwas actually defaulted to SHA256 after validation. This would strengthen the test.// After calling validateAPIKeyHashingConfig(), verify: if config.GatewayController.APIKeyHashing.Algorithm != constants.HashingAlgorithmSHA256 { t.Errorf("Expected algorithm to default to SHA256, got: %s", config.GatewayController.APIKeyHashing.Algorithm) }sdk/gateway/policy/v1alpha/api_key_hash_test.go (1)
116-126: Simplify error handling logic.The current pattern checks
err != nilthen separately checkserr != ErrNotFound, which is slightly awkward. Consider restructuring:♻️ Suggested simplification
- if err != nil { - if err != ErrNotFound { - t.Fatalf("Expected ErrNotFound, got: %v", err) - } - } - if valid { - t.Error("Validation should fail with incorrect plain text API key") - } + if err != nil && err != ErrNotFound { + t.Fatalf("Expected ErrNotFound or nil error, got: %v", err) + } + if valid { + t.Error("Validation should fail with incorrect plain text API key") + }sdk/gateway/policy/v1alpha/api_key.go (1)
173-179: ValidateAPIKey mutates status as a side effect.Setting
targetAPIKey.Status = Expiredduring validation is a side effect that could cause issues if the caller expects validation to be read-only. Consider handling expiry status updates in a separate method or returning expiry state without mutating.gateway/gateway-controller/pkg/storage/apikey_store.go (1)
243-375: Significant code duplication with policy package.The hash comparison functions (
compareAPIKeys,compareSHA256Hash,compareBcryptHash,compareArgon2id,decodeBase64) are duplicated verbatim fromsdk/gateway/policy/v1alpha/api_key.go. Consider extracting these to a shared internal package to reduce maintenance burden and ensure consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
gateway/gateway-builder/go.sumis excluded by!**/*.sumgateway/gateway-controller/go.sumis excluded by!**/*.sumgateway/policies/api-key-auth/v0.1.0/go.sumis excluded by!**/*.sumgateway/policy-engine/go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
gateway/configs/config.yamlgateway/gateway-builder/go.modgateway/gateway-controller/cmd/controller/main.gogateway/gateway-controller/go.modgateway/gateway-controller/pkg/api/handlers/handlers.gogateway/gateway-controller/pkg/apikeyxds/apikey_manager.gogateway/gateway-controller/pkg/apikeyxds/apikey_snapshot.gogateway/gateway-controller/pkg/config/api_key_hashing_test.gogateway/gateway-controller/pkg/config/config.gogateway/gateway-controller/pkg/constants/constants.gogateway/gateway-controller/pkg/models/api_key.gogateway/gateway-controller/pkg/storage/apikey_store.gogateway/gateway-controller/pkg/storage/interface.gogateway/gateway-controller/pkg/storage/memory.gogateway/gateway-controller/pkg/storage/sqlite.gogateway/gateway-controller/pkg/utils/api_key.gogateway/gateway-controller/pkg/utils/api_key_hash_test.gogateway/policies/api-key-auth/v0.1.0/go.modgateway/policy-engine/go.modsdk/gateway/policy/v1alpha/api_key.gosdk/gateway/policy/v1alpha/api_key_hash_test.gosdk/go.mod
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-23T10:41:47.584Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 389
File: gateway/gateway-controller/pkg/utils/api_key.go:257-380
Timestamp: 2025-12-23T10:41:47.584Z
Learning: In gateway/gateway-controller/pkg/utils/api_key.go, the RevokeAPIKey method intentionally proceeds with database deletion and xDS policy engine revocation even when the API key is not found in the memory store (apiKey is nil). This is by design to handle partial failure recovery: if a previous revocation attempt removed the key from memory but failed to clean up the database or policy engine, subsequent retries will still perform the cleanup operations to ensure eventual consistency.
Applied to files:
gateway/gateway-controller/pkg/storage/interface.gogateway/policies/api-key-auth/v0.1.0/go.modgateway/gateway-controller/pkg/storage/sqlite.gogateway/gateway-controller/pkg/models/api_key.gogateway/gateway-controller/pkg/apikeyxds/apikey_snapshot.gogateway/gateway-controller/pkg/apikeyxds/apikey_manager.gosdk/gateway/policy/v1alpha/api_key_hash_test.gogateway/gateway-controller/pkg/api/handlers/handlers.gogateway/gateway-controller/pkg/utils/api_key.gogateway/gateway-controller/pkg/storage/apikey_store.gogateway/gateway-controller/pkg/storage/memory.gogateway/gateway-controller/cmd/controller/main.gosdk/gateway/policy/v1alpha/api_key.go
📚 Learning: 2025-12-22T16:00:12.851Z
Learnt from: O-sura
Repo: wso2/api-platform PR: 569
File: gateway/policy-engine/internal/analytics/constants.go:100-101
Timestamp: 2025-12-22T16:00:12.851Z
Learning: In gateway/policy-engine/internal/analytics/constants.go, the constant APITypeKey is intentionally named with "Type" terminology (instead of "Kind") to maintain consistency with the analytics publisher platform, which uses "type" rather than "kind". The constant resolves to "x-wso2-api-kind" but the name APITypeKey is kept for cross-platform naming consistency.
Applied to files:
gateway/policies/api-key-auth/v0.1.0/go.modgateway/gateway-controller/pkg/constants/constants.go
📚 Learning: 2025-12-19T06:57:38.504Z
Learnt from: tharindu1st
Repo: wso2/api-platform PR: 514
File: gateway/gateway-controller/config/config.yaml:0-0
Timestamp: 2025-12-19T06:57:38.504Z
Learning: In gateway/gateway-controller/config/config.yaml, the default admin user with plaintext password "admin" is intentionally provided for testing purposes only and must be changed to secure hashed passwords for production deployments.
Applied to files:
gateway/configs/config.yaml
📚 Learning: 2025-12-19T07:14:39.045Z
Learnt from: nimsara66
Repo: wso2/api-platform PR: 521
File: gateway/gateway-controller/pkg/utils/llm_transformer.go:229-243
Timestamp: 2025-12-19T07:14:39.045Z
Learning: In the gateway LLM configuration flow, validation is performed in pkg/config/llm_validator.go before transformation in pkg/utils/llm_transformer.go, ensuring that required fields like upstream.Auth.Header and upstream.Auth.Value are non-nil before the transformer accesses them.
Applied to files:
gateway/gateway-controller/pkg/config/config.go
📚 Learning: 2025-12-23T11:46:56.427Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 389
File: gateway/gateway-controller/api/openapi.yaml:2259-2262
Timestamp: 2025-12-23T11:46:56.427Z
Learning: In gateway/gateway-controller/api/openapi.yaml, the `operations` field in the APIKey schema is intentionally defined as type `string` (not array), storing a stringified representation of API operations.
Applied to files:
gateway/gateway-controller/pkg/utils/api_key.go
🧬 Code graph analysis (8)
gateway/gateway-controller/pkg/storage/interface.go (3)
sdk/gateway/policy/v1alpha/api_key.go (1)
APIKey(19-40)gateway/gateway-controller/pkg/models/api_key.go (1)
APIKey(35-49)gateway/gateway-controller/pkg/api/generated/generated.go (1)
APIKey(365-389)
gateway/gateway-controller/pkg/models/api_key.go (2)
sdk/gateway/policy/v1alpha/api_key.go (2)
APIKey(19-40)APIKeyStatus(43-43)gateway/gateway-controller/pkg/api/generated/generated.go (2)
APIKey(365-389)APIKeyStatus(392-392)
gateway/gateway-controller/pkg/apikeyxds/apikey_manager.go (1)
gateway/gateway-controller/pkg/apikeyxds/apikey_snapshot.go (1)
MaskAPIKey(273-278)
gateway/gateway-controller/pkg/utils/api_key_hash_test.go (3)
gateway/gateway-controller/pkg/utils/api_key.go (1)
APIKeyService(118-123)gateway/gateway-controller/pkg/config/config.go (1)
APIKeyHashingConfig(299-302)gateway/gateway-controller/pkg/constants/constants.go (3)
HashingAlgorithmArgon2ID(136-136)HashingAlgorithmBcrypt(135-135)HashingAlgorithmSHA256(134-134)
sdk/gateway/policy/v1alpha/api_key_hash_test.go (1)
sdk/gateway/policy/v1alpha/api_key.go (4)
NewAPIkeyStore(82-87)APIKey(19-40)Active(53-53)ErrNotFound(61-61)
gateway/gateway-controller/pkg/api/handlers/handlers.go (2)
gateway/gateway-controller/pkg/config/config.go (1)
APIKeyHashingConfig(299-302)gateway/gateway-controller/pkg/utils/api_key.go (1)
NewAPIKeyService(126-134)
gateway/gateway-controller/pkg/config/api_key_hashing_test.go (2)
gateway/gateway-controller/pkg/constants/constants.go (3)
HashingAlgorithmSHA256(134-134)HashingAlgorithmBcrypt(135-135)HashingAlgorithmArgon2ID(136-136)gateway/gateway-controller/pkg/config/config.go (3)
Config(34-40)GatewayController(51-62)APIKeyHashingConfig(299-302)
gateway/gateway-controller/cmd/controller/main.go (2)
gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
NewAPIServer(73-112)gateway/gateway-controller/pkg/config/config.go (1)
GatewayController(51-62)
🪛 Gitleaks (8.30.0)
gateway/gateway-controller/pkg/utils/api_key_hash_test.go
[high] 38-38: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 63-63: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 94-94: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 132-132: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 159-159: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 190-190: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 227-227: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 252-252: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 267-267: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 303-303: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 323-323: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 353-353: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 388-388: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 460-460: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 500-500: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 501-501: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 662-662: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 704-704: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
sdk/gateway/policy/v1alpha/api_key_hash_test.go
[high] 35-35: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 83-83: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 117-117: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 141-141: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (29)
gateway/policies/api-key-auth/v0.1.0/go.mod (2)
3-3: Verify Go 1.25.1 minimum platform requirements are compatible with your deployment targets.Go 1.25 requires macOS 12 Monterey or later, with support for previous versions discontinued. Ensure all deployment environments meet this requirement. Additionally, GOMAXPROCS behavior changes in Go 1.25, where on Linux the runtime now considers the CPU bandwidth limit of cgroups; if lower than available CPUs, it defaults to the lower limit. This could affect container scheduling and resource allocation expectations in your gateway setup.
5-5: Verify that all cryptographic operations use only declared dependencies.The PR objectives mention adding API key hashing with algorithms (SHA-256, bcrypt, argon2id) implemented via
golang.org/x/crypto. If this policy module directly uses cryptographic functions for hash validation or operations, ensure the necessary crypto dependencies are explicitly declared inrequire. Relying solely on transitive dependencies from the SDK may cause dependency resolution issues if the SDK changes its dependency tree.gateway/gateway-controller/pkg/utils/api_key_hash_test.go (2)
1-774: Excellent test coverage for API key hashing.The test suite comprehensively covers multiple hashing algorithms (Argon2id, bcrypt, SHA256), configuration switching, mixed format validation, and migration scenarios.
Note: Static analysis flagged the test API keys as potential secrets. These warnings are false positives since these are test fixtures with the "apip_test" prefix and are not actual production keys.
259-293: No action needed—SHA256 implementation correctly uses random salt as expected.The implementation at
gateway/gateway-controller/pkg/utils/api_key.go:1151-1175confirms thathashAPIKeyWithSHA256does generate a random salt viarand.Read()on line 1157 and incorporates it into the hash output format ($sha256$<salt_hex>$<hash_hex>). The test expectation thathash1 != hash2for the same input is correct, and the test comment accurately describes this behavior.gateway/gateway-builder/go.mod (1)
11-14: Verify the golang.org/x/crypto version.Ensure that version v0.46.0 is valid and has no known security vulnerabilities. This is the same verification requested for other modules in this PR.
gateway/gateway-controller/go.mod (1)
23-23: Appropriate direct dependency addition.Adding
golang.org/x/cryptoas a direct dependency is correct since the gateway-controller now directly implements API key hashing functionality using cryptographic algorithms.Verify that version v0.46.0 is valid and secure (same verification as requested for other modules).
sdk/go.mod (1)
11-11: Appropriate direct dependency addition.Adding
golang.org/x/cryptoas a direct dependency aligns with the SDK's use of cryptographic operations for API key hashing.Verify that version v0.46.0 is valid and secure (same verification as requested for other modules).
gateway/policy-engine/go.mod (1)
46-46: golang.org/x/crypto v0.46.0 is valid and free from known vulnerabilities.Version v0.46.0 exists on the Go module proxy and is the latest available release. No known CVEs affect this version; previous security fixes from v0.45.0 are included.
gateway/gateway-controller/pkg/storage/interface.go (1)
154-159: LGTM! Clean interface extension for ID-based lookup.The new
GetAPIKeyByIDmethod follows the existing interface patterns and is well-documented. It complements the existingGetAPIKeyByKeymethod and enables ID-based retrieval, which is essential for hash-based authentication flows.gateway/configs/config.yaml (1)
220-231: LGTM! Well-documented configuration with sensible defaults.The new
api_key_hashingconfiguration block is clearly documented with:
- Reasonable defaults (enabled with SHA-256)
- Clear listing of supported algorithms
- Security recommendations for production (argon2id)
- Warning about plaintext storage when disabled
The choice of SHA-256 as the default provides a good balance between performance and security for most use cases, while allowing upgrades to bcrypt or argon2id for higher security requirements.
gateway/gateway-controller/pkg/storage/sqlite.go (1)
1164-1203: LGTM! Implementation follows established patterns.The
GetAPIKeyByIDimplementation correctly mirrors the structure ofGetAPIKeyByKey(Lines 1204-1242), including:
- Proper handling of the nullable
expires_atfield usingsql.NullTime- Consistent error wrapping with
ErrNotFoundfor missing keys- Selection of all required fields from the
api_keystableThe query uses the indexed
idcolumn (primary key), ensuring efficient lookups.gateway/gateway-controller/pkg/apikeyxds/apikey_snapshot.go (1)
136-147: LGTM! Revocation logic correctly updated for ID-based operations.The signature change from
RevokeAPIKey(apiKeyValue string)toRevokeAPIKey(apiId, apiKeyID, apiKeyValue string)properly supports the new ID-based API key handling.The behavior change on Lines 143-146 (logging a warning and returning
nilinstead of an error when the key is not found) is appropriate and aligns with the eventual consistency model noted in the learnings. This allows revocation to succeed even when the key was previously removed from memory, ensuring cleanup operations can complete.Based on learnings, this design intentionally allows revocation to proceed with database and policy engine cleanup even when the API key is not found in memory, supporting partial failure recovery scenarios.
gateway/gateway-controller/pkg/apikeyxds/apikey_manager.go (1)
71-92: LGTM - Signature and logging updates align with ID-based revocation.The expanded
RevokeAPIKeysignature correctly includesapiKeyIDfor ID-based operations, and the addedapi_idin the success log improves traceability.gateway/gateway-controller/pkg/config/api_key_hashing_test.go (1)
27-108: Good test coverage for API key hashing configuration validation.The table-driven tests cover the key scenarios including disabled states, valid algorithms, invalid algorithms, and case-insensitivity.
gateway/gateway-controller/pkg/constants/constants.go (1)
127-136: LGTM - Well-defined constants for API key handling and hashing.The constants follow existing conventions and provide good cryptographic strength (32 bytes = 256 bits of entropy for the random portion).
sdk/gateway/policy/v1alpha/api_key_hash_test.go (2)
35-35: Static analysis false positives - test fixture API keys.The Gitleaks alerts for "generic-api-key" are false positives. These are intentional test fixtures using fake API keys with the
apip_prefix. Consider adding a.gitleaksignoreentry or inline comment to suppress these warnings in test files.Also applies to: 83-83, 117-117, 141-141
31-197: Good test coverage for hashed API key validation and revocation.The tests properly exercise the Argon2id hashing flow, including validation with correct/incorrect keys, non-existent API handling, and revocation lifecycle.
gateway/gateway-controller/cmd/controller/main.go (1)
303-304: LGTM - Correct wiring of API key hashing configuration.The
APIKeyHashingconfig is correctly passed toNewAPIServer, enabling runtime hashing configuration for API key operations.gateway/gateway-controller/pkg/storage/memory.go (3)
44-46: LGTM - Correct migration to ID-based storage structure.The refactored storage uses API key IDs as primary identifiers with a nested map structure (
configID → keyID → APIKey), which correctly supports the hashing scenario where plain-text keys cannot be used for lookups.Also applies to: 59-59
508-519: LGTM - Clean ID-based retrieval implementation.
GetAPIKeyByIDcorrectly replaces the value-based lookup with ID-based retrieval.
559-583: LGTM - Proper cleanup in RemoveAPIKeyByID.The method correctly removes from both the main map and the per-API map, with appropriate cleanup of empty nested maps.
gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
85-86: LGTM - Correct propagation of hashing configuration.The
apiKeyHashingConfigparameter is properly added toNewAPIServerand passed through toNewAPIKeyService, enabling configurable API key hashing behavior.Also applies to: 101-101
gateway/gateway-controller/pkg/config/config.go (1)
298-303: Secure defaults for API key hashing configuration.Good choice enabling hashing by default with SHA-256. This ensures production deployments have API key protection out of the box.
Also applies to: 496-499
sdk/gateway/policy/v1alpha/api_key.go (2)
283-311: Hash comparison implementation looks correct.The multi-algorithm hash comparison with constant-time comparison for security-sensitive operations is well-implemented. The fallback to plain-text comparison handles migration scenarios appropriately.
Also applies to: 316-404
416-440: API key parsing with backward compatibility.Good approach using
LastIndexfor the separator and gracefully falling back when hex decoding fails, ensuring backward compatibility with existing keys.gateway/gateway-controller/pkg/storage/apikey_store.go (1)
111-146: Revoke method correctly uses hash comparison.The revocation logic properly looks up by ID, verifies the provided key against the stored hash, and cleans up both maps. This aligns with the retrieved learning about proceeding with revocation even when keys are not found in memory for eventual consistency.
gateway/gateway-controller/pkg/utils/api_key.go (3)
136-149: Reasonable security parameters for hashing algorithms.The Argon2id parameters (m=64MiB, t=1, p=4) prioritize memory-hardness which is appropriate for API keys. Bcrypt cost=12 provides good security margin. SHA-256 with 32-byte salt is sufficient for the use case.
716-801: API key generation with hashing flow is well-structured.The flow of generating plain key, hashing for storage, and preserving plain key temporarily in
PlainAPIKeyfor response generation is a clean separation of concerns.
837-847: Good handling of response key formatting.Properly appending hex-encoded ID to the plain key for response, and avoiding exposure of hashed keys when
PlainAPIKeyis unavailable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
gateway/gateway-controller/pkg/config/config.go (1)
1105-1136: Consider normalizing the algorithm value during validation.The validation correctly performs case-insensitive comparison but doesn't normalize the stored value. While the usage in
hashAPIKeyalso usesstrings.ToLower(), normalizing here would eliminate scatteredToLower()calls and prevent potential inconsistencies if new code forgets the case conversion.♻️ Optional: Normalize algorithm to lowercase during validation
// validateAPIKeyHashingConfig validates the API key hashing configuration func (c *Config) validateAPIKeyHashingConfig() error { // If hashing is disabled, skip validation if !c.GatewayController.APIKeyHashing.Enabled { return nil } // If hashing is enabled but no algorithm is provided, default to SHA256 if c.GatewayController.APIKeyHashing.Algorithm == "" { c.GatewayController.APIKeyHashing.Algorithm = constants.HashingAlgorithmSHA256 return nil } + // Normalize algorithm to lowercase + c.GatewayController.APIKeyHashing.Algorithm = strings.ToLower(c.GatewayController.APIKeyHashing.Algorithm) + // If hashing is enabled and algorithm is provided, validate it's one of the supported ones validAlgorithms := []string{ constants.HashingAlgorithmSHA256, constants.HashingAlgorithmBcrypt, constants.HashingAlgorithmArgon2ID, } isValidAlgorithm := false for _, alg := range validAlgorithms { - if strings.ToLower(c.GatewayController.APIKeyHashing.Algorithm) == alg { + if c.GatewayController.APIKeyHashing.Algorithm == alg { isValidAlgorithm = true break } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
gateway/gateway-builder/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
gateway/gateway-builder/go.modgateway/gateway-controller/pkg/config/config.gogateway/gateway-controller/pkg/utils/api_key.go
🚧 Files skipped from review as they are similar to previous changes (1)
- gateway/gateway-builder/go.mod
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/utils/api_key.go:1380-1388
Timestamp: 2026-01-09T06:08:59.754Z
Learning: In gateway/gateway-controller/pkg/utils/api_key.go, the SetHashingConfig and GetHashingConfig methods are test-only utilities used in api_key_hash_test.go to dynamically switch hashing configurations during sequential tests. They are not called in production code paths where the hashing configuration is set once during APIKeyService initialization via NewAPIKeyService.
📚 Learning: 2026-01-09T06:08:59.754Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/utils/api_key.go:1380-1388
Timestamp: 2026-01-09T06:08:59.754Z
Learning: In gateway/gateway-controller/pkg/utils/api_key.go, the SetHashingConfig and GetHashingConfig methods are test-only utilities used in api_key_hash_test.go to dynamically switch hashing configurations during sequential tests. They are not called in production code paths where the hashing configuration is set once during APIKeyService initialization via NewAPIKeyService.
Applied to files:
gateway/gateway-controller/pkg/config/config.gogateway/gateway-controller/pkg/utils/api_key.go
📚 Learning: 2026-01-09T05:58:05.318Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/storage/memory.go:484-503
Timestamp: 2026-01-09T05:58:05.318Z
Learning: In gateway/gateway-controller/pkg/storage/memory.go, during API key rotation via the StoreAPIKey method, the API key ID is never modified. When an existing API key is found by name (existingKeyID != ""), the incoming apiKey.ID is guaranteed to be the same as existingKeyID, so no ID mismatch can occur during updates.
Applied to files:
gateway/gateway-controller/pkg/config/config.gogateway/gateway-controller/pkg/utils/api_key.go
📚 Learning: 2025-12-19T07:14:39.045Z
Learnt from: nimsara66
Repo: wso2/api-platform PR: 521
File: gateway/gateway-controller/pkg/utils/llm_transformer.go:229-243
Timestamp: 2025-12-19T07:14:39.045Z
Learning: In the gateway LLM configuration flow, validation is performed in pkg/config/llm_validator.go before transformation in pkg/utils/llm_transformer.go, ensuring that required fields like upstream.Auth.Header and upstream.Auth.Value are non-nil before the transformer accesses them.
Applied to files:
gateway/gateway-controller/pkg/config/config.go
📚 Learning: 2025-12-18T08:49:15.812Z
Learnt from: RakhithaRR
Repo: wso2/api-platform PR: 504
File: gateway/policies/mcp-auth/v0.1.0/policy-definition.yaml:118-191
Timestamp: 2025-12-18T08:49:15.812Z
Learning: The policy engine in the gateway validates configuration paths (e.g., `${config.policy_configurations.jwtauth_v010.*}`) in a case-insensitive manner, so references using lowercase (jwtauth_v010) will correctly match configuration keys using mixed case (JWTAuth_v010) in the Helm values or config files.
Applied to files:
gateway/gateway-controller/pkg/config/config.go
📚 Learning: 2025-12-23T10:41:47.584Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 389
File: gateway/gateway-controller/pkg/utils/api_key.go:257-380
Timestamp: 2025-12-23T10:41:47.584Z
Learning: In gateway/gateway-controller/pkg/utils/api_key.go, the RevokeAPIKey method intentionally proceeds with database deletion and xDS policy engine revocation even when the API key is not found in the memory store (apiKey is nil). This is by design to handle partial failure recovery: if a previous revocation attempt removed the key from memory but failed to clean up the database or policy engine, subsequent retries will still perform the cleanup operations to ensure eventual consistency.
Applied to files:
gateway/gateway-controller/pkg/utils/api_key.go
📚 Learning: 2026-01-09T05:57:36.635Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/storage/apikey_store.go:219-222
Timestamp: 2026-01-09T05:57:36.635Z
Learning: In gateway/gateway-controller/pkg/storage/apikey_store.go, during API key rotation, the API key ID and name remain constant—only the API key value changes. Therefore, when addToAPIMapping finds an existing key by name, the existingKeyID will always equal the incoming apiKey.ID.
Applied to files:
gateway/gateway-controller/pkg/utils/api_key.go
📚 Learning: 2026-01-09T06:08:35.284Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: sdk/gateway/policy/v1alpha/api_key.go:119-123
Timestamp: 2026-01-09T06:08:35.284Z
Learning: In sdk/gateway/policy/v1alpha/api_key.go, during API key rotation via the StoreAPIKey method, the API key ID is never modified. When an existing API key is found by name (existingKeyID != ""), the incoming apiKey.ID is guaranteed to be the same as existingKeyID, so no ID mismatch can occur during updates.
Applied to files:
gateway/gateway-controller/pkg/utils/api_key.go
📚 Learning: 2025-12-23T11:46:56.427Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 389
File: gateway/gateway-controller/api/openapi.yaml:2259-2262
Timestamp: 2025-12-23T11:46:56.427Z
Learning: In gateway/gateway-controller/api/openapi.yaml, the `operations` field in the APIKey schema is intentionally defined as type `string` (not array), storing a stringified representation of API operations.
Applied to files:
gateway/gateway-controller/pkg/utils/api_key.go
🧬 Code graph analysis (2)
gateway/gateway-controller/pkg/config/config.go (1)
gateway/gateway-controller/pkg/constants/constants.go (3)
HashingAlgorithmSHA256(134-134)HashingAlgorithmBcrypt(135-135)HashingAlgorithmArgon2ID(136-136)
gateway/gateway-controller/pkg/utils/api_key.go (7)
sdk/gateway/policy/v1alpha/api_key.go (1)
APIKey(19-40)gateway/gateway-controller/pkg/models/api_key.go (1)
APIKey(35-49)gateway/gateway-controller/pkg/api/generated/generated.go (2)
APIKey(365-389)ApiKey(242-242)gateway/gateway-controller/pkg/storage/memory.go (1)
ConfigStore(31-46)gateway/gateway-controller/pkg/config/config.go (1)
APIKeyHashingConfig(299-302)gateway/gateway-controller/pkg/apikeyxds/apikey_snapshot.go (1)
MaskAPIKey(273-278)gateway/gateway-controller/pkg/constants/constants.go (6)
APIKeySeparator(131-131)APIKeyLen(130-130)APIKeyPrefix(129-129)HashingAlgorithmSHA256(134-134)HashingAlgorithmBcrypt(135-135)HashingAlgorithmArgon2ID(136-136)
🔇 Additional comments (22)
gateway/gateway-controller/pkg/config/config.go (4)
52-62: LGTM!The
GatewayControllerstruct is cleanly extended with the newAPIKeyHashingfield. The koanf tag follows the existing naming convention.
298-302: LGTM!Clean struct definition with appropriate fields for configurable hashing. The koanf tags follow the project's naming conventions.
496-499: Good security default.Enabling hashing by default with SHA-256 is the right approach - this ensures production deployments are secure unless explicitly disabled.
654-657: LGTM!Validation call properly integrated into the existing validation chain.
gateway/gateway-controller/pkg/utils/api_key.go (18)
21-44: LGTM!Imports are appropriately organized with the new cryptographic dependencies (
crypto/sha256,crypto/subtle,golang.org/x/crypto/argon2,golang.org/x/crypto/bcrypt) required for the hashing functionality.
104-108: LGTM!Clean struct for representing parsed API key components.
117-134: LGTM!Clean extension of
APIKeyServicewith the hashing configuration. The dependency injection pattern is appropriate.
136-149: LGTM!The hashing parameters are appropriately tuned for API keys:
- Argon2id: 64 MiB memory with 4 threads provides good brute-force resistance
- bcrypt: Cost 12 is within the recommended range (10-15)
- SHA-256: 32-byte salt is sufficient for high-entropy API keys
Since API keys are cryptographically random (not user-chosen passwords), these parameters offer a good balance between security and performance.
723-800: LGTM!The key generation flow correctly:
- Generates a cryptographically random plain key
- Hashes it for secure storage
- Stores the hashed value in the persisted
APIKeyfield- Temporarily holds the plain key in
PlainAPIKeyfor returning to the user (not persisted per the model)This ensures the plain key is only visible during generation/rotation.
305-338: LGTM!The revocation flow implements proper security practices:
- Returns success for invalid format (prevents key format enumeration)
- Looks up by ID first, then verifies with hash comparison
- Uses
compareAPIKeysfor timing-safe hash comparisonThis approach avoids leaking information about key existence or validity.
837-848: LGTM!The response building correctly distinguishes between:
- Generation/Rotation: Returns the formatted plain key with hex-encoded ID
- Existing keys: Returns
nilto avoid exposing hashed valuesThis ensures plain keys are only visible during initial creation.
1121-1146: LGTM!The hashing dispatch logic correctly:
- Validates non-empty input
- Respects the
Enabledflag for backward compatibility- Routes to the appropriate algorithm implementation
The fallback to SHA-256 for unrecognized algorithms is acceptable since the configuration validation should catch invalid values at startup.
1148-1174: LGTM!Proper SHA-256 implementation with:
- 32-byte cryptographically random salt
- Standard
$sha256$<salt>$<hash>format for parseability- Hex encoding for safe storage
1176-1190: Note: bcrypt has a 72-byte input limit.The current API key length (69 bytes:
apip_+ 64 hex chars) is within bcrypt's 72-byte limit, but it's close. If the key format changes in the future (longer prefix or more random bytes), bcrypt would silently truncate the input.This is informational for future reference—no action needed now.
1192-1217: LGTM!Well-implemented Argon2id hashing:
- Proper random salt generation
- Parameters embedded in the hash format for forward compatibility
- Using
RawStdEncodingfor compact base64 representation
1219-1248: LGTM!Robust key comparison with:
- Auto-detection of hash algorithm by prefix
- Proper routing to algorithm-specific comparison
- Constant-time fallback for plain text (migration scenario)
This enables seamless migration from unhashed to hashed keys.
1250-1283: LGTM!Correct SHA-256 hash verification:
- Parses the
$sha256$<salt>$<hash>format with validation- Recomputes hash using stored salt
- Uses
subtle.ConstantTimeComparefor timing-safe comparison
1285-1295: LGTM!Correctly delegates to
bcrypt.CompareHashAndPasswordwhich handles timing-safe comparison internally.
1297-1341: LGTM!Comprehensive Argon2id verification:
- Validates format and version compatibility
- Extracts embedded parameters for accurate recomputation
- Uses
subtle.ConstantTimeComparefor timing-safe comparison
1353-1377: LGTM!Clean API key parsing:
- Uses
LastIndexto correctly handle keys that may contain the separator- Provides backward compatibility fallback if hex decoding fails
- Proper validation of separator position
1379-1387: LGTM!Based on learnings, these are test-only utilities for dynamically switching hashing configurations during sequential tests. The lack of synchronization is acceptable given the documented test-only usage.
110-115: No action required. TheXDSManagerinterface'sRevokeAPIKeymethod signature is correctly implemented byAPIKeyStateManager(the only concrete implementation), which includes theapiKeyIDparameter. All call sites pass the required parameters correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
sdk/gateway/policy/v1alpha/api_key.go (2)
155-165: Simplify by removing redundant nil check.The nil check on line 161 is redundant. Since you return early on line 157 when
existsis false,apiKeycannot be nil at line 161 (whenexistsis false, the zero valuenilwould have already caused an early return).♻️ Proposed simplification
apiKey, exists := aks.apiKeys[parsedAPIkey.ID] if !exists { return false, ErrNotFound } -// Find the API key that matches the provided plain text key (by comparing against hashed values) -if apiKey != nil { - if compareAPIKeys(parsedAPIkey.APIKey, apiKey.APIKey) { - targetAPIKey = apiKey - } +// Find the API key that matches the provided plain text key (by comparing against hashed values) +if compareAPIKeys(parsedAPIkey.APIKey, apiKey.APIKey) { + targetAPIKey = apiKey }
226-236: Simplify by removing redundant nil check.Similar to ValidateAPIKey, the nil check on line 232 is redundant since you return early on line 228 when the key doesn't exist.
♻️ Proposed simplification
apiKey, exists := aks.apiKeys[parsedAPIkey.ID] if !exists { return nil } -// Find the API key that matches the provided plain text key -if apiKey != nil { - if compareAPIKeys(parsedAPIkey.APIKey, apiKey.APIKey) { - matchedKey = apiKey - } +// Find the API key that matches the provided plain text key +if compareAPIKeys(parsedAPIkey.APIKey, apiKey.APIKey) { + matchedKey = apiKey }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sdk/gateway/policy/v1alpha/api_key.go
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/utils/api_key.go:1380-1388
Timestamp: 2026-01-09T06:09:06.281Z
Learning: In gateway/gateway-controller/pkg/utils/api_key.go, the SetHashingConfig and GetHashingConfig methods are test-only utilities used in api_key_hash_test.go to dynamically switch hashing configurations during sequential tests. They are not called in production code paths where the hashing configuration is set once during APIKeyService initialization via NewAPIKeyService.
📚 Learning: 2026-01-09T06:08:39.423Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: sdk/gateway/policy/v1alpha/api_key.go:119-123
Timestamp: 2026-01-09T06:08:39.423Z
Learning: In sdk/gateway/policy/v1alpha/api_key.go, during API key rotation via the StoreAPIKey method, the API key ID is never modified. When an existing API key is found by name (existingKeyID != ""), the incoming apiKey.ID is guaranteed to be the same as existingKeyID, so no ID mismatch can occur during updates.
Applied to files:
sdk/gateway/policy/v1alpha/api_key.go
📚 Learning: 2026-01-09T05:58:05.318Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/storage/memory.go:484-503
Timestamp: 2026-01-09T05:58:05.318Z
Learning: In gateway/gateway-controller/pkg/storage/memory.go, during API key rotation via the StoreAPIKey method, the API key ID is never modified. When an existing API key is found by name (existingKeyID != ""), the incoming apiKey.ID is guaranteed to be the same as existingKeyID, so no ID mismatch can occur during updates.
Applied to files:
sdk/gateway/policy/v1alpha/api_key.go
📚 Learning: 2026-01-09T05:57:36.635Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/storage/apikey_store.go:219-222
Timestamp: 2026-01-09T05:57:36.635Z
Learning: In gateway/gateway-controller/pkg/storage/apikey_store.go, during API key rotation, the API key ID and name remain constant—only the API key value changes. Therefore, when addToAPIMapping finds an existing key by name, the existingKeyID will always equal the incoming apiKey.ID.
Applied to files:
sdk/gateway/policy/v1alpha/api_key.go
📚 Learning: 2025-12-23T10:41:47.584Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 389
File: gateway/gateway-controller/pkg/utils/api_key.go:257-380
Timestamp: 2025-12-23T10:41:47.584Z
Learning: In gateway/gateway-controller/pkg/utils/api_key.go, the RevokeAPIKey method intentionally proceeds with database deletion and xDS policy engine revocation even when the API key is not found in the memory store (apiKey is nil). This is by design to handle partial failure recovery: if a previous revocation attempt removed the key from memory but failed to clean up the database or policy engine, subsequent retries will still perform the cleanup operations to ensure eventual consistency.
Applied to files:
sdk/gateway/policy/v1alpha/api_key.go
📚 Learning: 2026-01-09T06:09:06.281Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/utils/api_key.go:1380-1388
Timestamp: 2026-01-09T06:09:06.281Z
Learning: In gateway/gateway-controller/pkg/utils/api_key.go, the SetHashingConfig and GetHashingConfig methods are test-only utilities used in api_key_hash_test.go to dynamically switch hashing configurations during sequential tests. They are not called in production code paths where the hashing configuration is set once during APIKeyService initialization via NewAPIKeyService.
Applied to files:
sdk/gateway/policy/v1alpha/api_key.go
🧬 Code graph analysis (1)
sdk/gateway/policy/v1alpha/api_key.go (3)
gateway/gateway-controller/pkg/models/api_key.go (1)
APIKey(35-49)gateway/gateway-controller/pkg/api/generated/generated.go (2)
APIKey(365-389)Revoked(48-48)gateway/gateway-controller/pkg/utils/api_key.go (1)
ParsedAPIKey(105-108)
🔇 Additional comments (13)
sdk/gateway/policy/v1alpha/api_key.go (13)
3-17: LGTM! Appropriate cryptographic imports.The imports properly support the new hashing functionality with well-established libraries. Good use of
crypto/subtlefor constant-time comparisons.
45-49: LGTM! Clean public API for parsed API keys.The
ParsedAPIKeytype provides a clear contract for representing parsed API key components.
77-78: LGTM! Efficient ID-based storage structure.The transition to nested maps enables O(1) lookups by ID, which is essential for the hash-based validation flow.
Also applies to: 85-85
98-141: LGTM! Correct ID-based storage logic.The method properly handles both insert and update cases, with appropriate conflict detection and consistent updates to both storage maps.
167-212: LGTM! Comprehensive validation logic.The validation correctly checks API key ownership (line 172), status, expiration, and operation access. The ID verification ensures the key belongs to the requested API, which is an important security check.
238-250: LGTM! Correct revocation with idempotent behavior.The method properly handles both successful revocation and the case where the key doesn't exist (idempotent operation). Cleanup is performed on both storage maps.
252-285: LGTM! Clean bulk operations on ID-based storage.Both methods correctly handle the nested map structure. RemoveAPIKeysByAPI properly iterates by ID, and ClearAll reinitializes with the correct map types.
287-316: LGTM! Secure hash comparison pipeline.The function correctly routes to the appropriate hash comparison based on prefixes and uses constant-time comparisons throughout (including the plain text fallback on line 315). This is crucial for preventing timing attacks.
318-351: LGTM! Correct SHA-256 verification with constant-time comparison.The function properly parses the custom SHA-256 format, decodes the salt and hash, recomputes the hash, and uses constant-time comparison for security.
353-363: LGTM! Correct bcrypt verification.The function correctly delegates to the bcrypt library, which handles constant-time comparison internally.
365-409: LGTM! Comprehensive Argon2id verification.The function properly parses the Argon2id format, validates the version, extracts parameters, and performs constant-time comparison. The implementation correctly handles the complexity of the Argon2id algorithm.
411-445: LGTM! Robust parsing with backward compatibility.The
decodeBase64function handles different base64 encodings gracefully. TheparseAPIKeyfunction correctly splits on the last dot (handling keys with embedded dots) and includes a fallback for non-hex-encoded IDs to maintain backward compatibility.
447-457: LGTM! Proper cleanup of nested map structure.The function correctly removes the key by ID and cleans up empty maps to prevent memory accumulation.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @gateway/gateway-controller/pkg/utils/api_key.go:
- Around line 117-134: The hashingConfig field can be nil and will cause a panic
when hashAPIKey reads s.hashingConfig.Enabled; either ensure NewAPIKeyService
never accepts nil by validating hashingConfig and returning an error or
defaulting to a sane config (update NewAPIKeyService to enforce/populate
hashingConfig), or add a nil guard inside hashAPIKey that treats a nil
hashingConfig as disabled (check s.hashingConfig != nil before accessing Enabled
and handle accordingly); reference APIKeyService.hashingConfig, NewAPIKeyService
and hashAPIKey when making the change.
🧹 Nitpick comments (1)
gateway/gateway-controller/pkg/utils/api_key.go (1)
314-338: RedundantmatchedKeyvariable.Both
apiKeyandmatchedKeyare assigned the same value (existingAPIKey) on lines 335-336. Consider consolidating to reduce confusion.♻️ Suggested simplification
- var apiKey *models.APIKey - var matchedKey *models.APIKey + var apiKey *models.APIKey existingAPIKey, err := s.store.GetAPIKeyByID(parsedAPIkey.ID) ... // Find the API key that matches the provided plain text key if existingAPIKey != nil { if s.compareAPIKeys(parsedAPIkey.APIKey, existingAPIKey.APIKey) { apiKey = existingAPIKey - matchedKey = existingAPIKey } }Then use
apiKeyinstead ofmatchedKeyin lines 406-407.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gateway/gateway-controller/pkg/utils/api_key.go
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/utils/api_key.go:1380-1388
Timestamp: 2026-01-09T06:09:06.281Z
Learning: In gateway/gateway-controller/pkg/utils/api_key.go, the SetHashingConfig and GetHashingConfig methods are test-only utilities used in api_key_hash_test.go to dynamically switch hashing configurations during sequential tests. They are not called in production code paths where the hashing configuration is set once during APIKeyService initialization via NewAPIKeyService.
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/storage/memory.go:484-503
Timestamp: 2026-01-09T05:58:05.318Z
Learning: In gateway/gateway-controller/pkg/storage/memory.go, during API key rotation via the StoreAPIKey method, the API key ID is never modified. When an existing API key is found by name (existingKeyID != ""), the incoming apiKey.ID is guaranteed to be the same as existingKeyID, so no ID mismatch can occur during updates.
📚 Learning: 2026-01-09T05:58:05.318Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/storage/memory.go:484-503
Timestamp: 2026-01-09T05:58:05.318Z
Learning: In gateway/gateway-controller/pkg/storage/memory.go, during API key rotation via the StoreAPIKey method, the API key ID is never modified. When an existing API key is found by name (existingKeyID != ""), the incoming apiKey.ID is guaranteed to be the same as existingKeyID, so no ID mismatch can occur during updates.
Applied to files:
gateway/gateway-controller/pkg/utils/api_key.go
📚 Learning: 2026-01-09T06:09:06.281Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/utils/api_key.go:1380-1388
Timestamp: 2026-01-09T06:09:06.281Z
Learning: In gateway/gateway-controller/pkg/utils/api_key.go, the SetHashingConfig and GetHashingConfig methods are test-only utilities used in api_key_hash_test.go to dynamically switch hashing configurations during sequential tests. They are not called in production code paths where the hashing configuration is set once during APIKeyService initialization via NewAPIKeyService.
Applied to files:
gateway/gateway-controller/pkg/utils/api_key.go
📚 Learning: 2025-12-23T10:41:47.584Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 389
File: gateway/gateway-controller/pkg/utils/api_key.go:257-380
Timestamp: 2025-12-23T10:41:47.584Z
Learning: In gateway/gateway-controller/pkg/utils/api_key.go, the RevokeAPIKey method intentionally proceeds with database deletion and xDS policy engine revocation even when the API key is not found in the memory store (apiKey is nil). This is by design to handle partial failure recovery: if a previous revocation attempt removed the key from memory but failed to clean up the database or policy engine, subsequent retries will still perform the cleanup operations to ensure eventual consistency.
Applied to files:
gateway/gateway-controller/pkg/utils/api_key.go
📚 Learning: 2026-01-09T05:57:36.635Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/storage/apikey_store.go:219-222
Timestamp: 2026-01-09T05:57:36.635Z
Learning: In gateway/gateway-controller/pkg/storage/apikey_store.go, during API key rotation, the API key ID and name remain constant—only the API key value changes. Therefore, when addToAPIMapping finds an existing key by name, the existingKeyID will always equal the incoming apiKey.ID.
Applied to files:
gateway/gateway-controller/pkg/utils/api_key.go
📚 Learning: 2026-01-09T06:08:39.423Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: sdk/gateway/policy/v1alpha/api_key.go:119-123
Timestamp: 2026-01-09T06:08:39.423Z
Learning: In sdk/gateway/policy/v1alpha/api_key.go, during API key rotation via the StoreAPIKey method, the API key ID is never modified. When an existing API key is found by name (existingKeyID != ""), the incoming apiKey.ID is guaranteed to be the same as existingKeyID, so no ID mismatch can occur during updates.
Applied to files:
gateway/gateway-controller/pkg/utils/api_key.go
📚 Learning: 2025-12-22T16:00:12.851Z
Learnt from: O-sura
Repo: wso2/api-platform PR: 569
File: gateway/policy-engine/internal/analytics/constants.go:100-101
Timestamp: 2025-12-22T16:00:12.851Z
Learning: In gateway/policy-engine/internal/analytics/constants.go, the constant APITypeKey is intentionally named with "Type" terminology (instead of "Kind") to maintain consistency with the analytics publisher platform, which uses "type" rather than "kind". The constant resolves to "x-wso2-api-kind" but the name APITypeKey is kept for cross-platform naming consistency.
Applied to files:
gateway/gateway-controller/pkg/utils/api_key.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/utils/api_key.go
📚 Learning: 2025-12-23T11:46:56.427Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 389
File: gateway/gateway-controller/api/openapi.yaml:2259-2262
Timestamp: 2025-12-23T11:46:56.427Z
Learning: In gateway/gateway-controller/api/openapi.yaml, the `operations` field in the APIKey schema is intentionally defined as type `string` (not array), storing a stringified representation of API operations.
Applied to files:
gateway/gateway-controller/pkg/utils/api_key.go
🧬 Code graph analysis (1)
gateway/gateway-controller/pkg/utils/api_key.go (4)
gateway/gateway-controller/pkg/models/api_key.go (1)
APIKey(35-49)gateway/gateway-controller/pkg/config/config.go (1)
APIKeyHashingConfig(299-302)gateway/gateway-controller/pkg/apikeyxds/apikey_snapshot.go (1)
MaskAPIKey(273-278)gateway/gateway-controller/pkg/constants/constants.go (6)
APIKeySeparator(131-131)APIKeyLen(130-130)APIKeyPrefix(129-129)HashingAlgorithmSHA256(134-134)HashingAlgorithmBcrypt(135-135)HashingAlgorithmArgon2ID(136-136)
🔇 Additional comments (15)
gateway/gateway-controller/pkg/utils/api_key.go (15)
104-108: LGTM!The
ParsedAPIKeytype is well-defined and cleanly separates the API key from its ID component.
723-732: LGTM!The generation flow correctly separates plain and hashed keys: the hashed value is stored in
APIKeyfor persistence, whilePlainAPIKeyis only kept temporarily for the response and is never persisted (as indicated by thejson:"-" db:"-"tags in the model).Also applies to: 781-800
837-848: LGTM!The response correctly returns the formatted plain API key only when
PlainAPIKeyis available (during generation/rotation), and returnsnilotherwise. This prevents accidental exposure of hashed keys in API responses.
1121-1146: LGTM!The hash function dispatches correctly based on algorithm, with a sensible SHA256 default for unrecognized algorithms.
1148-1174: LGTM!The SHA256 implementation correctly uses random salts and a well-defined storage format. While SHA256 isn't ideal for low-entropy passwords, it's acceptable for high-entropy API keys (32 random bytes provide 256 bits of entropy), making brute-force attacks infeasible.
1176-1190: Note: bcrypt has a 72-byte input limit.The current API key format (
apip_+ 64 hex chars = 69 bytes) is within bcrypt's 72-byte limit. However, if the key format ever changes to be longer, bcrypt will silently truncate the input, weakening security. Consider documenting this constraint or adding a length check.
1192-1217: LGTM!The Argon2id implementation correctly follows the PHC string format and uses proper parameter encoding.
1219-1248: LGTM!The comparison function correctly auto-detects hash types and provides a secure fallback using constant-time comparison for plain text keys during migration scenarios.
1250-1283: LGTM!The SHA256 comparison correctly parses the format, recomputes the hash with the stored salt, and uses constant-time comparison.
1297-1341: LGTM!The Argon2id comparison correctly parses stored parameters (supporting different configurations), validates the version, and uses constant-time comparison. The flexibility to use stored parameters rather than hardcoded values enables backward compatibility if parameters change.
1353-1377: LGTM!The parsing correctly uses
LastIndexto handle potential dots in the API key portion, and provides backward compatibility for non-hex-encoded IDs.
1379-1387: LGTM!Based on learnings, these methods are test-only utilities used to switch hashing configurations during sequential tests. They're not called in production code paths where the configuration is set once during initialization.
1094-1101: LGTM!The key generation uses cryptographically secure random bytes with sufficient entropy (256 bits).
865-981: LGTM!The rotation correctly generates a new hashed key while preserving the existing key's identity (ID, name). The
PlainAPIKeyis temporarily stored for the response, consistent with the generation flow.
136-149: > Likely an incorrect or invalid review comment.
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/pkg/storage/memory.go (1)
555-574: Potential nil map panic when accessing nested map inRemoveAPIKeyByID.Line 560 accesses
cs.apiKeysByAPI[apiId][id]without first verifying thatcs.apiKeysByAPI[apiId]exists. IfapiIdis not in the map, Go returnsnilfor the inner map, and then accessingnil[id]will return the zero value (not panic), but the subsequent operations on lines 566-571 assume the outer map exists.🐛 Proposed fix
func (cs *ConfigStore) RemoveAPIKeyByID(apiId, id string) error { cs.mu.Lock() defer cs.mu.Unlock() + apiKeys, apiIdExists := cs.apiKeysByAPI[apiId] + if !apiIdExists { + return ErrNotFound + } + _, exists := apiKeys[id] - _, exists := cs.apiKeysByAPI[apiId][id] if !exists { return ErrNotFound } // Remove from apiKeysByAPI map - apiKeys, _ := cs.apiKeysByAPI[apiId] delete(apiKeys, id) // Clean up empty maps if len(cs.apiKeysByAPI[apiId]) == 0 { delete(cs.apiKeysByAPI, apiId) } return nil }
🤖 Fix all issues with AI agents
In @gateway/gateway-controller/pkg/utils/api_key.go:
- Around line 136-149: The Argon2id parameter set is inconsistent with RFC 9106
for 64 MiB memory: update argon2Time from 1 to 3 when using argon2Memory=64*1024
and argon2Threads=4 (p=4) so the iteration count matches the lower-memory
profile; alternatively, if you must keep argon2Time=1, reduce argon2Memory to
~47*1024 KiB per OWASP guidance. Leave argon2KeyLen and argon2SaltLen as-is.
🧹 Nitpick comments (4)
gateway/gateway-controller/pkg/apikeyxds/apikey_manager.go (1)
135-141:MaskAPIKeyis duplicated.This method is identical to
MaskAPIKeyinapikey_snapshot.go(lines 272-277). Consider extracting it to a shared utility to avoid duplication.gateway/gateway-controller/pkg/storage/memory.go (1)
504-515: Potential nil map panic when accessing nested map.If
apiIddoes not exist incs.apiKeysByAPI, accessingcs.apiKeysByAPI[apiId][id]will return the zero value (nil) for the inner map lookup, which is safe in Go. However, for consistency with other methods and clarity, consider checkingapiIdexistence first.♻️ Suggested improvement for clarity
func (cs *ConfigStore) GetAPIKeyByID(apiId, id string) (*models.APIKey, error) { cs.mu.RLock() defer cs.mu.RUnlock() + apiKeys, exists := cs.apiKeysByAPI[apiId] + if !exists { + return nil, ErrNotFound + } + apiKey, exists := apiKeys[id] - apiKey, exists := cs.apiKeysByAPI[apiId][id] if !exists { return nil, ErrNotFound } return apiKey, nil }sdk/gateway/policy/v1alpha/api_key.go (2)
141-142: Consider usingRLockinstead ofLockfor read-heavy validation.
ValidateAPIKeyis a read operation that doesn't modify state (except potentially updatingStatustoExpiredon line 179). UsingLockinstead ofRLockmay cause unnecessary contention. If status updates are needed, consider separating the read and write operations.
167-170: Redundant API ID verification.The API ID check on lines 167-170 is redundant since the lookup on line 151 already retrieves the key from
aks.apiKeysByAPI[apiId], guaranteeing thattargetAPIKey.APIId == apiId.♻️ Suggested removal
if targetAPIKey == nil { return false, ErrNotFound } - // Check if the API key belongs to the specified API - if targetAPIKey.APIId != apiId { - return false, nil - } - // Check if the API key is active if targetAPIKey.Status != Active {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
gateway/gateway-controller/cmd/controller/main.gogateway/gateway-controller/pkg/apikeyxds/apikey_manager.gogateway/gateway-controller/pkg/storage/apikey_store.gogateway/gateway-controller/pkg/storage/memory.gogateway/gateway-controller/pkg/utils/api_key.gosdk/gateway/policy/v1alpha/api_key.go
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/utils/api_key.go:1380-1388
Timestamp: 2026-01-09T06:09:06.281Z
Learning: In gateway/gateway-controller/pkg/utils/api_key.go, the SetHashingConfig and GetHashingConfig methods are test-only utilities used in api_key_hash_test.go to dynamically switch hashing configurations during sequential tests. They are not called in production code paths where the hashing configuration is set once during APIKeyService initialization via NewAPIKeyService.
📚 Learning: 2026-01-09T06:08:39.423Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: sdk/gateway/policy/v1alpha/api_key.go:119-123
Timestamp: 2026-01-09T06:08:39.423Z
Learning: In sdk/gateway/policy/v1alpha/api_key.go, during API key rotation via the StoreAPIKey method, the API key ID is never modified. When an existing API key is found by name (existingKeyID != ""), the incoming apiKey.ID is guaranteed to be the same as existingKeyID, so no ID mismatch can occur during updates.
Applied to files:
sdk/gateway/policy/v1alpha/api_key.gogateway/gateway-controller/pkg/storage/memory.gogateway/gateway-controller/pkg/apikeyxds/apikey_manager.gogateway/gateway-controller/pkg/utils/api_key.gogateway/gateway-controller/pkg/storage/apikey_store.gogateway/gateway-controller/cmd/controller/main.go
📚 Learning: 2026-01-09T05:58:05.318Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/storage/memory.go:484-503
Timestamp: 2026-01-09T05:58:05.318Z
Learning: In gateway/gateway-controller/pkg/storage/memory.go, during API key rotation via the StoreAPIKey method, the API key ID is never modified. When an existing API key is found by name (existingKeyID != ""), the incoming apiKey.ID is guaranteed to be the same as existingKeyID, so no ID mismatch can occur during updates.
Applied to files:
sdk/gateway/policy/v1alpha/api_key.gogateway/gateway-controller/pkg/apikeyxds/apikey_manager.gogateway/gateway-controller/pkg/utils/api_key.gogateway/gateway-controller/pkg/storage/apikey_store.gogateway/gateway-controller/cmd/controller/main.go
📚 Learning: 2025-12-23T10:41:47.584Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 389
File: gateway/gateway-controller/pkg/utils/api_key.go:257-380
Timestamp: 2025-12-23T10:41:47.584Z
Learning: In gateway/gateway-controller/pkg/utils/api_key.go, the RevokeAPIKey method intentionally proceeds with database deletion and xDS policy engine revocation even when the API key is not found in the memory store (apiKey is nil). This is by design to handle partial failure recovery: if a previous revocation attempt removed the key from memory but failed to clean up the database or policy engine, subsequent retries will still perform the cleanup operations to ensure eventual consistency.
Applied to files:
sdk/gateway/policy/v1alpha/api_key.gogateway/gateway-controller/pkg/storage/memory.gogateway/gateway-controller/pkg/apikeyxds/apikey_manager.gogateway/gateway-controller/pkg/utils/api_key.gogateway/gateway-controller/pkg/storage/apikey_store.gogateway/gateway-controller/cmd/controller/main.go
📚 Learning: 2026-01-09T05:57:36.635Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/storage/apikey_store.go:219-222
Timestamp: 2026-01-09T05:57:36.635Z
Learning: In gateway/gateway-controller/pkg/storage/apikey_store.go, during API key rotation, the API key ID and name remain constant—only the API key value changes. Therefore, when addToAPIMapping finds an existing key by name, the existingKeyID will always equal the incoming apiKey.ID.
Applied to files:
sdk/gateway/policy/v1alpha/api_key.gogateway/gateway-controller/pkg/storage/memory.gogateway/gateway-controller/pkg/apikeyxds/apikey_manager.gogateway/gateway-controller/pkg/utils/api_key.gogateway/gateway-controller/pkg/storage/apikey_store.gogateway/gateway-controller/cmd/controller/main.go
📚 Learning: 2026-01-09T06:09:06.281Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/utils/api_key.go:1380-1388
Timestamp: 2026-01-09T06:09:06.281Z
Learning: In gateway/gateway-controller/pkg/utils/api_key.go, the SetHashingConfig and GetHashingConfig methods are test-only utilities used in api_key_hash_test.go to dynamically switch hashing configurations during sequential tests. They are not called in production code paths where the hashing configuration is set once during APIKeyService initialization via NewAPIKeyService.
Applied to files:
sdk/gateway/policy/v1alpha/api_key.gogateway/gateway-controller/pkg/storage/memory.gogateway/gateway-controller/pkg/utils/api_key.gogateway/gateway-controller/pkg/storage/apikey_store.gogateway/gateway-controller/cmd/controller/main.go
📚 Learning: 2026-01-09T05:57:57.216Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/storage/memory.go:484-503
Timestamp: 2026-01-09T05:57:57.216Z
Learning: In gateway/gateway-controller/pkg/storage/memory.go, ensure that when rotating API keys via StoreAPIKey, the API key ID on the incoming object must equal the existing key's ID if a key with the same name already exists (existingKeyID != ""). Do not modify apiKey.ID during update; add an explicit check or assertion to prevent ID mismatches. This guarantees updates don't change identity and helps maintain consistency.
Applied to files:
gateway/gateway-controller/pkg/storage/memory.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/utils/api_key.go
📚 Learning: 2025-12-23T11:46:56.427Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 389
File: gateway/gateway-controller/api/openapi.yaml:2259-2262
Timestamp: 2025-12-23T11:46:56.427Z
Learning: In gateway/gateway-controller/api/openapi.yaml, the `operations` field in the APIKey schema is intentionally defined as type `string` (not array), storing a stringified representation of API operations.
Applied to files:
gateway/gateway-controller/pkg/utils/api_key.go
🧬 Code graph analysis (3)
sdk/gateway/policy/v1alpha/api_key.go (2)
gateway/gateway-controller/pkg/models/api_key.go (1)
APIKey(35-49)gateway/gateway-controller/pkg/api/generated/generated.go (2)
APIKey(365-389)Revoked(48-48)
gateway/gateway-controller/pkg/apikeyxds/apikey_manager.go (1)
gateway/gateway-controller/pkg/apikeyxds/apikey_snapshot.go (1)
MaskAPIKey(273-278)
gateway/gateway-controller/cmd/controller/main.go (4)
gateway/gateway-controller/pkg/apikeyxds/apikey_snapshot.go (1)
NewAPIKeySnapshotManager(53-67)gateway/gateway-controller/pkg/apikeyxds/apikey_manager.go (1)
NewAPIKeyStateManager(38-44)gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
NewAPIServer(73-112)gateway/gateway-controller/pkg/config/config.go (1)
GatewayController(51-62)
🔇 Additional comments (11)
gateway/gateway-controller/pkg/apikeyxds/apikey_manager.go (1)
71-92: LGTM!The updated
RevokeAPIKeysignature correctly includesapiKeyIDto support ID-based lookups with hash comparison. The addedapi_idin the success log improves traceability.gateway/gateway-controller/pkg/storage/memory.go (1)
44-45: LGTM!The refactored nested map structure (
map[string]map[string]*models.APIKey) correctly supports per-API, ID-based key storage. TheStoreAPIKeylogic properly handles both insert and update scenarios with appropriate map initialization.Also applies to: 57-58, 452-502
gateway/gateway-controller/cmd/controller/main.go (2)
109-110: LGTM!The initialization sequence correctly wires the new API key management components. The
APIKeyHashingconfiguration is properly passed toNewAPIServer, enabling configurable hashing behavior throughout the API key lifecycle.Also applies to: 303-304
172-182: LGTM!Initial API key snapshot generation is correctly conditional on persistent mode and existing keys, ensuring the policy engine receives the complete state on startup.
sdk/gateway/policy/v1alpha/api_key.go (1)
96-137: LGTM!The
StoreAPIKeylogic correctly handles the nested map structure with proper initialization and update semantics.gateway/gateway-controller/pkg/utils/api_key.go (4)
781-800: LGTM!The pattern of storing the hashed key in
APIKeywhile temporarily holding the plain key inPlainAPIKeyis a good security practice. This ensures the plain key is only available during generation/rotation and never persisted.
837-847: LGTM!The response building correctly uses
PlainAPIKeywhen available (during generation/rotation) and returnsnilfor existing keys, preventing exposure of hashed values.
1379-1387: Acknowledged: Test-only utilities.Based on learnings,
SetHashingConfigandGetHashingConfigare test-only utilities for dynamically switching hashing configurations during tests. They are not called in production code paths.
1130-1131: Potential nil pointer dereference ifhashingConfigis nil.If
hashingConfigisnil, accessings.hashingConfig.Enabledwill panic. While the constructor should always receive a valid config, consider adding a nil check for defensive programming.🐛 Proposed fix
func (s *APIKeyService) hashAPIKey(plainAPIKey string) (string, error) { if plainAPIKey == "" { return "", fmt.Errorf("API key cannot be empty") } // If hashing is disabled, return the plain API key + if s.hashingConfig == nil || !s.hashingConfig.Enabled { - if !s.hashingConfig.Enabled { return plainAPIKey, nil }⛔ Skipped due to learnings
Learnt from: thivindu Repo: wso2/api-platform PR: 644 File: gateway/gateway-controller/pkg/utils/api_key.go:1380-1388 Timestamp: 2026-01-09T06:09:06.281Z Learning: In gateway/gateway-controller/pkg/utils/api_key.go, the SetHashingConfig and GetHashingConfig methods are test-only utilities used in api_key_hash_test.go to dynamically switch hashing configurations during sequential tests. They are not called in production code paths where the hashing configuration is set once during APIKeyService initialization via NewAPIKeyService.gateway/gateway-controller/pkg/storage/apikey_store.go (2)
88-123: LGTM!The
Revokemethod correctly implements ID-based lookup followed by hash comparison. The logging provides good traceability for both success and failure cases.
163-198: LGTM!The
addToAPIMappingandremoveFromAPIMappingmethods correctly handle the nested map structure with proper initialization and cleanup of empty maps.
| const ( | ||
| // Argon2id parameters (recommended for production security) | ||
| argon2Time = 1 // Number of iterations | ||
| argon2Memory = 64 * 1024 // Memory usage in KiB (64 MiB) | ||
| argon2Threads = 4 // Number of threads | ||
| argon2KeyLen = 32 // Length of derived key in bytes | ||
| argon2SaltLen = 16 // Length of salt in bytes | ||
|
|
||
| // bcrypt parameters (alternative hashing method) | ||
| bcryptCost = 12 // Cost parameter for bcrypt (recommended: 10-15) | ||
|
|
||
| // SHA-256 parameters | ||
| sha256SaltLen = 32 // Length of salt in bytes for SHA-256 | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OWASP Argon2id recommended parameters 2024 2025
💡 Result:
Short answer — current OWASP guidance (Password Storage Cheat Sheet) and the Argon2 RFC (RFC 9106) recommendations:
-
OWASP (Password Storage Cheat Sheet) — Argon2id recommended parameter sets (pick one):
- m = 47104 KiB (46 MiB), t = 1, p = 1.
- m = 19456 KiB (19 MiB), t = 2, p = 1.
- m = 12288 KiB (12 MiB), t = 3, p = 1.
- m = 9216 KiB (9 MiB), t = 4, p = 1.
- m = 7168 KiB (7 MiB), t = 5, p = 1. [1]
-
RFC 9106 (Argon2 specification / CFRG guidance) — “uniformly safe” example recommendations:
- First recommended (high-memory): Argon2id, m = 2^21 KiB (2 GiB), t = 1, p = 4; salt 128 bits, tag 256 bits.
- Second recommended (lower-memory): Argon2id, m = 2^16 KiB (64 MiB), t = 3, p = 4; salt 128 bits, tag 256 bits.
- RFC also gives a stepwise procedure: pick Argon2id, choose p (recommend p=4), pick maximum affordable memory/time, choose salt ≥128 bits (64 bits minimum if constrained), then tune t. [2]
Notes: OWASP’s cheat sheet is the practical developer guidance (smaller-memory, single-threaded options); RFC 9106 gives stronger/high-memory profiles for environments that can afford them. Choose parameters that make a single hash take an acceptable time on your platform (e.g., ~100–500 ms for interactive logins), store the parameters with each hash, and review them periodically. [1][2]
Sources: OWASP Password Storage Cheat Sheet; RFC 9106 (Argon2). [1][2]
Adjust Argon2id iteration count to match RFC 9106 recommendations for configured memory.
The current parameters use argon2Time=1 (iterations) with argon2Memory=64*1024 KiB (64 MiB) and argon2Threads=4. Per RFC 9106, the recommended lower-memory profile for 64 MiB is t=3 with p=4, not t=1. If retaining t=1, reduce memory to approximately 47 MiB per OWASP Password Storage Cheat Sheet. The current salt length (16 bytes / 128 bits) and key length (32 bytes) are correct.
🤖 Prompt for AI Agents
In @gateway/gateway-controller/pkg/utils/api_key.go around lines 136 - 149, The
Argon2id parameter set is inconsistent with RFC 9106 for 64 MiB memory: update
argon2Time from 1 to 3 when using argon2Memory=64*1024 and argon2Threads=4 (p=4)
so the iteration count matches the lower-memory profile; alternatively, if you
must keep argon2Time=1, reduce argon2Memory to ~47*1024 KiB per OWASP guidance.
Leave argon2KeyLen and argon2SaltLen as-is.
| // compareAPIKeys compares API keys for external use | ||
| // Returns true if the plain API key matches the hash, false otherwise | ||
| // If hashing is disabled, performs plain text comparison | ||
| func compareAPIKeys(providedAPIKey, storedAPIKey string) bool { | ||
| if providedAPIKey == "" || storedAPIKey == "" { | ||
| return false | ||
| } | ||
|
|
||
| // Check if it's an SHA-256 hash (format: $sha256$<salt_hex>$<hash_hex>) | ||
| if strings.HasPrefix(storedAPIKey, "$sha256$") { | ||
| return compareSHA256Hash(providedAPIKey, storedAPIKey) | ||
| } | ||
|
|
||
| // Check if it's a bcrypt hash (starts with $2a$, $2b$, or $2y$) | ||
| if strings.HasPrefix(storedAPIKey, "$2a$") || | ||
| strings.HasPrefix(storedAPIKey, "$2b$") || | ||
| strings.HasPrefix(storedAPIKey, "$2y$") { | ||
| return compareBcryptHash(providedAPIKey, storedAPIKey) | ||
| } | ||
|
|
||
| // Check if it's an Argon2id hash | ||
| if strings.HasPrefix(storedAPIKey, "$argon2id$") { | ||
| err := compareArgon2id(providedAPIKey, storedAPIKey) | ||
| return err == nil | ||
| } | ||
|
|
||
| // If no hash format is detected and hashing is enabled, try plain text comparison as fallback | ||
| // This handles migration scenarios where some keys might still be stored as plain text | ||
| return subtle.ConstantTimeCompare([]byte(providedAPIKey), []byte(storedAPIKey)) == 1 | ||
| } | ||
|
|
||
| // compareSHA256Hash validates an encoded SHA-256 hash and compares it to the provided password. | ||
| // Expected format: $sha256$<salt_hex>$<hash_hex> | ||
| // Returns true if the plain API key matches the hash, false otherwise | ||
| func compareSHA256Hash(apiKey, encoded string) bool { | ||
| if apiKey == "" || encoded == "" { | ||
| return false | ||
| } | ||
|
|
||
| // Parse the hash format: $sha256$<salt_hex>$<hash_hex> | ||
| parts := strings.Split(encoded, "$") | ||
| if len(parts) != 4 || parts[1] != "sha256" { | ||
| return false | ||
| } | ||
|
|
||
| // Decode salt and hash from hex | ||
| salt, err := hex.DecodeString(parts[2]) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| storedHash, err := hex.DecodeString(parts[3]) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| // Compute hash of the provided key with the stored salt | ||
| hasher := sha256.New() | ||
| hasher.Write([]byte(apiKey)) | ||
| hasher.Write(salt) | ||
| computedHash := hasher.Sum(nil) | ||
|
|
||
| // Constant-time comparison | ||
| return subtle.ConstantTimeCompare(computedHash, storedHash) == 1 | ||
| } | ||
|
|
||
| // compareBcryptHash validates an encoded bcrypt hash and compares it to the provided password. | ||
| // Returns true if the plain API key matches the hash, false otherwise | ||
| func compareBcryptHash(apiKey, encoded string) bool { | ||
| if apiKey == "" || encoded == "" { | ||
| return false | ||
| } | ||
|
|
||
| // Compare the provided key with the stored bcrypt hash | ||
| err := bcrypt.CompareHashAndPassword([]byte(encoded), []byte(apiKey)) | ||
| return err == nil | ||
| } | ||
|
|
||
| // compareArgon2id parses an encoded Argon2id hash and compares it to the provided password. | ||
| // Expected format: $argon2id$v=19$m=65536,t=3,p=4$<salt_b64>$<hash_b64> | ||
| func compareArgon2id(apiKey, encoded string) error { | ||
| parts := strings.Split(encoded, "$") | ||
| if len(parts) != 6 || parts[1] != "argon2id" { | ||
| return fmt.Errorf("invalid argon2id hash format") | ||
| } | ||
|
|
||
| // parts[2] -> v=19 | ||
| var version int | ||
| if _, err := fmt.Sscanf(parts[2], "v=%d", &version); err != nil { | ||
| return err | ||
| } | ||
| if version != argon2.Version { | ||
| return fmt.Errorf("unsupported argon2 version: %d", version) | ||
| } | ||
|
|
||
| // parts[3] -> m=65536,t=3,p=4 | ||
| var mem uint32 | ||
| var iters uint32 | ||
| var threads uint8 | ||
| var t, m, p uint32 | ||
| if _, err := fmt.Sscanf(parts[3], "m=%d,t=%d,p=%d", &m, &t, &p); err != nil { | ||
| return err | ||
| } | ||
| mem = m | ||
| iters = t | ||
| threads = uint8(p) | ||
|
|
||
| // decode salt and hash (try RawStd then Std) | ||
| salt, err := decodeBase64(parts[4]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| hash, err := decodeBase64(parts[5]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| derived := argon2.IDKey([]byte(apiKey), salt, iters, mem, threads, uint32(len(hash))) | ||
| if subtle.ConstantTimeCompare(derived, hash) == 1 { | ||
| return nil | ||
| } | ||
| return errors.New("API key mismatch") | ||
| } | ||
|
|
||
| // decodeBase64 decodes a base64 string, trying RawStdEncoding first, then StdEncoding | ||
| func decodeBase64(s string) ([]byte, error) { | ||
| b, err := base64.RawStdEncoding.DecodeString(s) | ||
| if err == nil { | ||
| return b, nil | ||
| } | ||
| // try StdEncoding as a fallback | ||
| return base64.StdEncoding.DecodeString(s) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Significant code duplication across packages.
The hash comparison logic (compareAPIKeys, compareSHA256Hash, compareBcryptHash, compareArgon2id, decodeBase64) is duplicated in three locations:
sdk/gateway/policy/v1alpha/api_key.go(this file)gateway/gateway-controller/pkg/storage/apikey_store.gogateway/gateway-controller/pkg/utils/api_key.go
Consider extracting these functions into a shared crypto/hashing utility package to improve maintainability and reduce the risk of inconsistencies.
1 participant
Purpose
Hash the api keys is to prevent exposing the api keys in an event where the database is compromised.
GitHub Discussion - #365
Fixes: wso2/api-platform/issues#609
Approach
This pull request introduces support for configurable API key hashing in the gateway controller, enhancing security and flexibility for API key management. By default API key hashing is enabled and SHA256 is used as the default hashing algorithm. Furthermore, strong hashing algorithms such as bcrypt and argon2id are supported . The changes add new configuration options for API key hashing, update the in-memory API key store to support hashed keys, and provide validation and tests for the new configuration. Additionally, the API key revocation logic is updated to work with hashed keys, and new hashing algorithms are supported.
API Key Hashing Support and Configuration
api_key_hashingconfiguration block toconfig.yaml, allowing API key hashing to be enabled/disabled and the algorithm (sha256,bcrypt, orargon2id) to be selected. Defaults to SHA-256 and is enabled by default.APIKeyHashingConfigstruct and related fields/methods in the configuration package, including validation logic for supported algorithms and default values. [1] [2] [3] [4] [5]API Key Store and Model Updates
APIKeyStore) to remove direct lookups by plain API key value, and to support hash-based comparisons for revocation and retrieval. [1] [2] [3] [4] [5] [6] [7]APIKeymodel to store the hashed API key and added a temporary field for the plain API key (not persisted).API Key Revocation and Service Changes
Wiring and Dependency Updates
Constants and Dependency Management
golang.org/x/cryptofor cryptographic operations. [1] [2] [3]Note
Disabling API key hashing in production environments is highly discouraged.
Increase the resource allocations for the gateway controller and policy engine containers from the docker compose if strong hashing algorithms such as bcrypt and argon2id are used.
Recommended resource limits -
Summary by CodeRabbit
New Features
Behavior Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.