amadan
P7: fold in three notes from Fable's final review (#175)
The racing-renewal test now counts Puts, so it pins the per-app lock and not only the outcome the ETag check would also give. Verify refuses a record signed more than five minutes ahead of the verifier's clock. The door socket is bound and chmodded in a private directory and renamed into place, so it is never reachable at the umask's mode. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Human
Prompt
continue (Fable final review notes)
From this commit's own message. The prompt ledger has no entries for it — which is what a squashed, rebased or amended commit looks like, and what every commit looks like in a repo without a ledger.
Summary
Three review notes: pin the publication lock in its test, bound SignedAt, and close the socket's mode window.
4 files changed,
+65
−3
internal/console/apidoorkeys_test.go+10 −0internal/doorkey/doorkey.go+9 −0internal/doorkey/doorkey_test.go+23 −0internal/edge/door.go+23 −3
CI cancelled — superseded by a newer push — run details
diff --git a/internal/console/apidoorkeys_test.go b/internal/console/apidoorkeys_test.go| index 19f96cb08..fa8acca4f 100644 |
| --- a/internal/console/apidoorkeys_test.go |
| +++ b/internal/console/apidoorkeys_test.go |
| @@ -12,6 +12,7 @@ import ( |
| "os" |
| "strings" |
| "sync" |
| + "sync/atomic" |
| "testing" |
| "time" |
| @@ -231,10 +232,12 @@ type blockingStore struct { |
| once sync.Once |
| entered chan struct{} |
| release chan struct{} |
| + puts atomic.Int32 // Puts to key, counted before any blocking |
| } |
| func (b *blockingStore) Put(ctx context.Context, key string, body []byte, ifMatch string) (string, error) { |
| if key == b.key { |
| + b.puts.Add(1) |
| first := false |
| b.once.Do(func() { first = true }) |
| if first { |
| @@ -295,6 +298,13 @@ func TestDoorKeyRevokeIsNotOverwrittenByARacingRenewal(t *testing.T) { |
| if revokeCode != http.StatusOK { |
| t.Fatalf("revoke = %d %s", revokeCode, revokeOut) |
| } |
| + // Exactly two writes pins the per-app lock itself, not just the outcome |
| + // (Fable's final review): without the lock the revoke writes while the |
| + // renewal is held, the renewal's stale-ETag Put then fails, and its retry |
| + // makes a third. The ETag check alone would still land revision 2. |
| + if n := bs.puts.Load(); n != 2 { |
| + t.Fatalf("%d Puts to the record, want 2: the revoke did not wait for the renewal's lock", n) |
| + } |
| rec := readDoorRecord(t, fx) |
| if rec.Revision != 2 || rec.Admits(tok) { |
| t.Fatalf("bucket after the race: revision %d, admits the revoked key %v", rec.Revision, rec.Admits(tok)) |
diff --git a/internal/doorkey/doorkey.go b/internal/doorkey/doorkey.go| index 2fe6307c4..efcc57417 100644 |
| --- a/internal/doorkey/doorkey.go |
| +++ b/internal/doorkey/doorkey.go |
| @@ -204,9 +204,18 @@ func Verify(pub ed25519.PublicKey, wire []byte, now time.Time) (Record, error) { |
| if !now.Before(rec.ExpiresAt) { |
| return Record{}, errors.New("doorkey: record expired") |
| } |
| + // A record dated in the future would otherwise stay valid for Lifetime |
| + // from that date. Only the root key signs, but the bound belongs here. |
| + if rec.SignedAt.After(now.Add(MaxClockSkew)) { |
| + return Record{}, errors.New("doorkey: record signed in the future") |
| + } |
| return rec, nil |
| } |
| +// MaxClockSkew is how far ahead of a verifier's clock a record's SignedAt |
| +// may be before Verify refuses it. |
| +const MaxClockSkew = 5 * time.Minute |
| + |
| // VerifyRevision is the publisher's read: the revision a validly signed |
| // record carries, expired or not, so the next publish can stay above it. |
| func VerifyRevision(pub ed25519.PublicKey, wire []byte) (uint64, error) { |
diff --git a/internal/doorkey/doorkey_test.go b/internal/doorkey/doorkey_test.go| index e21cf880d..fc752bbbf 100644 |
| --- a/internal/doorkey/doorkey_test.go |
| +++ b/internal/doorkey/doorkey_test.go |
| @@ -80,6 +80,29 @@ func TestSignVerifyExpiryAndKeys(t *testing.T) { |
| } |
| } |
| +// A record signed more than MaxClockSkew ahead of the verifier's clock is |
| +// refused, so a future-dated record cannot outlive Lifetime from now. |
| +func TestVerifyRefusesAFutureDatedRecord(t *testing.T) { |
| + pub, priv, _ := ed25519.GenerateKey(rand.Reader) |
| + _, entry, _ := NewToken("bae", "titogo") |
| + r := rec1(entry) |
| + r.SignedAt = time.Now().Add(time.Hour) |
| + r.ExpiresAt = r.SignedAt.Add(Lifetime) |
| + wire, err := Sign(priv, r) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := Verify(pub, wire, time.Now()); err == nil { |
| + t.Fatal("a record signed an hour ahead verified") |
| + } |
| + r.SignedAt = time.Now().Add(time.Minute) |
| + r.ExpiresAt = r.SignedAt.Add(Lifetime) |
| + wire, _ = Sign(priv, r) |
| + if _, err := Verify(pub, wire, time.Now()); err != nil { |
| + t.Fatalf("a record a minute ahead (within skew) was refused: %v", err) |
| + } |
| +} |
| + |
| // VerifyRevision is the publisher's read: signature and shape, not expiry, |
| // so an expired record still yields the revision it carried. |
| func TestVerifyRevisionIgnoresExpiry(t *testing.T) { |
diff --git a/internal/edge/door.go b/internal/edge/door.go| index 96c6bad23..c9438f5bb 100644 |
| --- a/internal/edge/door.go |
| +++ b/internal/edge/door.go |
| @@ -36,6 +36,7 @@ import ( |
| "net" |
| "net/http" |
| "os" |
| + "path/filepath" |
| "strings" |
| "sync" |
| "time" |
| @@ -165,13 +166,32 @@ func (e *Edge) openDoor(path string) (closeDoor func(), err error) { |
| } else if !errors.Is(err, os.ErrNotExist) { |
| return nil, err |
| } |
| - ln, err := net.Listen("unix", path) |
| + // Bind in a private (0700) directory beside the target, chmod there, and |
| + // only then rename into place, so the socket is never reachable at the |
| + // umask's mode (Fable's final review). The rename stays on one |
| + // filesystem, and the directory's setgid group still applies because the |
| + // socket is created under /run/carlos. |
| + stage, err := os.MkdirTemp(filepath.Dir(path), ".door-") |
| if err != nil { |
| return nil, err |
| } |
| - if err := os.Chmod(path, 0o660); err != nil { |
| + defer os.RemoveAll(stage) |
| + staged := filepath.Join(stage, "s") |
| + ln, err := net.Listen("unix", staged) |
| + if err != nil { |
| + return nil, err |
| + } |
| + if ul, ok := ln.(*net.UnixListener); ok { |
| + // The listener unlinks its own path on Close; after the rename that |
| + // path is gone, and closeDoor removes the real one itself. |
| + ul.SetUnlinkOnClose(false) |
| + } |
| + if err := os.Chmod(staged, 0o660); err != nil { |
| + ln.Close() |
| + return nil, err |
| + } |
| + if err := os.Rename(staged, path); err != nil { |
| ln.Close() |
| - os.Remove(path) |
| return nil, err |
| } |
| // ReadHeaderTimeout for the public listeners' Slowloris reason; no |