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 heartbeats #1669

Closed
wants to merge 2 commits into from
Closed

Remove heartbeats #1669

wants to merge 2 commits into from

Conversation

@levitte
Copy link
Member

@levitte levitte commented Oct 6, 2016

Note: there are two commits. One removes heartbeats enough without changing the public API and is suitable to merge into OpenSSL_1_1_0-stable and master. The other one removes the heartbeats API entirely, and is more suitable for 1.2.0, whenever we get there.

levitte added 2 commits Oct 6, 2016
The public API still exists, but doesn't do anything any more.
@kroeckx
Copy link
Member

@kroeckx kroeckx commented Oct 6, 2016

So it has a merge conflict.

@levitte levitte force-pushed the levitte:remove-heartbeats branch to 59db14e Oct 6, 2016
@levitte
Copy link
Member Author

@levitte levitte commented Oct 6, 2016

Not any more

@kaduk
Copy link
Contributor

@kaduk kaduk commented Oct 6, 2016

I'm curious why the previous state of affairs with DTLS-only heartbeats was deemed to be undesirable in preference of no heartbeat support at all.

@levitte
Copy link
Member Author

@levitte levitte commented Oct 7, 2016

@kaduk, I'm afraid I don't understand your question... Mind you, I haven't been part of the heartbeats thing from the start, I'm just acting on an expressed desire to remove the heartbeats feature from OpenSSL.

@richsalz
Copy link
Contributor

@richsalz richsalz commented Oct 7, 2016

Concern that someone, somewhere was using it.

@vdukhovni
Copy link

@vdukhovni vdukhovni commented Oct 7, 2016

I also don't see why this is on the agenda. If we don't trust the buffer management in 1.1.0 and master, we need to fix that, removing heartbeats from DTLS is solving yesterday's problem. It seems to me that DTLS heartbeats are a plausibly useful feature, for which we should be able to provide a correct implementation.

@richsalz
Copy link
Contributor

@richsalz richsalz commented Oct 7, 2016

They are perhaps theoretically useful, but nobody is using it as far as we can tell.

@vdukhovni
Copy link

@vdukhovni vdukhovni commented Oct 7, 2016

There is very little DTLS use period. But if we are supporting it, I think the right posture is to proudly say that heartbleed is not a problem because our code is a lot more solid and we stand behind the implementation, than to remove it. We have cleaner buffer management, fuzzers, code review, ... it is I think time to stop paying attention to heartbleed and solve real problems.

@richsalz
Copy link
Contributor

@richsalz richsalz commented Oct 7, 2016

There’s an emotional aspect here as well. KILL IT

@vdukhovni
Copy link

@vdukhovni vdukhovni commented Oct 7, 2016

The emotional aspect is irrelevant. If DTLS heartbeats remain in TLS 1.3, then we should continue to support the feature. Only if TLS 1.3 is removing DTLS heartbeats, then I could be talked into this on account of upcoming obsolescence..

@ekasper
Copy link
Contributor

@ekasper ekasper commented Oct 7, 2016

We're not proudly supporting it - the test is thoroughly broken, nobody external has even noticed that, and nobody in the team has stepped up to fix it.

@kaduk
Copy link
Contributor

@kaduk kaduk commented Oct 7, 2016

Thanks for all the discussion. The test being broken and no one having noticed that does seem to give support to the belief that it is unused. Maybe some of that information could go in the commit message, though?

@tmshort
Copy link
Contributor

@tmshort tmshort commented Oct 17, 2016

My recollection is that even the ASA/AnyConnect that uses DTLS does not use Heartbeats; a data packet is used so that the VPN layer can "see" the keepalive/heartbeat.

@richsalz
Copy link
Contributor

@richsalz richsalz commented Oct 19, 2016

do the appropriate merge. finally.

@richsalz richsalz closed this Oct 19, 2016
@richsalz richsalz reopened this Oct 19, 2016
@richsalz
Copy link
Contributor

@richsalz richsalz commented Oct 19, 2016

closed by mistake.

@kroeckx
Copy link
Member

@kroeckx kroeckx commented Oct 22, 2016

@dwmw2: Do you have any comments about the usefulness of heartbeat for PMTUD in case of DTLS?

@dwmw2
Copy link
Contributor

@dwmw2 dwmw2 commented Oct 22, 2016

@tmshort is correct; AnyConnect does it all at the data layer, using a one-byte packet type as the first byte of the payload. We do extend the protocol somewhat in OpenConnect (allowing standard DTLS, AEAD ciphersuites, proper negotiation, etc.) but we hadn't gone as far as using heartbeats to replace the Dead Peer Detection and keepalive, even though I suppose an argument could be made for doing so and regaining that byte.

So MTU detection is done with large DPD payload packets, and we have no current plan to change that. And Cisco themselves have it implemented in hardware and don't even seem to have plans to catch up to DTLSv1.0 let alone change anything else.

@richsalz
Copy link
Contributor

@richsalz richsalz commented Nov 13, 2016

ping @levitte you want me to do this? My delete key is ready 👍

@t-j-h
t-j-h approved these changes Nov 13, 2016
levitte added a commit that referenced this pull request Nov 13, 2016
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from #1669)
@richsalz
Copy link
Contributor

@richsalz richsalz commented Nov 13, 2016

@levitte, the first commit is merged. if you want to rebase or close or make a new one (or want me to do it) that holds only the second commit, which would be on-hold, let me know.

@levitte
Copy link
Member Author

@levitte levitte commented Nov 13, 2016

Whatever I have to do, I'll do tomorrow.

@kaduk
Copy link
Contributor

@kaduk kaduk commented Nov 15, 2016

Can I request that the commit message say something about why the code is being removed?

@kaduk
Copy link
Contributor

@kaduk kaduk commented Nov 15, 2016

Can I request that the commit message say something about why the code is being removed?

Oops, apparently I can't, because the commit was pushed already even though the PR is not closed.

@richsalz
Copy link
Contributor

@richsalz richsalz commented Nov 15, 2016

Too late now.

And it should be immediately obvious to everyone about openssl and heartbeats.

@levitte
Copy link
Member Author

@levitte levitte commented Nov 15, 2016

It is possible to add a note...

@richsalz
Copy link
Contributor

@richsalz richsalz commented Nov 15, 2016

Sure. But we don’t justify any other commits.

@levitte
Copy link
Member Author

@levitte levitte commented Nov 15, 2016

It seems this PR was a bit too confusing.

Question: do we want the "almost entire" removal in the 1.1.0 branch as well? That was my original intent. If we do, I'll prepare a separate PR for that.

Meanwhile, this is now entirely merged into master.

@levitte levitte closed this Nov 15, 2016
levitte added a commit that referenced this pull request Nov 15, 2016
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from #1669)
@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Nov 15, 2016

No...we should not remove this from a stable branch.

@levitte
Copy link
Member Author

@levitte levitte commented Nov 15, 2016

Ok then. Thank you.

@richsalz
Copy link
Contributor

@richsalz richsalz commented Nov 15, 2016

Doesn’t that change the API?

Senior Architect, Akamai Technologies
Member, OpenSSL Dev Team
IM: richsalz@jabber.at Twitter: RichSalz

From: Richard Levitte [mailto:notifications@github.com]
Sent: Tuesday, November 15, 2016 6:47 PM
To: openssl/openssl
Cc: Salz, Rich; State change
Subject: Re: [openssl/openssl] Remove heartbeats (#1669)

It seems this PR was a bit too confusing.

Question: do we want the "almost entire" removal in the 1.1.0 branch as well? That was my original intent. If we do, I'll prepare a separate PR for that.

Meanwhile, this is now entirely merged into master.


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openssl_openssl_pull_1669-23issuecomment-2D260595090&d=DgMFaQ&c=96ZbZZcaMF4w0F4jpN6LZg&r=4LM0GbR0h9Fvx86FtsKI-w&m=KCXo2IWDBc5F2bcbWzMP3gBBEv5fvJqj16B_5MGhY10&s=JhX1y2eX9nicNzr6aWZF9Je5HQYQk07tDdxgDO7oUOw&e=, or mute the threadhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AG-5FilER8rhnXRXgOyB1J9UAoAor6bUPqks5q-2DX-2DvgaJpZM4KPuw8&d=DgMFaQ&c=96ZbZZcaMF4w0F4jpN6LZg&r=4LM0GbR0h9Fvx86FtsKI-w&m=KCXo2IWDBc5F2bcbWzMP3gBBEv5fvJqj16B_5MGhY10&s=ppL0VI6sOcDmjP5xIvUn7iGOERugXNVq1b7RiKZ2m78&e=.

@levitte
Copy link
Member Author

@levitte levitte commented Nov 15, 2016

Errr... ah, you mean we finally got to the point where we need to create the OpenSSL_1_1_x branch?

@levitte
Copy link
Member Author

@levitte levitte commented Nov 15, 2016

Actually, no... or, well, we could add back the "heartbeats" item to %disabled in Configure to make that perfectly clear, and we could make enable-heartbeats a no-op, and leave it at that. Would that be preferable?

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Nov 15, 2016

Well, technically, this would be an ABI/API break so isn't eligible for 1.1.1, it would have to be 1.2.0

@tmshort
Copy link
Contributor

@tmshort tmshort commented Nov 15, 2016

Keep the configuration interface the same. As a followup to my previous comment, The Cisco ASA was immune to Heartbleed because it didn't recognize the heartbeat record type. What happens now if a malicious client decides to send a heartbeat? Does the connection close, I presume?

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Nov 15, 2016

As of 1.1.0c (commit 436a2a0) any unrecognised record type results in the connection closing.

@richsalz
Copy link
Contributor

@richsalz richsalz commented Nov 15, 2016

You have to revert that "completely remove" commit. Source code that worked in 1.1.0 no longer works, configuration scripts no longer work (we talked about this with #1814 ), and binaries that used to work will probably now break.

+1 to revert that change, and then put this rest of this MR on hold.

@levitte
Copy link
Member Author

@levitte levitte commented Nov 15, 2016

I'll revert and make a new PR for pending 1.2.0 changes. I'm pretty sure those will start piling up and we might be ready for a 1.1.x branch.

@levitte
Copy link
Member Author

@levitte levitte commented Nov 15, 2016

Reverted

@FdaSilvaYY
Copy link
Contributor

@FdaSilvaYY FdaSilvaYY commented Nov 15, 2016

This PR has two commits : e72040c, and 6c62f9e
Only the second last was revert. Is it OK ?

@richsalz
Copy link
Contributor

@richsalz richsalz commented Nov 15, 2016

Yes, that is exactly the right thing. HEARTBEAT functionality was removed, but the API and compatibility with 1.1.0 was not changed.

@levitte
Copy link
Member Author

@levitte levitte commented Nov 16, 2016

@FdaSilvaYY, I've kept the complete removal in a separate PR, #1928

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet