SSH agent forwarding by fabiovincenzi · Pull Request #65 · G-Research/git-proxy

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

No description provided.

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.

docs/SSH_ARCHITECTURE.md

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.

A user could use any key type that GitHub or another provider supports, so for example U2F keys use sk-ecdsa-sha2-nistp256@openssh.com, which as you can see includes a flags and counter additionally to the signature.

My feeling would be to get this PR in with the common key types and maybe add a todo/issue for some further testing.

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

src/service/routes/users.js

src/service/routes/config.js

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)?

…ismatch-bug fix: proxy preparations mismatch bug

…itest-migration-from-service

fix: demo video

…om-service refactor: migrate tests to Vitest + TS

…dency

Resolved merge conflicts: - Removed migrated test files (.js -> .ts for Vitest) - Accepted upstream package.json/lock (Vitest migration) - Merged src/proxy/index.ts (SSH server + upstream refactoring) - Kept src/service/routes/users.ts (SSH fingerprint functions) - Accepted upstream CLI, UI, and test files Significant upstream changes: - Migration from Mocha to Vitest for all tests - Type system refactoring in UI - Updated dependencies and workflows SSH features from our branch preserved where possible.

Restore SSH key management functionality lost during upstream merge: - Add SSH key list display with name, fingerprint, and date - Add SSH key addition dialog with name and public key fields - Add SSH key deletion with confirmation - Integrate with existing ssh.ts service API - Display snackbar notifications for success/error states This allows users to manage their SSH keys directly from their profile page for SSH-based git operations.

chore: fix NPM publish flow

feat: automatic Docker build and upload to Docker Hub

fix(deps): update npm - li-cli - experimental/li-cli/package.json

fix(deps): update npm - website - website/package.json

fix(deps): update dependency uuid to v13 - - package.json

….json

chore(deps): update npm to v30 - li-cli - experimental/li-cli/package.json (major)

feat: Improve push filtering

…ckerfile

chore(deps): pin httpd docker tag to dd17859 - localgit - localgit/dockerfile

…e.yml

chore(deps): update github-actions - workflows - .github/workflows/e2e.yml

…ges/git-proxy-cli/package.json

fix(deps): update dependency axios to ^1.13.4 - git-proxy-cli - packages/git-proxy-cli/package.json

…ples - plugins/git-proxy-plugin-samples/package.json

…mples-manager fix(deps): update dependency express to ^5.2.1 - git-proxy-plugin-samples - plugins/git-proxy-plugin-samples/package.json

fix: enable skipped proxy tests

fix(deps): update npm - - package.json

Signed-off-by: Fabio Vincenzi <93596376+fabiovincenzi@users.noreply.github.com>

dcoric reviewed Nov 21, 2025

View reviewed changes

dgl reviewed Nov 24, 2025

View reviewed changes

jescalada reviewed Nov 24, 2025

View reviewed changes

docs/SSH_ARCHITECTURE.md Outdated Show resolved Hide resolved

jescalada reviewed Nov 27, 2025

View reviewed changes

jescalada and others added 26 commits November 29, 2025 15:27

chore: fix failing cypress test

7b2f178

chore: improve default repo creation test

3afa917

Merge pull request finos#1284 from jescalada/1282-proxyPreparations-m…

9f24d3f

…ismatch-bug fix: proxy preparations mismatch bug

Merge branch 'main' of https://github.com/finos/git-proxy into 1150-v…

8e02a94

…itest-migration-from-service

chore: improve user endpoint error handling in UI

498d3cb

Merge branch 'main' into improve-endpoint-status-codes

9ed8515

test: fix failing e2e test due to improper error message parsing

b372027

Merge branch 'main' into drop-jwk-to-pem

1835aba

fix: demo video

ead3eca

Merge branch 'main' into fix-demo-video

c461e1d

Merge pull request finos#1298 from tabathad/fix-demo-video

0b5f0b9

fix: demo video

Merge branch 'main' into 1150-vitest-migration-from-service

90b879a

Merge pull request finos#1202 from jescalada/1150-vitest-migration-fr…

3d54835

…om-service refactor: migrate tests to Vitest + TS

fix(deps): update npm to v5 - - package.json

618a4e1

Merge branch 'main' into improve-endpoint-status-codes

651f538

Merge branch 'main' into drop-jwk-to-pem

7a54fef

test: convert missing jwtAuthHandler test and remove jwk-to-pem depen…

9b808ef

…dency

feat(api): add SSH config endpoint for UI

cb99e2c

refactor(proxy): extract HTTPS clone logic using Strategy pattern

345d333

feat(ssh): implement SSH agent forwarding for repository cloning

992fdae

refactor(ssh): extract common SSH command execution logic

7e652d0

fix(ui): correct SSH URL generation in Code button

8936225

feat(ui): include SSH agent forwarding flag in clone command

a128cdd

fix: revert singleBranch option in pullRemote action

dd71f28

jescalada and others added 30 commits January 27, 2026 01:32

Merge branch 'main' into 1334-npm-release-fix

50b43cb

Merge pull request finos#1352 from finos/1334-npm-release-fix

e186944

chore: fix NPM publish flow

Merge branch 'main' into failing-tests

15b60f5

Merge branch 'main' into docker-build-and-upload-to-hub

4767e78

Merge pull request finos#1339 from finos/docker-build-and-upload-to-hub

cb352eb

feat: automatic Docker build and upload to Docker Hub

fix(deps): update npm - li-cli - experimental/li-cli/package.json

085fa74

Merge pull request finos#1308 from finos/renovate/li-cli-manager

7124000

fix(deps): update npm - li-cli - experimental/li-cli/package.json

fix(deps): update npm - website - website/package.json

7261392

Merge pull request finos#1309 from finos/renovate/website-manager

79a79c4

fix(deps): update npm - website - website/package.json

fix(deps): update dependency uuid to v13 - - package.json

61a850d

Merge pull request finos#1224 from finos/renovate/major-13-npm

8e75ce9

fix(deps): update dependency uuid to v13 - - package.json

chore(deps): update npm to v30 - li-cli - experimental/li-cli/package…

75068c8

….json

Merge pull request finos#1083 from finos/renovate/li-cli-major-30-npm

ebf7c2e

chore(deps): update npm to v30 - li-cli - experimental/li-cli/package.json (major)

Merge branch 'main' into view-failed-pushes

29c7539

Merge pull request finos#1349 from qube-rt/view-failed-pushes

b30d708

feat: Improve push filtering

chore(deps): pin httpd docker tag to dd17859 - localgit - localgit/do…

a9af2f5

…ckerfile

Merge pull request finos#1369 from finos/renovate/localgit-manager

7a7e368

chore(deps): pin httpd docker tag to dd17859 - localgit - localgit/dockerfile

chore(deps): update github-actions - workflows - .github/workflows/e2…

4770c94

…e.yml

Merge pull request finos#1370 from finos/renovate/workflows-manager

e3f2bf9

chore(deps): update github-actions - workflows - .github/workflows/e2e.yml

fix(deps): update dependency axios to ^1.13.4 - git-proxy-cli - packa…

c00deed

…ges/git-proxy-cli/package.json

Merge pull request finos#1371 from finos/renovate/git-proxy-cli-manager

9a65e5f

fix(deps): update dependency axios to ^1.13.4 - git-proxy-cli - packages/git-proxy-cli/package.json

fix(deps): update dependency express to ^5.2.1 - git-proxy-plugin-sam…

b5b8cdd

…ples - plugins/git-proxy-plugin-samples/package.json

Merge pull request finos#1372 from finos/renovate/git-proxy-plugin-sa…

4a21364

…mples-manager fix(deps): update dependency express to ^5.2.1 - git-proxy-plugin-samples - plugins/git-proxy-plugin-samples/package.json

Merge branch 'main' into failing-tests

1d4b1ff

Merge pull request finos#1357 from fabiovincenzi/failing-tests

3a27260

fix: enable skipped proxy tests

fix(deps): update npm - - package.json

1d295c5

chore: run formatter

3e39d2d

Merge pull request finos#1307 from finos/renovate/manager

4a738dc

fix(deps): update npm - - package.json

refactor: make Proxy.stop() await all server shutdowns in parallel

74c109e

Merge branch 'main' into ssh-agent-on-pr987

aee9788

Signed-off-by: Fabio Vincenzi <93596376+fabiovincenzi@users.noreply.github.com>

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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants