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

Backport 2.28: Re-enable 5 tests disabled because of an old OpenSSL bug #5990

Merged
merged 3 commits into from
Aug 16, 2022
Merged

Backport 2.28: Re-enable 5 tests disabled because of an old OpenSSL bug #5990

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7d0e658
Re-enable 7 tests disabled in mbedtls-2.28 because of an old OpenSSL bug
zhangsenWang Jun 29, 2022
1c981f5
skip test with openssl client because it will timeout with certain se…
zhangsenWang Aug 2, 2022
9d5b399
rebase with latest 2.28 branch
zhangsenWang Aug 16, 2022
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
36 changes: 15 additions & 21 deletions tests/ssl-opt.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9203,20 +9203,17 @@ run_test "DTLS fragmenting: 3d, gnutls client, DTLS 1.0" \
0 \
-s "fragmenting handshake message"

## Interop test with OpenSSL might trigger a bug in recent versions (including
## all versions installed on the CI machines), reported here:
## Bug report: https://github.com/openssl/openssl/issues/6902
## They should be re-enabled once a fixed version of OpenSSL is available
## (this should happen in some 1.1.1_ release according to the ticket).
skip_next_test
## The four tests below require 1.1.1a or higher version of openssl, otherwise
## it might trigger a bug due to openssl (https://github.com/openssl/openssl/issues/6902)
requires_openssl_next
requires_config_enabled MBEDTLS_SSL_PROTO_DTLS
requires_config_enabled MBEDTLS_RSA_C
requires_config_enabled MBEDTLS_ECDSA_C
client_needs_more_time 4
requires_max_content_len 2048
run_test "DTLS fragmenting: 3d, openssl server, DTLS 1.2" \
-p "$P_PXY drop=8 delay=8 duplicate=8" \
"$O_SRV -dtls1_2 -verify 10" \
"$O_NEXT_SRV -dtls1_2 -verify 10" \
"$P_CLI dtls=1 debug_level=2 \
crt_file=data_files/server8_int-ca2.crt \
key_file=data_files/server8.key \
Expand All @@ -9225,15 +9222,15 @@ run_test "DTLS fragmenting: 3d, openssl server, DTLS 1.2" \
-c "fragmenting handshake message" \
-C "error"

skip_next_test
requires_openssl_next
requires_config_enabled MBEDTLS_SSL_PROTO_DTLS
requires_config_enabled MBEDTLS_RSA_C
requires_config_enabled MBEDTLS_ECDSA_C
client_needs_more_time 4
requires_max_content_len 2048
run_test "DTLS fragmenting: 3d, openssl server, DTLS 1.0" \
-p "$P_PXY drop=8 delay=8 duplicate=8" \
"$O_SRV -dtls1 -verify 10" \
"$O_NEXT_SRV -dtls1 -verify 10" \
"$P_CLI dgram_packing=0 dtls=1 debug_level=2 \
crt_file=data_files/server8_int-ca2.crt \
key_file=data_files/server8.key \
Expand All @@ -9242,7 +9239,7 @@ run_test "DTLS fragmenting: 3d, openssl server, DTLS 1.0" \
-c "fragmenting handshake message" \
-C "error"

skip_next_test
requires_openssl_next
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding requires_openssl_next doesn't do anything on its own, except for skipping the test. requires_openssl_next should only be used when the test case calls $O_NEXT_CLI or $O_NEXT_SRV. (In fact ssl-opt.sh could be improved to detect calls to $O_NEXT_xxx and not have to explicitly call requires_openssl_next; same for many other requires_xxx.)

So now the test case is running with the default OpenSSL version, not the “next” version. And it's passing.

I think that the bug was only with an OpenSSL server, and this test case uses an OpenSSL client against an Mbed TLS client. So it was a mistake to disable it at the time. And now we should fix this by removing skip_next_test, not by adding a requirement on openssl_next.

Also applies to development.

requires_config_enabled MBEDTLS_SSL_PROTO_DTLS
requires_config_enabled MBEDTLS_RSA_C
requires_config_enabled MBEDTLS_ECDSA_C
Expand All @@ -9260,7 +9257,7 @@ run_test "DTLS fragmenting: 3d, openssl client, DTLS 1.2" \

# -nbio is added to prevent s_client from blocking in case of duplicated
# messages at the end of the handshake
skip_next_test
requires_openssl_next
requires_config_enabled MBEDTLS_SSL_PROTO_DTLS
requires_config_enabled MBEDTLS_RSA_C
requires_config_enabled MBEDTLS_ECDSA_C
Expand Down Expand Up @@ -10267,32 +10264,29 @@ run_test "DTLS proxy: 3d, min handshake, server-initiated renego, nbio" \
-s "Extra-header:" \
-c "HTTP/1.0 200 OK"

## Interop tests with OpenSSL might trigger a bug in recent versions (including
## all versions installed on the CI machines), reported here:
## Bug report: https://github.com/openssl/openssl/issues/6902
## They should be re-enabled once a fixed version of OpenSSL is available
## (this should happen in some 1.1.1_ release according to the ticket).
skip_next_test
## The three tests below require 1.1.1a or higher version of openssl, otherwise
## it might trigger a bug due to openssl (https://github.com/openssl/openssl/issues/6902)
requires_openssl_next
client_needs_more_time 6
not_with_valgrind # risk of non-mbedtls peer timing out
run_test "DTLS proxy: 3d, openssl server" \
-p "$P_PXY drop=5 delay=5 duplicate=5 protect_hvr=1" \
"$O_SRV -dtls1 -mtu 2048" \
"$O_NEXT_SRV -dtls1 -mtu 2048" \
"$P_CLI dgram_packing=0 dtls=1 hs_timeout=500-60000 tickets=0" \
0 \
-c "HTTP/1.0 200 OK"

skip_next_test # see above
requires_openssl_next
client_needs_more_time 8
not_with_valgrind # risk of non-mbedtls peer timing out
run_test "DTLS proxy: 3d, openssl server, fragmentation" \
-p "$P_PXY drop=5 delay=5 duplicate=5 protect_hvr=1" \
"$O_SRV -dtls1 -mtu 768" \
"$O_NEXT_SRV -dtls1 -mtu 768" \
"$P_CLI dgram_packing=0 dtls=1 hs_timeout=500-60000 tickets=0" \
0 \
-c "HTTP/1.0 200 OK"

skip_next_test # see above
requires_openssl_next
Copy link
Contributor

Choose a reason for hiding this comment

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

It only makes sense to call requires_openssl_next if the test case calls $O_NEXT_CLI or $O_NEXT_SRV. So please call $O_NEXT_SRV.

Why is the CI passing with the default OpenSSL version though? This test case is supposed to run into the OpenSSL bug. Maybe the OpenSSL bug is only triggered with certain seed values? In which case we should probably diversify this test case so that it exercises more.

Copy link
Contributor

Choose a reason for hiding this comment

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

Re “seed values” — in “3d” tests the UDP proxy pseudorandomly drops/duplicates/delays packets, based on the parameters passed to $P_PXY (which is programs/test/udp_proxy). The pseudorandomness uses rand(), with a seed that is the $SEED variable or --seed option passed to ssl-opt.sh.

The openssl bug was only triggered for a certain sequence of messages, so it's likely that it's only triggered for certain seed values. The CI always uses the same seed so that CI runs are easily reproducible, and I guess this seed value doesn't trigger the bug in this particular test case.

To confirm this guess, we should try running the current code with different seeds (hopefully the seed value in the old bug description should still work).

client_needs_more_time 8
not_with_valgrind # risk of non-mbedtls peer timing out
run_test "DTLS proxy: 3d, openssl server, fragmentation, nbio" \
Expand Down