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.

Sign up for GitHub

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

Jump to bottom

compat.sh: Skip static ECDH cases if unsupported in openssl #7137

Merged
merged 6 commits into from
Feb 28, 2023
Merged

compat.sh: Skip static ECDH cases if unsupported in openssl #7137

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1c0e4c0
compat.sh: skip static ECDH cases if unsupported in openssl
lpy4105 Feb 20, 2023
5e780df
Only use standard cipher name
lpy4105 Feb 21, 2023
a64c277
compat.sh: Skip all *ECDH_* ciphersuites
lpy4105 Feb 22, 2023
f01ac3a
Remove explicit ECDH exclusion for Travis CI
lpy4105 Feb 22, 2023
07d5085
Skip ECDH ciphersuites for O->m pair
lpy4105 Feb 22, 2023
9e7bb2a
Update some comments
lpy4105 Feb 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ jobs:
- tests/scripts/test_psa_constant_names.py
- tests/ssl-opt.sh
# Modern OpenSSL does not support fixed ECDH or null ciphers.
yanrayw marked this conversation as resolved.
Show resolved Hide resolved
- tests/compat.sh -p OpenSSL -e 'NULL\|ECDH_'
- tests/compat.sh -p OpenSSL -e 'NULL'
- tests/scripts/travis-log-failure.sh
# GnuTLS supports CAMELLIA but compat.sh doesn't properly enable it.
- tests/compat.sh -p GnuTLS -e 'CAMELLIA'
Expand Down
17 changes: 17 additions & 0 deletions tests/compat.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,16 @@ add_mbedtls_ciphersuites()
esac
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I know it wasn't a requirement of #1785, but since compat.sh is now skipping static ECDH cipher suites automatically, I think we should remove the explicit exclusions in .travis.yml and all.sh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No explicit exclusions found in all.sh, so I only removed that in .travis.yml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also found this doesn't work for a more recent OpenSSL (e.g 1.1.1f on Travis CI).

mbedtls/tests/compat.sh

Lines 823 to 824 in ffb92b0

if grep 'Cipher is (NONE)' $CLI_OUT >/dev/null; then
RESULT=1

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, the comment (which was added later by a different person) is wrong. Cipher is (NONE) indicates a null cipher, not a cipher that isn't supported, and that's still true at least in OpenSSL 1.1.1. So what this piece of code really does is to automatically mark null cipher suites as skipped if OpenSSL doesn't support them.

I don't think we need to change the code: it's working well enough. But before we forget, please fix the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I see. But Cipher is (NONE) is also printed if there is a invalid ciphersuite in old OpenSSL. So I misunderstood the code here. Let me update the comment.

# o_check_ciphersuite STANDARD_CIPHER_SUITE
yanrayw marked this conversation as resolved.
Show resolved Hide resolved
o_check_ciphersuite()
{
if [ "${O_SUPPORT_ECDH}" = "NO" ]; then
case "$1" in
*ECDH_*) SKIP_NEXT="YES"
esac
fi
}

setup_arguments()
{
O_MODE=""
Expand Down Expand Up @@ -603,6 +613,11 @@ setup_arguments()
;;
esac

case $($OPENSSL ciphers ALL) in
yanrayw marked this conversation as resolved.
Show resolved Hide resolved
*ECDH-ECDSA*) O_SUPPORT_ECDH="YES";;
*) O_SUPPORT_ECDH="NO";;
esac

if [ "X$VERIFY" = "XYES" ];
then
M_SERVER_ARGS="$M_SERVER_ARGS ca_file=data_files/test-ca_cat12.crt auth_mode=required"
Expand Down Expand Up @@ -1033,6 +1048,7 @@ for MODE in $MODES; do
start_server "OpenSSL"
translate_ciphers m $M_CIPHERS
for i in $ciphers; do
o_check_ciphersuite "${i%%=*}"
run_client mbedTLS ${i%%=*} ${i#*=}
done
stop_server
Expand All @@ -1042,6 +1058,7 @@ for MODE in $MODES; do
start_server "mbedTLS"
translate_ciphers o $O_CIPHERS
for i in $ciphers; do
o_check_ciphersuite "${i%%=*}"
run_client OpenSSL ${i%%=*} ${i#*=}
done
stop_server
Expand Down