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

Jump to bottom

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 1 commit
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
10 changes: 10 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,15 @@ Dictionary::Ptr ApiActions::ExecuteCommand(const ConfigObject::Ptr& object, cons
if (!endpointPtr)
return ApiActions::CreateResult(404, "Can't find a valid endpoint for '" + resolved_endpoint + "'.");

/* Check if the endpoint zone can access the checkable */
Zone::Ptr endpointZone = endpointPtr->GetZone();
if (!endpointZone->CanAccessObject(checkable)) {
julianbrost marked this conversation as resolved.
Show resolved Hide resolved
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 +815,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
88 changes: 79 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,58 @@ 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
Copy link
Member

Choose a reason for hiding this comment

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

A bit of indentation...

<< ": 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
Copy link
Member

Choose a reason for hiding this comment

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

... wouldn’t hurt.

<< ": " << checkableName << " does not exist";
return Empty;
}

/* Check if the child zone can access the checkable, and if it's the same endpoint zone */
if (!zone->CanAccessObject(checkable) && zone != endpointZone) {
julianbrost marked this conversation as resolved.
Show resolved Hide resolved
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 +1240,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 +1267,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 && !origin->FromZone->CanAccessObject(command_endpoint->GetZone())) {
Copy link
Member

Choose a reason for hiding this comment

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

Apropos, I consider zoneA->CanAccessObject(zoneB) kinda confusing. Doesn’t this -according to CanAccessObject() code- query for a child/parent relationship?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of
origin->FromZone->CanAccessObject(command_endpoint->GetZone())
is it better to use
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.

If this is what you wanna check for, yes. IMAO it's more descriptive. But let's Julian cross-check first..

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the return path for the response. The request was sent to the endpoint command_endpoint so this is supposed to check whether the response is coming from (directly or indirectly) that endpoint. That should be checked by both variants but with IsChildOf() that might be a bit more obvious (especially given that CanAccessObject() would also do GetZone() internally, but that wouldn't do the same, greetings to #8740).

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 +1373,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;
}