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

Change: Add workaround for non-standard SSH ports #925

Merged
merged 5 commits into from
Feb 3, 2023

Conversation

MitchDrage
Copy link
Contributor

@MitchDrage MitchDrage commented Feb 2, 2023

What:
This PR is to address #924
I don't think this is the most elegent solution - it would be preferable to use Paramiko's handling of non-standard ports, but TBH, I can't decypher how they are doing it right now.
For now, this PR includes the workaround I've built to get my solution working.

What I don't think this PR will solve is hashed or salted hostnames.

Why:
To allow SSHConnection to connect on non-standard ports and store/resuse known_host entries.

How:
I've used local edits per the PR. Verfied using the script referenced in #924.
I've confirmed:

  • Clear known_hosts, run script using Paramiko to add the host to the known_hosts file, then run SSHConnection = successful connection using the known_hosts file.
  • Clear known_hosts, connect using SSHConnection, answer 'yes' to adding the server to known_hosts, disconnect and run again, second connection doesn't prompt to add to known_hosts = successful.

Checklist:

  • Tests
  • Conventional commit message
  • Documentation - N/A

@MitchDrage MitchDrage requested a review from a team as a code owner February 2, 2023 10:28
Copy link
Contributor

@bjoernricks bjoernricks left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution! Could you add a new "global" variable DEFAULT_SSH_PORT = 22 and use it instead. Using such a variable improves the readability. Also could you add a Change: before you commit message. In that case the message gets added to the next release notes. It's the conventional commits thing.

@codecov
Copy link

codecov bot commented Feb 2, 2023

Codecov Report

Merging #925 (e4b3e6b) into main (1f206aa) will decrease coverage by 0.19%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##             main     #925      +/-   ##
==========================================
- Coverage   98.44%   98.26%   -0.19%     
==========================================
  Files          58       58              
  Lines        4311     4319       +8     
  Branches     1032     1037       +5     
==========================================
  Hits         4244     4244              
- Misses         53       59       +6     
- Partials       14       16       +2     
Impacted Files Coverage Δ
gvm/connections.py 84.25% <25.00%> (-2.74%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bjoernricks
Copy link
Contributor

And you need to run black on the code. At best activate git commit hooks by running poetry run autohooks activate --force.

@MitchDrage MitchDrage changed the title Add workaround for non-standard SSH ports Change: Add workaround for non-standard SSH ports Feb 2, 2023
@MitchDrage
Copy link
Contributor Author

I've made the updates you requested. Please let me know if there is anything else you would like me to do.

@bjoernricks
Copy link
Contributor

Oh didn't recognize that DEFAULT_SSH_PORT already existed. nice :-)

@bjoernricks bjoernricks self-requested a review February 3, 2023 06:39
@bjoernricks bjoernricks enabled auto-merge (squash) February 3, 2023 07:34
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