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

[#1880] Support disabling features marked as legacy in OpenSSL 3.0 #1883

Merged
merged 3 commits into from
Jan 26, 2023

Conversation

andrey-utkin
Copy link
Contributor

@andrey-utkin andrey-utkin commented Jul 22, 2022

This changeset adds the ability to disable the features marked as legacy in OpenSSL 3.0: Blowfish, CAST5 and RIPEMD160.

It also adds a forgotten cmakedefine bit for ENABLE_IDEA as a separate commit.

Note that the optionality of CAST5 turned out to be problematic.
It has been achieved, but a lot of test coverage is bound to CAST5 as many tests use src/tests/data/keyrings/1/. These tests were partially or fully disabled. Nearly 66k lines worth of tests are under #ifdef ENABLE_CAST5. I still suggest to accept this changeset for now, and gradually update the existing tests. They should be less direct about telling which file they want to use, but explicit about which properties they rely on, if any. It's a lot of unglamorous work, and I think we don't need to do it in one go.

The many CI failures on RPM-based distros come from Ribose YUM repo key issue, which is unrelated.

@codecov
Copy link

codecov bot commented Jul 22, 2022

Codecov Report

Base: 81.94% // Head: 81.92% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (0fc2697) compared to base (0c13012).
Patch coverage: 63.33% of modified lines in pull request are covered.

❗ Current head 0fc2697 differs from pull request most recent head b2c7d49. Consider uploading reports for the commit b2c7d49 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1883      +/-   ##
==========================================
- Coverage   81.94%   81.92%   -0.02%     
==========================================
  Files         142      142              
  Lines       29246    29300      +54     
==========================================
+ Hits        23966    24005      +39     
- Misses       5280     5295      +15     
Impacted Files Coverage Δ
src/lib/crypto/backend_version.cpp 100.00% <ø> (ø)
src/lib/crypto/cipher_botan.cpp 77.52% <ø> (ø)
src/lib/crypto/symmetric.cpp 84.56% <ø> (ø)
src/tests/generatekey.cpp 87.33% <0.00%> (-0.38%) ⬇️
src/tests/support.h 100.00% <ø> (ø)
src/tests/ffi-enc.cpp 80.83% <28.57%> (-1.78%) ⬇️
src/tests/ffi.cpp 86.28% <57.14%> (-0.02%) ⬇️
src/tests/support.cpp 81.99% <80.00%> (-0.07%) ⬇️
src/tests/ffi-key.cpp 80.26% <80.95%> (+0.01%) ⬆️
src/lib/rnp.cpp 79.94% <100.00%> (+0.06%) ⬆️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@andrey-utkin andrey-utkin force-pushed the autkin-1880-ossl-legacy-disabling branch from e93752e to 03b9314 Compare July 25, 2022 16:39
@andrey-utkin andrey-utkin marked this pull request as ready for review July 25, 2022 16:39
@andrey-utkin
Copy link
Contributor Author

Please see the top message of this PR for details.

Additionally, I'd like to explain how I tested the changes I've been doing. I haven't rolled the code to build openssl in various configurations. We don't have such scripts in the repo and I didn't want to write them. I have, however, tested various Botan build configurations, leaning on the scripts we do have in this repo.

#!/bin/bash
set -e
set -x

export CFLAGS='-O0 -ggdb3' CXXFLAGS='-O0 -ggdb3'

for backend in botan; do # no openssl yet
  for blowfish in on off; do
    for cast128 in on off; do
      for ripemd160 in on off; do

        git restore ci/botan-modules
        [[ $blowfish == off ]]  && sed -i -e '/blowfish/d' ci/botan-modules
        [[ $cast128 == off ]]   && sed -i -e '/cast128/d' ci/botan-modules
        [[ $ripemd160 == off ]] && sed -i -e '/rmd160/d' ci/botan-modules

        git clean -dxf
        rm -rf /tmp/rnp*

        cat > run-this <<EOF
#!/bin/bash
set -e
set -x

dnf -y install gtest-devel

cp -a /rnp /rnp.container
cd /rnp.container
useradd rnpuser
chown -R root:rnpuser .
export USE_STATIC_DEPENDENCIES=yes
ci/install_noncacheable_dependencies.sh
ci/install_cacheable_dependencies.sh
. ci/env.inc.sh

# ci/main.sh:35
export LD_LIBRARY_PATH="\${GPG_INSTALL}/lib:\${BOTAN_INSTALL}/lib:\${JSONC_INSTALL}/lib:\${RNP_INSTALL}/lib:\$LD_LIBRARY_PATH"

cmake \
-DCMAKE_PREFIX_PATH="\${BOTAN_INSTALL};\${JSONC_INSTALL};\${GPG_INSTALL}" \
-DCMAKE_INTERPROCEDURAL_OPTIMIZATION=ON \
-DCMAKE_INSTALL_PREFIX=/usr \
-DCMAKE_INSTALL_LIBDIR=lib \
-DCMAKE_VERBOSE_MAKEFILE=ON \
-DBUILD_SHARED_LIBS=ON \
-DBUILD_TESTING=ON \
-DGIT_EXECUTABLE=/bin/false \
-DDOWNLOAD_GTEST=OFF \
-DDOWNLOAD_RUBYRNP=OFF \
-DCRYPTO_BACKEND=$backend \
-DENABLE_BLOWFISH=$blowfish \
-DENABLE_CAST128=$cast128 \
-DENABLE_RIPEMD160=$ripemd160 \
. || bash -i
make -j4 || bash -i

chmod -R g+w src/tests
chown -R rnpuser Testing
chmod -R a+rx /root # LOL
# original tweak to accomodate cli_tests.test_backend_version
export PATH="\${GPG_INSTALL}/bin:\${BOTAN_INSTALL}/bin:\$PATH"
sudo -u rnpuser "PATH=\$PATH"  ctest -j4 --output-on-failure || bash -i
EOF
        chmod a+x run-this

        docker run -v $PWD:/rnp -it fedora:35 /rnp/run-this
      done
    done
  done
done

@andrey-utkin andrey-utkin force-pushed the autkin-1880-ossl-legacy-disabling branch from 03b9314 to e3c7876 Compare August 24, 2022 13:34
Copy link
Contributor

@ronaldtse ronaldtse left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @andrey-utkin !

@andrey-utkin
Copy link
Contributor Author

@ni4 @antonsviridenko review please.

Copy link
Contributor

@ni4 ni4 left a comment

Choose a reason for hiding this comment

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

@andrey-utkin sorry, somehow missed this.
Here I see two major problems:

  • disabling certain cipher/hash should not limit library on loading/writing data. I.e. we still must be able to read packets/display information about it, but fail in cases when algorithm is attempted to be used (for symmetric ciphers - encrypt/decrypt data, for hash - calculate or verify signature which involves this hash)
  • we should not disable entire test suites, which are based on key which is encrypted with CAST5 algorithm. Instead just create additional copy of the same key, encrypted to other algorithm (unprotect/protect back/write to file).

@ni4 ni4 force-pushed the autkin-1880-ossl-legacy-disabling branch from e3c7876 to fc5752e Compare December 23, 2022 14:46
@ni4 ni4 force-pushed the autkin-1880-ossl-legacy-disabling branch from fc5752e to 5a0486a Compare January 12, 2023 13:14
@ni4 ni4 force-pushed the autkin-1880-ossl-legacy-disabling branch from 5a0486a to 2c0f4cb Compare January 23, 2023 15:38
@ni4 ni4 self-requested a review January 23, 2023 15:44
@ni4 ni4 self-requested a review January 23, 2023 16:58
@ronaldtse
Copy link
Contributor

@ni4 feel free to merge since you've completed it. Thank you!

@ni4
Copy link
Contributor

ni4 commented Jan 24, 2023

@ronaldtse Thanks. Let's just wait for #1974 to solve linting issue/make sure macOS 12 runner works fine.

andrey-utkin and others added 3 commits January 25, 2023 17:02
As these are put into "legacy" provider in OpenSSL 3.0, and are not
available by default and may completely disappear in future.

It suggests that while it's the simplest short-term way to build RNP to
not use algorithms marked as legacy in OpenSSL 3.0.
@ni4 ni4 force-pushed the autkin-1880-ossl-legacy-disabling branch from 2c0f4cb to b2c7d49 Compare January 25, 2023 15:02
@ni4
Copy link
Contributor

ni4 commented Jan 26, 2023

Okay, all green now - merging.

Copy link
Contributor

@ni4 ni4 left a comment

Choose a reason for hiding this comment

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

Now it's good to go :)

@ni4 ni4 merged commit 100d4f6 into master Jan 26, 2023
@ni4 ni4 deleted the autkin-1880-ossl-legacy-disabling branch January 26, 2023 11:04
@ni4 ni4 added this to the v0.17.0 milestone Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants