Improve implementations for skipping and conditions.

[junaruga]

Cc: @samuel40791765

This PR has 3 commits and mainly improves the things I noticed in the #855.

The 1st commit is to replace return with omit or pend in the tests. I believe that the omit and pend are more useful than return in the tests. Because we can see the skipped tests and reasons in the testing log.

Note that I think the part pend "AWS-LC's FIPS mode is decided at compile time" is pend not omit. The reason we have to skip the tests is there is only one AWS-LC case compiled with a non-FIPS option in CI. I assume that we can improve this part when we add one more AWS-LC case compiled with a FIPS option.

The 2nd commit is to refactor the conditions from this perspective. I checked this PR on my forked branch. Especially the AWS-LC's skipping part in the testing result is here.

The 3rd commit is to fix a bug that the test test_fips_mode_get_with_fips_mode_set is not executed in OpenSSL 3.0 FIPS. According to the current implementation below, the OpenSSL::OPENSSL_FIPS always returns true on OpenSSL 3.0.0 and later versions. I checked the test test_fips_mode_get_with_fips_mode_set was executed on an OpenSSL 3 FIPS case, and skipped on an OpenSSL 3 non-FIPS case.

openssl/ext/openssl/ossl.c

Lines 994 to 1004 in d725783

	rb_define_const(mOSSL, "OPENSSL_FIPS",
	/* OpenSSL 3 is FIPS-capable even when it is installed without fips option */
	#if OSSL_OPENSSL_PREREQ(3, 0, 0)
	Qtrue
	#elif defined(OPENSSL_FIPS)
	Qtrue
	#elif defined(OPENSSL_IS_AWSLC) // AWS-LC FIPS can only be enabled during compile time.
	FIPS_mode() ? Qtrue : Qfalse
	#else
	Qfalse
	#endif

[samuel40791765]

The 1st commit is to replace return with omit or pend in the tests. I believe that the omit and pend are more useful than return in the tests. Because we can see the skipped tests and reasons in the testing log.

Thanks! I noticed and really enjoyed the additional visibility omit and pend provided, but had been slightly hesitant to use it since I wanted to make the coding pattern in the same test files consistent. This helps with that.

I assume that we can improve this part when we add one more AWS-LC case compiled with a FIPS option.

That would be awesome :). Triggering AWS-LC in FIPS mode is an additional -DFIPS=1 compilation flag during the build process.

[junaruga]

The 1st commit is to replace return with omit or pend in the tests. I believe that the omit and pend are more useful than return in the tests. Because we can see the skipped tests and reasons in the testing log.

Thanks! I noticed and really enjoyed the additional visibility omit and pend provided, but had been slightly hesitant to use it since I wanted to make the coding pattern in the same test files consistent. This helps with that.

All right!

I assume that we can improve this part when we add one more AWS-LC case compiled with a FIPS option.

That would be awesome :). Triggering AWS-LC in FIPS mode is an additional -DFIPS=1 compilation flag during the build process.

Note I removed the following lines, and rebased the PR, and I confirmed that the test_fips_mode_is_reentrant test passed on the aws-lc case. (log). So, there are no additional pend lines on this PR.

diff --git a/test/openssl/test_fips.rb b/test/openssl/test_fips.rb
index 8c8ce72..ea3e316 100644
--- a/test/openssl/test_fips.rb
+++ b/test/openssl/test_fips.rb
@@ -28,8 +28,6 @@ class OpenSSL::TestFIPS < OpenSSL::TestCase
   end

   def test_fips_mode_is_reentrant
-    pend "AWS-LC's FIPS mode is decided at compile time" if aws_lc?
-
     assert_separately(["-ropenssl"], <<~"end;")
       OpenSSL.fips_mode = false
       OpenSSL.fips_mode = false
@@ -40,7 +38,6 @@ class OpenSSL::TestFIPS < OpenSSL::TestCase
     unless ENV["TEST_RUBY_OPENSSL_FIPS_ENABLED"]
       omit "Only for FIPS mode environment"
     end
-    pend "AWS-LC's FIPS mode is decided at compile time" if aws_lc?

     assert_separately(["-ropenssl"], <<~"end;")
       begin

After we merge this PR, you can contribute the AWS-LC FIPS case on CI. I don't have a plan to contribute it by myself.

By the way, in the OpenSSL upstream project, the term "FIPS mode" is used to enable FIPS in OS level. We use the term "FIPS module" to just enable OpenSSL library's FIPS. We are using the term in .github/workflows/test.yml as I was told in the upstream OpenSSL project. The term "FIPS mode" in OpenSSL.fips_mode is a legacy coming from the C APIs FIPS_mode and FIPS_mode_set that were used in OpenSSL 1.1/1.0 in my assumption.

[rhenium]

I'm not a fan of using omit everwhere because of how test-unit handles it. IMO, just returning is fine as long as it's well-documented, and we're reasonably sure that the condition won't accidentally makes the test case skipped when it should run. Can we keep its usage minimal, only when an attention is required?

For example, LibreSSL removed NPN support several years ago and they are not going to add it again. Seeing the 25 extra lines of output (5 tests, 5 lines each) every time I run rake test locally with LibreSSL would be annoying and not useful. Honestly, I don't even like the two omissions I currently see when running the test suite in my usual environment (OpenSSL 3.x, not in the FIPS mode), though they've been bareable. I must admit that I haven't been consistent about which to use when skipping something on LibreSSL or very old OpenSSL versions (after all I don't usually use them myself).

On a related note, those NPN tests should probably be updated to use libressl? rather than checking the existence of a method, as it's more prone to errors.

[rhenium]

We can assume all libssl versions support TLS 1.2 and TLS 1.3, so a check for TLS 1.2 is no longer necessary. #841 overlooked this.

I agree an explicit omission would be useful here, since TLS 1.1 is not typically disabled, and this should be noted.

Suggested change

	omit unless supported.include?(OpenSSL::SSL::TLS1_1_VERSION) &&
	supported.include?(OpenSSL::SSL::TLS1_2_VERSION)
	omit "TLS 1.1 is required to run this test" unless supported.include?(OpenSSL::SSL::TLS1_1_VERSION)

[rhenium]

Why is this changed?

[junaruga]

Sorry, this change is because the line length was more than 80 bytes. I didn't like it. I was working with a sleepy brain. I shouldn't include this change in this commit. I will revert this part.

[rhenium]

#861 is an alternative to this. This test case should run with LibreSSL too.

[junaruga]

I'm not a fan of using omit everwhere because of how test-unit handles it. IMO, just returning is fine as long as it's well-documented, and we're reasonably sure that the condition won't accidentally makes the test case skipped when it should run. Can we keep its usage minimal, only when an attention is required?

For example, LibreSSL removed NPN support several years ago and they are not going to add it again. Seeing the 25 extra lines of output (5 tests, 5 lines each) every time I run rake test locally with LibreSSL would be annoying and not useful. Honestly, I don't even like the two omissions I currently see when running the test suite in my usual environment (OpenSSL 3.x, not in the FIPS mode), though they've been bareable. I must admit that I haven't been consistent about which to use when skipping something on LibreSSL or very old OpenSSL versions (after all I don't usually use them myself).

On a related note, those NPN tests should probably be updated to use libressl? rather than checking the existence of a method, as it's more prone to errors.

I was thinking you were positive about using the omit because we worked to add the omit_on_fips and omit_on_non_fips in the past. However, if you don't like using the omit, I am okay. I will withdraw the 2nd commit replacing the return with omit. Then we may work on another PR to replace omit with return in some parts. Though I am not sure what cases do you want to keep the omit or you don't want to use omit at all.

[junaruga]

If OpenSSL.fips_mode can return true in the AWS-LC FIPS app configuring properly to load FIPS module, maybe we don't need skipping logic of the aws_lc? used with omit_on_fips or omit_on_non_fips. I will add the commit about removing such necessary aws_lc? parts on this PR.

[junaruga]

I would close this PR, and will send another PR because the subject of the PR is now changing.

[junaruga]

If OpenSSL.fips_mode can return true in the AWS-LC FIPS app configuring properly to load FIPS module, maybe we don't need skipping logic of the aws_lc? used with omit_on_fips or omit_on_non_fips. I will add the commit about removing such necessary aws_lc? parts on this PR.

I confirmed that OpenSSL.fips_mode returned true in AWS-LC FIPS, without modifying any configuration files. The command log is below. So, we can remove the above parts of the aws_lc? used with omit_on_fips or omit_on_non_fips. I also confirmed that the OpenSSL.fips_mode returned false in AWS-LC non-FIPS.

$ pwd
/home/jaruga/git/aws-lc

$ git branch
* (HEAD detached at v1.46.1)
  main

$ cmake -DCMAKE_INSTALL_PREFIX=$HOME/.local/aws-lc-1.46.1-fips -DCMAKE_INSTALL_LIBDIR=lib -DFIPS=1

$ make -j8

$ make install

$ ~/.local/aws-lc-1.46.1-fips/bin/openssl version
OpenSSL 1.1.1 (compatible; AWS-LC 1.46.1)

$ pwd
/home/jaruga/var/git/ruby/openssl

$ bundle install --path app

$ MAKEFLAGS="V=1" \
  RUBY_OPENSSL_EXTCFLAGS="-O0 -g3 -ggdb3 -gdwarf-5" \
  bundle exec rake compile -- \
  --with-openssl-dir="$HOME/.local/aws-lc-1.46.1-fips"

$ bundle exec ruby -I ./lib -ropenssl.so -e 'p OpenSSL.fips_mode'
true

[samuel40791765]

Really appreciate the help with fixing the FIPS portion of the AWS-LC test cases!

After we merge this PR, you can contribute the AWS-LC FIPS case on CI. I don't have a plan to contribute it by myself.

I had initially considered this, but the truth is our current FIPS release doesn't have support for Ruby integration. While we've recently added Ruby support to AWS-LC, it's only compatible with our latest releases. We're planning to validate these new changes as part of our upcoming AWS-LC FIPS v4.0.0 release. Once that validation is complete, we can revisit this. We'll keep you updated on our progress with the v4.0.0 validation.