HarperFast / HarperFast/harper

add_ssh_key / update_ssh_key private keys reach the operations log: `key` is missing from the processLocalTransaction strip list

Open Beginner friendly
#2,199 0 comments 0 reactions 0 assignees View on GitHub
area:security bug
Dominant language
JavaScript
Stars
89
Forks
10
Avg merge
2d 2h
Merged PRs (30d)
205

Description

`processLocalTransaction` logs every operation body at `info` and strips a list of secret-bearing fields — but not `key`, which is where `add_ssh_key` and `update_ssh_key` carry an SSH **private key**. So the private key is written to the operations log verbatim.

`core/server/serverHelpers/serverUtilities.ts:98-111`:

```js
const {
hdb_user, hdbAuthHeader, password, payload, credentials,
registryAuth, value, values, envelope,
...cleanBody
} = req.body;
operationLog.info(cleanBody); // :111 — `key` is not stripped
```

## The lists already disagree with each other

harper-pro's replication path strips this exact field, deliberately. `harper-pro/replication/logRedaction.ts:13`:

```ts
const SENSITIVE_OPERATION_FIELDS = ['token', 'key', 'password', 'hdbAuthHeader'];
```

with a header comment naming the reason: *"Some replicated/forwarded operations carry secrets — SSH private keys (add_ssh_key / update_ssh_key) …"*. So `key` was already identified as private-key material that must not reach logs, the replication send/receive paths were hardened for it, and the operations log never got the same treatment.

## Two confirmed exposure paths

1. **Origin, client-supplied `key`.** The log line runs *before* the handler, so any sealing the handler does cannot help — the plaintext key as posted is already in the operations log.
2. **Any peer receiving a replicated `add_ssh_key`/`update_ssh_key`.** `harper-pro/replication/replicationConnection.ts:2952` carefully calls `redactOperationForLog` for its own debug line, then passes the **unredacted** object to `server.operation(...)` on the next line. That reaches `operation()` at `serverUtilities.ts:311` → `processLocalTransaction({ body: operation }, …)` → the log line at `:111`, with `key` already populated on the received body. Where the cluster has secret custody this logs the `enc:v1:` envelope (not plaintext, but still the stored credential); on a node with **no** custody, `sealSSHKey` passes the key through unchanged, so peers log the private key in the clear.

Worth noting what is *not* affected, since it is easy to assume otherwise: on the origin, `add_ssh_key generate=true` avoids path 1 entirely, because `req.key` is still `undefined` when the log line runs. The generate path is exposed only via path 2, on peers.

## Reachability

Gated on `harperLogger.logLevel` ∈ {`info`, `debug`, `trace`}; the default is `warn` (`core/static/defaultConfig.yaml`, `logging.level`). So it is off by default — but info/debug is a routine troubleshooting setting, and operations logs are commonly shipped off-host, where a private key then lands in a log aggregator with a different retention and access model than `/ssh/`.

## Fix

Add `key` to the destructured strip list at `serverUtilities.ts:98-109`. One word, and it covers `add_ssh_key` and `update_ssh_key` together. Worth a comment tying it to the `logRedaction.ts` list so the two do not drift again — and it may be worth reconciling the two lists outright, since `token` is stripped by one and not the other.

## Provenance

Found and confirmed by code reading during a `deep-review` of HarperFast/harper-pro#594 (server-side SSH keygen). Pre-existing and unrelated to that PR's diff; filed here rather than there because the defect and the fix are both in core. Cross-ref: HarperFast/harper-pro#594.

Contributor guide

Open the contributing guide

Research direction

Start in core/server/serverHelpers/serverUtilities.ts:98-111 and compare its processLocalTransaction strip list with harper-pro/replication/logRedaction.ts:13. Verify the operations log no longer receives the key field for add_ssh_key and update_ssh_key, including replicated operations, and consider the payload's suggested comment or list reconciliation to prevent drift.

Written by the indexing model from the issue text.

Assessment

Tech stack
nodejs, typescript
Domain
backend, security
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
86/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.