-
Notifications
You must be signed in to change notification settings - Fork 5
One Agent to rule them all #298
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
base: main
Are you sure you want to change the base?
Conversation
/* Construct the AttackDetected protobuf structure to be sent via gRPC to the Agent */ | ||
func GetAttackDetectedProto(res utils.InterceptorResult) *protos.AttackDetected { | ||
return &protos.AttackDetected{ | ||
Token: globals.AikidoConfig.Token, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetAttackDetectedProto now reads globals.AikidoConfig.Token (hidden global dependency), mixing configuration access into message construction.
Feedback
Post a comment with the following structure to provide feedback on this finding:
@AikidoSec feedback: [FEEDBACK]
Aikido will process this feedback into learnings to give better review comments in the future.
More info
|
||
func serversCleanupRoutine(server *ServerData) { | ||
for _, token := range globals.GetServersTokens() { | ||
server := globals.GetServer(token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function parameter 'server' is shadowed by local declaration 'server := globals.GetServer(token)'.
Feedback
Post a comment with the following structure to provide feedback on this finding:
@AikidoSec feedback: [FEEDBACK]
Aikido will process this feedback into learnings to give better review comments in the future.
More info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems a good one to fix? ^^
return agentPID; | ||
} | ||
|
||
bool Agent::RemoveSocketFiles() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function RemoveSocketFiles returns a boolean named 'failed' (true on failure), making the return semantics ambiguous/inverted and prone to incorrect usage.
Feedback
Post a comment with the following structure to provide feedback on this finding:
@AikidoSec feedback: [FEEDBACK]
Aikido will process this feedback into learnings to give better review comments in the future.
More info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems valid, see:
if (!this->RemoveSocketFiles()) {
AIKIDO_LOG_WARN("Failed to remove some socket files, will try to re-spawn Aikido Agent...\n");
} else {
AIKIDO_LOG_INFO("Successfully removed old socket files!\n");
}
if now-lastConnectionTime > MinServerInactivityForCleanup { | ||
// Server has been inactive for more than 10 minutes | ||
log.Infof("Server has been inactive for more than 10 minutes, unregistering...") | ||
server_utils.Unregister(token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a brief chance that we might delete a just created server because LastConnectionTime
is 0
until GetCloudConfig
is called? Should we set LastConnectionTime
on creation of a server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very good point, thanks for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
while ((ent = readdir(dir)) != NULL) { | ||
std::string filename(ent->d_name); | ||
if (filename.find(".sock") != std::string::npos) { | ||
socketPath = aikidoRunFolder + "/" + filename; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prob good idea to test connection? ping
/ pong
on gRPC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to grab latest one? readdir sorting might be random?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in theory there should be only one there, but indeed we might end up with 2 sockets. Not sure how I would test the grpc connection over the socket, but will look to make this more robust.
if (this->agentPid != 0) { | ||
AIKIDO_LOG_WARN("Aikido Agent already running!\n"); | ||
return true; | ||
std::string Agent::GetSocketPath() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Class Agent now mixes filesystem discovery, /proc parsing, and process spawning responsibilities (single responsibility violation).
Feedback
Post a comment with the following structure to provide feedback on this finding:
@AikidoSec feedback: [FEEDBACK]
Aikido will process this feedback into learnings to give better review comments in the future.
More info
if (this->agentPid == 0) { | ||
AIKIDO_LOG_WARN("Aikido Agent not running!\n"); | ||
return; | ||
bool Agent::SpawnDetached(std::string aikidoAgentPath, std::string initData, std::string token) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function SpawnDetached performs forking and daemonization without explanatory comments.
Feedback
Post a comment with the following structure to provide feedback on this finding:
@AikidoSec feedback: [FEEDBACK]
Aikido will process this feedback into learnings to give better review comments in the future.
More info
Uh oh!
There was an error while loading. Please reload this page.