Ssl op allow eof #11735
Ssl op allow eof #11735
Conversation
|
Oh...there probably should be a CHANGES entry for this |
Done. Please reapprove. |
beldmit
added
approval: done
|
Looks like the travis failure is relevant |
|
Fixed and Travis is happy. @mattcaswell could you please reconfirm? |
|
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. |
beldmit
added
approval: otc review pending
and removed
approval: done
|
Ping @openssl/otc |
mattcaswell
added
approval: done
|
I wonder if it's not more useful to have an option that instead gives a different distinct error message. I think the "SSL_ERROR_SYSCALL with errno=0" really doesn't tell much, and isn't something we want as an API that people need to check. If this is actually the API we do want, I think that special case should get documented in the SSL_get_error() manpage, and maybe also the SSL_shutdown() manpage. I'm also wondering if we should backport this to the 1.1.1 branch. If we go with the current patch, it would just be a define and documentation for 1.1.1. |
kroeckx
removed
the
approval: done
|
People from Nginx I had a conversation think that the change was not necessary at all and the best solution would be to revert this patch because otherwise they will have to support both variants. Personally I plan to use this feature as an option to test against known "broken" (omitting close_notify) implementations. |
|
So what do the nginx people intend to do?
|
|
I asked them to respond here. |
|
As already mentioned by Dmitry, we here at nginx don't think the change was necessary. As Matt already said above in the comments to SSL_CONF_cmd.pod change, the error was always reported. The only issue is that SSL_ERROR_SYSCALL with a 0 errno is not properly documented. On the other hand, the behaviour was present since ancient OpenSSL versions, and actually tested in various software using OpenSSL library, including nginx. A better solution, in our opinion, would be to document the error instead. Right now the situation in OpenSSL 3.0 is that the error reporting behaviour was changed, and, if we are going to support OpenSSL 3.0, we have to introduce specific error testing for OpenSSL 3.0. And at the same time we have to support previous error reporting, since we support OpenSSL versions starting from OpenSSL 0.9.8, as well as other libraries such as BoringSSL and LibreSSL, which still report connection close with SSL_ERROR_SYSCALL with a 0 errno. For obvious reasons we don't want to support multiple code paths to test for the same error. Especially keeping in mind that due to BoringSSL and LibreSSL we probably have to support these multiple code paths forever. It would be really helpful if the change in question was reverted and the existing behaviour (that is, SSL_ERROR_SYSCALL with a 0 errno) was documented instead. |
beldmit
force-pushed
the
ssl_op_allow_eof
branch
2 times, most recently
from
f35874f
to
55032de
Compare
|
@kroeckx, all your change requests are processed and Travis will be happy soon |
|
Travis is happy. |
| */ | ||
|
|
||
| static int test_incorrect_shutdown(int tst) |
There was a problem hiding this comment.
Since the codepath is entirely different I would prefer this to be an independent top-level test instead of a sub-flow of test_shutdown
|
Travis is happy. The incorrect shutdown tests are separated, the description of the -ignore_unexpected_eof option is adjusted. |
Partially fixes openssl#11209. Before OpenSSL 3.0 in case when peer does not send close_notify, the behaviour was to set SSL_ERROR_SYSCALL error with errno 0. This behaviour has changed. The SSL_OP_IGNORE_UNEXPECTED_EOF restores the old behaviour for compatibility's sake.
mattcaswell
added
approval: done
openssl-machine
added
approval: ready to merge
|
This pull request is ready to merge |
Partially fixes #11209. Before OpenSSL 3.0 in case when peer does not send close_notify, the behaviour was to set SSL_ERROR_SYSCALL error with errno 0. This behaviour has changed. The SSL_OP_IGNORE_UNEXPECTED_EOF restores the old behaviour for compatibility's sake. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Kurt Roeckx <kurt@roeckx.be> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from #11735)
Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Kurt Roeckx <kurt@roeckx.be> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from #11735)
|
Merged. Many thanks to all involved! |
This is the 2nd part of fixing #11209
Checklist