HarperFast / HarperFast/harper-pro

add_ssh_key: durable writes precede an untimed api.github.com fetch, so a concurrent delete_ssh_key leaves a revoked key live on every peer

Open
#724 0 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
JavaScript
Stars
3
Forks
0
Avg merge
1d 21h
Merged PRs (30d)
80

Description

`addSSHKey` commits all of its local durable state, then awaits an unbounded network fetch, then replicates. A `delete_ssh_key` arriving in that window deletes the key locally and still lets the add reach every peer — leaving a key the operator believes they revoked live across the cluster.

## The window

`security/sshKeyOperations.ts`, in order:

1. `:209` write the key file
2. `:222` append the SSH config block
3. `:233` create `known_hosts`
4. `:242` `await fetch('https://api.github.com/meta')` — **no `AbortSignal`**
5. `:258` `await replicateOperation(req)`

The fetch is only reached when `hostname === 'github.com'`, which is the documented primary use case. With no signal it inherits undici's defaults — a peer that accepts the connection and stays silent leaves it pending past 12s and on to the ~300s headers timeout. So the gap between "local state committed" and "peers have the key" is minutes wide, and the caller has not yet received the response carrying `public_key`.

## Reproduced

```
T0 add_ssh_key{name:deploy, hostname:github.com} → writes deploy.key, appends #deploy, stalls in fetch
T1 delete_ssh_key{name:deploy} → strips #deploy, unlinks deploy.key,
replicates delete → peers: "does not exist" (fails)
T2 fetch returns → add replicates → peers ADD deploy successfully

ORIGIN deploy.key present: false | config has #deploy: false
PEERS received, in order: [delete_ssh_key deploy, add_ssh_key deploy]
```

Both calls return success (`Deleted ssh key: deploy`, `Added ssh key: deploy`). The operator deleted the key, sees it gone on the node they called, and every peer holds the private key — a revoked deploy key surviving cluster-wide.

No deliberate concurrency is required. A client whose HTTP timeout (or the 120s cross-thread forward timeout at `core/server/serverHelpers/registeredOperations.ts:46`) fires during the stall, followed by a retry that gets `Key already exists` and then a cleanup `delete_ssh_key`, produces exactly this ordering.

## Secondary consequence

The same ordering is the *cause* side of #694: if the caller gives up during the stall, the node keeps a key whose public half was never delivered to anyone and cannot be retrieved afterwards.

## Fix

Three independent parts, all worth doing:

1. Give the fetch `signal: AbortSignal.timeout(5000)` — the pattern is already used at `core/resources/models/backendHelpers.ts:27`.
2. Move the best-effort `known_hosts` work (the GitHub fetch and the `known_hosts` append) to **after** `replicateOperation`, so durable and replicated state are established before any optional network I/O.
3. Hold a per-name lock across the whole operation including the fetch, so a concurrent `delete_ssh_key` for the same name cannot interleave (see #722, which needs the same lock).

## Provenance

Confirmed by code reading and reproduction with the real call sequence and a stalled fetch, during a `deep-review` of harper-pro#594. The fetch is pre-existing; `generate: true` is what makes the un-replicated response payload irreplaceable. Related: #694, #722, #723.

Contributor guide

Open the contributing guide

Research direction

Start in security/sshKeyOperations.ts at the durable writes, GitHub metadata fetch, and replicateOperation sequence; compare the timeout pattern in core/resources/models/backendHelpers.ts:27 and the forwarding timeout in core/server/serverHelpers/registeredOperations.ts:46. Reproduce the stalled-fetch interleaving with delete_ssh_key, then verify bounded network work, serialized same-name operations, and no revoked key reaching peers.

Written by the indexing model from the issue text.

Assessment

Tech stack
nodejs, typescript
Domain
backend, distributed-systems, security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.