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

Don't send Heartbeat messages when connection is unauthenticated #7698

Closed
wants to merge 1 commit into from

Conversation

mcktr
Copy link
Member

@mcktr mcktr commented Dec 5, 2019

Do not send Heartbeat messages when the connection is not authenticated.

Context

When a new JSON-RPC connection is established we start to sent heartbeat messages, even if the connection is not authenticated (e.g. certificate not signed). If the connection stalls for some reason and the answer to the heartbeat message is not received in time we disconnect the client.

if (m_NextHeartbeat != 0 && m_NextHeartbeat < Utility::GetTime()) {
Log(LogWarning, "JsonRpcConnection")
<< "Client for endpoint '" << m_Endpoint->GetName() << "' has requested "
<< "heartbeat message but hasn't responded in time. Closing connection.";
Disconnect();

Problem

On unauthenticated connections we don't set the m_Endpoint variable, only when authenticated. In the log message from above we use m_Endpoint variable which will result in a crash.

if (authenticated)
m_Endpoint = Endpoint::GetByName(identity);

I think it is not necessary to send heartbeat messages on unauthenticated connections. But I may not see the full scope here. I'll leave this PR as draft and maybe @dnsmichi and @Al2Klimov can have a look? :-)

This may have an influence on #7569 and #7687.

Do not send Heartbeat messages when the connection is not authenticated.
@mcktr mcktr added bug Something isn't working area/distributed Distributed monitoring (master, satellites, clients) labels Dec 5, 2019
@dnsmichi dnsmichi self-assigned this Dec 6, 2019
@dnsmichi
Copy link
Contributor

I'm not sure about the impact here, we'll have to investigate. I know where you're coming from, but applying this change just hides the crash and leak within the JSON messaging backend.

@Al2Klimov
Copy link
Member

I'd prefer #7747.

@dnsmichi
Copy link
Contributor

dnsmichi commented Feb 5, 2020

I'm going with #7747 as well, if you don't mind @mcktr

@mcktr
Copy link
Member Author

mcktr commented Feb 5, 2020

I also opt for #7747, closing here.

@mcktr mcktr closed this Feb 5, 2020
@Al2Klimov Al2Klimov deleted the bugfix/heartbeat-unauthenticated-connections branch February 5, 2020 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/distributed Distributed monitoring (master, satellites, clients) bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants