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.

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

Scope Accuracy Rework #772

Merged
merged 22 commits into from
Nov 14, 2023
Merged

Scope Accuracy Rework #772

merged 22 commits into from
Nov 14, 2023

Conversation

TheTechromancer
Copy link
Collaborator

@TheTechromancer TheTechromancer commented Oct 8, 2023

This PR makes several important changes to BBOT's event handling logic that fixes numerous small and annoying edge cases (including #757, which prompted this effort) and generally simplifies the code.

Because BBOT does speculative discovery via internal events (events that are distributed only to scan modules, but not to output modules - such as far-out DNS resolutions, speculated open ports, unverified URLs, etc.), and because when outputting to a graph such as Neo4j, these chains of events must be preserved to prevent orphans, there was a lot of ugly logic that accumulated over time where we were making special exceptions for Neo4j. This caused unexpected behaviors, and led to even more exceptions for those exceptions, etc.

Originally, I used BBOT's built-in event deduplication logic to shepherd these chains of events and prevent orphans in the graph. But the implementation was overly complex, difficult to maintain and still had bugs. So it has been completely reworked and these two systems have been decoupled.

The result is two cleaner and simpler systems instead of one overly complex one. Extensive tests have been written to ensure the systems work as intended.

By necessity, a new DNS mock system has been added which enables more elaborate and more effective tests.

This was a challenging rework. If you're interested in the details, please see the thread for this issue where I typed out my thought process.

@codecov
Copy link

codecov bot commented Oct 15, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (1224dcf) 92% compared to head (99ae8e9) 92%.
Report is 1 commits behind head on dev.

Files Patch % Lines
bbot/core/event/base.py 93% 2 Missing ⚠️
bbot/core/helpers/dns.py 98% 1 Missing ⚠️
bbot/scanner/manager.py 99% 1 Missing ⚠️
...bot/test/test_step_1/test_manager_deduplication.py 100% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##             dev    #772    +/-   ##
======================================
+ Coverage     92%     92%    +1%     
======================================
  Files        286     287     +1     
  Lines      17290   17901   +611     
======================================
+ Hits       15831   16459   +628     
+ Misses      1459    1442    -17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@liquidsec
Copy link
Collaborator

I am approving for dev, but my intention is to test this extensively prior to the dev->stable push. However, I can't fully grok all of it in a reasonable amount of time. I see no issues through my review, and the extensive tests add a lot of reassurance. The potential consequences of scope mishaps warrants an extra bit of caution, though.

@TheTechromancer TheTechromancer force-pushed the subdomains-duplicate-fix branch 2 times, most recently from 1736f46 to 00de082 Compare November 9, 2023 17:30
@TheTechromancer TheTechromancer force-pushed the subdomains-duplicate-fix branch 2 times, most recently from f3840cc to c66c7fa Compare November 13, 2023 21:03
@TheTechromancer TheTechromancer force-pushed the subdomains-duplicate-fix branch from 0c14b71 to 99ae8e9 Compare November 13, 2023 22:00
Copy link
Contributor

@batgoose batgoose left a comment

Choose a reason for hiding this comment

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

Will test changes when able

@TheTechromancer TheTechromancer merged commit 1b0b278 into dev Nov 14, 2023
@TheTechromancer TheTechromancer deleted the subdomains-duplicate-fix branch December 15, 2023 16:05
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.

3 participants