Skip to content

Fix TLS connection closure (source side)#2811

Merged
lbudai merged 7 commits intosyslog-ng:masterfrom
MrAnno:tls-shutdown
Jun 19, 2020
Merged

Fix TLS connection closure (source side)#2811
lbudai merged 7 commits intosyslog-ng:masterfrom
MrAnno:tls-shutdown

Conversation

@MrAnno
Copy link
Copy Markdown
Collaborator

@MrAnno MrAnno commented Jun 28, 2019 •
edited
Loading

This pull request fixes the TLS connection closure on the source (receiver) side by sending back close_notify alerts.

RFC5425:

Once the transport receiver gets close_notify from the transport sender, it MUST reply with a close_notify unless it becomes aware that the connection has already been closed by the transport sender (e.g., the closure was indicated by TCP).

TODO:

  • The destination (sender) side has to be fixed as well (in a separate PR):

A transport sender MUST close the associated TLS connection if the connection is not expected to deliver any syslog messages later. It MUST send a TLS close_notify alert before closing the connection. A transport sender (TLS client) MAY choose to not wait for the transport receiver's close_notify alert and simply close the connection, thus generating an incomplete close on the transport receiver (TLS server) side.

Implementing this on the destination side is more complicated, we might need to introduce log_proto_client_shutdown() -> log_transport_shutdown() because SSL_shutdown() requires non-blocking write operations, so fd watches should be registered during shutdown. Currently, we destroy/shutdown transports after stopping watches and deiniting LogWriter. Another tricky part is log_writer_reopen_deferred().

Fixes #2703

@kira-syslogng
Copy link
Copy Markdown
Contributor

kira-syslogng commented Jun 28, 2019

Build SUCCESS

@MrAnno MrAnno added this to the syslog-ng-3.23 milestone Jun 28, 2019
@lbudai lbudai self-requested a review July 8, 2019 08:02
@MrAnno
Copy link
Copy Markdown
Collaborator Author

MrAnno commented Jul 12, 2019

@kira-syslogng retest this please

@kira-syslogng
Copy link
Copy Markdown
Contributor

kira-syslogng commented Jul 12, 2019

Build FAILURE

@MrAnno
Copy link
Copy Markdown
Collaborator Author

MrAnno commented Jul 19, 2019 •
edited
Loading

@kira-syslogng retest this please

@MrAnno
Copy link
Copy Markdown
Collaborator Author

MrAnno commented Jul 19, 2019

@kira-syslogng do perftest

@kira-syslogng
Copy link
Copy Markdown
Contributor

kira-syslogng commented Jul 19, 2019

Build SUCCESS

@kira-syslogng
Copy link
Copy Markdown
Contributor

kira-syslogng commented Jul 19, 2019

Kira-performance-test: Build SUCCESS

@MrAnno MrAnno removed this from the syslog-ng-3.23 milestone Aug 7, 2019
@MrAnno MrAnno mentioned this pull request Jan 17, 2020
Merged
@furiel
Copy link
Copy Markdown
Collaborator

furiel commented Mar 16, 2020

Could you please resolve the conflict?

If the source side is working, although the PR itself is incomplete, I think it would be nice to merge at least that part. The issue was raised for the source side.

We can still open a new issue for the destination though.

@kira-syslogng
Copy link
Copy Markdown
Contributor

kira-syslogng commented Mar 16, 2020

Build FAILURE

@kira-syslogng
Copy link
Copy Markdown
Contributor

kira-syslogng commented Mar 16, 2020

Kira-performance-test: Build FAILURE

@MrAnno
Copy link
Copy Markdown
Collaborator Author

MrAnno commented Mar 16, 2020

@furiel Good idea.

I'll remove the WIP flag after running some internal tests.

@MrAnno
Copy link
Copy Markdown
Collaborator Author

MrAnno commented Mar 16, 2020

@kira-syslogng do perftest

@MrAnno MrAnno changed the title [WIP] Fix TLS connection closure [WIP] Fix TLS connection closure (source side) Mar 16, 2020
@kira-syslogng
Copy link
Copy Markdown
Contributor

kira-syslogng commented Mar 16, 2020

Build SUCCESS

@kira-syslogng
Copy link
Copy Markdown
Contributor

kira-syslogng commented Mar 16, 2020

Kira-performance-test: Build SUCCESS

@lbudai lbudai removed their request for review March 17, 2020 16:48
@MrAnno MrAnno changed the title [WIP] Fix TLS connection closure (source side) Fix TLS connection closure (source side) Apr 2, 2020
@kira-syslogng
Copy link
Copy Markdown
Contributor

kira-syslogng commented Apr 2, 2020

Build SUCCESS

furiel
furiel reviewed Apr 6, 2020
View reviewed changes
@kira-syslogng
Copy link
Copy Markdown
Contributor

kira-syslogng commented Apr 16, 2020

Build SUCCESS

furiel
furiel previously approved these changes Apr 17, 2020
View reviewed changes
@kira-syslogng
Copy link
Copy Markdown
Contributor

kira-syslogng commented Apr 17, 2020

Build SUCCESS

furiel
furiel previously approved these changes Apr 17, 2020
View reviewed changes
pprindeville
pprindeville reviewed Apr 24, 2020
View reviewed changes
@MrAnno MrAnno added this to the syslog-ng-3.28 milestone Jun 4, 2020
@kira-syslogng
Copy link
Copy Markdown
Contributor

kira-syslogng commented Jun 16, 2020

Build SUCCESS

furiel
furiel previously approved these changes Jun 17, 2020
View reviewed changes
@MrAnno MrAnno mentioned this pull request Jun 18, 2020
Merged
2 tasks
lbudai
lbudai reviewed Jun 19, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@lbudai lbudai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix.
I have only one comment - if you won't have the time to resolve it, I'm okay with merge this as-is.

lib/transport/transport-tls.c Outdated
Copy link
Copy Markdown
Collaborator

@lbudai lbudai Jun 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd factor out this part of the function (as this part does not modify the return value, it would be more readable).

Copy link
Copy Markdown
Collaborator Author

@MrAnno MrAnno Jun 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted, thanks.
The method name might not be the best.

@kira-syslogng
Copy link
Copy Markdown
Contributor

kira-syslogng commented Jun 19, 2020

Build FAILURE

MrAnno added 7 commits June 19, 2020 13:56
@MrAnno
transport: make log_transport_stream_socket_free_method() public
1c72c99 
Signed-off-by: László Várady <laszlo.varady@protonmail.com>
@MrAnno
transport: derive LogTransportTLS from LogTransportSocket
dba5e33 
LogTransportTLS is a LogTransportSocket specialization, for example,
it should also call shutdown() before destroying the transport.

Signed-off-by: László Várady <laszlo.varady@protonmail.com>
@MrAnno
transport-tls: reset cond only when SSL_read() was successful
dbc2615 
cond = 0 means LogTransport users (LogProtos) can set the direction freely,
therefore it should be reset in case of a successful read.

Signed-off-by: László Várady <laszlo.varady@protonmail.com>
@MrAnno
transport-tls: fix the error handling of read()
ac132cb 
OpenSSL (>= 1.0.2) does not differentiate between zero and negative return
values anymore (see docs). SSL_ERROR_ZERO_RETURN should be checked to
detect a connection close.

Signed-off-by: László Várady <laszlo.varady@protonmail.com>
@MrAnno
transport-tls: send close_notify back to peer before closing connection
2611742 
RFC 5425:
Once the transport receiver gets a close_notify from the transport sender,
it MUST reply with a close_notify unless it becomes aware that the
connection has already been closed by the transport sender.

SSL_shutdown() is responsible for sending close_notify shutdown alerts.

This fixes the source side. Destinations (senders) have to be fixed as well.

Signed-off-by: László Várady <laszlo.varady@protonmail.com>
@MrAnno
news: bugfix-2811
d4a972a 
Signed-off-by: László Várady <laszlo.varady@protonmail.com>
@MrAnno
transport-tls: extract the error handling of tls_send_shutdown
9ea319d 
Signed-off-by: László Várady <laszlo.varady@protonmail.com>
@kira-syslogng
Copy link
Copy Markdown
Contributor

kira-syslogng commented Jun 19, 2020

Build SUCCESS

@lbudai
Copy link
Copy Markdown
Collaborator

lbudai commented Jun 19, 2020

@MrAnno : thanks.
I'm merging this, as the last change is just a cosmetic one.

@lbudai lbudai merged commit 1ec1d79 into syslog-ng:master Jun 19, 2020
MrAnno added a commit to MrAnno/syslog-ng that referenced this pull request Jun 25, 2020
@MrAnno
news: remove entry syslog-ng#2811
394ae9d 
This entry is from the previous release.

Signed-off-by: László Várady <laszlo.varady@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@pprindeville pprindeville pprindeville left review comments

@lbudai lbudai lbudai approved these changes

@furiel furiel furiel left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

syslog-ng-3.28

Development

Successfully merging this pull request may close these issues.

Validation of TLS close_notify per RFC spec

5 participants

@MrAnno @kira-syslogng @furiel @lbudai @pprindeville