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.

Sign up for GitHub

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

Jump to bottom

Implement FIPS functions, adding OpenSSL FIPS mode case on CI. #608

Merged
merged 2 commits into from
May 15, 2023
Merged

Implement FIPS functions, adding OpenSSL FIPS mode case on CI. #608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3142d18
CI: Add OpenSSL FIPS mode case.
junaruga Mar 16, 2023
285baec
Implement FIPS functions on OpenSSL 3.
junaruga Mar 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 32 additions & 2 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ jobs:

test-openssls:
name: >-
${{ matrix.openssl }}
${{ matrix.openssl }} ${{ matrix.name_extra || '' }}
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
Expand All @@ -70,6 +70,9 @@ jobs:
- libressl-3.5.3
- libressl-3.6.1
- libressl-3.7.0 # Development release
fips_enabled: [ false ]
include:
- { os: ubuntu-latest, ruby: "3.0", openssl: openssl-3.0.8, fips_enabled: true, append_configure: 'enable-fips', name_extra: 'fips' }
steps:
- name: repo checkout
uses: actions/checkout@v3
Expand All @@ -83,7 +86,7 @@ jobs:
tar xf ${{ matrix.openssl }}.tar.gz && cd ${{ matrix.openssl }}
# shared is required for 1.0.x.
./Configure --prefix=$HOME/.openssl/${{ matrix.openssl }} --libdir=lib \
shared linux-x86_64
shared linux-x86_64 ${{ matrix.append_configure }}
make depend
;;
libressl-*)
Expand All @@ -98,6 +101,26 @@ jobs:
make -j4
make install_sw

- name: prepare openssl fips
run: make install_fips
working-directory: tmp/build-openssl/${{ matrix.openssl }}
if: matrix.fips_enabled

- name: set the open installed directory
run: >
sed -e "s|OPENSSL_DIR|$HOME/.openssl/${{ matrix.openssl }}|"
test/openssl/fixtures/ssl/openssl_fips.cnf.tmpl >
test/openssl/fixtures/ssl/openssl_fips.cnf
if: matrix.fips_enabled

- name: set openssl config file path for fips.
run: echo "OPENSSL_CONF=$(pwd)/test/openssl/fixtures/ssl/openssl_fips.cnf" >> $GITHUB_ENV
if: matrix.fips_enabled

- name: set fips enviornment variable for testing.
run: echo "TEST_RUBY_OPENSSL_FIPS_ENABLED=true" >> $GITHUB_ENV
if: matrix.fips_enabled

- name: load ruby
uses: ruby/setup-ruby@v1
with:
Expand All @@ -112,3 +135,10 @@ jobs:
- name: test
run: rake test TESTOPTS="-v --no-show-detail-immediately" OSSL_MDEBUG=1
timeout-minutes: 5
if: ${{ !matrix.fips_enabled }}
Copy link
Member Author

Choose a reason for hiding this comment

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

This condition is a temporary workaround. If you remove the if, you can reproduce the issue #603 on the fips mode case on CI.


# Run only the passing tests on the FIPS mode as a temporary workaround.
# TODO Fix other tests, and run all the tests on FIPS mode.
- name: test on fips mode
run: ruby -Ilib test/openssl/test_fips.rb
if: matrix.fips_enabled
25 changes: 21 additions & 4 deletions ext/openssl/ossl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,11 @@ static VALUE
ossl_fips_mode_get(VALUE self)
{

#ifdef OPENSSL_FIPS
#if OSSL_OPENSSL_PREREQ(3, 0, 0)
VALUE enabled;
enabled = EVP_default_properties_is_fips_enabled(NULL) ? Qtrue : Qfalse;
return enabled;
#elif OPENSSL_FIPS
VALUE enabled;
enabled = FIPS_mode() ? Qtrue : Qfalse;
return enabled;
Expand All @@ -442,8 +446,18 @@ ossl_fips_mode_get(VALUE self)
static VALUE
ossl_fips_mode_set(VALUE self, VALUE enabled)
{

#ifdef OPENSSL_FIPS
#if OSSL_OPENSSL_PREREQ(3, 0, 0)
if (RTEST(enabled)) {
if (!EVP_default_properties_enable_fips(NULL, 1)) {
ossl_raise(eOSSLError, "Turning on FIPS mode failed");
}
} else {
if (!EVP_default_properties_enable_fips(NULL, 0)) {
ossl_raise(eOSSLError, "Turning off FIPS mode failed");
}
}
return enabled;
#elif OPENSSL_FIPS
if (RTEST(enabled)) {
int mode = FIPS_mode();
if(!mode && !FIPS_mode_set(1)) /* turning on twice leads to an error */
Expand Down Expand Up @@ -1198,7 +1212,10 @@ Init_openssl(void)
* Boolean indicating whether OpenSSL is FIPS-capable or not
*/
rb_define_const(mOSSL, "OPENSSL_FIPS",
#ifdef OPENSSL_FIPS
/* OpenSSL 3 is FIPS-capable even when it is installed without fips option */
#if OSSL_OPENSSL_PREREQ(3, 0, 0)
Qtrue
#elif OPENSSL_FIPS
Qtrue
#else
Qfalse
Expand Down
19 changes: 19 additions & 0 deletions test/openssl/fixtures/ssl/openssl_fips.cnf.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
config_diagnostics = 1
openssl_conf = openssl_init

# It seems that the .include needs an absolute path.
.include OPENSSL_DIR/ssl/fipsmodule.cnf

[openssl_init]
providers = provider_sect
alg_section = algorithm_sect

[provider_sect]
fips = fips_sect
base = base_sect

[base_sect]
activate = 1

[algorithm_sect]
default_properties = fips=yes
32 changes: 28 additions & 4 deletions test/openssl/test_fips.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,44 @@
if defined?(OpenSSL)

class OpenSSL::TestFIPS < OpenSSL::TestCase
def test_fips_mode_get_is_true_on_fips_mode_enabled
unless ENV["TEST_RUBY_OPENSSL_FIPS_ENABLED"]
omit "Only for FIPS mode environment"
end

assert_separately([{ "OSSL_MDEBUG" => nil }, "-ropenssl"], <<~"end;")
assert OpenSSL.fips_mode == true, ".fips_mode should return true on FIPS mode enabled"
end;
end

def test_fips_mode_get_is_false_on_fips_mode_disabled
if ENV["TEST_RUBY_OPENSSL_FIPS_ENABLED"]
omit "Only for non-FIPS mode environment"
end

assert_separately([{ "OSSL_MDEBUG" => nil }, "-ropenssl"], <<~"end;")
message = ".fips_mode should return false on FIPS mode disabled. " \
"If you run the test on FIPS mode, please set " \
"TEST_RUBY_OPENSSL_FIPS_ENABLED=true"
assert OpenSSL.fips_mode == false, message
end;
end

Copy link

Choose a reason for hiding this comment

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

Shouldn't this test cases be enabled permanently, also outside of CI?

Or shouldn't there rather be one test case, which will compare the expected result based on the env variables?

Copy link
Member Author

@junaruga junaruga Mar 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Right now I don't find a good way to check if the OpenSSL is running on the FIPS mode enabled or disabled. Because the OpenSSL.fips_mode itself is a logic to check if the OpenSSL is running on the FIPS mode enabled or disabled.

Perhaps, one possible way to check the FIPS mode or not with the OpenSSL 3 is, to add a source code of the command fips_enabled just to return the exit status 0 or 1 using the EVP_default_properties_is_fips_enabled(NULL) such as (the example in my proof of concept repository), build it and use in the testing process. The code can be below. A challenge of this way is when OpenSSL is not set up properly for the FIPS mode unintentionally, the EVP_default_properties_is_fips_enabled(NULL) returns 0 (disabled), and the test is skipped unintentionally. Perhaps with keeping the current tests, and in addition, we can add the new test with the command fips_enabled.

fips_enabled.c

#include <stdio.h>
#include <openssl/evp.h>

int main(int argc, char *argv[])
{
    int status = EXIT_SUCCESS;
    int fips_enabled = 0;

    fips_enabled = EVP_default_properties_is_fips_enabled(NULL);
    status = fips_enabled ? EXIT_SUCCESS : EXIT_FAILURE;

    exit(status);
}

If you guys like this way, I can add the source code to this repository and can add new tests. But I would like that it is in another PR. Because the current tests running on CI can test the new implementation on the CI.

Copy link

Choose a reason for hiding this comment

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

The FIPS mode is enabled in CI, so it is in know state. Or are you afraid that somebody who would by a chance run the test suite on their FIPS enabled system would differ from default expectation? I don't think this is very likely scenario.

IOW I believe it is safe to assume that by default, FIPS is disabled and the OpenSSL.fips_mode == false condition is true.

Copy link
Member Author

Choose a reason for hiding this comment

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

The FIPS mode is enabled in CI, so it is in know state. Or are you afraid that somebody who would by a chance run the test suite on their FIPS enabled system would differ from default expectation? I don't think this is very likely scenario.

Yes. Maybe in my understanding, that is the case I am afraid of. I noticed that OpenSSL.fips_mode returns false on FIPS enabled system on OpenSSL 3, then I opened the issue ticket. Let's say that I want to notice by seeing the failure of the unit test when the OpenSSL.fips_mode will unintentionally return false on FIPS enabled system on OpenSSL "4".

IOW I believe it is safe to assume that by default, FIPS is disabled and the OpenSSL.fips_mode == false condition is true.

I still cannot imagine how do you want to change the condition. Could you share your suggestion for the change with the result of the git diff or diff -u <original_file> <new_file> for the test/openssl/test_fips.rb by doing checkout the branch used for this PR in my forked repository? How can the change look like?

Copy link
Member Author

@junaruga junaruga Mar 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

My another suggestion is below, removing the ENV["CI"] from the condition. It can test the assert OpenSSL.fips_mode == false on local environment outside CI.

$ git diff
diff --git a/test/openssl/test_fips.rb b/test/openssl/test_fips.rb
index dc92bde..e12f1d2 100644
--- a/test/openssl/test_fips.rb
+++ b/test/openssl/test_fips.rb
@@ -5,22 +5,25 @@ if defined?(OpenSSL)
 
 class OpenSSL::TestFIPS < OpenSSL::TestCase
   def test_fips_mode_get_is_true_on_fips_mode_enabled
-    unless ENV["CI"] && ENV["TEST_RUBY_OPENSSL_FIPS_ENABLED"]
+    unless ENV["TEST_RUBY_OPENSSL_FIPS_ENABLED"]
       omit "Only for on FIPS mode environment on CI"
     end
 
     assert_separately([{ "OSSL_MDEBUG" => nil }, "-ropenssl"], <<~"end;")
-      assert OpenSSL.fips_mode == true, ".fips_mode returns true on FIPS mode enabled"
+      assert OpenSSL.fips_mode == true, ".fips_mode should return true on FIPS mode enabled"
     end;
   end
 
   def test_fips_mode_get_is_false_on_fips_mode_disabled
-    unless ENV["CI"] && !ENV["TEST_RUBY_OPENSSL_FIPS_ENABLED"]
+    if ENV["TEST_RUBY_OPENSSL_FIPS_ENABLED"]
       omit "Only for non-FIPS mode environment on CI"
     end
 
     assert_separately([{ "OSSL_MDEBUG" => nil }, "-ropenssl"], <<~"end;")
-      assert OpenSSL.fips_mode == false, ".fips_mode returns false on FIPS mode disabled"
+      message = ".fips_mode should return false on FIPS mode disabled. " \
+                "If you run the test on FIPS mode, please set " \
+                "TEST_RUBY_OPENSSL_FIPS_ENABLED=true"
+      assert OpenSSL.fips_mode == false, message
     end;
   end
 
@@ -35,10 +38,10 @@ class OpenSSL::TestFIPS < OpenSSL::TestCase
     assert_separately([{ "OSSL_MDEBUG" => nil }, "-ropenssl"], <<~"end;")
       begin
         OpenSSL.fips_mode = true
-        assert OpenSSL.fips_mode == true, ".fips_mode returns true when .fips_mode=true"
+        assert OpenSSL.fips_mode == true, ".fips_mode should return true when .fips_mode=true"
 
         OpenSSL.fips_mode = false
-        assert OpenSSL.fips_mode == false, ".fips_mode returns false when .fips_mode=false"
+        assert OpenSSL.fips_mode == false, ".fips_mode should return false when .fips_mode=false"
       rescue OpenSSL::OpenSSLError
         pend "Could not set FIPS mode (OpenSSL::OpenSSLError: \#$!); skipping"
       end

def test_fips_mode_is_reentrant
OpenSSL.fips_mode = false
OpenSSL.fips_mode = false
end

def test_fips_mode_get
return unless OpenSSL::OPENSSL_FIPS
def test_fips_mode_get_with_fips_mode_set
omit('OpenSSL is not FIPS-capable') unless OpenSSL::OPENSSL_FIPS
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that I used the message OpenSSL is not FIPS-capable, because the word "OpenSSL" in the message is used as a meaning of the SSL Library in this context including LibreSSL cases, as well as the "openssl" in this repository name "OpenSSL for Ruby". The word "FIPS-capable" comes from the OpenSSL::OPENSSL_FIPS's comment: "Boolean indicating whether OpenSSL is FIPS-capable or not".

I see the following error message in the ext/openssl/ossl.c. So, another candidate of the message is "OpenSSL does not support FIPS mode" to align with the error message.

ossl_raise(eOSSLError, "This version of OpenSSL does not support FIPS mode");


junaruga marked this conversation as resolved.
Show resolved Hide resolved
assert_separately([{ "OSSL_MDEBUG" => nil }, "-ropenssl"], <<~"end;")
begin
OpenSSL.fips_mode = true
assert OpenSSL.fips_mode == true, ".fips_mode returns true when .fips_mode=true"
assert OpenSSL.fips_mode == true, ".fips_mode should return true when .fips_mode=true"

OpenSSL.fips_mode = false
assert OpenSSL.fips_mode == false, ".fips_mode returns false when .fips_mode=false"
assert OpenSSL.fips_mode == false, ".fips_mode should return false when .fips_mode=false"
rescue OpenSSL::OpenSSLError
pend "Could not set FIPS mode (OpenSSL::OpenSSLError: \#$!); skipping"
end
Expand Down