diff --git a/CHANGELOG.md b/CHANGELOG.md index e0983ad..d7ed5c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ The following emojis are used to highlight certain changes: ### Fixed +- `GetIPNS` no longer returns an IPNS record whose EOL has already passed. An expired record is cryptographically invalid, so it is treated as not found, and when multiple routers answer the first non-expired record is returned. [#154](https://github.com/ipfs/someguy/pull/154) - `/routing/v1/peers/{peerid}` now serves addresses cache-first, the same way `/routing/v1/providers/{cid}` does. It answers from the cached address book and host peerstore before falling back to a DHT lookup, so a relay-dependent peer that is absent from peer routing but recently seen as a provider is no longer answered with an empty result. See [`docs/peer-address-caching.md`](https://github.com/ipfs/someguy/blob/main/docs/peer-address-caching.md). - A completed identify now prunes a peer's cached addresses down to its current advertised set (signed peer record or identify listen addresses) plus any live-connection address, instead of unioning forever. This stops stale certhashes, dead relay circuits, and rotated NAT ports from accumulating across provider lookups and gossip. - Multiaddrs in `/routing/v1` responses are returned in a stable sorted order. They previously came back in nondeterministic order, so repeated requests for the same peer or provider returned the same addresses shuffled differently. diff --git a/server_routers.go b/server_routers.go index 347ac86..56790c3 100644 --- a/server_routers.go +++ b/server_routers.go @@ -246,12 +246,48 @@ func (mi *manyIter[T]) Close() error { return err } +// isExpiredIPNSRecord reports whether an EOL-type IPNS record has passed its +// validity. A record past its EOL is cryptographically invalid, so returning it +// only hands the caller a record that fails validation. Records with a non-EOL +// validity type or an unreadable validity are treated as not expired. +// +// Every backend already validates the record it returns, so this is a +// re-check rather than the first line of defense. We still do it because a +// signature is immutable and verified once, but EOL is time-varying: the +// longer a record lingers (a cache hit, a stored copy, clock drift between +// our infrastructure and the producer), the more likely it has expired +// between the backend's check and the moment we hand it to the user. +// parallelRouter is first-result-wins with no revalidation, so without this +// the aggregator could let an expired record win the race. See +// https://github.com/ipfs/boxo/pull/1166 for the upstream cache-control fix. +func isExpiredIPNSRecord(rec *ipns.Record) bool { + if rec == nil { + return false + } + validityType, err := rec.ValidityType() + if err != nil || validityType != ipns.ValidityEOL { + return false + } + validity, err := rec.Validity() + if err != nil { + return false + } + return time.Now().After(validity) +} + func (r parallelRouter) GetIPNS(ctx context.Context, name ipns.Name) (*ipns.Record, error) { switch len(r.routers) { case 0: return nil, routing.ErrNotFound case 1: - return r.routers[0].GetIPNS(ctx, name) + rec, err := r.routers[0].GetIPNS(ctx, name) + if err != nil { + return nil, err + } + if isExpiredIPNSRecord(rec) { + return nil, routing.ErrNotFound + } + return rec, nil } ctx, cancel := context.WithCancel(ctx) @@ -284,6 +320,11 @@ func (r parallelRouter) GetIPNS(ctx context.Context, name ipns.Name) (*ipns.Reco case res := <-results: switch res.err { case nil: + // An expired record is invalid and must not be served; treat it + // as not found and keep waiting for another router to answer. + if isExpiredIPNSRecord(res.val) { + continue + } return res.val, nil case routing.ErrNotFound, routing.ErrNotSupported: continue diff --git a/server_routers_test.go b/server_routers_test.go index 127045c..b10998b 100644 --- a/server_routers_test.go +++ b/server_routers_test.go @@ -89,6 +89,17 @@ func makeIPNSRecord(t *testing.T, sk crypto.PrivKey, opts ...ipns.Option) (*ipns return record, rawRecord } +func makeExpiredIPNSRecord(t *testing.T, sk crypto.PrivKey) *ipns.Record { + cid, err := cid.Decode("bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4") + require.NoError(t, err) + + // EOL in the past so the record is already expired + record, err := ipns.NewRecord(sk, path.FromCid(cid), 1, time.Now().Add(-time.Hour), time.Second*20) + require.NoError(t, err) + + return record +} + func TestGetIPNS(t *testing.T) { t.Parallel() @@ -158,6 +169,47 @@ func TestGetIPNS(t *testing.T) { _, err := r.GetIPNS(ctx, name) require.ErrorIs(t, err, routing.ErrNotFound) }) + + t.Run("Expired Record Treated As Not Found", func(t *testing.T) { + ctx := context.Background() + + expired := makeExpiredIPNSRecord(t, sk) + + mr1 := &mockRouter{} + mr1.On("GetIPNS", mock.Anything, name).Return(expired, nil) + + r := parallelRouter{ + routers: []router{ + composableRouter{ipns: mr1}, + }, + } + + _, err := r.GetIPNS(ctx, name) + require.ErrorIs(t, err, routing.ErrNotFound) + }) + + t.Run("Skips Expired Record For Valid One", func(t *testing.T) { + ctx := context.Background() + + expired := makeExpiredIPNSRecord(t, sk) + + mr1 := &mockRouter{} + mr1.On("GetIPNS", mock.Anything, name).Return(expired, nil) + + mr2 := &mockRouter{} + mr2.On("GetIPNS", mock.Anything, name).Return(rec, nil) + + r := parallelRouter{ + routers: []router{ + composableRouter{ipns: mr1}, + composableRouter{ipns: mr2}, + }, + } + + getRec, err := r.GetIPNS(ctx, name) + require.NoError(t, err) + require.EqualValues(t, rec, getRec) + }) } func TestPutIPNS(t *testing.T) {