diff --git a/docs/docs/CHANGELOG.md b/docs/docs/CHANGELOG.md index 98b40b3f..442bed01 100644 --- a/docs/docs/CHANGELOG.md +++ b/docs/docs/CHANGELOG.md @@ -36,6 +36,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix an obscure case where Anubis in subrequest mode could allow redirects to invalid domains with strange instructions. - Fix `path_regex` and CEL `path` rules not matching when using Traefik `forwardAuth` middleware. Anubis now checks `X-Forwarded-Uri` (Traefik) in addition to `X-Original-URI` (nginx) when resolving the request path in subrequest mode ([#1628](https://github.com/TecharoHQ/anubis/issues/1628)). - Validate bounds in the CEL `randInt` helper so non-positive or platform-overflowing arguments surface a typed CEL error instead of an evaluator panic. +- Pin docs deployment images to immutable digests with `imagePullPolicy: IfNotPresent`, and have the docs-deploy workflow overlay the just-built digest via `kustomize edit set image` so each rollout references an auditable artifact instead of a floating `:main` tag. The docs `Dockerfile` now pins `node` and `nginx-micro` base images to specific versions. +- Fix a race in the bbolt store where the asynchronous cleanup scheduled by an expired read could delete a value that had just been refreshed; the delete now only fires when the key still carries the same expired generation it observed. ## v1.25.0: Necron diff --git a/lib/store/bbolt/bbolt.go b/lib/store/bbolt/bbolt.go index eb7b3401..8df2fbaa 100644 --- a/lib/store/bbolt/bbolt.go +++ b/lib/store/bbolt/bbolt.go @@ -50,6 +50,33 @@ func (s *Store) Delete(ctx context.Context, key string) error { }) } +// deleteIfExpired removes key only if it still carries the exact expiry that an +// expired Get observed and that expiry is still in the past. +// +// Get runs in a read-only transaction, so it can only schedule cleanup +// asynchronously. Between observing the expiry and this delete running, another +// request may Set a fresh value for the same key. Re-reading and matching the +// observed expiry inside the write transaction makes the timestamp act as a +// generation token: a refreshed value carries a different, future expiry and is +// therefore left untouched (see AWOO-015). +func (s *Store) deleteIfExpired(ctx context.Context, key string, observed time.Time) error { + return s.bdb.Update(func(tx *bbolt.Tx) error { + valueBkt := tx.Bucket([]byte(key)) + if valueBkt == nil { + return nil + } + + expiry, err := time.Parse(time.RFC3339Nano, string(valueBkt.Get([]byte("expiry")))) + if err != nil || !expiry.Equal(observed) || !time.Now().After(expiry) { + // Unparseable, refreshed to a different generation, or no longer + // expired: leave it for cleanup or a later Get to handle. + return nil + } + + return tx.DeleteBucket([]byte(key)) + }) +} + // Get a value from the datastore. // // Because each value is stored in its own bucket with data and expiry keys, @@ -77,7 +104,7 @@ func (s *Store) Get(ctx context.Context, key string) ([]byte, error) { } if time.Now().After(expiry) { - go s.Delete(context.Background(), key) + go s.deleteIfExpired(context.Background(), key, expiry) return fmt.Errorf("%w: %q", store.ErrNotFound, key) } diff --git a/lib/store/bbolt/bbolt_test.go b/lib/store/bbolt/bbolt_test.go index 2e67b630..dea99a6c 100644 --- a/lib/store/bbolt/bbolt_test.go +++ b/lib/store/bbolt/bbolt_test.go @@ -4,8 +4,10 @@ import ( "encoding/json" "path/filepath" "testing" + "time" "github.com/TecharoHQ/anubis/lib/store/storetest" + "go.etcd.io/bbolt" ) func TestImpl(t *testing.T) { @@ -20,3 +22,154 @@ func TestImpl(t *testing.T) { storetest.Common(t, Factory{}, json.RawMessage(data)) } + +// newTestStore returns a Store backed by a throwaway bbolt database that is +// closed when the test finishes. +func newTestStore(t *testing.T) *Store { + t.Helper() + + db, err := bbolt.Open(filepath.Join(t.TempDir(), "db"), 0600, nil) + if err != nil { + t.Fatalf("can't open bbolt database: %v", err) + } + t.Cleanup(func() { db.Close() }) + + return &Store{bdb: db} +} + +// mustSet writes a value with the given relative expiry, failing the test on error. +func mustSet(t *testing.T, s *Store, key, value string, expiry time.Duration) { + t.Helper() + + if err := s.Set(t.Context(), key, []byte(value), expiry); err != nil { + t.Fatalf("Set(%q): %v", key, err) + } +} + +// readExpiry returns the expiry timestamp currently stored for key, as a Get +// would parse it. It fails the test if the bucket or expiry is missing. +func readExpiry(t *testing.T, s *Store, key string) time.Time { + t.Helper() + + var out time.Time + if err := s.bdb.View(func(tx *bbolt.Tx) error { + b := tx.Bucket([]byte(key)) + if b == nil { + t.Fatalf("bucket %q missing", key) + } + + expiry, err := time.Parse(time.RFC3339Nano, string(b.Get([]byte("expiry")))) + if err != nil { + return err + } + out = expiry + return nil + }); err != nil { + t.Fatalf("reading expiry for %q: %v", key, err) + } + + return out +} + +// rawData reads the raw data value for key directly, bypassing the expiry check +// in Get so tests can observe whether a bucket physically exists. It returns nil +// when the bucket is absent. +func rawData(t *testing.T, s *Store, key string) []byte { + t.Helper() + + var out []byte + if err := s.bdb.View(func(tx *bbolt.Tx) error { + b := tx.Bucket([]byte(key)) + if b == nil { + return nil + } + data := b.Get([]byte("data")) + out = make([]byte, len(data)) + copy(out, data) + return nil + }); err != nil { + t.Fatalf("reading data for %q: %v", key, err) + } + + return out +} + +// TestDeleteIfExpired guards against AWOO-015: a stale async delete scheduled by +// an expired Get must not erase a value that was refreshed (or otherwise differs +// from) the generation it observed. +func TestDeleteIfExpired(t *testing.T) { + const key = "challenge" + + for _, tt := range []struct { + setup func(t *testing.T, s *Store) time.Time + name string + wantValue string + wantPresent bool + }{ + { + name: "deletes the observed expired generation", + setup: func(t *testing.T, s *Store) time.Time { + mustSet(t, s, key, "old", -time.Minute) + return readExpiry(t, s, key) + }, + wantPresent: false, + }, + { + name: "preserves a refreshed generation", + setup: func(t *testing.T, s *Store) time.Time { + mustSet(t, s, key, "old", -time.Minute) + observed := readExpiry(t, s, key) + mustSet(t, s, key, "fresh", time.Hour) + return observed + }, + wantPresent: true, + wantValue: "fresh", + }, + { + name: "skips on generation mismatch", + setup: func(t *testing.T, s *Store) time.Time { + mustSet(t, s, key, "old", -time.Minute) + // An expiry we never wrote: even though the stored value is + // expired, it is a different generation and must be left alone. + return time.Now().Add(-2 * time.Hour) + }, + wantPresent: true, + wantValue: "old", + }, + { + name: "skips a non-expired observation", + setup: func(t *testing.T, s *Store) time.Time { + mustSet(t, s, key, "live", time.Hour) + return readExpiry(t, s, key) + }, + wantPresent: true, + wantValue: "live", + }, + { + name: "no-op when bucket is absent", + setup: func(t *testing.T, s *Store) time.Time { + return time.Now().Add(-time.Hour) + }, + wantPresent: false, + }, + } { + t.Run(tt.name, func(t *testing.T) { + s := newTestStore(t) + observed := tt.setup(t, s) + + if err := s.deleteIfExpired(t.Context(), key, observed); err != nil { + t.Fatalf("deleteIfExpired(%q): %v", key, err) + } + + got := rawData(t, s, key) + switch { + case tt.wantPresent && got == nil: + t.Fatalf("key %q: want present with value %q, got deleted", key, tt.wantValue) + case tt.wantPresent && string(got) != tt.wantValue: + t.Errorf("key %q: want value %q, got %q", key, tt.wantValue, string(got)) + case !tt.wantPresent && got != nil: + t.Errorf("key %q: want deleted, got value %q", key, string(got)) + } + }) + } +}