Fix SSH brute-force success detection and error handling #1184
Conversation
- Ensure brute_force returns empty dict on failure and non-empty on success - Prevent false positives caused by empty regex in ssh_brute module
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRefactors SSH brute-force connection handling in a single file to add socket and Paramiko exception handling, timeout support, and stricter transport-based validation. The public function signature remains unchanged while internal logic gains more robust error handling with try/except/finally flow and explicit transport state verification before returning connection success. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip ✨ Issue Enrichment is now available for GitHub issues!CodeRabbit can now help you manage issues more effectively:
Disable automatic issue enrichmentTo disable automatic issue enrichment, add the following to your issue_enrichment:
auto_enrich:
enabled: falseThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
nettacker/core/lib/ssh.py (4)
12-17: Consider removing unused*argsand adding a docstring.The
*argsparameter is never used. Based onBaseEngine.run()which callsaction(**sub_step), only keyword arguments are passed. Additionally, per coding guidelines, public APIs in the nettacker package should have docstrings.- def brute_force(self, *args, **kwargs): + def brute_force(self, **kwargs): + """ + Attempt SSH authentication with the given credentials. + + Returns a dict with connection details on success, empty dict on failure. + """ host = kwargs["host"]
22-40: Line 26 exceeds the 99-character limit.The
auth_strategyline is too long. Consider breaking it up for readability and to comply with the line length guideline.try: + if password: + auth_strategy = Password(username=username, password_getter=lambda: password) + else: + auth_strategy = NoneAuth(username=username) + connection.connect( hostname=host, port=port, - auth_strategy=Password(username=username, password_getter=lambda: password) if password else NoneAuth(username=username), + auth_strategy=auth_strategy, timeout=timeout, look_for_keys=False, allow_agent=False, banner_timeout=timeout, )The transport state verification before returning is a good defensive check—this correctly ensures the connection is truly established before reporting success.
48-49: Bareexcept Exceptionmay hide unexpected bugs.While catching all exceptions prevents scan crashes, it can mask programming errors. Consider logging unexpected exceptions at DEBUG level to aid troubleshooting.
+ except (socket.timeout, socket.error, ConnectionRefusedError, OSError): + return {} except Exception: + logging.debug("Unexpected error during SSH brute-force: %s:%s", host, port, exc_info=True) return {}
50-54: Consider narrowing the exception or adding debug logging in cleanup.Silently swallowing all exceptions during close can hide issues. While cleanup failures shouldn't propagate, logging at DEBUG level helps diagnose connection issues.
finally: try: connection.close() - except Exception: - pass + except Exception: + logging.debug("Error closing SSH connection to %s:%s", host, port, exc_info=True)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nettacker/core/lib/ssh.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use 4-space indents in Python code
Limit lines to 99 characters (ruff/ruff-format/isort profile=black)
Module and file names should use lower_snake_case
Function and variable names should use lower_snake_case
Class names should use PascalCase
Constants should use UPPER_SNAKE_CASE
Keep functions small and add type hints where practical
Files:
nettacker/core/lib/ssh.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/lib/ssh.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/lib/ssh.py
🧬 Code graph analysis (1)
nettacker/core/lib/ssh.py (2)
nettacker/core/lib/base.py (1)
BaseEngine(28-313)nettacker/core/lib/telnet.py (1)
brute_force(9-22)
🪛 Ruff (0.14.8)
nettacker/core/lib/ssh.py
12-12: Unused method argument: args
(ARG002)
35-40: Consider moving this statement to an else block
(TRY300)
48-48: Do not catch blind exception: Exception
(BLE001)
53-54: try-except-pass detected, consider logging the exception
(S110)
53-53: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
nettacker/core/lib/ssh.py (1)
1-5: LGTM!The added imports for
socket,AuthenticationException, andSSHExceptionare appropriate for the robust error handling introduced in this PR.
pUrGe12
left a comment
pUrGe12
left a comment
There was a problem hiding this comment.
You should remove the bloat code and please run pre-commit for ruff to handle the styling.
| return {} | ||
| except SSHException: | ||
| return {} | ||
| except (socket.timeout, socket.error, ConnectionRefusedError, OSError): |
There was a problem hiding this comment.
You don't need to use both OSError and socket.error. Python3+ they are basically aliases. Also, you really aren't doing anything special by handling them separately.
| port = kwargs["port"] | ||
| username = kwargs["username"] | ||
| password = kwargs["password"] | ||
| password = kwargs.get("password") |
There was a problem hiding this comment.
don't change what's not broken
| import logging | ||
|
|
||
| from paramiko import SSHClient, AutoAddPolicy | ||
| import socket |
There was a problem hiding this comment.
you "probably" don't need this here... look at the other comment below
2 participants
Summary
This PR improves the SSH brute-force module reliability by fixing authentication success detection and making error handling consistent with Nettacker’s engine expectations.
What was changed
How it was tested
ssh_brutepy_compileImpact
ssh_bruteChecklist