Add support to pick the server cert by specified alias #2429
Conversation
WalkthroughIntroduces certificate alias selection for TLS/SSL server communications. Adds Changes
Sequence DiagramsequenceDiagram
participant Client
participant SSLEngine
participant AliasBasedKeyManager
participant WrappedKeyManager
Client->>SSLEngine: initiate TLS connection
SSLEngine->>AliasBasedKeyManager: chooseServerAlias(keyType, issuers, engine)
alt preferredKeyAlias configured and available
AliasBasedKeyManager->>AliasBasedKeyManager: verify alias in server aliases
AliasBasedKeyManager-->>SSLEngine: return preferredKeyAlias
else alias not configured or unavailable
AliasBasedKeyManager->>WrappedKeyManager: chooseServerAlias(keyType, issuers, socket)
WrappedKeyManager-->>AliasBasedKeyManager: return default alias
AliasBasedKeyManager-->>SSLEngine: return default alias
end
SSLEngine->>AliasBasedKeyManager: getCertificateChain(alias)
AliasBasedKeyManager->>WrappedKeyManager: getCertificateChain(alias)
WrappedKeyManager-->>AliasBasedKeyManager: X509Certificate[]
AliasBasedKeyManager-->>SSLEngine: X509Certificate[]
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/ServerConnFactoryBuilder.java (1)
129-130: Consider validating the configured alias exists in the keystore.While the alias extraction logic is correct, there's no validation that the configured
ServerCertificateAliasactually exists in the keystore. If users misconfigure the alias, the system will silently fall back to default behavior without any warning, making troubleshooting difficult.Consider adding validation after line 152 (after keyStore is loaded):
kmfactory.init(keyStore, keyPassword.toCharArray()); + if (requiredAlias != null && !keyStore.containsAlias(requiredAlias)) { + log.warn(name + " Configured ServerCertificateAlias '" + requiredAlias + + "' not found in keystore. Available aliases: " + + java.util.Collections.list(keyStore.aliases())); + } if (requiredAlias != null) {modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/AliasBasedKeyManager.java (1)
57-64: Consider adding debug logging when preferred alias is unavailable.The logic correctly validates that the preferred alias is compatible with the requested key type and issuers before returning it. However, when the preferred alias is not in the available aliases list, the method silently falls back to the default selection without logging. This could make it difficult to diagnose configuration issues.
Consider adding debug logging:
public String chooseServerAlias(String keyType, Principal[] issuers, Socket socket) { String[] aliases = keyManager.getServerAliases(keyType, issuers); if (aliases != null && Arrays.asList(aliases).contains(preferredKeyAlias)) { return preferredKeyAlias; } + // Log at debug level to help diagnose why preferred alias wasn't used + if (log.isDebugEnabled()) { + log.debug("Preferred alias '" + preferredKeyAlias + "' not available for keyType=" + keyType + + ", falling back to default selection"); + } return keyManager.chooseServerAlias(keyType, issuers, socket); }Note: You'll need to add a logger field:
private static final Log log = LogFactory.getLog(AliasBasedKeyManager.class);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/AliasBasedKeyManager.java(1 hunks)modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/ServerConnFactoryBuilder.java(2 hunks)
🔇 Additional comments (5)
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/ServerConnFactoryBuilder.java (1)
153-163: LGTM! Clean conditional wrapping logic.The implementation correctly wraps
X509KeyManagerinstances withAliasBasedKeyManagerwhen an alias is configured, while maintaining backward compatibility by falling back to the default behavior when no alias is specified. Theinstanceofcheck ensures type safety.modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/AliasBasedKeyManager.java (4)
37-40: LGTM! Constructor is clean and thread-safe.The constructor properly initializes final fields, ensuring thread safety for concurrent SSL handshakes.
42-55: LGTM! Proper delegation pattern.These methods correctly delegate to the wrapped key manager without modification, which is appropriate since alias preference only needs to affect server alias selection.
66-69: LGTM! Correct reuse of server alias selection logic.Delegating to
chooseServerAliaswith a null socket is appropriate and safe. The preferred alias is selected before the socket parameter would be used, and null is a valid socket value in the X509KeyManager API for the fallback delegation.
71-79: LGTM! Proper delegation for certificate and key access.These methods correctly delegate certificate chain and private key retrieval to the wrapped manager, which is appropriate since alias selection is handled separately.
| /** | ||
| * Custom X509KeyManager that selects a specific alias for client and server certificates | ||
| */ | ||
| public class AliasBasedKeyManager extends X509ExtendedKeyManager { |
There was a problem hiding this comment.
Fix misleading class documentation.
The comment states this class "selects a specific alias for client and server certificates," but the implementation only customizes server alias selection. Client alias methods (chooseClientAlias at line 48) delegate directly to the wrapped key manager without applying the preferred alias logic.
Apply this diff:
/**
- * Custom X509KeyManager that selects a specific alias for client and server certificates
+ * Custom X509KeyManager that selects a specific server certificate alias
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Custom X509KeyManager that selects a specific alias for client and server certificates | |
| */ | |
| public class AliasBasedKeyManager extends X509ExtendedKeyManager { | |
| /** | |
| * Custom X509KeyManager that selects a specific server certificate alias | |
| */ | |
| public class AliasBasedKeyManager extends X509ExtendedKeyManager { |
🤖 Prompt for AI Agents
In
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/AliasBasedKeyManager.java
around lines 30 to 33, the class Javadoc incorrectly claims it "selects a
specific alias for client and server certificates" while the implementation only
applies the preferred-alias logic to server-side selection. Update the
class-level comment to accurately state that this KeyManager customizes server
(SSLServer) alias selection only and that client alias methods delegate to the
wrapped KeyManager (or alternatively implement symmetric client-alias behavior
if you prefer that change), ensuring the Javadoc matches the actual behavior.
2 participants
Purpose
$subject
Fixes wso2/api-manager#4395
Goals
Approach
AliasBasedKeyManager.AliasBasedKeyManager.User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit