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

package.json

jescalada · 2025-12-15T13:19:25Z

src/proxy/ssh/AgentForwarding.ts

+        // into a proper ParsedKey object.
+        const keys = identities.map((identity) => identity.publicKeyBlob);
+
+        console.log(`[LazyAgent] Returning ${keys.length} identities`);


Based on our meeting from earlier: We should look into two issues:

Why there are automatic SSH requests happening when switching to the user context (or in the background)

Why during these automatic SSH requests, ALL the keys in the system (presumably from ~/.ssh directory) are being loaded here

During manual requests, this fetches only keys added through ssh add

I'm wondering if this means that there are two different clients or agents running in parallel. We should only keep the one that gets the job done.

jescalada · 2025-12-15T13:23:31Z

src/db/file/users.ts

  return new Promise((resolve, reject) => {
    // Check if this key already exists for any user
-    findUserBySSHKey(publicKey)
+    findUserBySSHKey(publicKey.key)


Might be related to findUserBySSHKey: Check why email matters when setting up the SSH key, pinpoint where the check happens and modify it so that GitProxy gives an explicit error when the email doesn't match.

jescalada · 2025-12-15T13:26:43Z

src/config/generated/config.ts

+        typ: u(undefined, ''),
+      },
      { json: 'enabled', js: 'enabled', typ: true },
      { json: 'hostKey', js: 'hostKey', typ: u(undefined, r('HostKey')) },


Check if the hostKey config entries are actually being used or not (since removing them seemed to affect my original, failed SSH flow).

…ication-fix chore: remove unused NPM auth token and flags

…stead of exiting process

fix: improve handling of repository clones in the .remote folder

…of Request params

…core fix: update @types/express-serve-static-core to 5.1.1 and fix typing of Request params

…inked

docs(website): update website homepage

…vices

…inked

feat: Add reject reason

…inked # Conflicts: # src/ui/services/git-push.ts # src/ui/views/PushDetails/PushDetails.tsx

Signed-off-by: Kris West <kristopher.west@natwest.com>

chore: add andypols to list of maintainers

feat: enhance error handling in git-push and repo services

…sePush

…up on error

…mpatibility

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 requested changes Dec 15, 2025

View reviewed changes

fabiovincenzi force-pushed the ssh-agent-on-pr987 branch 2 times, most recently from 06ee7b2 to 0ff683e Compare December 18, 2025 15:37

jescalada and others added 23 commits January 14, 2026 12:12

chore: remove unused NPM auth token and flags

bfff0f7

Merge pull request finos#1351 from jescalada/1334-npm-release-authent…

c655f05

…ication-fix chore: remove unused NPM auth token and flags

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

6fda73c

chore: try unsetting the tokens and set to execute on PR for testing

d9f5e20

chore: attempt to fix Node version and remove env

4ebf646

chore: cleanup npm workflow

33a2a22

chore: add checks to publish Docker image only after merge to main

6bba9e6

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

ae77132

Merge branch 'main' into dockerfile-fix

92133d3

Merge pull request finos#1341 from jescalada/dockerfile-fix

930a5da

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

74d2bc9

fix: error message on fetching auth'd user

42edb34

docs: add JWT api authentication guide

4ea9433

feat: add validation function for commitConfig regex

e9e79b8

fix: add custom validation for config load and reload

c57b480

fix: extract validators into own file and implement validation chain

2bbd787

test: add configValidators test file

50041fc

test: add ConfigLoader tests for reloadConfiguration

6315345

test: add src/config/index tests, modify validation to throw error in…

a1984ee

…stead of exiting process

Merge branch 'main' into 1325-architecture-guide-draft

12e374a

fix(service): improve stop() to properly wait for server close

e5135ae

fix(test): correct mocks for proxy.test.ts

35f413d

fix(passport): use dynamic import to prevent stale db references

9178136

kriswest and others added 30 commits February 17, 2026 09:12

Merge branch 'main' into denis-coric/fix-1392-linked

f1bb0b2

Merge branch 'main' into tab1-update-website

f98acdc

fix: improve error handling in service responses and update tests

5cf407a

Merge branch 'main' into 1386-remote-folder-cleanup

aaeda4f

Merge pull request finos#1393 from finos/1386-remote-folder-cleanup

468bd12

fix: improve handling of repository clones in the .remote folder

fix: enhance error handling for user removal and repository deletion

7290c33

Merge branch 'main' into add-reject-reason

7343b67

fix: update @types/express-serve-static-core to 5.1.1 and fix typing …

1b03ca3

…of Request params

Merge pull request finos#1409 from finos/update-express-serve-static-…

f58250b

…core fix: update @types/express-serve-static-core to 5.1.1 and fix typing of Request params

feat: add ErrorBoundary component to handle errors in Dashboard layout

960bad5

Merge remote-tracking branch 'finos/main' into denis-coric/fix-1392-l…

5af1982

…inked

Merge branch 'main' into tab1-update-website

e1a6008

Merge pull request finos#1286 from tabathad/tab1-update-website

692531b

docs(website): update website homepage

chore: add @andypols to list of maintainers

78778ca

Merge branch 'main' into add-reject-reason

5316a81

fix: integration test

7ac5912

fix: improve error handling and logging in ErrorBoundary and auth ser…

fa6c123

…vices

Merge remote-tracking branch 'finos/main' into denis-coric/fix-1392-l…

67b85b2

…inked

Merge pull request finos#1396 from qube-rt/add-reject-reason

5c4329c

feat: Add reject reason

Merge branch 'main' into website-add-andy-to-maintainers

473c982

Merge remote-tracking branch 'finos/main' into denis-coric/fix-1392-l…

7ee282c

…inked # Conflicts: # src/ui/services/git-push.ts # src/ui/views/PushDetails/PushDetails.tsx

Apply suggestion from @kriswest

28aac41

Signed-off-by: Kris West <kristopher.west@natwest.com>

Merge pull request finos#1413 from finos/website-add-andy-to-maintainers

6713757

chore: add andypols to list of maintainers

Merge branch 'main' into denis-coric/fix-1392-linked

2c2b093

Merge pull request finos#1406 from dcoric/denis-coric/fix-1392-linked

b937878

feat: enhance error handling in git-push and repo services

fix(ssh): use authenticated user identity from transport layer in par…

ad24af3

…sePush

Merge remote-tracking branch 'upstream/main' into ssh-agent-on-pr987

a56700f

fix(pullRemote): restore concurrent request check and directory clean…

be7759a

…up on error

fix(ssh): add proper TypeScript types to SSH key route params

c4f36b7

fix(ssh): use path.join in test assertions for cross-platform path co…

79b6f7c

…mpatibility

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!

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!

8 participants