Backport 2.28: Re-enable 5 tests disabled because of an old OpenSSL bug #5990
Conversation
zhangsenWang
force-pushed
the
mbedtls-2.28
branch
from
f6b0417 to
ce02537
Compare
Signed-off-by: Zhangsen Wang <zhangsen.wang@arm.com>
minosgalanakis
added
bug
size-s
gilles-peskine-arm
changed the title
There was a problem hiding this comment.
This patch enables two test cases that were documented as failing, and they're passing. We need to understand why.
I think I know what's going on with the test with an openssl client: the bug is only on the server side, and this test case should not have been disabled since it doesn't use an openssl server. For the other one, which does use an openssl server, I have a conjecture, which is that only certain seed values trigger the bug. We need to check whether this is the case.
| "$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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
tests/ssl-opt.sh
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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.
…ed due to an openssl bug Signed-off-by: Zhangsen Wang <zhangsen.wang@arm.com>
zhangsenWang
changed the title
zhangsenWang
added
needs-review
zhangsenWang
added
approved
xkqian
added
the
needs-review
Description
Re-enable 5 tests disabled because of an old OpenSSL bug (#5077)
Backport of #5985
Status
READY
Steps to test or reproduce
with openssl 1.1.1a or higher version
export OPENSSL_NEXT=/path/to/openssl./tests/ssl-opt.sh -f "DTLS fragmenting: 3d, openssl"./tests/ssl-opt.sh -f "DTLS proxy: 3d, openssl"