Client renego refactoring #2598
Conversation
All cases could be handled by the single openssl s_client invocation loop: - dispatch and adjust comments to not loose them - remove the first s_client invocation: stuck connections are allready handled by the main loop - remove the second s_client invocation: normal case and server closed connections are allready handled by the main loop. The loop take care of the race between server connection close and s_client terminating too by doing another loop run, not closing STDIN. - special non HTTP case equivalent to ssl_reneg_attempts=2 - specialcase only the HTTP result printing to not change the output - openssl-timeout option clashe badly with the main loop logic: Introduce $OPENSSL_NOTIMEOUT
In the wait loop, I was relying on a 1s sleep to eliminate a possible late zero return value server close on the last attempt. - do globaly one more harmless "for" iteration and remove the sleep 1 for faster and more robust result - correct the non HTTP case iteration value - adjust the timeout to the conservative 6s in the non HTTP case, for HTTP case it become 33s - improve comments
- Recover the "not vulnerable" case (no mitigation) printing, cosmetic fix. - With the removing of all s_client invocation other than the main loop one, fix the init of the ERRFILE and TMPFILE: no need to append, no need to remove, inconditionally zap the content before the loop.
|
I don't understand the CI failed tests. The "broken pipe" error from subshells should not be propagated and affect the main shell script return code and does not in "normal" run. Does the CI test modifying the shell 'pipefail' 'errtrace' or 'errexit' options ? |
|
Haven´t really looked at it . Just re-ran the test once. I also don´t know what's going on. I would recommend to checkout and run the script from the "home dir" |
|
Is #2597 obsolete? |
|
Locally, all CI falling tests are OK: |
|
Sorry for the delayed response. Stuck in a project and other things shed their light ahead. It also works for me on my computers. However if you look more closely I am afraid the problem is not only the CI check. Please have a look @ https://github.com/drwetter/testssl.sh/actions/runs/11722187436/job/32988029778?pr=2598 and search for 17204. Maybe that line is exhausting the resources of the CI containers. Also when this is addressed we would need to test this against vulnerable hosts. Probably an old openssl version could help started with |
|
No, the broken pipes are perfectly "normal" but should not be reported as I said previously : #2598 (comment) Run prove -v |
|
My point being: the broken pipes are not in the one test which failed, they are in a lot of tests. We don't check for those errors yet. As this makes the CI runs useless I cannot merge this as it is now. You can submit a PR upgrading the CI test to Ubuntu 22.04 but it also needs to run under normal conditions for the use on 20.04 (haven´t check yet). I haven´t understood yet why that message occurs. My remark wrt "exhausting the resources" was just a guess. I would feel more comfortable if someone can confirm this. |
|
The '-e'/'set -e' bash modifier is for me responsible for the breakage. I extensively use sub-shell and execution pipes in my code, with expected pipe break. |
👍 |
testssl.sh worked as expected. Under the hood, broken pipes are expected as part of the fast loop exit strategy that relies as little as possible on timeout detection. But under the CI, testssl.sh output is garbled by the subshells stderr outputs, catched for some reason by 'prove -v'. Simply redirecting the stderr output of the offending command to /dev/null fixes the problem.
|
Ok CI tests breakage fixed. |
|
Thanks @Tazmaniac ! I am a little bit swamped with things. Just had a short look: What is |
|
It was introduced in the commit dab177f. Current version is affected by the bad interaction/bug too. |
|
Would this be ok for you ? |
Describe your changes
To correctly fix #2590 and potentially other reals corner cases, a complete refactoring of the test is needed.
It remove all the calls to openssl s_client other than the one in the big events driven testing loop.
The result is more robust, faster and cleaner.
It fixes a bad interaction bug with the --openssl-timeout option too.
As a side note, Akamai started to implement a mitigation (before renegotiation could only be enabled or disabled) . It close the connection after a completely unpredictable number of renegotiation (up to more than 30 seen) from the client point of view. So it could lead to potentially "false positive" (well it depend of your point of view...).
I will try later to reduce this noise, but this is not a regression.
What is your pull request about?
If it's a code change please check the boxes which are applicable
help()