W-15551780 | Changes to make SFTP connector FIPS compliance(support/2.0.x)

[dipeshkumardutta-sfemu]

Hi, thank you for your work. We understand that you want to merge your changes and move on from this issue, but we also want to maintain the safety, readability, and testability of our code. Because of this, we kindly ask that you confirm that you have fulfilled the prerequisites for each category of activity before merging your changes.

If the Pull Request has a dependency update:

• I have read the Release Notes for the new version (and intermediate versions if the jump would include more than one). Don't blindly trust that the dependency will honor SEMVER, always read carefully the release notes
• Java 8+ support is maintained
• The new version has no vulnerabilities
• A team member reviewed the Pull Request.
• No backward compatibility is broken.
• If it is a "provided" dependency, it has been checked that if it is suggested, the version number is the same.

For ALL the Releases:

• There is a task in GUS for this change.
• The corresponding Release Notes have been written and shared with the documentation team.
• The new code has tests and they have before and after methods to be sure that you are working with a clean environment and that you are leaving the environment clean after they finish.
• If you are using reflection, create a test to call the methods you are invoking, this way, if there is a change in the API, the test will be able to detect it.
• If the project has a parent pom and modules, the release pipeline only uploads the modules, so, please, make sure that the parent was deployed in the repositories before considering this release finished.
• Notify in the Slack Channel that the release is complete, so the Docs team can merge the Documentation and Release Notes PR. The docs team always checks in Anypoint Exchange that the connector is released before merging the release notes.
• Add the connector build to GUS, and assign that build to the GUS ticket.
• Create a change case request, based on the Change Case Management doc https://docs.google.com/document/d/1tMJlTTZLaXMLiYwxlMBFY1yJwBmejhc4-IP8HAXgDfY/edit#

NOTE: (applies only for Core-Connectors connectors and modules) The release process ends when you merge the pull request that updates the support branch to the new snapshot, please don't forget to do this.

Patch Version:

• The Pull Request has been peer-reviewed.
• No backward compatibility is broken.
• Documentation: (Required) Release Notes PR with compatibility table. Share it with the Docs team.

Minor Version:

• Have a document with the specification of the release.
• Create a slack channel (eg: rl-conninfra-sftp-1.1.1-new-chiper) to coordinate the release of this version with the documentation team and the architects of our team (at least one week before the release). Share the spec doc with the docs team.
• The documentation fork has been done and has been updated with the changes related to the new functionalities.
• The pull request has been reviewed by a peer and the team leader.
• No backward compatibility is broken.
• Public Documentation: Minor releases nearly always require a revision of the documentation because the release usually includes new features as well as bug fixes. Even if there are no user-facing changes, documentation has to be versioned for every minor release.
  • (Required) Release Notes PR with compatibility table. Share it with the Docs team.
  • (Likely required) Reference guide. Run the script and share the file with the Docs team.
  • (Assess) Often a minor release impacts UI and examples, so the documentation (user guides, examples, troubleshooting guides) should be reviewed and updated accordingly. Notify the docs team if any updates are needed.

Major Version:

• Have a document with the specification of the release.
• Create a slack channel (eg: rl-conninfra-sftp-2.0.0) to coordinate the release of this version with the documentation team and the architects of our team (at least one month before the release). Share the spec doc with the docs team.
• The documentation fork has been made and updated with the changes related to the new functionalities and possible compatibility problems with the previous version.
• The systems properties have been removed, detailing in the documentation the changes in the default behavior of the product.
• The TODO comments of the code have been reviewed.
• A colleague, the team leader, and the team architect have reviewed the Pull Request.
• Public Documentation: Major releases break backward compatibility and should never be released without documentation, as this breaks the user experience.
  • (Required) Upgrade and Migration Guide: Every major release must have a detailed upgrade guide (template) with information about what was changed and what steps the user has to take to ensure the connector works as expected. Share the details with the Docs team so they can update the guide.
  • (Required) Release Notes PR with compatibility table. Share it with the docs team.
  • (Required) Reference guide. Run the script and share the file with the Docs team.
  • User guide updates, new screenshots or examples (if applicable), share the updates with the docs team.

[acurci-at-mulesoft]

Who should provide it? As far as I understand, it is not part of the FIPS security providers. What will happen to applications deployed in non-FIPS environments when they upgrade to this version? Will it fail to find the dependency?

[dipeshkumardutta-sfemu]

Correct this is not a FIPS provider. But this is causing fips testcase failure because its get added into the security provider list. I have no idea why this security provider is there in the first place. Please let me know if you have any insight.
But without this(provided scope) the munit test cases are passing. Does it sufficient? If not then we have to add this into the security provider of fips validation pipeline to pass the fips testcase.
Let me know your opinition.

[acurci-at-mulesoft]

Hello, @dipeshkumardutta-sfemu ,
While analyzing the EdDSA issue, I discovered that the problem seems to originate from here. By default, the Apache SSHD Security Utils project is registering two security providers: BC and EdDSA. The good news is that there appears to be a mechanism to prevent this provider from being registered. Please take a look at the SecurityUtils.setAPrioriDisabledProvider() method and its JavaDoc. We might be able to use this method to selectively avoid registering EdDSA, depending on whether we are running in a FIPS-compliant environment.

[acurci-at-mulesoft]

Additionally, we need to test how the connector functions in a FIPS-compliant environment, particularly in terms of validating host keys. To my knowledge, this validation is typically handled by the EdDSA security provider. We must determine what happens to this feature when we opt not to register that provider. Could you please check if there are any test suites that specifically assess this validation?

[acurci-at-mulesoft]

FYI Check this:

Signatures/Keys: ssh-dss, ssh-rsa, rsa-sha2-256, rsa-sha2-512, nistp256, nistp384, nistp521 , 
ssh-ed25519 (requires eddsa optional module), 
[email protected], [email protected] , [email protected], [email protected], [email protected] , [email protected], [email protected], [email protected]

Is ssh-ed25519 the only one we would stop supporting in the case of not registering the EdDSA security provider?

[dipeshkumardutta-sfemu]

@acurci-at-mulesoft Yes as far as I get the information from internet, for host key verification EdDSA is used and in apache mina two security provider registerar is used BC and EdDSA. Looking through https://github.com/apache/mina-sshd/blob/e021a6f4597cb7259162169851bb9e4cb85e1a39/sshd-common/src/main/java/org/apache/sshd/common/util/security/eddsa/EdDSASecurityProviderRegistrar.java you see that it expects the class net.i2p.crypto.eddsa.EdDSAKey to exist
So it means that it must have et.i2p.crypto.eddsa.EdDSAKe to exists.
As you mentioned I ran the munit test, disabling the EdDSA in fips and disabling BC in non-fips mode. The test execution was successful. Below is the code to conditionally disable Security provider.

if (FIPS_140_2_MODEL.equals(System.getProperty(MULE_SECURITY_MODEL))) { LOGGER.info("Running in fips mode so disabling EdDSA provider"); SecurityUtils.setAPrioriDisabledProvider(SecurityUtils.EDDSA, true); } else { LOGGER.info("Running in non-fips mode so disabling BC provider"); SecurityUtils.setAPrioriDisabledProvider(SecurityUtils.BOUNCY_CASTLE, true); }

[arpitg-1]

Host Key Verification in case of Apache Mina is handled by two providers bc and eddsa. KnownHostKeyVerifier is called for verification of remote server keys & checks if the keys belong to which algos. So only the eddsa and similar curves are supported by eddsa dependency, rest all including non-ec and other ec curves are supported by bouncy castle itself.
code-mina

Also, as this eddsa library is not fips compliant it'll be better to release two separate artifacts (two separate libraries) one fips compliant and one fips non compliant. One can have more algos supported one can only have fips compliant algos supported. @acurci-at-mulesoft Please provide your opinion

[arpitg-1]

As discussed in the meeting, we'll be removing this dependency from the pom, since it is not FIPS compliant.

[acurci-at-mulesoft]

Considering the comments made in the pom on the issue of whether or not to use EdDSA, I think we should review the use of shared libraries to clearly understand which ones will be present and which ones will not.