feat: convert external key manager feature flag to runtime config with enum-based validation pattern#155
Conversation
…h enum-based validation pattern
a64af21 to
9e6a20a
Compare
ShankarSinghC
left a comment
There was a problem hiding this comment.
I see that in most places we are using absolute imports. Could you please review it once and change them to relative imports?
src/crypto/keymanager/config.rs
Outdated
There was a problem hiding this comment.
Do we need a separate file for this ? If not, can we please move this logic to src/config.rs ?
There was a problem hiding this comment.
src/config.rs has global application configuration - server,database,logging, src/crypto/keymanager/config.rs would be better choice for handling domain-specific configuration specific to key manager.
There was a problem hiding this comment.
I get the point about keeping domain-specific config closer to the key manager
That said, I’d still prefer keeping this in a single file for now. The logic is fairly small and having it split across multiple config files feels like extra indirection without much benefit at this stage.
Happy to revisit and split it out later if this grows or becomes more complex.
- Fixed undefined JWE_PRIVATE_KEY and JWE_PUBLIC_KEY constants - replaced with dynamically generated keys - Changed criterion_jwe_jws function signature to return Result<(), Box<dyn std::error::Error>> - Added Ok(()) at the end of criterion_jwe_jws function - Restored the missing AES decryption benchmark group - Added #[allow(clippy::expect_used, clippy::unwrap_in_result, unused_must_use)] at the top level - Added #[allow(clippy::expect_used)] attribute to criterion_jwe_jws function - Added #[allow(clippy::expect_used)] attributes to PEM conversion statements cargo clippy --all-features --all-targets now passes with only a future incompatibility warning from a dependency (num-bigint-dig v0.8.4) which is not our concern.
SanchithHegde
left a comment
There was a problem hiding this comment.
Other than that, looks good to me!
| #[cfg_attr( | ||
| all( | ||
| not(feature = "middleware"), | ||
| not(feature = "external_key_manager"), | ||
| not(feature = "key_custodian") | ||
| ), | ||
| allow(unused_mut) | ||
| )] |
There was a problem hiding this comment.
Why is this required only for v2 routes ?
There was a problem hiding this comment.
these feature flag gates are global not consolidated to v2 .
Placing the mutable pattern after v2 routes was a design choice during the refactor to consolidate feature gate blocks after all route groups were defined.
the earlier code without mut would throw shadowing problem with cargo clippy --all-features
ShankarSinghC
left a comment
There was a problem hiding this comment.
Can you add some test cases
Locker legacy -> https://api-reference.hyperswitch.io/locker-api-reference/cards/add-data-in-locker
General purpose locker -> https://api-reference.hyperswitch.io/locker-api-reference/locker-general-purpose-storage/add-data-in-locker
Utils function to obtain jws + jwe request https://github.com/juspay/hyperswitch-card-vault/blob/main/docs/guides/testing.md
This pull request refactors the configuration and runtime handling of the external key manager, removing the need for compile-time feature flags and instead using runtime configuration to enable, disable, or require mTLS for the external key manager. It introduces a new
KeyManagerModeenum to encapsulate the key manager's operational mode, updates configuration validation accordingly, and ensures all relevant code paths use the new runtime checks. This greatly improves flexibility and maintainability.Key changes include:
External Key Manager Configuration & Runtime Mode Handling
enabledandmtls_enabledfields toExternalKeyManagerConfig, allowing the external key manager and mTLS to be toggled at runtime via configuration instead of compile-time feature flags. Also added runtime validation for required fields.KeyManagerModeenum inkeymanager.rsto represent internal, external (plain), and external (mTLS) modes, with helper methods for checking mode and logic to derive the mode from config.get_dek_managerto select the implementation at runtime based onKeyManagerMode, instead of compile-time features.Configuration & Validation
url,cert,api_client.identity) when the external key manager or mTLS is enabled. [1] [2]Cargo.tomlfeatures to remove theexternal_key_manager_mtlsfeature and ensureexternal_key_manageralways enables the correct dependencies.Application & API Client Integration
ApiClientandTenantAppStateto receive and useKeyManagerModeat runtime, including mTLS identity/cert only if required. [1] [2] [3] [4]/key/transferroute and other external key manager logic are only registered or used when the external key manager is enabled, based on runtime config.Code Cleanup & Generalization
get_dek_managerthroughout the codebase to pass the runtime mode. [1] [2] [3]Testing
All changes verified with:
Diagnostics request :
Diagnostics response:
Saved card call from tenant :
Tartarus logs :

Retrieve call from tenant:
Tartarus logs :

Tests pass successfully with no compilation errors or warnings.
Fixed required check
All clippy warnings have been fixed:
Additional test
Scenario 1: Legacy Cards API - Add Card
Step 1: Create the plaintext request
Step 2: Encrypt the request
Step 3: Send the encrypted request
Step 4: Decrypt the response
Scenario 2: Legacy Cards API - Retrieve Card
Step 1: Create the plaintext request
Step 2: Encrypt the request
Step 3: Send the encrypted request
Step 4: Decrypt the response
Scenario 3: Legacy Cards API - Delete Card
Scenario 4: V2 General Purpose Vault - Add Data
Scenario 5: V2 General Purpose Vault - Retrieve Data
Scenario 6: V2 General Purpose Vault - Delete Data