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

Fix test_pkey_ec.rb on FIPS. #681

Merged
merged 4 commits into from
Sep 21, 2023
Merged

Conversation

junaruga
Copy link
Member

@junaruga junaruga commented Sep 9, 2023

This PR fixes #671 fixing the test_ECPrivateKey_encrypted test in test/openssl/test_pkey_ec.rb in FIPS case, adding the test file to the CI.

The PR has 2 commits.

The 1st commit is a rework of test_ed25519 in the test/openssl/test_pkey.rb in OpenSSL 3.1 and later versions FIPS case. The commit is to prepare the 2nd commit. I want to test it with asserting rather than pending. Because I think that raising the OpenSSL::PKey::PKeyError in the FIPS case is the expected behavior rather than pending status. I want to show that we are intentional for the raised error and error message.

The 2nd commit is the main commit to fix the test/openssl/test_pkey_ec.rb on the same way that is applied to the 1st commit by testing the FIPS case by asserting. I also added the test_pkey_ec.rb to the CI. I checked that the test file is executed on my forked repository. The result is "29 tests, 185 assertions" below.

https://github.com/junaruga/openssl/actions/runs/6131045483/job/16640679294#step:14:34

openssl-head fips

$ ruby -I./lib -ropenssl \
  -e 'Dir.glob "./test/openssl/{test_fips.rb,test_pkey.rb,test_pkey_ec.rb}", &method(:require)'
...
29 tests, 185 assertions, 0 failures, 0 errors, 0 pendings, 1 omissions, 0 notifications

As a reference, on the current master branch, the case's result is below.

https://github.com/ruby/openssl/actions/runs/6040202225/job/16390553438#step:14:46

$ ruby -I./lib -ropenssl \
  -e 'Dir.glob "./test/openssl/{test_fips.rb,test_pkey.rb}", &method(:require)'
...
12 tests, 51 assertions, 0 failures, 0 errors, 1 pendings, 1 omissions, 0 notifications

@rhenium
Copy link
Member

rhenium commented Sep 9, 2023

test_pkey.rb: Assert the ed25519 key FIPS case instead of pending.

I want this to be a separate test case. Let's make test_ed25519 focus on Ed25519(/Ed448)-specific features.

@junaruga
Copy link
Member Author

junaruga commented Sep 9, 2023

test_pkey.rb: Assert the ed25519 key FIPS case instead of pending.

I want this to be a separate test case. Let's make test_ed25519 focus on Ed25519(/Ed448)-specific features.

Okay. I understood your point. The test test_ed25519 should focus on only one thing to test ed25519 literally. I will separate the test case.

@junaruga
Copy link
Member Author

test_pkey.rb: Assert the ed25519 key FIPS case instead of pending.

I want this to be a separate test case. Let's make test_ed25519 focus on Ed25519(/Ed448)-specific features.

Okay. I understood your point. The test test_ed25519 should focus on only one thing to test ed25519 literally. I will separate the test case.

I did split the test_ed25519 for the FIPS.

@junaruga
Copy link
Member Author

I am rebasing. Now 4 commits.

The first commit is to test FIPS on local a bit easier. The 2nd commit is a bug of the test_fips.rb affecting other test files. The 3rd commit refactors the test_ed25519 in the test_pkey.rb. And the 4th commit is to fix the test_pkey_ec.rb in FIPS.

And I want to fix other test_pkey*.rb test files in FIPS using the omit_on_fips method in another PR.

@junaruga junaruga changed the title test_pkey_ec.rb: Fix the test in FIPS case, adding the file to CI. Fix test_pkey_ec.rb on FIPS. Sep 19, 2023
@junaruga
Copy link
Member Author

Just moment, I want to refactor.

@junaruga
Copy link
Member Author

Just moment, I want to refactor.

OK. Now I am ready for reviewing again. Could you review?

test/openssl/test_fips.rb Outdated Show resolved Hide resolved
test/openssl/test_pkey.rb Outdated Show resolved Hide resolved
test/openssl/utils.rb Outdated Show resolved Hide resolved
Run the test with `assert_separately` for the `false` value of the
`OpenSSL.fips_mode` not to affect other tests.
test/openssl/utils.rb Outdated Show resolved Hide resolved
@junaruga
Copy link
Member Author

I rebased the PR fixing the issues. Could you review the PR again?

@junaruga
Copy link
Member Author

I rebased fixing the issue again.

I changed the omit_on_fips from the following omit_on_fips(&filter_block) to the following omit_on_fips again, as we don't need to set an additional filter argument to the omit_on_fips for now.

  def omit_on_fips(&filter_block)
    filter_block ||= proc { true }

    return unless OpenSSL.fips_mode && filter_block.call
  
    omit 'An encryption used in the test is not FIPS-approved'
  end
  def omit_on_fips
    return unless OpenSSL.fips_mode
  
    omit 'An encryption used in the test is not FIPS-approved'
  end

@junaruga
Copy link
Member Author

junaruga commented Sep 21, 2023

Sorry, I am refactoring again.

* Split the test in the FIPS case as another test.
* test/openssl/utils.rb: Add omit_on_fips and omit_on_non_fips methods.
@junaruga
Copy link
Member Author

Sorry, I am refactoring again.

All right. I refactored the 3rd commit, and rebased. Could you review again?

Copy link
Member

@rhenium rhenium left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you!

@rhenium rhenium merged commit 250b74b into ruby:master Sep 21, 2023
43 checks passed
@junaruga junaruga deleted the wip/fix-test-pkey-ec branch September 21, 2023 18:20
@junaruga
Copy link
Member Author

Thank you too!

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

Successfully merging this pull request may close these issues.

test_pkey_ec.rb test failures in OpenSSL FIPS
2 participants