8000 2362 bb get agent logs by mssalvatore · Pull Request #2384 · guardicore/monkey · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 13 commits into from
Oct 3, 2022
Merged

2362 bb get agent logs #2384

merged 13 commits into from
Oct 3, 2022

Conversation

mssalvatore
Copy link
Collaborator

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

  • Have you added an explanation of what your changes do and why you'd like to include them?
  • Is the TravisCI build passing?
  • Was the CHANGELOG.md updated to reflect the changes?
  • Was the documentation framework updated to reflect the changes?
  • Have you checked that you haven't introduced any duplicate code?

Testing Checklist

  • Added relevant unit tests?
  • Have you successfully tested your changes locally? Elaborate:

    Tested by running ETE tests from Jenkins and examining the log archive.

  • If applicable, add screenshots or log transcripts of the feature working

Reduces time to download logs by approx. 40%, but may be unnecessary
after resolving #2383
@mssalvatore mssalvatore force-pushed the 2362-bb-get-agent-logs branch from 99d9052 to e369ef2 Compare October 2, 2022 20:58

def _get_log_file_path(self, agent: Agent, machines: Mapping[MachineID, Machine]) -> Path:
try:
machine_ip = machines[agent.machine_id].network_interfaces[0].ip
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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"
Copy link
Contributor

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

Copy link
Collaborator Author

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.

Copy link
Contributor

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

Copy link
Collaborator Author

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.

Copy link
Contributor
@VakarisZ VakarisZ left a 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"
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Copy link
Contributor

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

Comment on lines +41 to +46
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)
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

@codecov
Copy link
codecov bot commented Oct 3, 2022

Codecov Report

Base: 61.15% // Head: 61.20% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (cfd49db) compared to base (de435e2).
Patch coverage: 100.00% of modified lines in pull request are covered.

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     
Impacted Files Coverage Δ
monkey/monkey_island/cc/resources/agent_logs.py 100.00% <100.00%> (ø)
.../cc/agent_event_handlers/handle_ping_scan_event.py
...land/cc/agent_event_handlers/scan_event_handler.py 100.00% <0.00%> (ø)
...key/monkey_island/cc/setup/agent_event_handlers.py 42.10% <0.00%> (+4.01%) ⬆️

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mssalvatore mssalvatore merged commit eb16969 into develop Oct 3, 2022
@mssalvatore mssalvatore deleted the 2362-bb-get-agent-logs branch October 3, 2022 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BB: missing logs
3 participants
0