SSH agent forwarding #65

fabiovincenzi · 2025-11-20T19:45:32Z

No description provided.

…duplication in GitProtocol

…pattern

… ssh2.Client

dcoric

I like how you improved the code organization and type safety!

dgl

A few security minded comments. I put all the comments here, even if there's some overlap with the PR this is based on.

dgl · 2025-11-24T04:44:21Z

src/proxy/ssh/sshHelpers.ts

+    tryKeyboard: false,
+    readyTimeout: 30000,
+    agent: customAgent,
+    algorithms: {


Is there a reason to set these algorithms? It looks like ssh2 will use somewhat sensible defaults (which will at least include things like ed25519 host keys, which isn't included here). (I see this is mostly moved from the other PR, but it looks like the server isn't specifying algorithms, I'd do the same for the client side.)

Done. Removed explicit algorithm configuration to use ssh2's defaults

dgl · 2025-11-24T04:51:23Z

src/proxy/ssh/GitProtocol.ts

+   */
+  hasFlushPacket(): boolean {
+    const bufStr = this.buffer.toString('utf8');
+    return bufStr.includes('0000');


It feels like this is too rudimentary, take the Linux kernel repo:

$ git log 0000 error: short object ID 0000 is ambiguous hint: The candidates are: hint: 000006155029 commit 2025-06-30 - arm64: defconfig: Enable STM32 Octo Memory Manager and OcstoSPI driver [about 26 more omitted]

and that's just prefixes, in this case a 0000 anywhere in a head's commit ID would collide. There's not a security problem here I don't think, just breaks the protocol, but we implemented a better parser before (38915e7) so it would be good to use it (move it somewhere common?).

(https://github.com/not-an-aardvark/lucky-commit could be useful to test this, if needed.)

Fixed. Moved the proper pkt-line parser from parsePush.ts to a shared module (src/proxy/processors/pktLineParser.ts)

dgl · 2025-11-24T05:16:03Z

src/proxy/ssh/sshHelpers.ts

+  if (!client.agentForwardingEnabled) {
+    throw new Error(
+      'SSH agent forwarding is required. Please connect with: ssh -A\n' +
+        'Or configure ~/.ssh/config with: ForwardAgent yes',


Saying ssh -A isn't really actionable in the context most users will see this error. It would be nicer to give people the one liner of:

git config --global core.sshCommand "ssh -A"

In particular configuring this on the git level is far less dangerous than fully enabling agent forwarding.

We also need to be careful with this message, suggesting users enable agent forwarding globally isn't desirable (e.g. https://attack.mitre.org/techniques/T1563/001/) -- it might be useful to let this message be overridden through config too as local policies on how to configure this may vary.

Addressed. Updated error message. Also made the message configurable via ssh.agentForwardingErrorMessage in config.

dgl · 2025-11-24T05:22:59Z

src/proxy/ssh/server.ts

+      if (repoPath.startsWith('/')) {
+        repoPath = repoPath.substring(1);
+      }
      const isReceivePack = command.includes('git-receive-pack');


Use startsWith not include to match the check above. (I doubt anyone would make a repo with git-receive-pack in it, but if they did, this could result in some surprises.)

dgl · 2025-11-24T05:56:36Z

src/proxy/ssh/sshHelpers.ts

+      cipher: ['aes128-gcm' as any, 'aes256-gcm' as any, 'aes128-ctr' as any, 'aes256-ctr' as any],
+      hmac: ['hmac-sha2-256' as any, 'hmac-sha2-512' as any],
+    },
+  };


This should also set hostVerifier too and verify the key is as expected.

It would be good to include github.com and gitlab.com host keys by default so this just works for installations, with a way to configure additional keys.

Implemented. Added hostVerifier with default host keys for github.com and gitlab.com. Additional hosts can be configured via ssh.knownHosts in config.

jescalada · 2025-11-24T14:53:59Z

docs/SSH_ARCHITECTURE.md

+
+### 3. Security Chain Validation Uses HTTPS
+
+**Important**: Even though the client uses SSH to connect to the proxy, the **security chain validation** (pullRemote action) clones the repository using **HTTPS**.


I'm wondering about this caveat here - wouldn't there be any issues if the GitProxy instance running internally were to make HTTPS calls to GitHub? I think it's odd that we enforce SSH communication to GitHub when the user makes a push, but we don't enforce it when going through the various push actions...

…rable

…ection

jescalada

Looks good so far! 🚀 I just have some general comments about the code.

jescalada · 2025-11-27T04:35:36Z

src/cli/ssh-key.ts

 }

+// Calculate SHA-256 fingerprint from SSH public key
+// Note: This function is duplicated in src/service/routes/users.js to keep CLI and server independent


Do we really need to keep the CLI and server independent? Since the CLI is already importing a few things from the parent package.

Perhaps we could extract this function to src/service/routes/utils.ts for better testing and dealing with potential bugs. 🤔

jescalada · 2025-11-27T04:46:16Z

src/db/index.ts

  sink.findUserBySSHKey(sshKey);
 export const getUsers = (query?: Partial<UserQuery>): Promise<User[]> => sink.getUsers(query);
 export const deleteUser = (username: string): Promise<void> => sink.deleteUser(username);
-export const updateUser = (user: Partial<User>): Promise<void> => sink.updateUser(user);


Why is the Partial here being removed? I think this was changed at some point to allow proper typing for activeDirectory.ts and some test files.

We could alternatively fix the usages, but it makes the most sense for a DB update function (triggered through a PATCH endpoint) to take a partial version of the related entity.

jescalada · 2025-11-27T04:52:21Z

src/proxy/processors/pktLineParser.ts

Might want to rename this to packetLineParser.ts for better searchability!

Alternatively, we could rename this to parsePushUtils and include the other helper functions from parsePush such as getCommitData, getPackData, etc.

This might make the parsePush action a bit easier to understand.

jescalada · 2025-11-27T05:07:27Z

src/proxy/ssh/AgentForwarding.ts

I wonder if we should add checks before accessing private fields. If ssh2 renames any of those, we might end up with a silent or non-descriptive error.

jescalada · 2025-11-27T05:12:02Z

src/proxy/ssh/AgentForwarding.ts

+
+  if (chanMgr && chanMgr._channels) {
+    // Find first available channel ID
+    while (chanMgr._channels[localChan] !== undefined) {


Wonder if this wouldn't cause a race condition or clashing of some sort.. Since it's an async function that's relying on and modifying SSH client internals. 🤔

Are we sure that openTemporaryAgentChannel won't get called multiple times in a short period of time?

jescalada · 2025-11-27T05:31:03Z

src/proxy/ssh/AgentProxy.ts

+    // But ssh2 expects only the raw signature bytes (without the algorithm wrapper)
+    // because Protocol.authPK will add the algorithm wrapper itself
+
+    // Parse the blob to extract just the signature bytes


Are we 100% sure that the SSH signature format will always be the same? Just wondering if support for alternative algorithms is necessary.

If SSH connections to GH always use the same algorithm, maybe this isn't needed, but I'm not sure if connections between the user and GitProxy might require additional support.

jescalada · 2025-11-27T05:34:27Z

src/proxy/ssh/GitProtocol.ts

+ * Git uses pkt-line format: [4 byte hex length][payload]
+ * Special packet "0000" (flush packet) indicates end of section
+ */
+class PktLineParser {


Nit: rename to PacketLineParser for searchability/consistency

jescalada · 2025-11-27T05:39:17Z

src/service/routes/users.js

Seems this file is duplicated (users.ts). We should keep the TS version only!

jescalada · 2025-11-27T05:40:26Z

src/service/routes/config.js

Same as the previous comment, these changes should be added to /src/service/routes/config.ts 🙂

jescalada · 2025-11-27T05:43:02Z

src/ui/views/User/UserProfile.tsx

The UserProfile is starting to get big... Perhaps we could extract the SSH-related stuff into its own component(s)?

fabiovincenzi added 16 commits November 20, 2025 18:33

refactor(ssh): add PktLineParser and base function to eliminate code …

51a4a35

…duplication in GitProtocol

feat(ssh): implement server-side SSH agent forwarding with LazyAgent …

f6fb9eb

…pattern

feat(ssh): add SSH helper functions for connection setup and validation

61b3595

refactor(ssh): simplify server.ts and pullRemote using helper functions

3e0e5c0

docs: add SSH proxy architecture documentation

4a2b273

fix(ssh): correct ClientWithUser to extend ssh2.Connection instead of…

0f3d3b8

… ssh2.Client

feat: add dependencies for SSH key management

39be87e

feat(db): add PublicKeyRecord type for SSH key management

dbef641

feat(db): implement SSH key management for File database

9545ac2

feat(db): implement SSH key management for MongoDB

24d499c

feat(db): update database wrapper with correct SSH key types

df603ef

feat(api): add SSH key management endpoints

7e5d6d9

feat(ui): add SSH service for API calls

59aef6e

feat(ui): add SSH key management UI and clone tabs

ebfff2d

feat(cli): update SSH key deletion to use fingerprint

0570c4c

chore: add SSH key fingerprint API and UI updates

e5da79c

dcoric reviewed Nov 21, 2025

View reviewed changes

dgl reviewed Nov 24, 2025

View reviewed changes

jescalada reviewed Nov 24, 2025

View reviewed changes

fabiovincenzi added 5 commits November 26, 2025 11:35

refactor(ssh): remove explicit SSH algorithm configuration

ab0bdbe

fix(ssh): use existing packet line parser

b72d222

feat(ssh): improve agent forwarding error message and make it configu…

55d06ab

…rable

fix(ssh): use startsWith instead of includes for git-receive-pack det…

f6281d6

…ection

feat(ssh): add SSH host key verification to prevent MitM attacks

5e3e13e

jescalada reviewed Nov 27, 2025

View reviewed changes


		### 3. Security Chain Validation Uses HTTPS

		Important: Even though the client uses SSH to connect to the proxy, the security chain validation (pullRemote action) clones the repository using HTTPS.

SSH agent forwarding #65

Are you sure you want to change the base?

SSH agent forwarding #65

Uh oh!

Conversation

fabiovincenzi commented Nov 20, 2025

Uh oh!

dcoric left a comment

Choose a reason for hiding this comment

Uh oh!

dgl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jescalada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants