s_server: Do not use SSL_sendfile when KTLS is not being used #17788
Conversation
Fix a bug in `openssl s_server -WWW` where it would attempt to invoke `SSL_sendfile` if `-ktls -sendfile` was passed on the command line, even if KTLS has not actually been enabled, for example because it is not supported by the host. Since `SSL_sendfile` is only supported when KTLS is actually being used, this resulted in a failure to serve requests. Fixes openssl#17503.
hlandau
added
branch: master
There was a problem hiding this comment.
Taking advice on how to test this, as most usage of s_server in the test suite seems to be incidental via TLSProxy rather than testing of s_server itself.
Yeah. We've actually been quite poor about testing for the apps. The TLSProxy tests are really about testing the library and only incidentally use s_server (and in fact I would like to rewrite them at some point to not have that dependency). We recently agreed to a new testing policy (https://github.com/openssl/technical-policies/blob/master/policies/testing.md) which mandates that we should add tests when fixing bugs in the applications - so we really do need to figure out a test for this.
We are probably going to need a new test recipe that basically starts "s_server" using sendfile and confirms that "s_client" can connect to it. This seems quite hard...but a nettle we are going to need to grasp sometime soon.
There is a "get out clause" in the test policy which says we don't have to write tests "where writing the test would result in disproportionately more effort than writing the code being tested"
|
The testing issue relates to #17267. |
|
Updated to add a warning when sendfile was requested but a request was served without it. |
IMO this is well within this clause. Writing a completely new test recipe that would not be trivial for this few line fix would be disproportionately more effort. However! We should at least create an issue to add tests for s_server/s_client apps. |
|
Updated. |
t8m
removed
the
approval: otc review pending
There was a problem hiding this comment.
@t8m - I added the 3.0 branch tag for this since this is a bug fix. Do you agree with the backport?
mattcaswell
added
approval: done
|
Yes, OK for 3.0 as well. |
|
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
mattcaswell
added
approval: ready to merge
|
Pushed to master and 3.0 |
Fix a bug in `openssl s_server -WWW` where it would attempt to invoke `SSL_sendfile` if `-ktls -sendfile` was passed on the command line, even if KTLS has not actually been enabled, for example because it is not supported by the host. Since `SSL_sendfile` is only supported when KTLS is actually being used, this resulted in a failure to serve requests. Fixes #17503. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #17788) (cherry picked from commit aea68b0)
Fix a bug in `openssl s_server -WWW` where it would attempt to invoke `SSL_sendfile` if `-ktls -sendfile` was passed on the command line, even if KTLS has not actually been enabled, for example because it is not supported by the host. Since `SSL_sendfile` is only supported when KTLS is actually being used, this resulted in a failure to serve requests. Fixes #17503. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #17788)
Fix a bug in
openssl s_server -WWWwhere it would attempt to invokeSSL_sendfileif-ktls -sendfilewas passed on the command line, even if KTLS has not actually been enabled, for example because it is not supported by the host. SinceSSL_sendfileis only supported when KTLS is actually being used, this resulted in a failure to serve requests.Fixes #17503.
Taking advice on how to test this, as most usage of
s_serverin the test suite seems to be incidental via TLSProxy rather than testing ofs_serveritself.