Skip to content

SSH agent forwarding#65

Open
fabiovincenzi wants to merge 1226 commits into
G-Research-Forks:denis-coric/ssh-flowfrom
fabiovincenzi:ssh-agent-on-pr987
Open

SSH agent forwarding#65
fabiovincenzi wants to merge 1226 commits into
G-Research-Forks:denis-coric/ssh-flowfrom
fabiovincenzi:ssh-agent-on-pr987

Conversation

@fabiovincenzi

Copy link
Copy Markdown
Collaborator

No description provided.

@dcoric dcoric left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dgl dgl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/proxy/ssh/sshHelpers.ts Outdated
tryKeyboard: false,
readyTimeout: 30000,
agent: customAgent,
algorithms: {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/proxy/ssh/GitProtocol.ts Outdated
*/
hasFlushPacket(): boolean {
const bufStr = this.buffer.toString('utf8');
return bufStr.includes('0000');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/proxy/ssh/server.ts Outdated
if (repoPath.startsWith('/')) {
repoPath = repoPath.substring(1);
}
const isReceivePack = command.includes('git-receive-pack');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

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],
},
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread docs/SSH_ARCHITECTURE.md Outdated

@jescalada jescalada left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🤔

Comment thread 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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/proxy/ssh/AgentForwarding.ts Outdated

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

// 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: rename to PacketLineParser for searchability/consistency

Comment thread src/service/routes/users.js Outdated
Comment thread src/service/routes/config.js Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread package.json
// into a proper ParsedKey object.
const keys = identities.map((identity) => identity.publicKeyBlob);

console.log(`[LazyAgent] Returning ${keys.length} identities`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/db/file/users.ts
return new Promise((resolve, reject) => {
// Check if this key already exists for any user
findUserBySSHKey(publicKey)
findUserBySSHKey(publicKey.key)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@fabiovincenzi fabiovincenzi force-pushed the ssh-agent-on-pr987 branch 2 times, most recently from 06ee7b2 to 0ff683e Compare December 18, 2025 15:37
andypols and others added 23 commits March 19, 2026 13:29
chore(deps): update github-actions - workflows - .github/workflows/ci.yml
chore(deps): update httpd:2.4 docker digest to 331548c - localgit - localgit/dockerfile
…github-actions

chore(deps): update dependency node to v24 - workflows - .github/workflows/unused-dependencies.yml
chore(deps): update node.js to v24 - - dockerfile
Making a couple of small changes to the meeting minutes template that I have to do regularly. 

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

chore: update meeting_minutes.md issue template
…ithub-actions

chore(deps): update actions/upload-artifact action to v6 - workflows - .github/workflows/e2e.yml
chore(deps): update github/codeql-action digest to 72c0b0e - workflows - .github/workflows/codeql.yml
…ithub-actions

chore(deps): update docker/setup-buildx-action action to v4 - workflows - .github/workflows/docker-publish.yml
…ithub-actions

chore(deps): update github-actions to v7 - workflows - .github/workflows/e2e.yml (major)
re-vlad and others added 30 commits June 3, 2026 15:10
…ile DB

- Add lib/datastore.js (mongo + neDB) and lib/args.js
- Route all migration scripts through createDatastoreFromArgv
- Update Migration-guide for DB_TYPE and file DB paths
feat(migrate): DB tooling for v1.19.2 -> v2.0.0 (URLs + emails + ACL audit)
Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>
Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>
…ce (finos#1554)

The server port, HTTPS server port, UI host, UI port and HTTPS UI port
could only be set via environment variables, with defaults hard-coded in
src/config/env.ts - unlike every other setting, which lives in the config
schema/file. This legacy split is confusing for operators (finos#1553).

Move these five settings into the config schema and proxy.config.json
defaults, mirroring GIT_PROXY_COOKIE_SECRET: the environment variable
(when set) takes precedence over the config file, which takes precedence
over the built-in default. env.ts now exposes the raw environment values
(undefined when unset) so the config file can supply the default.

- Add serverPort, httpsServerPort, uiHost, uiPort and httpsUiPort to
  config.schema.json and proxy.config.json; regenerate config types and
  schema reference docs.
- Resolve them with environment precedence in mergeConfigurations and add
  getServerPort/getHttpsServerPort/getUIHost/getHttpsUIPort getters;
  getUIPort now reads the merged config.
- Switch the proxy, service, urls and auth-route call sites to the getters,
  reading lazily instead of at module load. This also fixes the OIDC auth
  redirect, which fell back to UI port 3000 instead of the configured port.

Co-authored-by: Kris West <kristopher.west@natwest.com>
* fix: apply security best practices

Signed-off-by: StepSecurity Bot <bot@stepsecurity.io>

* chore: `npm run format`

* chore: delete .pre-commit-config.yaml

Removes `.pre-commit-config.yaml` as we already have `.husky/pre-commit`

* ci: update interval from daily to weekly

* ci: refactor dependabot.yml for simplicity

Consolidate npm and docker updates into single entries and adjust directories for better organization.

* ci: delete renovate.json to prevent duplicate bump PRs

* chore: npm run format

* Update .github/dependabot.yml

Co-authored-by: Kris West <kristopher.west@natwest.com>
Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

* Update .github/dependabot.yml

Co-authored-by: Kris West <kristopher.west@natwest.com>
Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

* Update .github/dependabot.yml

Co-authored-by: Kris West <kristopher.west@natwest.com>
Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

* Update .github/dependabot.yml

Co-authored-by: Kris West <kristopher.west@natwest.com>
Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

---------

Signed-off-by: StepSecurity Bot <bot@stepsecurity.io>
Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>
Co-authored-by: Juan Escalada <juanescalada175@gmail.com>
Co-authored-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>
Co-authored-by: Kris West <kristopher.west@natwest.com>
* docs: fix community call calendar link

Provides instructions on how to get the Community Call recurring meeting invite (which can be done via the same meeting link).

Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

* docs: fix/clarify slack access in README

Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

* Update README.md

Co-authored-by: Kris West <kristopher.west@natwest.com>
Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

---------

Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>
Co-authored-by: Kris West <kristopher.west@natwest.com>
… updates

Bumps the github-actions group with 10 updates in the / directory:

| Package | From | To |
| --- | --- | --- |
| [step-security/harden-runner](https://github.com/step-security/harden-runner) | `2.19.3` | `2.19.4` |
| [actions/checkout](https://github.com/actions/checkout) | `6.0.2` | `6.0.3` |
| [codecov/codecov-action](https://github.com/codecov/codecov-action) | `5.5.4` | `6.0.1` |
| [cypress-io/github-action](https://github.com/cypress-io/github-action) | `7.3.0` | `7.4.0` |
| [actions/dependency-review-action](https://github.com/actions/dependency-review-action) | `4.8.2` | `5.0.0` |
| [docker/setup-buildx-action](https://github.com/docker/setup-buildx-action) | `4.0.0` | `4.1.0` |
| [docker/login-action](https://github.com/docker/login-action) | `4.1.0` | `4.2.0` |
| [crazy-max/ghaction-github-runtime](https://github.com/crazy-max/ghaction-github-runtime) | `3.1.0` | `4.0.0` |
| [docker/setup-compose-action](https://github.com/docker/setup-compose-action) | `e29e0ecd235838be5f2e823f8f512a72dc55f662` | `dd8b913e8081779e7a75dd4ffd066e6ba62a289c` |
| [release-drafter/release-drafter](https://github.com/release-drafter/release-drafter) | `7.3.0` | `7.3.1` |



Updates `step-security/harden-runner` from 2.19.3 to 2.19.4
- [Release notes](https://github.com/step-security/harden-runner/releases)
- [Commits](step-security/harden-runner@v2.19.3...9af89fc)

Updates `actions/checkout` from 6.0.2 to 6.0.3
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@de0fac2...df4cb1c)

Updates `codecov/codecov-action` from 5.5.4 to 6.0.1
- [Release notes](https://github.com/codecov/codecov-action/releases)
- [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md)
- [Commits](codecov/codecov-action@75cd116...e79a696)

Updates `cypress-io/github-action` from 7.3.0 to 7.4.0
- [Release notes](https://github.com/cypress-io/github-action/releases)
- [Changelog](https://github.com/cypress-io/github-action/blob/master/CHANGELOG.md)
- [Commits](cypress-io/github-action@dace029...948d67d)

Updates `actions/dependency-review-action` from 4.8.2 to 5.0.0
- [Release notes](https://github.com/actions/dependency-review-action/releases)
- [Commits](actions/dependency-review-action@3c4e3dc...a1d282b)

Updates `docker/setup-buildx-action` from 4.0.0 to 4.1.0
- [Release notes](https://github.com/docker/setup-buildx-action/releases)
- [Commits](docker/setup-buildx-action@v4...v4.1.0)

Updates `docker/login-action` from 4.1.0 to 4.2.0
- [Release notes](https://github.com/docker/login-action/releases)
- [Commits](docker/login-action@4907a6d...650006c)

Updates `crazy-max/ghaction-github-runtime` from 3.1.0 to 4.0.0
- [Release notes](https://github.com/crazy-max/ghaction-github-runtime/releases)
- [Commits](crazy-max/ghaction-github-runtime@3cb05d8...04d248b)

Updates `docker/setup-compose-action` from e29e0ecd235838be5f2e823f8f512a72dc55f662 to dd8b913e8081779e7a75dd4ffd066e6ba62a289c
- [Release notes](https://github.com/docker/setup-compose-action/releases)
- [Commits](docker/setup-compose-action@e29e0ec...dd8b913)

Updates `release-drafter/release-drafter` from 7.3.0 to 7.3.1
- [Release notes](https://github.com/release-drafter/release-drafter/releases)
- [Commits](release-drafter/release-drafter@c2e2804...693d20e)

---
updated-dependencies:
- dependency-name: step-security/harden-runner
  dependency-version: 2.19.4
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: github-actions
- dependency-name: actions/checkout
  dependency-version: 6.0.3
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: github-actions
- dependency-name: codecov/codecov-action
  dependency-version: 6.0.1
  dependency-type: direct:production
  update-type: version-update:semver-major
  dependency-group: github-actions
- dependency-name: cypress-io/github-action
  dependency-version: 7.4.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: github-actions
- dependency-name: actions/dependency-review-action
  dependency-version: 5.0.0
  dependency-type: direct:production
  update-type: version-update:semver-major
  dependency-group: github-actions
- dependency-name: docker/setup-buildx-action
  dependency-version: 4.1.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: github-actions
- dependency-name: docker/login-action
  dependency-version: 4.2.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: github-actions
- dependency-name: crazy-max/ghaction-github-runtime
  dependency-version: 4.0.0
  dependency-type: direct:production
  update-type: version-update:semver-major
  dependency-group: github-actions
- dependency-name: docker/setup-compose-action
  dependency-version: dd8b913e8081779e7a75dd4ffd066e6ba62a289c
  dependency-type: direct:production
  dependency-group: github-actions
- dependency-name: release-drafter/release-drafter
  dependency-version: 7.3.1
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: github-actions
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [lint-staged](https://github.com/lint-staged/lint-staged) from 16.2.7 to 17.0.5.
- [Release notes](https://github.com/lint-staged/lint-staged/releases)
- [Changelog](https://github.com/lint-staged/lint-staged/blob/main/CHANGELOG.md)
- [Commits](lint-staged/lint-staged@v16.2.7...v17.0.5)

---
updated-dependencies:
- dependency-name: lint-staged
  dependency-version: 17.0.5
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
…thub-actions-ee4cbb3667

chore(deps): bump the github-actions group across 1 directory with 10 updates
…-staged-17.0.5

chore(deps-dev): bump lint-staged from 16.2.7 to 17.0.5
Co-authored-by: Kris West <kristopher.west@natwest.com>
Signed-off-by: Fabio Vincenzi <93596376+fabiovincenzi@users.noreply.github.com>
Co-authored-by: Kris West <kristopher.west@natwest.com>
Signed-off-by: Fabio Vincenzi <93596376+fabiovincenzi@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.