Skip to content

Harden request auth and fix handler/source bugs - #11

Merged
turkosaurus merged 2 commits into
mainfrom
turk/bugs
Jun 28, 2026
Merged

Harden request auth and fix handler/source bugs#11
turkosaurus merged 2 commits into
mainfrom
turk/bugs

Conversation

@turkosaurus

Copy link
Copy Markdown
Contributor

Security fixes

  • CIDR allowlist was non-functional — the out-of-range branch logged and
    wrote 403 but had no return, so execution continued and the encrypted
    secret was written to the response body. Added the missing returns (also on
    the decrypt-error and per-entry verify-error paths).
  • Response key was unauthenticated — the signature covered only the secret
    name, so a captured request could be replayed with the attacker's own
    ClientPubKey to have the secret re-encrypted to them. The signed message
    now binds name + ClientPubKey + timestamp + nonce.
  • No replay protection — added a MaxClockSkew (30s) freshness window and a
    seen-nonce cache to reject exact replays within that window. Eviction runs in
    a background sweeper (Server.Close stops it).

Correctness fixes

  • Env.Load checked the original-case key against a lowercase map, dropping all
    but the last secret for mixed-case services.
  • Onepass.Load keyed services by original case; the server looks them up
    lowercased.
  • WriteRegistry leaked its file handle.
  • Register appended duplicate entries when re-registering a name with a
    .env suffix.

Tests

New server_test.go: happy path, pubkey-substitution rejected, out-of-CIDR
rejected, replay rejected, stale-timestamp rejected. Full suite passes under
-race.

Security (server.go):
- Add missing returns so the CIDR allowlist and decrypt-error paths
  actually block instead of falling through to serving the secret.
- Bind ClientPubKey, timestamp, and a single-use nonce into the signed
  request (crypto.go, client.go) so a captured request can't be replayed
  with a substituted response key.
- Reject stale/future requests outside MaxClockSkew and exact replays via
  a nonce cache swept by a background goroutine; add Server.Close to stop it.

Correctness:
- Env.Load: check the lowercase key so mixed-case services keep all secrets.
- Onepass.Load: key services lowercase to match server lookup.
- WriteRegistry: close the file handle.
- Register: normalize name so re-registering updates instead of duplicating.

Add server_test.go covering the rejection paths and the happy path.
@turkosaurus turkosaurus self-assigned this Jun 28, 2026
@turkosaurus turkosaurus added the bug Something isn't working label Jun 28, 2026
Lock in the two correctness fixes that previously ran without assertions:
- Env.Load retains all secrets for a mixed-case service name.
- Register normalizes names so re-registering updates instead of duplicating.
@turkosaurus
turkosaurus merged commit 89d90e8 into main Jun 28, 2026
1 check passed
@turkosaurus
turkosaurus deleted the turk/bugs branch June 28, 2026 05:18
turkosaurus added a commit that referenced this pull request Jun 28, 2026
Reconcile the remote-registry feature with the request-auth hardening that
merged via #11. Conflicts resolved by keeping #10's architecture (Registry
interface, registry polling, AllowRequestFunc policy, handlePost, body limit)
and folding in #11's hardening:

- server.go: nonce/timestamp binding via requestMessage, MaxClockSkew freshness
  check, seen-nonce replay cache + sweeper, Server.Close, drop decrypted-name
  log. CIDR enforcement is via #10's AllowCIDR policy (returns 403).
- registry.go: #10's FileRegistry already carries the file-close and name
  normalization fixes; the old free functions are dropped.
- tests: port the hardening regression tests and Register-dedup test to #10's
  FileRegistry/NewServer API.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant