Skip to content

Fix tool errors from agent session + combined relay attack tool - #87

Merged
mkultraWasHere merged 8 commits into
mainfrom
nops/impacket-dep
Jul 14, 2026
Merged

mkultraWasHere merged 8 commits into
mainfrom
nops/impacket-dep

Conversation

@mkultraWasHere

@mkultraWasHere mkultraWasHere commented Jul 13, 2026 •

Copy link
Copy Markdown
Contributor

Fixes tool errors observed in agent session c0dd9138 and adds provisioning for coercion scripts.

Added

  • impacket_ntlmrelay_attack — combined NTLM relay + coercion tool that starts ntlmrelayx as a background subprocess, waits for listeners to bind, fires coercion (PetitPotam/DFSCoerce/ShadowCoerce), monitors for success indicators, and returns combined output in a single tool call
  • _build_ntlmrelayx_args extracted from impacket_ntlmrelayx for reuse by the combined tool
  • _build_coercion_command, _wait_for_relay_ready, _wait_for_relay_result, _kill_relay helpers for relay orchestration
  • scripts/install_coercion_tools.sh — auto-clones PetitPotam, DFSCoerce, and ShadowCoerce repos to /opt/ at sandbox provision time via dependencies.scripts
  • Readiness checks for nmap, petitpotam, dfscoerce, shadowcoerce in capability.yaml
  • impacket>=0.12.0 as a Python dependency — ensures the runtime venv has impacket importable by sys.executable
  • 24 unit tests covering nmap port quoting, certipy domain handling, ntlmrelayx arg building, coercion command building, and relay orchestration helpers

Changed

  • certipy_find now accepts structured target, username, domain, password, nt_hash params and routes through self.certipy() instead of raw args passthrough — prevents the agent from inventing nonexistent flags like -domain
  • Base certipy() method handles username="user@domain" and DOMAIN\user formats without double-appending domain
  • ad-attack-patterns skill references impacket_ntlmrelay_attack as the preferred tool for NTLM relay workflows

Breaking

  • certipy_find signature changed: target is now a required first positional arg, args is optional. Agents calling the old certipy_find(args=[...]) form must update to certipy_find(target="...", args=[...]). The generic certipy() method is unchanged as a fallback.

Fixed

  • nmap_service_scan no longer fails when the agent passes quoted or whitespace-padded port specs — cleaned before passing to nmap
  • Impacket tools (GetNPUsers, secretsdump, ntlmrelayx, lookupsid) no longer fail with ModuleNotFoundError when the runtime Python differs from the system Python where impacket is installed
  • ntlmrelayx no longer times out after 30s before coercion can fire — the combined tool handles the full relay lifecycle
  • Coercion command builder no longer passes both -p and -hashes when both are provided — hashes take priority
  • Relay drain loop capped at 5s total to prevent indefinite reads with noisy relay output

Notes

  • Version bumped to 2.1.0
  • Coercion scripts are now auto-installed via dependencies.scripts and validated via checks

mkultraWasHere and others added 4 commits July 12, 2026 21:11
The runtime's sys.executable didn't have impacket importable because
it was installed under a different Python version. Declaring it as a
capability dependency ensures the runtime venv has it.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… combined relay+coerce tool

Fixes three issues observed in agent session c0dd9138:

1. nmap_service_scan: strip wrapping quotes from ports parameter —
   agent sent '"80,1433"' which nmap rejected as illegal port spec.

2. certipy_find: add structured auth params (target, username, domain,
   password, nt_hash) instead of raw args passthrough. Prevents agent
   from inventing nonexistent flags like -domain. Also fix base
   certipy() to handle username="user@domain" without double-appending.

3. impacket_ntlmrelay_attack: new combined tool that starts ntlmrelayx
   as a background subprocess, waits for listeners to bind, fires
   coercion (petitpotam/dfscoerce/shadowcoerce), monitors for success
   indicators, and returns combined output. Solves the sequencing
   problem where ntlmrelayx must be running before coercion fires.

   Includes: _build_ntlmrelayx_args (extracted from existing method),
   _build_coercion_command, _wait_for_relay_ready,
   _wait_for_relay_result, _kill_relay helpers.

24 unit tests covering all changes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses a set of tool failures observed in a prior agent session by hardening several network-ops tools and adding a combined NTLM relay + coercion workflow so agents can run relay and coercion in the correct sequence within a single tool call.

Changes:

  • Hardened nmap_service_scan port handling and improved Certipy authentication argument construction (including a breaking certipy_find signature update).
  • Refactored ntlmrelayx argument construction and added a new combined impacket_ntlmrelay_attack tool with relay lifecycle orchestration helpers.
  • Added unit tests covering the above fixes and updated skill guidance + capability dependency/version metadata.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
capabilities/network-ops/tools/nmap.py Strips wrapping quotes from ports before passing to nmap -p.
capabilities/network-ops/tools/impacket.py Adds relay orchestration helpers, refactors ntlmrelayx arg building, and introduces combined impacket_ntlmrelay_attack.
capabilities/network-ops/tools/certipy.py Updates base certipy() username/domain handling and changes certipy_find to structured auth params routed through certipy().
capabilities/network-ops/tests/test_tool_fixes.py New unit tests for the nmap/certipy/impacket fixes and relay helper behavior.
capabilities/network-ops/skills/ad-attack-patterns/SKILL.md Updates guidance to prefer the new combined relay+coerce tool for NTLM relay workflows.
capabilities/network-ops/capability.yaml Bumps capability version and adds an impacket>=0.12.0 dependency.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread capabilities/network-ops/tools/nmap.py Outdated
Comment thread capabilities/network-ops/tools/impacket.py Outdated
Comment thread capabilities/network-ops/tools/impacket.py Outdated
Comment thread capabilities/network-ops/tools/impacket.py Outdated
Comment thread capabilities/network-ops/tools/impacket.py Outdated
Comment thread capabilities/network-ops/tools/impacket.py Outdated
Comment thread capabilities/network-ops/tests/test_tool_fixes.py Outdated
Comment thread capabilities/network-ops/tests/test_tool_fixes.py Outdated
Comment thread capabilities/network-ops/tests/test_tool_fixes.py Outdated
- Strip whitespace before quotes in nmap ports parameter
- Use asyncio.get_running_loop() instead of deprecated get_event_loop()
- Prefer hashes over password in coercion command (mutually exclusive)
- Derive relay readiness/coercion/monitoring timeouts from relay_timeout
  so total runtime respects the documented contract
- Guard test stubs to avoid clobbering real dreadnode package if present

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Comment thread capabilities/network-ops/tools/certipy.py
Comment thread capabilities/network-ops/tools/impacket.py
Comment thread capabilities/network-ops/capability.yaml
Don't append @Domain when username contains backslash (NTLM-style
DOMAIN\user format).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Comment thread capabilities/network-ops/tools/nmap.py
Comment thread capabilities/network-ops/tools/impacket.py
… loop

- nmap: compute cleaned ports before the truthy check to avoid passing
  empty string to -p
- relay drain loop: use a 5s total deadline instead of per-line timeout
  to prevent indefinite reads when ntlmrelayx is noisy

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comment thread capabilities/network-ops/tools/impacket.py
@mkultraWasHere
mkultraWasHere marked this pull request as ready for review July 14, 2026 00:27
…ecks

Add install_coercion_tools.sh to clone PetitPotam, DFSCoerce, and
ShadowCoerce repos at sandbox provision time. Add checks entries so the
capability warns if they're missing at runtime.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comment on lines +2445 to +2447
try:
# Readiness gets at most half the total budget, capped at 30s
ready_timeout = min(relay_timeout // 2, 30)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Refuted. The parameter defaults to 120 and @tool_method(catch=True) wraps the call. No agent would pass zero/negative — adding validation is defending against an impossible scenario.

Comment on lines +2466 to +2468
coerce_result = await execute(
coerce_cmd, timeout=coerce_timeout, env=env
)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Refuted. execute() does its own os.environ.copy() + env.update() internally (execute.py:78-80). Passing raw env is correct and consistent with every other tool call in the codebase.

@mkultraWasHere
mkultraWasHere merged commit bd592f2 into main Jul 14, 2026
6 checks passed
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.

2 participants