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

AF-61 ProxySSH for AWS and improved functionality #87

Merged
merged 18 commits into from
Dec 13, 2024
Merged

AF-61 ProxySSH for AWS and improved functionality #87

merged 18 commits into from
Dec 13, 2024

Conversation

sparshev
Copy link
Collaborator

@sparshev sparshev commented Dec 2, 2024

This PR implements full SSH debug access, including:

  • Rewriting of the proxyssh implementation to support SCP/Port pass-through
    • Integration tests for proxyssh for SSH/SCP/Port forwarding to ensure functionality will stay with us
  • Support for AWS machines SSH connection - now the remote machine can be connected with the private key

Related Issue

#61

Motivation and Context

Have no idea how we can live without that

How Has This Been Tested?

Manually & Automatically

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@sparshev sparshev added the enhancement New feature or request label Dec 2, 2024
@sparshev sparshev self-assigned this Dec 2, 2024
tests/helper/ssh.go Fixed Show fixed Hide fixed
tests/helper/ssh.go Fixed Show fixed Hide fixed
* Using address for session key potentially could lead to security
  issues, so replaced by SessionID which should have less collisions
* Not storing generated password or key in DB, only hash & key
* Moved to ECDSA instead of RSA since takes less space and farely strong
lib/proxyssh/proxy.go Dismissed Show dismissed Hide dismissed
tests/helper/ssh.go Dismissed Show dismissed Hide dismissed
tests/helper/ssh.go Dismissed Show dismissed Hide dismissed
@sparshev sparshev requested a review from svenevs December 10, 2024 00:50
Copy link
Collaborator

@svenevs svenevs left a comment

Choose a reason for hiding this comment

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

This is awesome, way better than the previous implementation by a landslide 🥳

I added a number of review comments, some are simple quick-to-address spelling issues. But a number of the other comments probably should get closed as #wontfix or #irrelevant, just jotted down all the thoughts I had going through the code. Nice work!

docs/openapi.yaml Outdated Show resolved Hide resolved
lib/crypt/crypt.go Show resolved Hide resolved
lib/crypt/crypt.go Show resolved Hide resolved
lib/crypt/ssh_key.go Show resolved Hide resolved
lib/drivers/docker/driver.go Show resolved Hide resolved
lib/util/cmd.go Outdated Show resolved Hide resolved
tests/helper/files.go Show resolved Hide resolved
tests/helper/ssh.go Show resolved Hide resolved
tests/helper/ssh.go Show resolved Hide resolved
docs/openapi.yaml Outdated Show resolved Hide resolved
svenevs
svenevs previously approved these changes Dec 11, 2024
Copy link
Collaborator

@svenevs svenevs left a comment

Choose a reason for hiding this comment

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

Pending CI getting fixed, looks good to me! :shipit:

Hash is generated with random salt by default and if we don't know
salt (our case) - we can't pull from DB and remove the access quickly.
So instead I store just hash there with empty salt. This works, because
the access is temporary and one-shot, so technically no issues, but
if we will change that at some point - this mechanism is needed to be
reworked.
@sparshev
Copy link
Collaborator Author

Found a bug thanks to tests - I implemented hashing of the passwords and forgot it stores random salt, so we can't easily generate the same on ssh request without knowing salt of it.

So instead I store just hash there with empty salt. This works, because the access is temporary and one-shot, so technically no issues, but if we will change that at some point - this mechanism is needed to be reworked.

svenevs
svenevs previously approved these changes Dec 11, 2024
Apparently the Mac & Lin network layers (or most probably ssh clients of
different versions) are handling the input a bit differently. If you will
easily get away with just buffer as input on Mac and SSH (9.6) client
will not close the src->dst channel - it's not the case with Linux SSH
(9.2p1) client: it will do that and you would never see any response from
the server. So instead we now emulating the ssh terminal input as well
and not closing the input until it's necessary.

Also I found a wrong src->dst messages, so flipped them over and grately
improved the ssh testing where it knocks the mock SSHD first and then
retries the same with proxy. And last thing - found a couple of lost
errors because serve is running as a goroutine and overall increased
verbosity of the mock sshd, so should be much easier to debug now.
@sparshev
Copy link
Collaborator Author

Apparently the Mac & Lin network layers (or most probably ssh clients of different versions) are handling the input a bit differently. If you will easily get away with just buffer as input on Mac and SSH (9.6) client will not close the src->dst channel - it's not the case with Linux SSH (9.2p1) client: it will do that and you would never see any response from the server. So instead we now emulating the ssh terminal input as well and not closing the input until it's necessary.

Also I found a wrong src->dst messages, so flipped them over and grately improved the ssh testing where it knocks the mock SSHD first and then retries the same with proxy. And last thing - found a couple of lost errors because serve is running as a goroutine and overall increased verbosity of the mock sshd, so should be much easier to debug now.

svenevs
svenevs previously approved these changes Dec 12, 2024
svenevs
svenevs previously approved these changes Dec 12, 2024
@sparshev sparshev merged commit 06dfb0a into main Dec 13, 2024
7 checks passed
@sparshev sparshev deleted the AF-61 branch December 13, 2024 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants