Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MINIFICPP-2524 Add FIPS compliancy support #1925

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

lordgamez
Copy link
Contributor

This change adds the option to enable FIPS mode in the OpenSSL library for cryptographic functions used by MiNiFi C++. This includes the following:

OpenSSL discussion about this issue: openssl/openssl#26378

https://issues.apache.org/jira/browse/MINIFICPP-2524


Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced
    in the commit message?

  • Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.

  • Has your PR been rebased against the latest commit within the target branch (typically main)?

  • Is your initial contribution a single, squashed commit?

For code changes:

  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE file?
  • If applicable, have you updated the NOTICE file?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.

@lordgamez lordgamez added the minifi-api Targets the minifi-api branch label Feb 6, 2025
@lordgamez lordgamez marked this pull request as ready for review February 7, 2025 08:13
@lordgamez lordgamez changed the base branch from minifi-api to main February 18, 2025 11:27
@lordgamez lordgamez removed the minifi-api Targets the minifi-api branch label Feb 18, 2025
@@ -59,7 +65,8 @@ function(use_openssl SOURCE_DIR BINARY_DIR)
no-module # disable dynamically loadable engines
no-pinshared # don't pin shared libraries in the process memory
enable-tfo # Enable TCP Fast Open
no-ssl) # disable SSLv3
no-ssl # disable SSLv3
no-engine)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this exactly do? INSTALL.md says:

Don't build support for loading engines.

But I'm not sure what loadable engines are. Is it related to FIPS in any way? Also, the FIPS target omits most of these, probably leading to some unnecessary extra build steps, consider adding some more options there, like no-tests, no-docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not that familiar with it, but there is an Engine API in OpenSSL that is controlled by this option. An engine that is loaded in OpenSSL is a hardware or software implementation used for performing cryptographic operations. Engines are not FIPS compatible and corresponding API is deprecated since OpenSSL 3.0, so for FIPS compliancy we should remove the engine support as well. (According to the documention it is also a good guidline for FIPS compliancy to remove anything that is deprecated in OpenSSL 3.0 as that is probably not FIPS compatible.)

Comment on lines -54 to -60
no-apps # disable executables
no-capieng # disable CAPI engine (legacy)
no-dso # disable dynamic libraries
no-docs # disable docs and manpages
no-legacy # disable legacy modules
no-module # disable dynamically loadable engines
no-pinshared # don't pin shared libraries in the process memory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are all these removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • no-apps is removed because we need the openssl binary to be packaged with fips
  • no-dso and no-module need to be enabled for shared libraries to be loaded in our case the fips provider which cannot be statically linked
  • IMO no-pinshared should not be enabled to have the loaded fips provider available through the lifetime of the application and avoid the accidental unloading of the fips shared library

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine with fips, but these are the library options, not the fips target options.

Copy link
Contributor Author

@lordgamez lordgamez Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But these options are meant for the OpenSSL library options for loading the FIPS module, not for the FIPS target.

  • We need the binary of the latest OpenSSL target, not the FIPS target
  • Without the options to be able to load shared libraries, loading of the FIPS module fails with the following error:
Failed to load FIPS module
INSTALL FAILED
4077F3DFF87D0000:error:07880025:common libcrypto routines:provider_init:reason(37):crypto/provider_core.c:949:name=fips

Copy link
Member

@szaszm szaszm Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aah ok. I thought both the binary and the fips module come from the fips target.

What about no-pinshared, is removing that necessary too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants