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

CONTRIBUTING.md: Update testing with debugging and FIPS use cases. [ci skip] #688

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

junaruga
Copy link
Member

I updated the CONTRIBUTING.md - Testing section, adding the debugging and FIPS use cases.

While how to test with OpenSSL FIPS is written in the GitHub Actions file test.yml, my motivation is this updated document may promote users to understand how to test/debug Ruby OpenSSL and even OpenSSL both non-FIPS and FIPS module cases.

You can check the updated CONTRIBUTING.md here on my forked repository.

What do you think?

@rhenium
Copy link
Member

rhenium commented Oct 25, 2023

I don't think we need two sections for installing OpenSSL from tarballs and from the Git repository. I find it a bit confusing as they currently do different things: the FIPS mode has nothing to do with whether installing from a tarball or from the master branch of the Git repository. I think we can have a single instruction for installing OpenSSL from the Git repository. (Perhaps suggest a git checkout openssl-3.1 for testing against the stable branch, if it isn't too verbose.)

After that, I think we can have two separate subsections for normal testing (rake test without OPENSSL_CONF environment variable) and FIPS mode testing (setting up the config file and using it with OPENSSL_CONF environment variable).

What do you think?

@junaruga
Copy link
Member Author

I don't think we need two sections for installing OpenSSL from tarballs and from the Git repository. I find it a bit confusing as they currently do different things: the FIPS mode has nothing to do with whether installing from a tarball or from the master branch of the Git repository. I think we can have a single instruction for installing OpenSSL from the Git repository. (Perhaps suggest a git checkout openssl-3.1 for testing against the stable branch, if it isn't too verbose.)

After that, I think we can have two separate subsections for normal testing (rake test without OPENSSL_CONF environment variable) and FIPS mode testing (setting up the config file and using it with OPENSSL_CONF environment variable).

What do you think?

Yes, I agree with your opinions! I will fix the document with your suggested ways.

@junaruga
Copy link
Member Author

I noticed that both "Ruby/OpenSSL" and "Ruby OpenSSL" are used in the documents while the "Ruby/OpenSSL" is mentioned below in the README. Which name do you want see in CONTRIBUTING.md in this PR? I would like to modify only CONTRIBUTING.md in this PR.

https://github.com/ruby/openssl/blob/master/README.md

Ruby/OpenSSL for disambiguation

$ grep -r 'Ruby/OpenSSL' .
./CONTRIBUTING.md:$ # in Ruby/OpenSSL's source directory
./ext/openssl/ossl_kdf.c: * Ruby/OpenSSL Project
./ext/openssl/ossl_kdf.c: * Copyright (C) 2007, 2017 Ruby/OpenSSL Project Authors
./History.md:Ruby/OpenSSL 3.1 will be maintained for the lifetime of Ruby 3.2.
./History.md:  1.1 and contains incompatible changes that affect Ruby/OpenSSL.
./History.md:    available when Ruby/OpenSSL is linked against OpenSSL 3.0.
./History.md:  newer but also less than 3.0. Ruby/OpenSSL v2.1.x and v2.2.x will not support
./README.md:or **Ruby/OpenSSL** for disambiguation.
./lib/openssl/pkey.rb:# Ruby/OpenSSL Project
./lib/openssl/pkey.rb:# Copyright (C) 2017 Ruby/OpenSSL Project Authors
./lib/openssl/pkcs5.rb:# Ruby/OpenSSL Project
./lib/openssl/pkcs5.rb:# Copyright (C) 2017 Ruby/OpenSSL Project Authors
./lib/openssl/ssl.rb:      # the context. As of Ruby/OpenSSL 2.1, this accessor method is

$ grep -r 'Ruby OpenSSL' .
./CONTRIBUTING.md:# Contributing to Ruby OpenSSL
./CONTRIBUTING.md:Thank you for your interest in contributing to Ruby OpenSSL!
./CONTRIBUTING.md:Ruby OpenSSL supports various versions of OpenSSL library. The test suite needs
./CONTRIBUTING.md:  _\- The Ruby OpenSSL team_

@junaruga junaruga marked this pull request as draft October 26, 2023 19:50
@junaruga junaruga force-pushed the wip/doc-contributing branch from 21322ec to 12c29b1 Compare October 26, 2023 20:42
@junaruga junaruga marked this pull request as ready for review October 26, 2023 20:44
@junaruga
Copy link
Member Author

I rebased fixing the things mentioned in the review. I checked the entire "With different versions of OpenSSL" section by Grammarly free version too just in case.

Now what do you think?

@junaruga junaruga force-pushed the wip/doc-contributing branch from 12c29b1 to d74a360 Compare October 26, 2023 21:00
CONTRIBUTING.md Outdated
$ bundle exec rake test
```

#### Testing in FIPS case

You can test the FIPS case with the OpenSSL FIPS config file.
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to add a short description why we specify a config file because using OpenSSL in FIPS mode is not very straightforward IMHO. (Please feel free to rephrase/rewrite)

To use OpenSSL 3.0 or later in FIPS approved manner,
you must load the `fips` and `base` provider and use the property query `fips=yes`
when fetching cryptographic algorithm implementations.
This must be done at the startup of a process in order to avoid implicitly loading `default` provider (the non-FIPS cryptographic algorithm implementations).
See also the man page [fips_module(7)](https://www.openssl.org/docs/manmaster/man7/fips_module.html).

We can use an OpenSSL configuration file to enforce this.
You can either appropriately modify the file located at `OpenSSL::Config::DEFAULT_CONFIG_FILE` or
temporarily override it with the `OPENSSL_CONF` environment variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Explaining the short description with "why" makes sense to me. I added your sentences rewriting a bit.

By the way, below is the result on RubyCI RHEL 9 server, enabling FIPS in the non-FIPS OS environment. I am not sure why the default provider is loaded with fips provider in the FIPS case.

[jaruga@rhel9 ~]$ OPENSSL_FORCE_FIPS_MODE=1 openssl list -providers
Providers:
  base
    name: OpenSSL Base Provider
    version: 3.0.7
    status: active
  default
    name: OpenSSL Default Provider
    version: 3.0.7
    status: active
  fips
    name: Red Hat Enterprise Linux 9 - OpenSSL FIPS Provider
    version: 3.0.7-38f89367593abbfc
    status: active

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you want to review this part again?

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me!

I'm not sure why it behaves so in RHEL, but I think the most important bit is that the property query fips=yes is enabled. In that case, implementations from the default provider are never actually used even if loaded, so it probably is fine, but it seems distracting.

@rhenium
Copy link
Member

rhenium commented Nov 4, 2023

Sorry for the slow replies.

I have added some comments inline, but it looks good to me otherwise.

@junaruga
Copy link
Member Author

junaruga commented Nov 4, 2023

Sorry for the slow replies.

I have added some comments inline, but it looks good to me otherwise.

Thank you for your review! I plan to fix these items reviewed by you on next Monday.

@junaruga junaruga force-pushed the wip/doc-contributing branch from d74a360 to fafe1af Compare November 6, 2023 11:15
@junaruga
Copy link
Member Author

junaruga commented Nov 6, 2023

Thank you for your review! I plan to fix these items reviewed by you on next Monday.

Thank you for your review. I rebased with modifying the document. Please let me know when you are ok for the document.

…i skip]

Update testing section, adding debugging and FIPS use cases.
@junaruga junaruga force-pushed the wip/doc-contributing branch from 22b7513 to b492f6c Compare November 6, 2023 13:02
@rhenium rhenium merged commit fac5cac into ruby:master Nov 6, 2023
@rhenium
Copy link
Member

rhenium commented Nov 6, 2023

Thank you. It looks much better now!

@junaruga junaruga deleted the wip/doc-contributing branch November 7, 2023 10:27
@junaruga
Copy link
Member Author

junaruga commented Nov 7, 2023

Thank you too! I hope the document helps people to test and debug Ruby OpenSSL.

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.

2 participants