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

ssh module refactoring separate auth/shell checks #331

Closed
wants to merge 2 commits into from

Conversation

nikaiw
Copy link
Contributor

@nikaiw nikaiw commented Jun 2, 2024

This pull request aims to refactor the code to make it more readable but also to properly separate the code that can be linked to authentication from the code linked to the test concerning the shell obtained

Copy link
Contributor

@NeffIsBack NeffIsBack left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the PR!

Please describe briefly (in the PR description) what has been done and why, so we know what this PR aims to achieve without diving into past issues etc.
If possible please also provide example output screenshots

Comment on lines -228 to -237
except AuthenticationException:
self.logger.fail(f"{username}:{process_secret(password)}")
except SSHException as e:
if "Invalid key" in str(e):
self.logger.fail(f"{username}:{process_secret(password)} Could not decrypt private key, error: {e}")
if "Error reading SSH protocol banner" in str(e):
self.logger.error(f"Internal Paramiko error for {username}:{process_secret(password)}, {e}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please preserve the current Exception handling, as with this we can catch some weird paramiko issues

Copy link
Contributor Author

@nikaiw nikaiw Sep 12, 2024

Choose a reason for hiding this comment

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

Those exception are already handled in create_conn_obj() However, I could add the logger calls to keep the previous behavior.

Edit: actually, I didn't remembered well, there are 2 connections, a first one to check that the service is present with conn_obj() which as exception handling. A second one with plaintext_login() for which I added handle_connection_failure() to handle all kind of exception

nxc/protocols/ssh.py Outdated Show resolved Hide resolved
@NeffIsBack NeffIsBack linked an issue Sep 1, 2024 that may be closed by this pull request
@NeffIsBack
Copy link
Contributor

While we are at it we should probably also catch the timeout error in #358.

@NeffIsBack NeffIsBack added the reviewed code Label for when a static code review was done label Sep 1, 2024
@NeffIsBack
Copy link
Contributor

@nikaiw any update on this?

* refactoring separate auth/shell checks
* move disable log statement to logger
@nikaiw
Copy link
Contributor Author

nikaiw commented Nov 16, 2024

hello @NeffIsBack sorry this was sitting in my dev folder for a while.
I'm not sure what I wanted to double check before commiting

@NeffIsBack NeffIsBack removed the reviewed code Label for when a static code review was done label Jan 2, 2025
@NeffIsBack
Copy link
Contributor

While i think the current implementation is not ideal, this PR introduces way too many problems with private key authentication, which work in the main branch. I tried fixing them in the last hour, but at this point i will need to refactor the implementation anyways.

I will continue working on ssh in #531, but closing this one for now.
Still thanks for your work, i will implement some ideas in the new PR!

@NeffIsBack NeffIsBack closed this Jan 2, 2025
@NeffIsBack NeffIsBack mentioned this pull request Jan 2, 2025
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix This Pull Request fixes a bug enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exception with ssh if channel is closed during interaction
2 participants