rnp icon indicating copy to clipboard operation
rnp copied to clipboard

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

Open andrey-utkin opened this issue 2 years ago • 3 comments

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.

andrey-utkin avatar Jul 22 '22 16:07 andrey-utkin

Codecov Report

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

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

:exclamation: 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%) :arrow_down:
src/tests/support.h 100.00% <ø> (ø)
src/tests/ffi-enc.cpp 80.83% <28.57%> (-1.78%) :arrow_down:
src/tests/ffi.cpp 86.28% <57.14%> (-0.02%) :arrow_down:
src/tests/support.cpp 81.99% <80.00%> (-0.07%) :arrow_down:
src/tests/ffi-key.cpp 80.26% <80.95%> (+0.01%) :arrow_up:
src/lib/rnp.cpp 79.94% <100.00%> (+0.06%) :arrow_up:
... 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.

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

codecov[bot] avatar Jul 22 '22 17:07 codecov[bot]

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 avatar Jul 25 '22 17:07 andrey-utkin

@ni4 @antonsviridenko review please.

andrey-utkin avatar Sep 01 '22 11:09 andrey-utkin

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

ronaldtse avatar Jan 24 '23 03:01 ronaldtse

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

ni4 avatar Jan 24 '23 10:01 ni4

Okay, all green now - merging.

ni4 avatar Jan 26 '23 11:01 ni4