Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove TLS/DTLS heartbeat extension #4856

Closed
hannob opened this issue Dec 6, 2017 · 13 comments

Comments

@hannob
Copy link
Contributor

commented Dec 6, 2017

The heartbeat extension became "famous" due to the Heartbleed bug. However there don't seem to be any real world use cases.

There are some anecdotal claims that heartbeats are used in DTLS, however these seem to always vanish once one asks for more specific information (like real world applications that use them). For TLS heartbeats seem to be merely a feature in order to have a feature.

In OpenSSL 1.0.2 heartbeats are still enabled by default and an extension is sent with every handshake. in 1.1 the default has changed, but the code is still there.

Unused code is problematic, as it will likely receive less testing and review, but it can still contain bugs and causes a maintenance burden. In order to simplify the OpenSSL code I recommend removing the heartbeat code.

@richsalz

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2017

We'll probably remove it in 1.2, whenever that is, not before.

@nmav

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2017

Searching for SSL_get_tlsext_heartbeat_pending or SSL_CTRL_DTLS_EXT_SEND_HEARTBEAT in codesearch.debian.net only brings up the openssl code or projects including them. I can, however, find 2 gnutls applications at debian using that functionality by searching for gnutls_heartbeat_ping, so there is at least some limited interest in that extension (that's not a big issue for us as we don't expose/enable it by default for apps).

@mattcaswell mattcaswell added this to the 1.2.0 milestone Jan 23, 2018
@richsalz richsalz modified the milestones: 1.2.0, Post 1.1.0 May 6, 2018
@richsalz richsalz added after 1.1.1 and removed after 1.1.1 labels May 6, 2018
@richsalz richsalz modified the milestones: Post 1.1.0, Assessed May 6, 2018
@kwatsen

This comment has been minimized.

Copy link

commented Jun 12, 2018

Regarding use-case, please note S7 in Section 4.1 in RFC 8071.

Heartbeats are needed to maintain persistent (not periodic) call-home connections, so that the session is up already whenever a controller application wants to send commands to the device.

@hannob

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2018

@kwatsen hypothetical use cases are no good reason to keep an unused feature.

Are you aware of actual applications making use of this?

@kwatsen

This comment has been minimized.

Copy link

commented Jun 12, 2018

https://mailarchive.ietf.org/arch/msg/netconf/MOzcZKp2rSxPVMTGdmmrVInwx2M

It seems that some companies wanted to use this feature, but now feel that, since it is no longer available, they'll do cleartext TCP keepalives. So, yes, it was going get used.

@richsalz

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2018

Your email doesn't show anyone using it. And, by the way, HEARTBEAT was only defined for DTLS, not TLS.

@kwatsen

This comment has been minimized.

Copy link

commented Jun 12, 2018

The email thread demonstrates that there is an intention is to use it:

A couple of companies are working on a solutions to implement devices, such as DPUs, based on the requirements of the Broadband Forum Technical Report TR-301 issue 2 “Architecture and Requirements for Fiber to the Distribution Point”, which requires TLS for the persistent NETCONF connection, for which the configuration of call home is to be by means of the ‘ietf-netconf-server’ module.

[note: ietf-netconf-server is defined in draft-ietf-netconf-netconf-client-server]

But, you're right, they are not using it currently, because it's not supported by OpenSSL, which I gather to be their preferred TLS library...

Separately, I'm unsure if RFC 6520 is only for DTLS, reading it quickly, it sure looks like it's meant for both. The abstract says as much. That said, my understanding, which may be wrong, is that it was originally intended for DTLS, but later expanded to TLS for the RFC. Or are you saying that OpenSSL only ever implemented it for DTLS?

@richsalz

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2018

I mis-remembered, must have been an earlier version, like you said. At any rate, OpenSSL supports it only for DTLS (UDP), in the upcoming release, but it is disabled by default. We have no plans to do RFC 6520 at this time.

@kwatsen

This comment has been minimized.

Copy link

commented Jun 12, 2018

Fair enough, but can I ask what it might take to support it for TLS also? I mean, would a source code donation cinch it, or is it beyond even that?

@richsalz

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2018

I suggest the netconf working group reach out to the security AD's and the TLS co-chairs to discuss. I have already emailed them a link to this issue.

I PR would help. I don't know if it would overcome concerns; I expect the team would have to vote.

@kwatsen

This comment has been minimized.

Copy link

commented Jun 18, 2018

[just back from a trip]

Hi Rich, what would you have the NETCONF WG discuss with the Security AD and TLS co-chairs? Is RFC 6520 in dispute, or are you looking for confirmation that RFC 8071's use-case is important?

@richsalz

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2018

I am suggesting you seek advice on whether TCP heartbeats is something to build on.

@kaduk

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2018

whether TCP heartbeats is something to build on

Why would we encourage using unauthenticated plaintext when crypto is available?

levitte added a commit to levitte/openssl that referenced this issue Mar 29, 2019
Also, Fixes openssl#4856
@levitte levitte referenced this issue Mar 29, 2019
1 of 1 task complete
@levitte levitte closed this in 558ea84 Mar 29, 2019
beldmit added a commit to beldmit/openssl that referenced this issue May 31, 2019
Fixes openssl#4856

Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from openssl#1928)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.