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

Use SHA256 for OCSP BasicResponse and Request #507

Merged
merged 2 commits into from
Apr 16, 2022

Conversation

jackorp
Copy link
Contributor

@jackorp jackorp commented Apr 12, 2022

SHA1 is not considered a safe cipher, and as such, it is by default disabled by a system-wide crypto policy on c9s and RHEL 9.

Calling the sign method of described classes on described OS raises an InvalidDigest exception.

This PR adjusts the behavior to use SHA256 by default instead of SHA1. Tests passed locally on this change.

An example of failing tests from the Ruby 3.1 test suite on such systems:

 13) Error:
OpenSSL::TestOCSP#test_basic_response_der:
OpenSSL::OCSP::OCSPError: invalid digest
    /builddir/build/BUILD/ruby-3.1.1/test/openssl/test_ocsp.rb:170:in `sign'
    /builddir/build/BUILD/ruby-3.1.1/test/openssl/test_ocsp.rb:170:in `test_basic_response_der'
 14) Error:
OpenSSL::TestOCSP#test_request_sign_verify:
OpenSSL::OCSP::OCSPError: invalid digest
    /builddir/build/BUILD/ruby-3.1.1/test/openssl/test_ocsp.rb:114:in `sign'
    /builddir/build/BUILD/ruby-3.1.1/test/openssl/test_ocsp.rb:114:in `test_request_sign_verify'
 15) Error:
OpenSSL::TestOCSP#test_basic_response_sign_verify:
OpenSSL::OCSP::OCSPError: invalid digest
    /builddir/build/BUILD/ruby-3.1.1/test/openssl/test_ocsp.rb:200:in `sign'
    /builddir/build/BUILD/ruby-3.1.1/test/openssl/test_ocsp.rb:200:in `test_basic_response_sign_verify'
 16) Error:
OpenSSL::TestOCSP#test_response_dup:
OpenSSL::OCSP::OCSPError: invalid digest
    /builddir/build/BUILD/ruby-3.1.1/test/openssl/test_ocsp.rb:298:in `sign'
    /builddir/build/BUILD/ruby-3.1.1/test/openssl/test_ocsp.rb:298:in `test_response_dup'
 17) Error:
OpenSSL::TestOCSP#test_request_is_signed:
OpenSSL::OCSP::OCSPError: invalid digest
    /builddir/build/BUILD/ruby-3.1.1/test/openssl/test_ocsp.rb:139:in `sign'
    /builddir/build/BUILD/ruby-3.1.1/test/openssl/test_ocsp.rb:139:in `test_request_is_signed'
 18) Error:
OpenSSL::TestOCSP#test_basic_response_dup:
OpenSSL::OCSP::OCSPError: invalid digest
    /builddir/build/BUILD/ruby-3.1.1/test/openssl/test_ocsp.rb:212:in `sign'
    /builddir/build/BUILD/ruby-3.1.1/test/openssl/test_ocsp.rb:212:in `test_basic_response_dup'
 19) Error:
OpenSSL::TestOCSP#test_request_der:
OpenSSL::OCSP::OCSPError: invalid digest
    /builddir/build/BUILD/ruby-3.1.1/test/openssl/test_ocsp.rb:99:in `sign'
    /builddir/build/BUILD/ruby-3.1.1/test/openssl/test_ocsp.rb:99:in `test_request_der'
 20) Error:
OpenSSL::TestOCSP#test_response:
OpenSSL::OCSP::OCSPError: invalid digest
    /builddir/build/BUILD/ruby-3.1.1/test/openssl/test_ocsp.rb:275:in `sign'
    /builddir/build/BUILD/ruby-3.1.1/test/openssl/test_ocsp.rb:275:in `test_response'
 21) Error:
OpenSSL::TestOCSP#test_response_der:
OpenSSL::OCSP::OCSPError: invalid digest
    /builddir/build/BUILD/ruby-3.1.1/test/openssl/test_ocsp.rb:286:in `sign'
    /builddir/build/BUILD/ruby-3.1.1/test/openssl/test_ocsp.rb:286:in `test_response_der'

@rhenium
Copy link
Member

rhenium commented Apr 12, 2022

it is by default disabled by a system-wide crypto policy on c9s and RHEL 9.

Does this mean we will have to update all test cases using SHA-1? ruby/openssl currently has quite a few of them.

@rhenium
Copy link
Member

rhenium commented Apr 12, 2022

We should rather give NULL as the hash algorithm to OCSP_*_sign() and avoid defining a default at all. That way OpenSSL will choose an algorithm depending on the key's type. Current versions of OpenSSL apparently use SHA-256 by default for RSA keys.

This will also fix signing OCSP requests/responses with an Ed25519/Ed448 key, which would currently end up with an error because PureEdDSA can't use SHA-1 or any other hash algorithm and specifically wants the NULL.

@jackorp
Copy link
Contributor Author

jackorp commented Apr 12, 2022

Does this mean we will have to update all test cases using SHA-1? ruby/openssl currently has quite a few of them.

Yes, that makes sense as the tests just fail [0] with SHA1. I have done a bit of work on this part and I can supply the necessary patches in a PR if that is acceptable. In most cases, substituting SHA1 with SHA256 seems to work OK in the context of Ruby build [1], and the tests pass.

[0] https://download.copr.fedorainfracloud.org/results/pvalena/ruby/centos-stream-9-x86_64/04083791-ruby/build.log.gz

[1] Log from the Ruby build where the patches are applied https://download.copr.fedorainfracloud.org/results/pvalena/ruby/centos-stream-9-x86_64/04083793-ruby/build.log.gz

For more details on this, you can see patches in c9s Ruby PR:
[2] test_pkey_rsa
[3] test_x509crl
[4] test_x509req
[5] https://gitlab.com/redhat/centos-stream/rpms/ruby/-/blob/f4910e89e43554916110ab3beb21bf62322131ef/ruby.spec#L952

Link [5] lines 952 to 970 contain substitutions that replace SHA1 with SHA256.

We should rather give NULL as the hash algorithm to OCSP_*_sign()

I'll change and test it in this PR and test with Ruby build and see if that works.

@jackorp
Copy link
Contributor Author

jackorp commented Apr 12, 2022

From simple builds and running this project's test suite, it seems that NULL as an argument works well.

@rhenium rhenium merged commit 553b328 into ruby:master Apr 16, 2022
@rhenium
Copy link
Member

rhenium commented Apr 16, 2022

Merged this PR. Thanks!

In most cases, substituting SHA1 with SHA256 seems to work OK in the context of Ruby build [1], and the tests pass.

Except for some DSA test cases (which should be disabled if not supported) I think nothing in ruby/openssl depends on SHA-1 specifically. We should do this.

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