Client renego refactoring #2598
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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()