-
Notifications
You must be signed in to change notification settings - Fork 3
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
Conversation
* 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
There was a problem hiding this 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!
There was a problem hiding this 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!
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.
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. |
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.
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. |
This PR implements full SSH debug access, including:
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
Checklist: