Fix OpenSSL::PKey.read that cannot parse PKey in the FIPS mode. #615

junaruga · 2023-04-17T17:44:24Z

This PR is to fix the following issue that is managed as one of the issues at #603 on the FIPS mode. This PR is based on the PR #608. So, please consider reviewing and merging the #608 first. Then I can rebase this PR on the latest master branch.

$ openssl genrsa -out key.pem 4096

$ ruby -e "require 'openssl'; OpenSSL::PKey.read(File.read('key.pem'))"
-e:1:in `read': Could not parse PKey (OpenSSL::PKey::PKeyError)
  from -e:1:in `<main>'

There are some commits on this PR. The first 3 commits are basically from the PR #603. Then the 4th commit is the main commit to fix this issue. The patch is the same with what I mentioned at #603 (comment). The 4th and 5th commits are to run the unit test test/openssl/test_pkey.rb.

https://github.com/junaruga/openssl/actions/runs/4681990779/jobs/8295272065#step:11:732

Before the main 4rd commit, the test_generic_oid_inspect and test_to_text was failing in the test/openssl/test_pkey.rb. So, running test/openssl/test_pkey.rb can test that this issue was fixed.

Note that the 3 tests test_ed25519, test_x25519 and test_compare? in the test/openssl/test_pkey.rb are still failing due to other issues. And I am working on it at the #603 (comment).

I confirmed that CI passed on my forked repository.
https://github.com/junaruga/openssl/actions/runs/4724029713/jobs/8380737782#step:12:64

junaruga · 2023-05-31T17:50:33Z

Rebased on the latest master branch.

junaruga · 2023-05-31T17:58:27Z

Tomorrow I manually will check if the new compiler warnings are printed or not on each case in the CI.

… mode. This commit is a workaround to avoid the error below that the `OpenSSL::PKey.read` fails with the OpenSSL 3.0 FIPS mode. ``` $ openssl genrsa -out key.pem 4096 $ ruby -e "require 'openssl'; OpenSSL::PKey.read(File.read('key.pem'))" -e:1:in `read': Could not parse PKey (OpenSSL::PKey::PKeyError) from -e:1:in `<main>' ``` The root cause is on the OpenSSL side. The `OSSL_DECODER_CTX_set_selection` doesn't apply the selection value properly if there are multiple providers, and a provider (e.g. "base" provider) handles the decoder implementation, and another provider (e.g. "fips" provider) handles the keys. The workaround is to create `OSSL_DECODER_CTX` variable each time without using the `OSSL_DECODER_CTX_set_selection`.

We want to run the unit tests in the FIPS mode too.

It's to test the `OpenSSL::PKey.read` in the `test/openssl/test_pkey.rb`. I added the pending status to the following tests failing on the FIPS mode case in the `test/openssl/test_pkey.rb`. * `test_ed25519` * `test_x25519` * `test_compare?`

junaruga · 2023-06-01T14:18:19Z

Tomorrow I manually will check if the new compiler warnings are printed or not on each case in the CI.

Manually checking the compiler warnings on the CI is very hard. First, I identified what CI cases I need to check on this PR's CI. To find the cases, I executed this PR and master branch's CIs with one commit #631 to check the compiler warnings.

diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index becf990..e553d06 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -19,6 +19,7 @@ jobs:
         # ubuntu-22.04 uses OpenSSL 3.0, ubuntu-20.04 uses OpenSSL 1.1.1
         os: [ ubuntu-22.04, ubuntu-20.04, macos-latest, windows-latest ]
         ruby: ${{ fromJson(needs.ruby-versions.outputs.versions) }}
+        fail_by_warn: [ true ]
         exclude:
           # uses non-standard MSYS2 OpenSSL 3 package
           - { os: windows-latest, ruby: head }
@@ -47,6 +48,10 @@ jobs:
         run:  echo "MAKEFLAGS=V=1" >> $GITHUB_ENV
         if: runner.os == 'Linux' || runner.os == 'macOS'
 
+      - name: set flags to fail by the compiler warnings.
+        run:  echo "RUBY_OPENSSL_EXTCFLAGS=-Werror" >> $GITHUB_ENV
+        if: matrix.fail_by_warn
+
       - name: compile
         run:  rake compile -- --enable-debug
 
@@ -75,6 +80,7 @@ jobs:
           - libressl-3.5.3
           - libressl-3.6.1
           - libressl-3.7.0 # Development release
+        fail_by_warn: [ true ]
         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' }
@@ -138,6 +144,10 @@ jobs:
         run:  echo "MAKEFLAGS=V=1" >> $GITHUB_ENV
         if: runner.os == 'Linux' || runner.os == 'macOS'
 
+      - name: set flags to fail by the compiler warnings.
+        run:  echo "RUBY_OPENSSL_EXTCFLAGS=-Werror" >> $GITHUB_ENV
+        if: matrix.fail_by_warn
+
       - name: compile
         run:  rake compile -- --enable-debug --with-openssl-dir=$HOME/.openssl/${{ matrix.openssl }}

The CI result for this PR + a commit of checking the compiler warnings
https://github.com/junaruga/openssl/actions/runs/5144488890

The CI result for the master branch (8aee87305d6ef7b353e682e5dff9180b047948bf) + a commit of checking the compiler warnings
https://github.com/ruby/openssl/actions/runs/5142916181?pr=631

ubuntu-22.04 2.6
ubuntu-22.04 3.1
ubuntu-22.04 3.2
ubuntu-22.04 head
ubuntu-22.04 truffleruby-head
ubuntu-20.04 2.6
ubuntu-20.04 truffleruby-head
macos-latest 2.6
macos-latest truffleruby-head

Then I will be checking if there are the compiler warnings in the cases in this PR's CI.

junaruga · 2023-06-01T14:50:50Z

Then I will be checking if there are the compiler warnings in the cases in this PR's CI.

I checked it manually. And I confirmed there are no new compiler warnings by this PR.

ubuntu-22.04 2.6: There are warnings. But these are not new by this PR.

 ../../../../ext/openssl/ossl_ssl.c: In function ‘ossl_start_ssl’:
../../../../ext/openssl/ossl_ssl.c:1761:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
 1761 |     VALUE io = rb_attr_get(self, id_i_io);
      |     ^~~~~
../../../../ext/openssl/ossl_ssl.c: In function ‘ossl_ssl_read_internal’:
../../../../ext/openssl/ossl_ssl.c:1948:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
 1948 |     VALUE io = rb_attr_get(self, id_i_io);
      |     ^~~~~
../../../../ext/openssl/ossl_ssl.c: In function ‘ossl_ssl_write_internal’:
../../../../ext/openssl/ossl_ssl.c:2049:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
 2049 |     VALUE io = rb_attr_get(self, id_i_io);
      |     ^~~~~

ubuntu-22.04 3.1: No warnings.
ubuntu-22.04 3.2: No warnings.
ubuntu-22.04 head: No warnings.

ubuntu-22.04 truffleruby-head: There is a warning. But it is not new by this PR.

../../../../ext/openssl/ossl_pkey.c:342:22: warning: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Wsingle-bit-bitfield-constant-conversion]
    arg->interrupted = 1;
                     ^ ~
1 warning generated.

ubuntu-20.04 2.6: There are warnings. But these are not new by this PR.

../../../../ext/openssl/ossl_ssl.c: In function ‘ossl_start_ssl’:
../../../../ext/openssl/ossl_ssl.c:1761:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
 1761 |     VALUE io = rb_attr_get(self, id_i_io);
      |     ^~~~~
../../../../ext/openssl/ossl_ssl.c: In function ‘ossl_ssl_read_internal’:
../../../../ext/openssl/ossl_ssl.c:1948:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
 1948 |     VALUE io = rb_attr_get(self, id_i_io);
      |     ^~~~~
../../../../ext/openssl/ossl_ssl.c: In function ‘ossl_ssl_write_internal’:
../../../../ext/openssl/ossl_ssl.c:2049:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
 2049 |     VALUE io = rb_attr_get(self, id_i_io);
      |     ^~~~~

ubuntu-20.04 truffleruby-head: There is a warning. But it is not new by this PR.

../../../../ext/openssl/ossl_pkey.c:342:22: warning: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Wsingle-bit-bitfield-constant-conversion]
    arg->interrupted = 1;
                     ^ ~

macos-latest 2.6: There are many warnings and notes for several .c files. And checking only the warnings for openssl/ossl_pkey.c that is modified in this PR, the links is https://github.com/ruby/openssl/actions/runs/5143196869/jobs/9257919215?pr=615#step:6:1697 and there are 4 warnings by the -Wcompound-token-split-by-macro below. The result is same with the current master branch's CI result https://github.com/ruby/openssl/actions/runs/5145354568/jobs/9262893433#step:6:1699.

clang -I. -I/Users/runner/hostedtoolcache/Ruby/2.6.10/x64/include/ruby-2.6.0/x86_64-darwin19 -I/Users/runner/hostedtoolcache/Ruby/2.6.10/x64/include/ruby-2.6.0/ruby/backward -I/Users/runner/hostedtoolcache/Ruby/2.6.10/x64/include/ruby-2.6.0 -I../../../../ext/openssl -DRUBY_EXTCONF_H=\"extconf.h\" -I/Users/runner/hostedtoolcache/Ruby/2.6.10/x64/openssl/include -I/Users/runner/hostedtoolcache/Ruby/2.6.10/x64/include -DENABLE_PATH_CHECK=0 -D_XOPEN_SOURCE -D_DARWIN_C_SOURCE -D_DARWIN_UNLIMITED_SELECT -D_REENTRANT   -fno-common -O3 -ggdb3 -Wall -Wextra -Wdeclaration-after-statement -Wdeprecated-declarations -Wdivision-by-zero -Wimplicit-function-declaration -Wimplicit-int -Wmisleading-indentation -Wpointer-arith -Wshorten-64-to-32 -Wwrite-strings -Wmissing-noreturn -Wno-constant-logical-operand -Wno-long-long -Wno-missing-field-initializers -Wno-overlength-strings -Wno-parentheses-equality -Wno-self-assign -Wno-tautological-compare -Wno-unused-parameter -Wno-unused-value -Wunused-variable -Wextra-tokens  -fno-common -pipe  -o ossl_pkey.o -c ../../../../ext/openssl/ossl_pkey.c
../../../../ext/openssl/ossl_pkey.c:260:28: warning: '(' and '{' tokens introducing statement expression appear in different macro expansion contexts [-Wcompound-token-split-by-macro]
    rb_block_call(args[1], rb_intern("each"), 0, NULL,
                           ^~~~~~~~~~~~~~~~~
...<snip>...
../../../../ext/openssl/ossl_pkey.c:260:28: warning: '}' and ')' tokens terminating statement expression appear in different macro expansion contexts [-Wcompound-token-split-by-macro]
    rb_block_call(args[1], rb_intern("each"), 0, NULL,
                           ^~~~~~~~~~~~~~~~~
...<snip>...
../../../../ext/openssl/ossl_pkey.c:561:9: warning: '(' and '{' tokens introducing statement expression appear in different macro expansion contexts [-Wcompound-token-split-by-macro]
    if (OSSL_PKEY_IS_PRIVATE(obj))
        ^~~~~~~~~~~~~~~~~~~~~~~~~
...<snip>...
../../../../ext/openssl/ossl_pkey.c:561:9: warning: '}' and ')' tokens terminating statement expression appear in different macro expansion contexts [-Wcompound-token-split-by-macro]
    if (OSSL_PKEY_IS_PRIVATE(obj))
        ^~~~~~~~~~~~~~~~~~~~~~~~~
...<snip>...
4 warnings generated.

macos-latest truffleruby-head: There is a warning. But it is not new by this PR.

../../../../ext/openssl/ossl_pkey.c:342:22: warning: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Wsingle-bit-bitfield-constant-conversion]
    arg->interrupted = 1;
                     ^ ~

junaruga requested a review from rhenium April 17, 2023 20:01

junaruga mentioned this pull request Apr 18, 2023

Support OpenSSL 3 FIPS #603

Open

junaruga force-pushed the wip/fips-read branch from 9526934 to f85e7ca Compare May 15, 2023 12:45

junaruga force-pushed the wip/fips-read branch from f85e7ca to c78bfa3 Compare May 31, 2023 17:50

junaruga added 3 commits June 1, 2023 12:17

Drop a common logic disabling the FIPS mode in the tests.

ab92baf

We want to run the unit tests in the FIPS mode too.

junaruga force-pushed the wip/fips-read branch from c78bfa3 to 8149cdf Compare June 1, 2023 10:24

junaruga merged commit 7e411b4 into ruby:master Jun 1, 2023

junaruga deleted the wip/fips-read branch June 1, 2023 14:54

rhenium mentioned this pull request Jun 2, 2023

CI: Check compiler warnings. #631

Merged

Esaron mentioned this pull request Sep 2, 2023

openssl 3: Can't construct RSA keys from any available serializable key data #679

Closed

tpowell-progress mentioned this pull request May 21, 2024

FIPS broken for Ruby on Windows with OpenSSL 3.x chef/chef#14389

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix OpenSSL::PKey.read that cannot parse PKey in the FIPS mode. #615

Fix OpenSSL::PKey.read that cannot parse PKey in the FIPS mode. #615

junaruga commented Apr 17, 2023 •

edited

Loading

junaruga commented May 31, 2023

junaruga commented May 31, 2023

junaruga commented Jun 1, 2023

junaruga commented Jun 1, 2023 •

edited

Loading

Fix OpenSSL::PKey.read that cannot parse PKey in the FIPS mode. #615

Fix OpenSSL::PKey.read that cannot parse PKey in the FIPS mode. #615

Conversation

junaruga commented Apr 17, 2023 • edited Loading

junaruga commented May 31, 2023

junaruga commented May 31, 2023

junaruga commented Jun 1, 2023

junaruga commented Jun 1, 2023 • edited Loading

junaruga commented Apr 17, 2023 •

edited

Loading

junaruga commented Jun 1, 2023 •

edited

Loading