ossl_pkey.c: Workaround: Decode with non-zero selections.

[junaruga]

This PR has 3 commits.

The 1st commit is to remove the pending method pend_on_openssl_issue_21493. The 2nd commit is the same with this PR #668. I will rebase this PR after the PR #668 is merged. It's needed by the 3rd commit. And the 3rd commit is a workaround for the issue openssl/openssl#21519, in the cases of the affected OpenSSL release versions.

I was thinking about my previous PR #664 to pend the related tests. And failing OpenSSL::PKey.read(der_encoded_string) with selection 0 is quite basic use case, and the issue may happen in other test files too. So, I thought a workaround is better than pending tests. Note the commit openssl/openssl@2acb0d3 is the essential fix for this issue.

I believe that the idea is simple. Manually try non-zero selections EVP_PKEY_KEY_PARAMETERS, EVP_PKEY_PUBLIC_KEY and EVP_PKEY_KEYPAIR in order, instead of specifying selection 0 (pick up whatever you can) option in the cases of the affected OpenSSL versions, and the provider number is more than equal 2. Because this issue can happen when a decoding provider is different from encrypt management provider.

Here are some notes.

• I cared a backward compatibility. It should be (almost) same in the 1 provider (typical OpenSSL 3 cases) or 0 providers (OpenSSL before 3 or LibreSSL cases).
• I am not sure if the naming of the ossl_count_provider_number function is the best. Please review it.
• I am not sure if the logic to allocate the selections dynamically is the best way considering this repository or Ruby's coding style. Please let me know if there is a better way.

Thanks.

[rhenium]

The workaround seems straightforward. Do you expect any difference in the behavior with explicitly specifying selections compared to using the "0" selection? I'm wondering if we can just continue to use the workaround code on all OpenSSL >= 3.0 versions even with the bugfix.

Unless it's necessary and we are sure it's safe to do, I prefer not to rely on the version number in the library code. We know distros often backport individual patches to OpenSSL without updating version numbers.

[junaruga]

The workaround seems straightforward. Do you expect any difference in the behavior with explicitly specifying selections compared to using the "0" selection? I'm wondering if we can just continue to use the workaround code on all OpenSSL >= 3.0 versions even with the bugfix.

Unless it's necessary and we are sure it's safe to do, I prefer not to rely on the version number in the library code. We know distros often backport individual patches to OpenSSL without updating version numbers.

All right. I can understand your point. My concern for using non-zero selections was a performance in the case trying to call the OSSL_DECODER_CTX_new_for_pkey more. However, when doing the benchmark with the script below (that is a minimal reproducer of test_compare?error) between the current PR's logic (with selection zero) and another logic (with non-zero selections) with the latest OpenSSL 3.2, the non-zero selections case was actually faster. So, I think we can safely update with the non-zero selections way, as you suggested.

$ cat ~/script/ruby_openssl_benchmark.sh
#!/bin/bash

set -eu

for n in $(seq 10); do
  OPENSSL_CONF=/home/jaruga/.local/openssl-3.2.0-dev-fips-debug-cf712830b7/ssl/openssl_fips.cnf \
    ruby -I./lib -ropenssl -e 'OpenSSL::PKey.read("0\x81\x9F0\r\x06\t*\x86H\x86\xF7\r\x01\x01\x01\x05\x00\x03\x81\x8D\x000\x81\x89\x02\x81\x81\x00\xCB\xC2\xC4\xB0\xD4@\xA7>\xD4\xFE>C\xA0\x1E\x17\x06\x03\xBDg\xC0-\xBF\x9C\xBF9T\x11\xA7F\xA0\xF1:\xA8\xD5\x87\xB0\xB1h\xA3\xC4E\x81\xEC\x93\x80O\nA7n\xBBS\x84\xF5\x9C\xF6H\xC7\x11\x04;\xB9\xFFX\xD6\xB6\xC2\xCFIZ\xC8\xDA\x87\xCB,\x10\x11R\xC5\x9A\x9D\\\xA4\x8B\x7FCx\x1E.\xFF\x19\x0F\xDAb\x86\x8C\n$<\x8C\x0E#z\x02\xB6\x14\x99\x973\xBDn=\xEF\xA3\x14\xDF\xE9y\xE0N\xA5\x17\xF2_\x14E9\x87\x02\x03\x01\x00\x01")'
  echo "."
done

echo OK

With selection 0.

$ time ~/script/ruby_openssl_benchmark.sh
... 
real  0m2.433s
user  0m2.261s
sys 0m0.153s

With non-zero selections.

$ time ~/script/ruby_openssl_benchmark.sh
...
real  0m2.354s
user  0m2.192s
sys 0m0.152s

I will rebase this PR on your way.

[junaruga]

I will rebase this PR on your way.

Rebased this PR!

[rhenium]

The workaround seems straightforward. Do you expect any difference in the behavior with explicitly specifying selections compared to using the "0" selection? I'm wondering if we can just continue to use the workaround code on all OpenSSL >= 3.0 versions even with the bugfix.
Unless it's necessary and we are sure it's safe to do, I prefer not to rely on the version number in the library code. We know distros often backport individual patches to OpenSSL without updating version numbers.

All right. I can understand your point. My concern for using non-zero selections was a performance in the case trying to call the OSSL_DECODER_CTX_new_for_pkey more. However, when doing the benchmark with the script below (that is a minimal reproducer of test_compare?error) between the current PR's logic (with selection zero) and another logic (with non-zero selections) with the latest OpenSSL 3.2, the non-zero selections case was actually faster. So, I think we can safely update with the non-zero selections way, as you suggested.

$ cat ~/script/ruby_openssl_benchmark.sh
#!/bin/bash

set -eu

for n in $(seq 10); do
  OPENSSL_CONF=/home/jaruga/.local/openssl-3.2.0-dev-fips-debug-cf712830b7/ssl/openssl_fips.cnf \
    ruby -I./lib -ropenssl -e 'OpenSSL::PKey.read("0\x81\x9F0\r\x06\t*\x86H\x86\xF7\r\x01\x01\x01\x05\x00\x03\x81\x8D\x000\x81\x89\x02\x81\x81\x00\xCB\xC2\xC4\xB0\xD4@\xA7>\xD4\xFE>C\xA0\x1E\x17\x06\x03\xBDg\xC0-\xBF\x9C\xBF9T\x11\xA7F\xA0\xF1:\xA8\xD5\x87\xB0\xB1h\xA3\xC4E\x81\xEC\x93\x80O\nA7n\xBBS\x84\xF5\x9C\xF6H\xC7\x11\x04;\xB9\xFFX\xD6\xB6\xC2\xCFIZ\xC8\xDA\x87\xCB,\x10\x11R\xC5\x9A\x9D\\\xA4\x8B\x7FCx\x1E.\xFF\x19\x0F\xDAb\x86\x8C\n$<\x8C\x0E#z\x02\xB6\x14\x99\x973\xBDn=\xEF\xA3\x14\xDF\xE9y\xE0N\xA5\x17\xF2_\x14E9\x87\x02\x03\x01\x00\x01")'
  echo "."
done

echo OK

I suspect this example is mostly measuring the time spent for Ruby and OpenSSL's initialization, but I don't think performance is a big concern here. I wanted to know if this doesn't change the decoding result. The test cases are successful, so I guess it's OK.

[junaruga]

I suspect this example is mostly measuring the time spent for Ruby and OpenSSL's initialization, but I don't think performance is a big concern here. I wanted to know if this doesn't change the decoding result. The test cases are successful, so I guess it's OK.

Ah I see. And yeah, the test cases are successful.

[junaruga]

I am rebasing by fixing the mentioned items. Could you review again? Thanks.

[junaruga]

I added the (EVP_PKEY_KEYPAIR) here to explain why we need to check the EVP_PKEY_KEYPAIR case first.

[rhenium]

Tests passed successfully on all versions. I think this is good to merge!

[junaruga]

Thanks for reviewing!