add ability to specify single target using --target #31

GreaterGoodest · 2020-04-16T00:29:07Z

Adds ability to run scan on single domain/ip by creating a temporary target folder.
All tests passed.

This closes #29

epi052

Thank you for the PR! There's a few changes I'd like to see before merging. Also, I'd prefer that the additional code had some tests written against it. You can check what's missing test coverage (don't worry about missing coverage you didn't create code for) with this command

coverage run --source=pipeline -m pytest  tests/test_recon/ tests/test_shell/ tests/test_web/ tests/test_models && coverage report -m

When you run this against your branch, you can see lines 239-240 and 242-244 don't have any test coverage.

pipeline/recon-pipeline.py                   399     10    97%   94, 104-105, 239-240, 242-244, 366-368

Tests for the area of the code base you're working on will be in tests/test_shell/test_recon_pipeline_shell.py and potentially in tests/test_recon/test_parsers.py (the mutual exclusion should likely be tested).

There are already tests that hit these functions, you'll just need to add a test case or two to the parameterize decorator. Let me know if you have any questions about any of the reviewed items or the tests, and thanks again!

epi052 · 2020-04-16T01:30:44Z

pipeline/recon/parsers.py

 )
+scan_parser.add_argument("--target", help="ip or domain to target")


Please see this comment about mutual exlcusion getting moved into the parser.

epi052 · 2020-04-16T11:47:33Z

pipeline/recon-pipeline.py

+        elif args.target:
+            with open("tmp_tgt", "w") as file:
+                file.write(args.target)
+                args.__statement__.arg_list[args.__statement__.arg_list.index("--target") + 1] = os.path.abspath(
+                    "tmp_tgt"
+                )


I like overwriting the ip/host passed to --target, however, let's remove --target from command and put --target-file in its place.

Currently, after line 248 executes, args.__statement__.arg_list looks something like this

['HTBScan', '--interface', 'tun0', '--local-scheduler', '--verbose', '--top-ports', '1000', '--rate', '100', '--target', '/tmp/recon-pipeline/tmp_tgt', '--results-dir', 'traceback-test1']

Since the TargetList class knows nothing about a --target why don't we overwrite --target with --target-file

['HTBScan', '--interface', 'tun0', '--local-scheduler', '--verbose', '--top-ports', '1000', '--rate', '100', '--target-file', '/tmp/recon-pipeline/tmp_tgt', '--results-dir', 'traceback-test1']

epi052 · 2020-04-16T12:01:03Z

pipeline/recon-pipeline.py

+            with open("tmp_tgt", "w") as file:
+                file.write(args.target)
+                args.__statement__.arg_list[args.__statement__.arg_list.index("--target") + 1] = os.path.abspath(
+                    "tmp_tgt"


I like the idea of a temp file, but i'm not crazy about it being hard-coded. I'm also not a fan of it being dropped in the user's cwd. Take a look at mkstemp. That will drop it in /tmp and give it a random name.

Should probably be good stewards and do a check around here for args.target and if found clean up the file created by mkstemp.

epi052 · 2020-04-16T12:05:39Z

pipeline/recon-pipeline.py

+        elif args.target:
+            with open("tmp_tgt", "w") as file:
+                file.write(args.target)
+                args.__statement__.arg_list[args.__statement__.arg_list.index("--target") + 1] = os.path.abspath(


This project uses pathlib for dealing with file system paths. For consistency, can you please swap os.path.abspath to its pathlib equivalent? Additionally, add a .expanduser() call before .resolve() as both .resolve and .abspath can have surprising results. There's an example here

>>> os.path.abspath("~/stuff") '/tmp/recon-pipeline/~/stuff' >>> Path("~/stuff").resolve() PosixPath('/tmp/recon-pipeline/~/stuff')

epi052 · 2020-04-16T12:07:20Z

pipeline/recon-pipeline.py

+            self.async_alert(style(f"[!] You can't use the --target and --target-file options in the same scan!"))
+            return
+        elif args.target:
+            with open("tmp_tgt", "w") as file:


For trivially small writes like this, pathlib can be more succinct.

Path("my_tmp_file").write_text(args.target)

It does the same things as the context manager. You'll need a reference to your file for passing the path into the arg_list, so you can split that statement into two parts

tgt_file = Path("my_tmp_file") tgt_file.write_text(args.target)

epi052 · 2020-04-16T12:08:49Z

pipeline/recon-pipeline.py

+        if args.target and args.target_file:
+            self.async_alert(style(f"[!] You can't use the --target and --target-file options in the same scan!"))
+            return


mutual exclusion should be enforced through argparse's add_mutually_exclusive_group when possible.

Also, either target-file or target is required, so add_mutually_exclusive_group(required=True) would be appropriate.

Check line 60 in recon.parsers for an example.

GreaterGoodest · 2020-04-16T14:09:57Z

made requested changes, working on adding tests/documentation

GreaterGoodest · 2020-04-16T16:56:08Z

all requested changes should be g2g

epi052

The changes look great, though there's a few more things to sort out, thanks for being patient!

epi052 · 2020-04-16T18:48:56Z

pipeline/recon/parsers.py

@@ -28,12 +28,15 @@
 # options for ReconShell's 'scan' command
 scan_parser = cmd2.Cmd2ArgumentParser()
 scan_parser.add_argument("scantype", choices_function=get_scans, help="which type of scan to run")
-scan_parser.add_argument(
+
+target_group = scan_parser.add_mutually_exclusive_group()


Let's add required=True to add_mutually_exclusive_group here.

epi052 · 2020-04-16T18:49:32Z

pipeline/recon/parsers.py

-scan_parser.add_argument(
+
+target_group = scan_parser.add_mutually_exclusive_group()
+target_group.add_argument(
    "--target-file",
    completer_method=cmd2.Cmd.path_complete,
    help="file created by the user that defines the target's scope; list of ips/domains (required)",


can remove (required) here from help string

epi052 · 2020-04-16T18:54:35Z

tests/test_recon/test_parsers.py

+def test_scan_parser_port_mutual_exclusion():
    with pytest.raises(SystemExit):
        scan_parser.parse_args(["FullScan", "--ports", "1000", "--top-ports", "1000", "--target-file", "required"])


+def test_scan_parser_tgt_mutual_exclusion():
+    with pytest.raises(SystemExit):
+        scan_parser.parse_args(["FullScan", "--top-ports", "1000", "--target-file", "required", "--target", "required"])
+
+


Instead of two tests that differ only by parameters, we can use parameterize; example below. May require some tweaking to be correct

@pytest.mark.parametrize("option_one, option_two", [("--ports", "--top-ports"), ("--target-file", "--target")]) def test_scan_parser_mutual_exclusion(option_one, option_two): with pytest.raises(SystemExit): scan_parser.parse_args(["FullScan", "--top-ports", "1000", option_one, "required", option_two, "required"])

epi052 · 2020-04-16T18:57:07Z

pipeline/recon-pipeline.py

+        if args.target:
+            tgt_file_fd, tgt_file_path = tempfile.mkstemp()  # temp file to hold target for later parsing
+            Path(tgt_file_path).write_text(args.target)
+            args.__statement__.arg_list[args.__statement__.arg_list.index("--target") + 1] = os.path.abspath(


os.path.abspath still present

Ref: #31 (comment)

epi052 · 2020-04-16T18:58:25Z

pipeline/recon-pipeline.py

@@ -235,6 +236,14 @@ def do_scan(self, args):
        # luigi --module pipeline.recon.web.webanalyze WebanalyzeScan --target-file tesla --top-ports 1000 --interface eth0
        command = ["luigi", "--module", scans.get(args.scantype)[0]]

+        if args.target:
+            tgt_file_fd, tgt_file_path = tempfile.mkstemp()  # temp file to hold target for later parsing


Can we add a tgt_file_path.unlink() (or whatever's appropriate to clean it up) at the end of the function?

GreaterGoodest · 2020-04-16T22:31:53Z

Requests integrated!

epi052

Changes look great, and thank you for submitting the first PR to the project!

@GreaterGoodest

Co-authored-by: Ryan Good <[email protected]> * added initial skeleton; restructured project directories * removed workers directive from luigi; changed input to tko-subs * changed masscan command to use config.tool_paths * linted __init__ files and updated docstring for get_scans * added per-file-ignores for linting * recon-pipeline linted * PoC working for amass results -> db; rudimentary db mgmt commands also * more linting * added database management commands to the shell * db_location passes through to all tasks; masscan results added to db * removed unused imports from masscan.py * added ParseNmapOutput class to handle parsing for database storage * cleaned up repeat code * searchsploit results stored in db * lint/format * gobuster scans now stored in database * fixed test_recon tests to use db_location * fixed web tests * tkosub entries recorded in db * subjack scan results stored in database * webanalyze results stored in db * refactored older commits to use newer helper functions * refactored older commits to use newer helper functions * aquatone results stored in database refactored a few scans to use dbmanager helper functions refactored db structure wrt headers/screenshots added 80/443 to web_ports in config.py * fixed a few queries and re-added webanalyze to FullScan * view targets/endpoints done * overhauled nmap parsing * print all nmap_results good, next to focus on filtering * complex nmap filters complete * nmap printing done * updated pipfile * view web-technologies complete * view searchsploit results complete * removed filesystem code from amass * targetlist moved to db only * targets,amass,masscan all cutover to full database; added view ports * nmap fully db compliant * aquatone and webtargets db compliant * gobuster uses db now * webanalyze db compliant * all scans except corscanner are db compliant * recon tests passing * web tests passing * linted files * added tests for helpers.py and parsers.py * refactored some redundant code * added tests to pre-commit * updated amass tests and pre-commit version * updated recon.targets tests * updated nmap tests * updated masscan tests * updated config tests * updated web targets tests * added gobuster tests * added aquatone tests * added subdomain takeover and webanalyze tests; updated test data * removed homegrown sqlite target in favor of the sqla implementation * added tests for recon-pipeline.py * fixed cluge function to set __package__ globally * updated amass tests * updated targets tests * updated nmap tests * updated masscan tests * updated aquatone tests * updated nmap tests to account for no searchsploit * updated nmap tests to account for no searchsploit * updated masscan tests * updated subjack/tkosub tests * updated web targets tests * updated webanalyze tests * added corscanner tests * linted DBManager a bit * fixed weird cyclic import issue that only happened during docs build; housekeeping * added models tests, removed test_install dir * updated docs a bit; sidenav is wonky * fixed readthedocs requirements.txt * fixed issue where view results werent populated directly after scan * added new tests to pipeline; working on docs * updated a few overlooked view command items * updated tests to reflect changes to shell * incremental push of docs update * documentation done * updated exploitdb install * updated exploitdb install * updated seclists install * parseamass updates db in the event of no amass output * removed corscanner * added pipenv shell to install instructions per @GreaterGoodest * added pipenv shell to install instructions per @GreaterGoodest * added check for chromium-browser during aquatone install; closes #26 * added check for old recon-tools dir; updated Path.resolve calls to Path.expanduser.resolve; fixed very specific import bug due to filesystem location * added CONTIBUTING.md; updated pre-commit hooks/README * added .gitattributes for linguist reporting * updated tests * fixed a few weird bugs found during test * updated README * updated asciinema links in README * updated README with view command video * updated other location for url scheme /status * add ability to specify single target using --target (#31) * updated a few items in docs and moved tool-dict to tools-dir * fixed issue where removing tempfile without --verbose caused scan to fail

epi052 requested changes Apr 16, 2020

View reviewed changes

GreaterGoodest force-pushed the add-target branch from 49536ef to 706bea0 Compare April 16, 2020 16:33

epi052 requested changes Apr 16, 2020

View reviewed changes

add ability to specify single target using --target

d43dd56

GreaterGoodest force-pushed the add-target branch from 706bea0 to d43dd56 Compare April 16, 2020 22:31

epi052 approved these changes Apr 17, 2020

View reviewed changes

epi052 merged commit f132150 into epi052:store-results-in-a-database-project Apr 17, 2020

GreaterGoodest deleted the add-target branch April 17, 2020 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ability to specify single target using --target #31

add ability to specify single target using --target #31

GreaterGoodest commented Apr 16, 2020

epi052 left a comment

epi052 Apr 16, 2020 •

edited

Loading

epi052 Apr 16, 2020

epi052 Apr 16, 2020

epi052 Apr 16, 2020

epi052 Apr 16, 2020

epi052 Apr 16, 2020

GreaterGoodest commented Apr 16, 2020

GreaterGoodest commented Apr 16, 2020

epi052 left a comment

epi052 Apr 16, 2020

epi052 Apr 16, 2020

epi052 Apr 16, 2020

epi052 Apr 16, 2020

epi052 Apr 16, 2020

GreaterGoodest commented Apr 16, 2020

epi052 left a comment

		)
		scan_parser.add_argument("--target", help="ip or domain to target")

add ability to specify single target using --target #31

add ability to specify single target using --target #31

Conversation

GreaterGoodest commented Apr 16, 2020

epi052 left a comment

Choose a reason for hiding this comment

epi052 Apr 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GreaterGoodest commented Apr 16, 2020

GreaterGoodest commented Apr 16, 2020

epi052 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GreaterGoodest commented Apr 16, 2020

epi052 left a comment

Choose a reason for hiding this comment

epi052 Apr 16, 2020 •

edited

Loading