Add FIPS support #2426
Add FIPS support #2426
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughCentralizes JCE provider handling and runtime algorithm/keystore selection across modules. Adds helper methods to add providers and to resolve KeyManager/TrustManager algorithms and keystore types based on the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (2)
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: 6
♻️ Duplicate comments (2)
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/certificatevalidation/pathvalidation/CertificatePathValidator.java (1)
125-140: Duplicate implementation—consolidate with CryptoUtil.This
addProvider()implementation duplicates the logic fromCryptoUtil.java(lines 141-155) with only a different exception type. As mentioned in the CryptoUtil review, this method appears in three files and should be extracted to a shared utility to eliminate duplication. Additionally, the same logic concern regarding theisEmptycheck applies here.modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/certificatevalidation/ocsp/OCSPVerifier.java (1)
292-307: Visibility inconsistency and duplicate implementation.This
addProvider()method is markedpublic static, while the identical implementations inCryptoUtil.java(line 141) andCertificatePathValidator.java(line 125) areprivate static. This visibility inconsistency suggests the three implementations were added independently. As noted in previous comments, consolidate all three into a shared utility class with consistent visibility.
🧹 Nitpick comments (5)
modules/securevault/src/main/java/org/apache/synapse/securevault/definition/IdentityKeyStoreInformation.java (3)
34-35: Consider adding JavaDoc for the constants.The constants are properly declared but lack documentation explaining their purpose. Consider adding a brief comment explaining why
PKIXis used when a custom JCE provider is configured.Example:
+ /** Algorithm to use when a custom JCE provider is configured */ private static final String PKIX = "PKIX"; + /** System property name for specifying the JCE provider (e.g., BC, BCFIPS) */ private static final String JCE_PROVIDER = "security.jce.provider";
78-85: Add JavaDoc and consider validating the provider value.The method lacks documentation explaining why
PKIXis selected when a JCE provider is configured. Additionally, the implementation accepts any non-empty string without validating whether it's a supported provider (e.g., "BC" or "BCFIPS" per the PR description).Consider this enhancement:
+ /** + * Determines the KeyManagerFactory algorithm based on the configured JCE provider. + * Returns PKIX when a custom JCE provider is set (e.g., BC, BCFIPS), otherwise + * returns the platform default algorithm. + * + * @return The KeyManagerFactory algorithm to use + */ private static String getManagerType() { String provider = System.getProperty(JCE_PROVIDER); if (StringUtils.isNotEmpty(provider)) { return PKIX; } else { return KeyManagerFactory.getDefaultAlgorithm(); } }Optional: If strict validation is desired, you could verify the provider value:
private static String getManagerType() { String provider = System.getProperty(JCE_PROVIDER); if ("BC".equals(provider) || "BCFIPS".equals(provider)) { return PKIX; } else { return KeyManagerFactory.getDefaultAlgorithm(); } }
78-85: Consider extracting the provider logic to a shared utility class.Based on the AI summary, this
getManagerType()pattern is duplicated across multiple files (TrustKeyStoreInformation, OPAClient, OCSPVerifier, and various SSL builder utilities). Consider centralizing this logic in a utility class to improve maintainability and ensure consistency.Example structure:
// New utility class public final class JCEProviderUtils { private static final String PKIX = "PKIX"; private static final String JCE_PROVIDER = "security.jce.provider"; private JCEProviderUtils() {} /** * Determines the KeyManagerFactory algorithm based on the configured JCE provider. * Returns PKIX when a custom JCE provider is set, otherwise returns the platform default. */ public static String getKeyManagerAlgorithm() { String provider = System.getProperty(JCE_PROVIDER); return StringUtils.isNotEmpty(provider) ? PKIX : KeyManagerFactory.getDefaultAlgorithm(); } // Similar methods for TrustManagerFactory, keystore types, etc. }Then simplify this file:
- private static String getManagerType() { - String provider = System.getProperty(JCE_PROVIDER); - if (StringUtils.isNotEmpty(provider)) { - return PKIX; - } else { - return KeyManagerFactory.getDefaultAlgorithm(); - } - } + // Use: KeyManagerFactory.getInstance(JCEProviderUtils.getKeyManagerAlgorithm())modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/ServerConnFactoryBuilder.java (1)
496-512: Consider consolidating the duplicate logic.Both
getKeyManagerType()andgetTrustManagerType()implement identical logic. Consider extracting a common helper method that accepts the default algorithm as a parameter.Example refactor:
private static String getAlgorithmType(String defaultAlgorithm) { String provider = System.getProperty(JCE_PROVIDER); if (StringUtils.isNotEmpty(provider)) { return PKIX; } else { return defaultAlgorithm; } } private static String getKeyManagerType() { return getAlgorithmType(KeyManagerFactory.getDefaultAlgorithm()); } private static String getTrustManagerType() { return getAlgorithmType(TrustManagerFactory.getDefaultAlgorithm()); }modules/core/src/main/java/org/apache/synapse/util/xpath/EncryptFunction.java (1)
139-146: Keystore type resolution logic is sound.The precedence order (explicit property → FIPS-aware default → legacy default) correctly implements the FIPS feature while maintaining backward compatibility. The use of
StringUtils.isNotEmpty()properly handles null and empty string cases.Consider adding JavaDoc to document the resolution precedence for future maintainers:
+ /** + * Determines the keystore type based on system properties. + * Precedence: 1) primary.key.type property, 2) BCFKS if security.jce.provider is set, 3) JKS default. + * + * @return The resolved keystore type + */ private static String getKeyType() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
modules/commons/src/main/java/org/apache/synapse/commons/crypto/CryptoUtil.java(4 hunks)modules/core/src/main/java/org/apache/synapse/message/store/impl/rabbitmq/RabbitMQStore.java(3 hunks)modules/core/src/main/java/org/apache/synapse/util/xpath/DecryptFunction.java(4 hunks)modules/core/src/main/java/org/apache/synapse/util/xpath/EncryptFunction.java(4 hunks)modules/extensions/src/main/java/org/apache/synapse/mediators/opa/OPAClient.java(4 hunks)modules/securevault/src/main/java/org/apache/synapse/securevault/definition/IdentityKeyStoreInformation.java(4 hunks)modules/securevault/src/main/java/org/apache/synapse/securevault/definition/TrustKeyStoreInformation.java(4 hunks)modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/certificatevalidation/ocsp/OCSPVerifier.java(5 hunks)modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/certificatevalidation/pathvalidation/CertificatePathValidator.java(4 hunks)modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/ClientConnFactoryBuilder.java(7 hunks)modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/ServerConnFactoryBuilder.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (16)
modules/securevault/src/main/java/org/apache/synapse/securevault/definition/IdentityKeyStoreInformation.java (2)
21-21: LGTM!The
StringUtilsimport is appropriately added to support the null/empty check in the newgetManagerType()method.
54-54: LGTM!The change to use
getManagerType()correctly implements the dynamic algorithm selection based on the JCE provider configuration.modules/securevault/src/main/java/org/apache/synapse/securevault/definition/TrustKeyStoreInformation.java (3)
21-21: LGTM!The import is appropriate for the string validation logic introduced in
getManagerType().
31-32: LGTM!Using constants for the algorithm name and system property key improves maintainability and reduces the risk of typos.
46-46: LGTM!The refactoring to use
getManagerType()properly abstracts the algorithm selection logic while maintaining backwards compatibility.modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/ServerConnFactoryBuilder.java (3)
77-78: LGTM!The constants are well-defined and appropriately scoped.
150-150: LGTM!The dynamic algorithm selection correctly delegates to the helper method, aligning with the FIPS support objective.
205-205: LGTM!The dynamic algorithm selection correctly delegates to the helper method, aligning with the FIPS support objective.
modules/core/src/main/java/org/apache/synapse/util/xpath/EncryptFunction.java (3)
18-18: LGTM! Good practice improvements.Adding the
StringUtilsimport and makingcipherInstancesMapexplicitlyfinalare both solid improvements—the latter prevents accidental reassignment and clearly signals the map reference is immutable.Also applies to: 47-47
48-50: LGTM! Well-defined constants.The constants are appropriately scoped and clearly named. The public visibility of
BCFKSaligns with its use as a keystore type identifier.
68-68: Dynamic keystore type selection looks correct.The change from
DEFAULT_KEYSTORE_TYPEtogetKeyType()properly implements FIPS-aware keystore selection for the 4-argument variant while leaving explicit type parameters (5-arg and 6-arg variants) unaffected. Theprimary.key.typeproperty provides an override mechanism.Verify that existing deployments using the 4-argument version are aware of the behavior change: when
security.jce.provideris set, the default keystore type switches from JKS to BCFKS. Users relying on JKS with FIPS can set theprimary.key.typesystem property.modules/core/src/main/java/org/apache/synapse/util/xpath/DecryptFunction.java (3)
48-51: LGTM! Good practice making the map final.The addition of FIPS-related constants is clear. Note that
BCFKSis public while the other constants are private—ensure this visibility is intentional if external classes need to reference this keystore type.
69-69: Dynamic keystore type selection correctly integrated.The call to
getKeyType()ensures that when no explicit keystore type is provided, the system selects BCFKS for FIPS providers or falls back to JKS, maintaining backward compatibility.
140-147: Keystore type resolution logic is sound.The priority order (explicit property → BCFKS for FIPS → JKS default) provides flexibility while ensuring FIPS-compliant keystore types are used when appropriate.
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/ClientConnFactoryBuilder.java (2)
77-78: Constants improve maintainability.The PKIX algorithm constant is appropriate for FIPS-compliant key and trust managers.
612-628: Algorithm selection logic is correct and consistent.The helper methods correctly return PKIX for FIPS providers and fall back to platform defaults otherwise. The symmetric implementation across
getKeyManagerType()andgetTrustManagerType()ensures consistent behavior.
modules/commons/src/main/java/org/apache/synapse/commons/crypto/CryptoUtil.java
Show resolved
Hide resolved
modules/core/src/main/java/org/apache/synapse/message/store/impl/rabbitmq/RabbitMQStore.java
Show resolved
Hide resolved
modules/extensions/src/main/java/org/apache/synapse/mediators/opa/OPAClient.java
Show resolved
Hide resolved
...evault/src/main/java/org/apache/synapse/securevault/definition/TrustKeyStoreInformation.java
Show resolved
Hide resolved
.../nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/ServerConnFactoryBuilder.java
Show resolved
Hide resolved
.../nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/ServerConnFactoryBuilder.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/commons/pom.xml(1 hunks)pom.xml(3 hunks)
🔇 Additional comments (1)
pom.xml (1)
809-809: Dependency updates incomplete and lacks FIPS support justification.The review identifies only the version upgrades (securevault 1.1.2 → 1.1.16, commons-vfs 2.2.0-wso2v22 → 2.2.0-wso2v23, smbj 0.11.3.wso2v1 → 0.11.3.wso2v2) but omits critical downgrades in axis2 (1.6.1-wso2v117 → 1.6.1-wso2v114) and axis2.transport (2.0.0-wso2v80 → 2.0.0-wso2v79). The commit message ("Add jaxen dependency") contains no mention of FIPS support, and no FIPS-related commits exist in the repository history, contradicting the claim that these updates align with PR objectives.
Likely an incorrect or invalid review comment.
This reverts commit 22cc307.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pom.xml (1)
810-814: Address the hardcoded version and security concerns for jaxen.This dependency addition was already reviewed and flagged for having a hardcoded version (inconsistent with project conventions) and potential security vulnerabilities from 2006. Please refer to the past review comment on this same line range for remediation steps.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/transports/core/vfs/pom.xml(0 hunks)pom.xml(4 hunks)
💤 Files with no reviewable changes (1)
- modules/transports/core/vfs/pom.xml
🔇 Additional comments (3)
pom.xml (3)
699-719: LGTM! Good defensive dependency management.The explicit test scope and targeted exclusions to block conflicting Log4j providers are appropriate for preventing dependency conflicts in the test classpath.
1564-1564: Commons-vfs2 patch update is routine and safe.The version bump from 2.2.0-wso2v22 to 2.2.0-wso2v23 is a minor patch release from the WSO2 fork. VFS transport modules (VFSTransportListener and VFSTransportSender) actively use commons-vfs2, and the standard API surface remains unchanged across patch versions. No compatibility concerns identified.
1671-1671: SMBJ patch upgrade from 0.11.3.wso2v1 to 0.11.3.wso2v2 is compatible with existing SMB2 operations.The codebase uses SMBJ through the commons-vfs2 abstraction layer (Smb2FileSystemConfigBuilder), which handles SMB2 encryption configuration. The patch-level version bump in the WSO2 fork indicates bug fixes only, with no documented breaking changes that would affect CIFS/SMB file operations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/maven.yml (1)
29-32: Verify the relevance of this debugging step to the PR objectives.This new step diagnoses logging-related dependency conflicts, but the PR is focused on adding FIPS support with Bouncy Castle providers. There's no apparent connection between logging dependencies and the FIPS feature.
Additionally, running
mvn dependency:tree -Dverboseadds overhead to every PR build.Please confirm:
- Is this debugging step intentionally included for a specific logging issue related to FIPS support?
- Or was this meant for a different PR/debugging session and should be removed or made conditional?
Remove exculsion Add slf4j in the exclusion Add slf4j in the exclusion Add GitHub Actions workflow for Micro Integrator build Update coverage.yml Update test.yml Update test.yml Update test.yml Add debug logging Add debug logging Add debug logging Add debug logging Update secure vault dependcies
kalaiyarasiganeshalingam
force-pushed
the
master
branch
from
10967a4 to
025a6be
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pom.xml(4 hunks)
🔇 Additional comments (5)
pom.xml (5)
815-818: LGTM: Appropriate logging dependency exclusion.The addition of pax-logging-api exclusion is appropriate for preventing logging framework conflicts in the OSGi environment. This complements the existing log4j exclusion.
800-804: Jaxen 2.0.0 is available in Maven Central and has no known CVE vulnerabilities.Jaxen 2.0.0 is available in Maven Central (published December 3, 2022) and is used by 5,312 components. The version shows 0 active CVE issues. The upgrade from the old 1.1.1 version is appropriate for modern Java development.
1671-1671: Verify what changes are included in smbj 0.11.3.wso2v2.Without access to WSO2 release notes or changelog for smbj 0.11.3.wso2v2, the specific changes between wso2v1 and wso2v2 cannot be confirmed. While SMB protocols do use cryptography, there's no documented evidence that this version increment is FIPS-related. Check WSO2's internal release documentation or git history to identify the actual changes in this version.
1564-1564: Verify commons-vfs2 wso2v23 FIPS-related changes through PR context.The update from wso2v22 to wso2v23 is confirmed in pom.xml, but public sources provide no release notes documenting FIPS-specific changes in this version. Since VFS primarily handles file operations and transport rather than cryptography (which relies on the underlying crypto provider), confirm via PR description or commit messages whether this version includes FIPS-relevant fixes and what those are.
809-809: Verify securevault 1.1.16 availability in configured repositories.Version 1.1.16 is not available on Maven Central (latest public version is 1.1.8). This version must be available in one of the configured WSO2 internal repositories (wso2-nexus, wso2.releases, or wso2.snapshots). Confirm that the build environment has access to the required repository and that this version exists before merging. The codebase uses standard SecretResolver and SecretResolverFactory APIs that are unlikely to have breaking changes across minor versions.
There was a problem hiding this comment.
Pull request overview
This PR adds support for FIPS (Federal Information Processing Standards) compliance by enabling the Bouncy Castle FIPS cryptographic provider alongside the standard Bouncy Castle provider. The implementation allows users to configure the JCE provider through system properties (-Dsecurity.jce.provider=BC or -Dsecurity.jce.provider=BCFIPS) when starting the micro-integrator server.
Key changes include:
- Updated security libraries to versions with FIPS support (securevault 1.1.16, commons.vfs 2.2.0-wso2v23, SMBJ 0.11.3.wso2v2)
- Added conditional logic to select PKIX algorithm for KeyManagerFactory and TrustManagerFactory when a JCE provider is specified
- Implemented BCFKS keystore type support when FIPS mode is enabled
- Refactored provider initialization logic to avoid duplicate provider registration
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Updated dependency versions for securevault, commons.vfs, SMBJ, and added jaxen library |
| ServerConnFactoryBuilder.java | Added helper methods to select manager algorithms based on JCE provider configuration |
| ClientConnFactoryBuilder.java | Added helper methods to select manager algorithms based on JCE provider configuration |
| CertificatePathValidator.java | Refactored provider initialization into a reusable method with conditional registration |
| OCSPVerifier.java | Refactored provider initialization into a reusable method with conditional registration |
| TrustKeyStoreInformation.java | Added helper method to select trust manager algorithm based on JCE provider |
| IdentityKeyStoreInformation.java | Added helper method to select key manager algorithm based on JCE provider |
| OPAClient.java | Added logic to dynamically select keystore type (BCFKS for FIPS mode) |
| EncryptFunction.java | Added logic to dynamically select keystore type based on JCE provider configuration |
| DecryptFunction.java | Added logic to dynamically select keystore type based on JCE provider configuration |
| RabbitMQStore.java | Added helper methods to select manager algorithms based on JCE provider configuration |
| CryptoUtil.java | Refactored provider initialization into a reusable method with conditional registration |
| modules/commons/pom.xml | Added jaxen dependency to commons module |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.../nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/ClientConnFactoryBuilder.java
Outdated
Show resolved
Hide resolved
...ult/src/main/java/org/apache/synapse/securevault/definition/IdentityKeyStoreInformation.java
Show resolved
Hide resolved
.../nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/ClientConnFactoryBuilder.java
Show resolved
Hide resolved
.../nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/ServerConnFactoryBuilder.java
Show resolved
Hide resolved
.../nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/ClientConnFactoryBuilder.java
Show resolved
Hide resolved
...http/src/main/java/org/apache/synapse/transport/certificatevalidation/ocsp/OCSPVerifier.java
Show resolved
Hide resolved
modules/commons/src/main/java/org/apache/synapse/commons/crypto/CryptoUtil.java
Show resolved
Hide resolved
.../nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/ServerConnFactoryBuilder.java
Show resolved
Hide resolved
modules/core/src/main/java/org/apache/synapse/util/xpath/DecryptFunction.java
Show resolved
Hide resolved
modules/core/src/main/java/org/apache/synapse/message/store/impl/rabbitmq/RabbitMQStore.java
Show resolved
Hide resolved
1 participant
Bouncy Castle Overview
Bouncy Castle is an open-source cryptography library for Java. It provides a wide range of cryptographic algorithms and utilities, including:
Bouncy Castle FIPS
Bouncy Castle FIPS is a special version of the Bouncy Castle cryptography library that has been validated against the FIPS 140-2 / 140-3 standards.
This implementation enables BC and FIPS compliance. You can enable it by running the MI server script with the appropriate argument, as shown below:
For Bouncy Castle (BC):
For Bouncy Castle FIPS (BCFIPS):
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.