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.

Sign up for GitHub

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

Fix update execution message discarded. refs #8616 #8627

Merged
merged 2 commits into from
Apr 13, 2023
Merged

Fix update execution message discarded. refs #8616 #8627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
912fdb9
Fix update execution message discarded
Jul 30, 2021
c5c1792
Allow to exec command on endpoint where the checkable is not present …
Apr 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions lib/icinga/apiactions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,6 +681,21 @@ Dictionary::Ptr ApiActions::ExecuteCommand(const ConfigObject::Ptr& object, cons
if (!endpointPtr)
return ApiActions::CreateResult(404, "Can't find a valid endpoint for '" + resolved_endpoint + "'.");

/* Return an error when
* the endpoint is different from the command endpoint of the checkable
* and the endpoint zone can't access the checkable.
* The endpoints are checked to allow for the case where command_endpoint is specified in the checkable
* but checkable is not actually present in the agent.
*/
Zone::Ptr endpointZone = endpointPtr->GetZone();
Endpoint::Ptr commandEndpoint = checkable->GetCommandEndpoint();
if (endpointPtr != commandEndpoint && !endpointZone->CanAccessObject(checkable)) {
return ApiActions::CreateResult(
409,
"Zone '" + endpointZone->GetName() + "' cannot access checkable '" + checkable->GetName() + "'."
);
}

Copy link
Member

Choose a reason for hiding this comment

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

OK this makes sense to me...

/* Get command */
String command;

Expand Down Expand Up @@ -806,6 +821,7 @@ Dictionary::Ptr ApiActions::ExecuteCommand(const ConfigObject::Ptr& object, cons
Dictionary::Ptr pending_execution = new Dictionary();
pending_execution->Set("pending", true);
pending_execution->Set("deadline", deadline);
pending_execution->Set("endpoint", resolved_endpoint);
Dictionary::Ptr executions = checkable->GetExecutions();

if (!executions)
Expand Down
92 changes: 83 additions & 9 deletions lib/icinga/clusterevents.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -761,9 +761,18 @@ Value ClusterEvents::ExecuteCommandAPIHandler(const MessageOrigin::Ptr& origin,
}
}

String executionUuid = params->Get("source");

if (params->Contains("endpoint")) {
Endpoint::Ptr execEndpoint = Endpoint::GetByName(params->Get("endpoint"));

if (!execEndpoint) {
Log(LogWarning, "ClusterEvents")
<< "Discarding 'execute command' message " << executionUuid
<< ": Endpoint " << params->Get("endpoint") << " does not exist";
return Empty;
}

Copy link
Member

Choose a reason for hiding this comment

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

So far so good...

if (execEndpoint != Endpoint::GetLocalEndpoint()) {
Zone::Ptr endpointZone = execEndpoint->GetZone();
Zone::Ptr localZone = Zone::GetLocalZone();
Expand All @@ -782,7 +791,7 @@ Value ClusterEvents::ExecuteCommandAPIHandler(const MessageOrigin::Ptr& origin,
if (!(childEndpoint->GetCapabilities() & (uint_fast64_t)ApiCapabilities::ExecuteArbitraryCommand)) {
double now = Utility::GetTime();
Dictionary::Ptr executedParams = new Dictionary();
executedParams->Set("execution", params->Get("source"));
executedParams->Set("execution", executionUuid);
executedParams->Set("host", params->Get("host"));

if (params->Contains("service"))
Expand All @@ -803,6 +812,62 @@ Value ClusterEvents::ExecuteCommandAPIHandler(const MessageOrigin::Ptr& origin,
return Empty;
}
}

Checkable::Ptr checkable;
Host::Ptr host = Host::GetByName(params->Get("host"));
if (!host) {
Log(LogWarning, "ClusterEvents")
<< "Discarding 'execute command' message " << executionUuid
<< ": host " << params->Get("host") << " does not exist";
return Empty;
}

if (params->Contains("service"))
checkable = host->GetServiceByShortName(params->Get("service"));
else
checkable = host;

if (!checkable) {
String checkableName = host->GetName();
if (params->Contains("service"))
checkableName += "!" + params->Get("service");

Log(LogWarning, "ClusterEvents")
<< "Discarding 'execute command' message " << executionUuid
<< ": " << checkableName << " does not exist";
return Empty;
}

/* Return an error when the endpointZone is different than the child zone and
* the child zone can't access the checkable.
* The zones are checked to allow for the case where command_endpoint is specified in the checkable
* but checkable is not actually present in the agent.
*/
if (!zone->CanAccessObject(checkable) && zone != endpointZone) {
double now = Utility::GetTime();
Dictionary::Ptr executedParams = new Dictionary();
executedParams->Set("execution", executionUuid);
executedParams->Set("host", params->Get("host"));

if (params->Contains("service"))
executedParams->Set("service", params->Get("service"));

executedParams->Set("exit", 126);
executedParams->Set(
"output",
"Zone '" + zone->GetName() + "' cannot access to checkable '" + checkable->GetName() + "'."
);
executedParams->Set("start", now);
executedParams->Set("end", now);

Dictionary::Ptr executedMessage = new Dictionary();
executedMessage->Set("jsonrpc", "2.0");
executedMessage->Set("method", "event::ExecutedCommand");
executedMessage->Set("params", executedParams);

listener->RelayMessage(nullptr, nullptr, executedMessage, true);
Copy link
Member

Choose a reason for hiding this comment

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

Huh? nullptr, nullptr? Not...

Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, I haven't put much thought into these parameters here as the existing event::ExecutedCommand message already used the same ones.

However, are you sure about origin? This is sending a response that originates here, so with the origin of the incoming message, that might prevent the response from being sent back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I tested, I agree with Julian, using origin, checkable the message is not sent back and the execution remains in the pending state

	"a9086343-4da7-44ae-8fd6-5f16641cbd5e" = {
		deadline = 1681388550.850034
		endpoint = "agent"
		pending = true
	}

Copy link
Member

Choose a reason for hiding this comment

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

Oops. 😅 🐘

return Empty;
}
}
}

Expand Down Expand Up @@ -1179,13 +1244,6 @@ Value ClusterEvents::ExecutedCommandAPIHandler(const MessageOrigin::Ptr& origin,

ObjectLock oLock (checkable);

if (origin->FromZone && !origin->FromZone->CanAccessObject(checkable)) {
Log(LogNotice, "ClusterEvents")
<< "Discarding 'update executions API handler' message for checkable '" << checkable->GetName()
<< "' from '" << origin->FromClient->GetIdentity() << "': Unauthorized access.";
return Empty;
}

if (!params->Contains("execution")) {
Log(LogNotice, "ClusterEvents")
<< "Discarding 'update executions API handler' message for checkable '" << checkable->GetName()
Expand Down Expand Up @@ -1213,6 +1271,22 @@ Value ClusterEvents::ExecutedCommandAPIHandler(const MessageOrigin::Ptr& origin,
return Empty;
}

Endpoint::Ptr command_endpoint = Endpoint::GetByName(execution->Get("endpoint"));
if (!command_endpoint) {
Log(LogNotice, "ClusterEvents")
<< "Discarding 'update executions API handler' message from '" << origin->FromClient->GetIdentity()
<< "': Command endpoint does not exists.";

return Empty;
}

if (origin->FromZone && !command_endpoint->GetZone()->IsChildOf(origin->FromZone)) {
Copy link
Member

Choose a reason for hiding this comment

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

The only problem I see are messages from the own zone, but then -I guess- !!origin->FromZone wouldn’t be true. (JsonRpcConnection::MessageHandler())

Log(LogNotice, "ClusterEvents")
<< "Discarding 'update executions API handler' message for checkable '" << checkable->GetName()
<< "' from '" << origin->FromClient->GetIdentity() << "': Unauthorized access.";
return Empty;
}

if (params->Contains("exit"))
execution->Set("exit", params->Get("exit"));

Expand Down Expand Up @@ -1303,7 +1377,7 @@ Value ClusterEvents::UpdateExecutionsAPIHandler(const MessageOrigin::Ptr& origin
updateMessage->Set("method", "event::UpdateExecutions");
updateMessage->Set("params", params);

listener->RelayMessage(origin, Zone::GetLocalZone(), updateMessage, true);
listener->RelayMessage(origin, checkable, updateMessage, true);
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

Choose a reason for hiding this comment

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

... origin, checkable? Not even origin? Please explain.


return Empty;
}