-
Notifications
You must be signed in to change notification settings - Fork 142
feat: adds ssh support for git operations #987
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
83a7496
to
399bf6c
Compare
@JamieSlome @coopernetes, please take a look when you get a chance |
399bf6c
to
f514691
Compare
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.
Just a preliminary review of the code changes. I'll run it and test more thoroughly afterwards!
addSSHKey(argv.username, argv.keyPath); | ||
} else if (argv.action === 'remove') { | ||
// TODO: Implement remove SSH key | ||
console.error('Error: SSH key: Remove action not implemented yet'); |
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.
Is the required removeSSHKey
function implemented now? Maybe all we need to do is plug it in here 😃
|
||
class SSHServer { | ||
constructor() { | ||
// TODO: Server config could go to config file |
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.
Just bringing attention to this TODO
// Get remote host from config | ||
const remoteUrl = new URL(config.getProxyUrl()); | ||
|
||
// TODO: Connection options could go to config |
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.
Bringing attention to this TODO
. Is there a reason why the default port is 22
in some places, but 2222
in the sample proxy.config.json
? 🤔
config.getSSHConfig().hostKey.privateKeyPath, | ||
), | ||
// Ensure these settings are explicitly set for the proxy connection | ||
windowSize: 1024 * 1024, // 1MB window size |
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.
Might be best to extract these (and the repeated ones from earlier in the file) into constants so they can be reused.
// Add SSH public key | ||
router.post('/:username/ssh-keys', async (req, res) => { | ||
if (!req.user) { | ||
res.status(401).json({ error: 'Authentication required' }); |
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.
I'm wondering if this could be 'Login required'
to make it a bit more clear that the user should login first. Since it will mainly be used as an API, the flow may not be obvious.
Feel free to ignore this if the flow (including login) is documented!
} | ||
|
||
// Strip the comment from the key (everything after the last space) | ||
const keyWithoutComment = publicKey.split(' ').slice(0, 2).join(' '); |
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.
Should we add some validation here to make sure that the key is in a valid format? Or would this be more appropriate for the db functions?
Right now, invalid formatting would get caught in the try/catch below, but the user-facing error wouldn't be descriptive.
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.
Review (Part 2): I managed to confirm that the general flow works as expected, but I'm wondering how to trigger the proxy action chain through SSH.
Other than that, improving user-facing errors and some documentation improvements would be great for UX. 🙂
// TODO: Server config could go to config file | ||
this.server = new ssh2.Server( | ||
{ | ||
hostKeys: [require('fs').readFileSync(config.getSSHConfig().hostKey.privateKeyPath)], |
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 line blows up when running git-proxy (npm run start
) if the file is not present:
[0] Service Listening on 8080
[0] node:fs:562
[0] return binding.open(
[0] ^
[0]
[0] Error: ENOENT: no such file or directory, open './.ssh/host_key'
[0] at Object.openSync (node:fs:562:18)
[0] at Object.readFileSync (node:fs:446:35)
[0] at new SSHServer (/home/juan/Desktop/Juan/Projects/git-proxy/src/proxy/ssh/server.js:11:34)
[0] at proxyPreparations (/home/juan/Desktop/Juan/Projects/git-proxy/src/proxy/index.ts:52:23)
[0] at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
[0] at async Object.start (/home/juan/Desktop/Juan/Projects/git-proxy/src/proxy/index.ts:68:3) {
[0] errno: -2,
[0] code: 'ENOENT',
[0] syscall: 'open',
[0] path: './.ssh/host_key'
[0] }
[0]
[0] Node.js v22.13.1
[0] npm run server exited with code 1
^C[1] npm run client exited with code SIGINT
Some validation here would be great, perhaps we can recover from the error and let git-proxy run, or stop the server and let the user know that they should either provide a valid file, or disable ssh in the config.
// TODO: Server config could go to config file | ||
this.server = new ssh2.Server( | ||
{ | ||
hostKeys: [require('fs').readFileSync(config.getSSHConfig().hostKey.privateKeyPath)], |
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.
Another issue I found, is that the path doesn't seem to resolve when using ~
. It might be nice to be able to set the config to ~/.ssh/host_key
so that it resolves to the home directory. Not a huge deal though! 👍🏼
findUser(username) | ||
.then((user) => { | ||
if (!user) { | ||
reject(new Error('User not found')); |
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 line gets successfully triggered when the user isn't found. However, the CLI just prints this out instead of bubbling up the User not found
error message (which is much more descriptive):
Error: SSH key: 'Request failed with status code 500'
} | ||
if (!user.publicKeys.includes(publicKey)) { | ||
user.publicKeys.push(publicKey); | ||
exports.updateUser(user).then(resolve).catch(reject); |
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.
Oddly, the exports
here is tripping up my runtime:
[0] Adding SSH key { targetUsername: 'admin', keyWithoutComment: '-----BEGIN OPENSSH' }
[0] Error adding SSH key: TypeError: exports.updateUser is not a function
[0] at <anonymous> (/home/juan/Desktop/Juan/Projects/git-proxy/src/db/file/users.ts:112:19)
[0] at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
Removing the exports
causes a TypeError
due to how the Promise is handled, but we can fix it like this:
exports.updateUser(user).then(resolve).catch(reject); | |
updateUser(user).then(() => resolve(user)).catch(reject); |
return; | ||
} | ||
user.publicKeys = user.publicKeys.filter((key) => key !== publicKey); | ||
exports.updateUser(user).then(resolve).catch(reject); |
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.
Same as above, this made the code run properly for me:
exports.updateUser(user).then(resolve).catch(reject); | |
updateUser(user).then(() => resolve(user)).catch(reject); |
I might be messing something up in my flow though. I'm executing npm run start
for git-proxy
and then separately executing the CLI commands via:
node ./packages/git-proxy-cli/index.js ssh-key --action add --username admin --keyPath /home/juan/.ssh/host_key.pub
}); | ||
|
||
proxyGitSsh.on('error', (err) => { | ||
console.error('[SSH] Remote SSH error with proxy key:', err); |
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.
Upon entering an invalid key, I trigger the following error on line 638:
git-proxy backend
[0] [GitHub SSH Debug] Handshake completed
[0] [GitHub SSH Debug] Outbound: Sending SERVICE_REQUEST (ssh-userauth)
[0] [GitHub SSH Debug] Inbound: Received EXT_INFO
[0] [GitHub SSH Debug] Inbound: Received SERVICE_ACCEPT (ssh-userauth)
[0] [GitHub SSH Debug] Outbound: Sending USERAUTH_REQUEST (none)
[0] [GitHub SSH Debug] Inbound: Received USERAUTH_FAILURE (publickey)
[0] [GitHub SSH Debug] Client: none auth failed
[0] [GitHub SSH Debug] Outbound: Sending USERAUTH_REQUEST (publickey -- check)
[0] [GitHub SSH Debug] Inbound: Received USERAUTH_FAILURE (publickey)
[0] [GitHub SSH Debug] Client: publickey (rsa-sha2-256) auth failed
[0] [GitHub SSH Debug] Client: publickey auth failed
[0] [SSH] Remote SSH error with proxy key: Error: All configured authentication methods failed
[0] at doNextAuth (/home/juan/Desktop/Juan/Projects/git-proxy/node_modules/ssh2/lib/client.js:863:21)
[0] at tryNextAuth (/home/juan/Desktop/Juan/Projects/git-proxy/node_modules/ssh2/lib/client.js:1080:7)
[0] at USERAUTH_FAILURE (/home/juan/Desktop/Juan/Projects/git-proxy/node_modules/ssh2/lib/client.js:428:11)
[0] at 51 (/home/juan/Desktop/Juan/Projects/git-proxy/node_modules/ssh2/lib/protocol/handlers.misc.js:408:16)
[0] at Protocol.onPayload (/home/juan/Desktop/Juan/Projects/git-proxy/node_modules/ssh2/lib/protocol/Protocol.js:2059:10)
[0] at AESGCMDecipherBinding.decrypt (/home/juan/Desktop/Juan/Projects/git-proxy/node_modules/ssh2/lib/protocol/crypto.js:1086:26)
[0] at Protocol.parsePacket [as _parse] (/home/juan/Desktop/Juan/Projects/git-proxy/node_modules/ssh2/lib/protocol/Protocol.js:2028:25)
[0] at Protocol.parse (/home/juan/Desktop/Juan/Projects/git-proxy/node_modules/ssh2/lib/protocol/Protocol.js:313:16)
[0] at Socket.<anonymous> (/home/juan/Desktop/Juan/Projects/git-proxy/node_modules/ssh2/lib/client.js:773:21)
[0] at Socket.emit (node:events:524:28) {
[0] level: 'client-authentication'
[0] }
[0] [SSH Debug] [518855.983928741] Outbound: Sending CHANNEL_DATA (r:0, 51)
[0] [SSH Debug] [518855.983928741] Outbound: Sending CHANNEL_EOF (r:0)
[0] [SSH Debug] [518855.983928741] Outbound: Sending CHANNEL_CLOSE (r:0)
[0] [GitHub SSH Debug] Outbound: Sending DISCONNECT (11)
[0] [SSH Debug] [518855.983928741] Inbound: CHANNEL_CLOSE (r:0)
[0] [SSH Debug] [518855.983928741] Inbound: Received DISCONNECT (11, "disconnected by user")
[0] [SSH Debug] [518855.983928741] Socket ended
[0] [SSH] Client disconnected
[0] [SSH] Client disconnected
[0] [SSH Debug] [518855.983928741] Socket closed
[0] [SSH] Client connection closed
[0] [GitHub SSH Debug] Socket ended
[0] [GitHub SSH Debug] Socket closed
However, the user facing CLI gives an unrelated error:
User-facing CLI
$ git clone ssh://git@localhost:2222/jescalada/git-proxy-experiments.git
Cloning into 'git-proxy-experiments'...
fatal: protocol error: bad line length character: Erro
Would be great if we could get a more descriptive error for this common scenario (bad key).
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.
In my case, this was because of my GitHub authentication not set up correctly.
|
||
// Execute the Git command on remote | ||
remoteGitSsh.exec( | ||
command, |
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.
I might be missing something in my configuration, but when I execute git push
, the git-proxy action chain isn't triggered:
$ git push ssh://git@localhost:2222/jescalada/git-proxy-experiments.git
Enumerating objects: 5, done.
Counting objects: 100% (5/5), done.
Delta compression using up to 12 threads
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), 331 bytes | 331.00 KiB/s, done.
Total 3 (delta 1), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (1/1), completed with 1 local object.
To ssh://localhost:2222/jescalada/git-proxy-experiments.git
fa06cb5..ea81914 main -> main
It directly updated my upstream repo instead of going through the proxy's approval process.
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.
That said, after setting up the public key in the GitHub SSH page, the SSH works great! The SSH key setup is worth explaining in detail in the ssh.md
guide.
package.json
Outdated
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.
The test script should be as follows (so that the ssh
tests get covered in the PR):
"test": "NODE_ENV=test ts-mocha './test/**/*.test.js' --exit",
Merging the current main into the PR might fix this too.
(I changed this by accident in one of my TS refactor PRs. 😓)
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.
Review (Part 3): Just took a look at the unit tests. It would be great to cover some more edge cases and get more granular testing by passing error messages through the ctx.reject
(if possible).
If you'd like, I can help out with the documentation refactor and unit test coverage 🙂
accept(); | ||
} else { | ||
console.log('[SSH] Rejecting unknown global request:', info.type); | ||
reject(); |
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.
Would it be better if we sent the error string as a reject
argument? Might be better for more granular testing.
expect(mockClient.on.calledWith('authentication')).to.be.true; | ||
}); | ||
|
||
describe('authentication', () => { |
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.
Would be great to add extra tests for authentication failure cases. These are a few passing sample tests I wrote:
describe('authentication', () => { | |
describe('authentication', () => { | |
it('should reject invalid public keys', async () => { | |
const mockCtx = { | |
method: 'publickey', | |
key: { | |
algo: 'ssh-rsa', | |
data: Buffer.from('invalid-key-data'), | |
comment: 'invalid-key', | |
}, | |
accept: sinon.stub(), | |
reject: sinon.stub(), | |
}; | |
mockDb.findUserBySSHKey.resolves(null); | |
server.handleClient(mockClient); | |
const authHandler = mockClient.on.withArgs('authentication').firstCall.args[1]; | |
await authHandler(mockCtx); | |
expect(mockCtx.reject.calledOnce).to.be.true; | |
expect(mockClient.username).to.be.null; | |
expect(mockClient.userPrivateKey).to.be.null; | |
}); | |
it('should reject invalid passwords', async () => { | |
const mockCtx = { | |
method: 'password', | |
username: 'test-user', | |
password: 'invalid-password', | |
accept: sinon.stub(), | |
reject: sinon.stub(), | |
}; | |
mockDb.findUser.resolves({ | |
username: 'test-user', | |
password: '$2a$10$mockHash', | |
}); | |
const bcrypt = require('bcryptjs'); | |
sinon.stub(bcrypt, 'compare').resolves(false); | |
server.handleClient(mockClient); | |
const authHandler = mockClient.on.withArgs('authentication').firstCall.args[1]; | |
await authHandler(mockCtx); | |
expect(mockDb.findUser.calledWith('test-user')).to.be.true; | |
expect(bcrypt.compare.calledWith('invalid-password', '$2a$10$mockHash')).to.be.true; | |
expect(mockCtx.reject.calledOnce).to.be.true; | |
}); | |
it('should handle a missing private key', async () => { | |
const mockCtx = { | |
method: 'publickey', | |
key: null, | |
accept: sinon.stub(), | |
reject: sinon.stub(), | |
}; | |
mockDb.findUserBySSHKey.resolves({ username: 'test-user' }); | |
server.handleClient(mockClient); | |
const authHandler = mockClient.on.withArgs('authentication').firstCall.args[1]; | |
await authHandler(mockCtx); | |
expect(mockCtx.reject.calledOnce).to.be.true; | |
expect(mockClient.username).to.be.null; | |
expect(mockClient.userPrivateKey).to.be.null; | |
}); |
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.
Extra authentication test case ideas:
- Malformed SSH keys
- Currently, malformed keys go through the
findUserBySSHKey
function which logs[SSH] No user found with this SSH key
instead of throwing error
- Currently, malformed keys go through the
- Invalid auth method (neither
publickey
norpassword
) - Nonexistent user
command: "git-upload-pack 'test/repo'", | ||
}; | ||
|
||
mockChain.executeChain.resolves({ |
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.
Would be great to include some cases with error: true
/blocked: true
:
Sample
I wrote a sample test for this, but it seems to be failing:
it('should handle git-upload-pack command with error in chain', async () => {
const mockInfo = {
command: "git-upload-pack 'test/repo'",
};
mockChain.executeChain.resolves({
error: true,
blocked: true,
});
server.handleSession(mockAccept, mockReject);
const execHandler = mockSession.on.withArgs('exec').firstCall.args[1];
await execHandler(mockAccept, mockReject, mockInfo);
expect(mockChain.executeChain.calledWith({
method: 'GET',
originalUrl: " 'test/repo",
isSSH: true,
})).to.be.true;
expect(mockStream.end.calledOnce).to.be.true;
expect(mockStream.exit.calledOnce).to.be.true;
expect(mockStream.exit.calledWith(1)).to.be.true;
});
Error:
2) SSHServer
handleSession
should handle git-upload-pack command with invalid repo:
TypeError: stream.write is not a function
at SSHServer.<anonymous> (src/proxy/ssh/server.js:664:18)
at Generator.next (<anonymous>)
at fulfilled (src/proxy/ssh/server.js:5:58)
at processTicksAndRejections (node:internal/process/task_queues:105:5)
|
||
it('should handle git-receive-pack command', async () => { | ||
const mockInfo = { | ||
command: "git-receive-pack 'test/repo'", |
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.
Other possible test cases for handleSession
:
- Invalid repo handling
- Currently gets stuck on
[GitHub SSH Debug] Client: Trying github.com on port 22 ...
without throwing error.executeChain
doesn't seem to get called
- Currently gets stuck on
- Edge case tests:
- Empty repository name, whitespace inputs
- SSH injection (example:
command: "git-upload-pack 'repo'; rm -rf /; #",
)
f514691
to
6667d15
Compare
This PR implements SSH support for Git operations, resolving #27. The implementation allows users to perform Git operations over SSH protocol, providing an alternative to HTTPS for repository access.
Changes
ssh2
libraryconfig.schema.json
docs/SSH.md
test/ssh/sshServer.test.js
Configuration
SSH support can be configured in the main configuration file:
Usage
Users can connect using standard Git SSH commands. The command format depends on the configured port:
If port is set to 22 (default SSH port):
If using a custom port (e.g., 2222):
Testing
Added comprehensive tests for SSH functionality:
Documentation
Added detailed documentation in
docs/SSH.md
covering:Future Improvements
This implementation provides a secure and reliable way to perform Git operations over SSH, giving users more flexibility in how they interact with their repositories.
Below is a summary of the most important changes:
SSH Feature Implementation:
ssh
configuration section inconfig.schema.json
andproxy.config.json
to enable SSH support, configure the port, and specify host key paths. ([[1]](https://github.com/finos/git-proxy/pull/987/files#diff-be1695b1e63a508d59982601f9e1fb7f58247deecb1e427adb77bcad758ae5e5R82-R111)
,[[2]](https://github.com/finos/git-proxy/pull/987/files#diff-c465aafa0fe603e2d28b017938f55e5ce3253aac7aa303efeabfc06a4ad52d5fR105-R112)
)docs/SSH.md
to provide comprehensive documentation for configuring, using, and troubleshooting the SSH feature. ([docs/SSH.mdR1-R165](https://github.com/finos/git-proxy/pull/987/files#diff-1d0301881738da7c699d7634669c5156c345a5094d6c012cfa7254cbdf15cbd2R1-R165)
)addSSHKey
function andssh-key
command). ([[1]](https://github.com/finos/git-proxy/pull/987/files#diff-ee51eb1c2264303569f2a8fa9f5bb2de1b6b51eed24eed943d079f341f4139b0R310-R363)
,[[2]](https://github.com/finos/git-proxy/pull/987/files#diff-ee51eb1c2264303569f2a8fa9f5bb2de1b6b51eed24eed943d079f341f4139b0R494-R524)
,[[3]](https://github.com/finos/git-proxy/pull/987/files#diff-c3c551630c7afb35431840ab36bd8cc771deb1fa385b73804eaf8da4b8db7e0cR1-R122)
)Database Enhancements:
addPublicKey
,removePublicKey
,findUserBySSHKey
). ([[1]](https://github.com/finos/git-proxy/pull/987/files#diff-d90c8ba033b20fb64a27fb6f16a94afa5ebc2f25bb7af6ec42cd260dcbf46710R43-R46)
,[[2]](https://github.com/finos/git-proxy/pull/987/files#diff-d90c8ba033b20fb64a27fb6f16a94afa5ebc2f25bb7af6ec42cd260dcbf46710R98-R155)
,[[3]](https://github.com/finos/git-proxy/pull/987/files#diff-c7d2c320d1f7fc0aad36b8bf616a7289ca4dcf00e6e057ba8a1d354fb25b74c0R84-R86)
)Configuration and Code Updates:
src/config/index.ts
to retrieve SSH-related settings (getSSHConfig
,getSSHProxyUrl
). ([[1]](https://github.com/finos/git-proxy/pull/987/files#diff-198a1431d7c5e26ea78bc6e54b34b54e6f8ab342dd48ea11013877e8466a87c5R33)
,[[2]](https://github.com/finos/git-proxy/pull/987/files#diff-198a1431d7c5e26ea78bc6e54b34b54e6f8ab342dd48ea11013877e8466a87c5R48-R58)
)package.json
to include thessh2
library and its TypeScript types for SSH server implementation. ([[1]](https://github.com/finos/git-proxy/pull/987/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R75)
,[[2]](https://github.com/finos/git-proxy/pull/987/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R90)
)Documentation Enhancements:
README.md
to include a link to the new SSH documentation. ([README.mdR88-R95](https://github.com/finos/git-proxy/pull/987/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R88-R95)
)These changes collectively enable the SSH feature, providing an alternative to HTTPS for secure Git operations while enhancing user control and security.