-
Notifications
You must be signed in to change notification settings - Fork 801
2362 bb get agent logs #2384
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
2362 bb get agent logs #2384
Conversation
Reduces time to download logs by approx. 40%, but may be unnecessary after resolving #2383
99d9052
to
e369ef2
Compare
|
||
def _get_log_file_path(self, agent: Agent, machines: Mapping[MachineID, Machine]) -> Path: | ||
try: | ||
machine_ip = machines[agent.machine_id].network_interfaces[0].ip |
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.
It would be best if we could somehow distinguish between the IP that got scanned VS internal IP's. The map, logs, etc. should use the IP that agent found in the network.
Imagine you are running a fully-remote company. All developers use remote desktop to access their dev. env. All of these servers are created from the same base image, which contains virtual-box. Now you decide to scan this network and all machines are vulnerable to the same ssh credentials. But when it comes to the map, all machines are displayed with the same IP address, because virtual-box has an adapter and all machines were spawned with the same adapter IP (because they all come from the same base image). I wonder what's the best way to differentiate the IP that got scanned from other, internal IP addresses that are less relevant to the display. Putting it at the start of the list is not explicit.
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.
Agreed. This is a hard problem and I'm not sure how to solve it yet. To the best of my knowledge, v1.13.0 and prior all have this issue, so doing this doesn't add a new bug/deficiency.
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.
The way to do it with our current API is to get exploitation/scan events that target this machine and get the IP from target. There are 2 problems: 1. that machines might have the same IP's that agent scans. 2. That we use irrelevant IP's gathered from machines.
Machine should be identified by the IP that got scanned, not by some arbitrary IP that it has.
There was a problem hiding this comment.
Choose a reason for hiding this co 8000 mment
The reason will be displayed to describe this comment to others. Learn more.
But since this is for zoo, where machines are clean, I don't think it matters yet
try: | ||
machine_ip = machines[agent.machine_id].network_interfaces[0].ip | ||
except IndexError: | ||
machine_ip = "UNKNOWN" |
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.
How is this possible? How did the agent communicate to the island if it doesn't have an IP address? If it was launched on the island, then how did the downloader access it? I think if this happens, then it's a bug we want to know about instantly, not when we check the logs to see a bunch of ...UNKNOWN...
files
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.
Other than logging an error (which I can do), I'm not sure what this component can do about that.
If the machine has no IP, then the test will not be able to determine that it communicated back, so the test will fail.
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.
This component probably shouldn't extract the IP in the first place. But the more important thing is to allow this component crash and burn rather than silently putting UNKNOWN
files, when they are probably a result of some error
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.
It's not silent. It logs an error and the test will fail, but it's not this components responsibility to pass or fail the test.
envs/monkey_zoo/blackbox/log_handlers/monkey_logs_downloader.py
Outdated
Show resolved
Hide resolved
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.
Minor fixes required
|
||
start_time = agent.start_time.strftime("%Y-%m-%d-%H-%M-%S") | ||
|
||
return self.log_dir_path / f"agent-{start_time}-{machine_ip}.log" |
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.
Maybe we can move the agent log name to a const?
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.
It's not a constant, it's an fstring. It'll change for each agent for each run.
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.
I think @ilija-lazoroski meant to move AGENT_PREFIX = "agent"
, and then f"{AGENT_PREFIX}
-{start_time}-{machine_ip}.log". Not sure it's that necessary though
log_file_path = self._get_log_file_path(agent, machines) | ||
log_contents = self.island_client.get_agent_log(agent.id) | ||
|
||
MonkeyLogsDownloader._write_log_to_file(log_file_path, log_contents) | ||
|
||
self.monkey_log_paths.append(log_file_path) |
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.
Zerologon agent logs are empty (and some other exploiters as well.). We should not be outputing empty log files. Probably a check if we even got any log.
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.
Sounds like a bug, but a different bug. Create an issue.
Use underscores to improve readability
Codecov ReportBase: 61.15% // Head: 61.20% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #2384 +/- ##
===========================================
+ Coverage 61.15% 61.20% +0.04%
===========================================
Files 550 550
Lines 14399 14410 +11
===========================================
+ Hits 8806 8819 +13
+ Misses 5593 5591 -2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
What does this PR do?
Download agent logs from new endpoint
Fixes #2362
Builds https://jenkins.guardi/view/Monkey/job/run_appimage_ete/730/ and https://jenkins.guardi/view/Monkey/job/run_msi_ete/603/ contain logs.
PR Checklist
Was the CHANGELOG.md updated to reflect the changes?Was the documentation framework updated to reflect the changes?Testing Checklist
Added relevant unit tests?If applicable, add screenshots or log transcripts of the feature working