amadan
objstore+console: three blockers from the implementation review
**Enabling an engine needed no member request.** `scan grant` checked only that the store was provisioned, so an operator could turn the engine on with `Requested` still nil — and nil reads as "not encrypted" everywhere downstream. A client-encrypted store nobody attested would have been swept and reported entirely clean, which is the exact failure the attestation exists to prevent. It also skipped the consent point: this engine reads tenant object bytes. Now a 409, the way Grant refuses ErrNotDeclared — an operator verb never invents the member's half. The CLI had the matching blind spot: `printScanStatus` returned early on a nil `Requested` and printed "not requested" while an engine was actively reading those objects. It now warns instead and carries on. **A re-attested store could never be swept again.** `storeIsDue` read `not_applicable` off the SIDECAR alone. A member moving from --client-encrypted back to --plaintext rewrites the record and leaves that flag behind, so the hourly pass skipped the store forever while `store status` reported "scanning: clamav". The record is the authority on the attestation; the sidecar is a cache of a decision made from it. **One unreadable object stalled a bucket's sweep forever.** A GetObject AccessDenied returned a plain error, which abandons the page unwritten — and the next tick re-lists the same page, meets the same object and fails identically. One SSE-KMS object outside the console's key policy, or one foreign-owned object in an adopted bucket, would have frozen that store's coverage permanently while every surface said "in progress". Denied is now a per-object verdict, which is what VerdictDenied was declared for and what the CLI's "N denied" line promised; with the shipped engine that number could never have been non-zero. NoSuchKey is "gone" — dropped, not counted: an object deleted between the listing and the fetch is neither covered nor a gap. Also: the CAS retry guard had no test, and mutating it to retry blindly left every test green. It has one now, and that test fails against the mutation. And Fresh/Reused split the scanned count by when the verdict was reached — a clean tag is reused while its ETag matches, so a completed rollup can be built almost entirely from judgements made against a signature database that is not the one named beside them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJkJkxZZLgW8X8A5tk69Sq
10 files changed,
+524
−11
internal/console/apiscan.go+26 −0internal/console/apiscan_test.go+63 −0internal/console/scanpass.go+19 −5internal/console/scanpass_test.go+72 −1internal/objstore/clamav.go+76 −0internal/objstore/clamav_test.go+94 −0internal/objstore/sidecar.go+9 −1internal/objstore/sweep.go+27 −1internal/objstore/sweep_test.go+123 −0scan.go+15 −3
diff --git a/internal/console/apiscan.go b/internal/console/apiscan.go| index f81146372..ba8ead373 100644 |
| --- a/internal/console/apiscan.go |
| +++ b/internal/console/apiscan.go |
| @@ -71,6 +71,14 @@ func (s *Server) scanReply(ctx context.Context, acct *Account, app string, rec * |
| } |
| if s.cfg.StoreScanner == nil { |
| out["engine"] = "" |
| + // A record can say "enabled" on a deployment whose engine has since |
| + // been unset. Silence there reads as health: status would print the |
| + // engine, the enforcement state and the coverage, and never mention |
| + // that nothing is running. Say it plainly instead. |
| + if rec.Scanning != nil && rec.Scanning.Enabled != nil { |
| + out["engine_error"] = "no scanning engine is configured on this deployment " + |
| + "(CARLOS_STORE_SCANNER) — the record says one is enabled, but no sweep is running" |
| + } |
| return out, nil |
| } |
| engine, version, perr := s.cfg.StoreScanner.Provenance(ctx) |
| @@ -258,6 +266,24 @@ func (s *Server) handleAPIScanGrant(w http.ResponseWriter, r *http.Request) { |
| "this store has not been granted yet — run `carlos store grant` first") |
| return |
| } |
| + // The member's request is a PRECONDITION, not a formality, and this is |
| + // the second place that has to hold the line. |
| + // |
| + // The attestation only exists in Requested. Enabling without one leaves |
| + // Requested nil, and nil reads as "not encrypted" everywhere downstream — |
| + // so a client-encrypted store nobody attested would be swept and reported |
| + // entirely clean, which is the exact failure §2.2 exists to prevent. It |
| + // also skips the consent point: this engine reads tenant object bytes, |
| + // and an operator must not be able to start that on their own. |
| + // |
| + // Same posture as objstore.Grant refusing ErrNotDeclared: an operator |
| + // verb never invents the member's half. |
| + if rec.Scanning == nil || rec.Scanning.Requested == nil { |
| + jsonError(w, http.StatusConflict, |
| + "the member has not requested scanning for this app — they run `carlos store scan request` first, "+ |
| + "and their attestation is what says whether these objects can be read at all") |
| + return |
| + } |
| engine, _, err := s.cfg.StoreScanner.Provenance(ctx) |
| if err != nil { |
| log.Printf("console: api scan grant %s/%s: engine: %v", acct.Sqid, app, err) |
diff --git a/internal/console/apiscan_test.go b/internal/console/apiscan_test.go| index 4d5ee20f1..7a3d09c12 100644 |
| --- a/internal/console/apiscan_test.go |
| +++ b/internal/console/apiscan_test.go |
| @@ -30,6 +30,16 @@ func scanArmed(t *testing.T, code string) (*consoleFixture, *objstore.MemScanner |
| return fx, sc, token |
| } |
| +// requestScanning is the member's half. An operator cannot enable an engine |
| +// without it — the attestation it carries is what says whether these objects |
| +// can be read at all. |
| +func requestScanning(t *testing.T, fx *consoleFixture, token string, encrypted bool) { |
| + t.Helper() |
| + if s, o := postJSON(t, scanURL(fx, "titogo"), map[string]any{"encrypted": encrypted}, token); s != http.StatusOK { |
| + t.Fatalf("scan request = %d %v", s, o) |
| + } |
| +} |
| + |
| // The member gate runs FIRST: a stranger gets the same byte-identical 404 the |
| // store doors give, never a 403 that would confirm the app exists. |
| func TestScanDoorsGateOrder(t *testing.T) { |
| @@ -89,6 +99,7 @@ func TestScanGrantRequires503WithoutScanner(t *testing.T) { |
| // refusing for the very reason it needs fixing. |
| func TestScanRevokeAndEnforceOffWorkWithoutScanner(t *testing.T) { |
| fx, _, token := scanArmed(t, "code-scan-nodeadlock") |
| + requestScanning(t, fx, token, false) |
| // Get into the bad state honestly: enable and enforce with an engine. |
| if status, out := postJSON(t, scanURL(fx, "titogo")+"/grant", map[string]any{}, token); status != http.StatusOK { |
| @@ -194,6 +205,7 @@ func TestScanEnforceWritesTheDenyToThePolicy(t *testing.T) { |
| fx, fake, token, _ := storeArmed(t, "code-scan-policy") |
| grantStore(t, fx, token) |
| fx.srv.cfg.StoreScanner = objstore.NewMemScanner() |
| + requestScanning(t, fx, token, false) |
| if status, out := postJSON(t, scanURL(fx, "titogo")+"/grant", map[string]any{}, token); status != http.StatusOK { |
| t.Fatalf("scan grant = %d %v", status, out) |
| @@ -291,3 +303,54 @@ func TestStoreStatusCarriesScanningBlock(t *testing.T) { |
| } |
| } |
| } |
| + |
| +// Enabling an engine requires the MEMBER's request, and specifically their |
| +// attestation. Without one, Requested stays nil — and nil reads as "not |
| +// encrypted" everywhere downstream, so a client-encrypted store nobody |
| +// attested would be swept and reported entirely clean. It also skips the |
| +// consent point: this engine reads tenant object bytes. |
| +func TestScanGrantRefusedWithoutMemberRequest(t *testing.T) { |
| + fx, _, token := scanArmed(t, "code-scan-no-request") |
| + |
| + status, out := postJSON(t, scanURL(fx, "titogo")+"/grant", map[string]any{}, token) |
| + if status != http.StatusConflict { |
| + t.Fatalf("scan grant with no member request = %d %v, want 409", status, out) |
| + } |
| + if msg := str(t, out, "error"); !strings.Contains(msg, "scan request") { |
| + t.Fatalf("error = %q, want it to name the verb the member runs", msg) |
| + } |
| + if rec := storedRecord(t, fx, "titogo"); rec.Scanning != nil && rec.Scanning.Enabled != nil { |
| + t.Fatal("a refused grant still enabled the engine") |
| + } |
| + |
| + // With the attestation on record it goes through. |
| + if s, o := postJSON(t, scanURL(fx, "titogo"), map[string]any{"encrypted": false}, token); s != http.StatusOK { |
| + t.Fatalf("scan request = %d %v", s, o) |
| + } |
| + if s, o := postJSON(t, scanURL(fx, "titogo")+"/grant", map[string]any{}, token); s != http.StatusOK { |
| + t.Fatalf("scan grant after a request = %d %v, want 200", s, o) |
| + } |
| +} |
| + |
| +// Status says so when the record names an engine this deployment no longer |
| +// has. Silence there reads as health: the reply would carry the engine, the |
| +// enforcement state and the coverage, and never mention that nothing runs. |
| +func TestScanStatusReportsMissingEngineOnEnabledRecord(t *testing.T) { |
| + fx, _, token := scanArmed(t, "code-scan-engine-gone") |
| + if s, o := postJSON(t, scanURL(fx, "titogo"), map[string]any{"encrypted": false}, token); s != http.StatusOK { |
| + t.Fatalf("scan request = %d %v", s, o) |
| + } |
| + if s, o := postJSON(t, scanURL(fx, "titogo")+"/grant", map[string]any{}, token); s != http.StatusOK { |
| + t.Fatalf("scan grant = %d %v", s, o) |
| + } |
| + fx.srv.cfg.StoreScanner = nil |
| + |
| + status, out := getJSON(t, scanURL(fx, "titogo"), token) |
| + if status != http.StatusOK { |
| + t.Fatalf("scan status = %d %v", status, out) |
| + } |
| + msg, _ := out["engine_error"].(string) |
| + if !strings.Contains(msg, "CARLOS_STORE_SCANNER") { |
| + t.Fatalf("engine_error = %q, want it to say no engine is configured", msg) |
| + } |
| +} |
diff --git a/internal/console/scanpass.go b/internal/console/scanpass.go| index b2eb689e8..c41ff6cf6 100644 |
| --- a/internal/console/scanpass.go |
| +++ b/internal/console/scanpass.go |
| @@ -117,7 +117,7 @@ func (s *Server) scanOneApp(ctx context.Context, acct, app string, budget, inter |
| return nil |
| } |
| - due, err := s.storeIsDue(ctx, acct, app, interval) |
| + due, err := s.storeIsDue(ctx, rec, acct, app, interval) |
| if err != nil { |
| return err |
| } |
| @@ -139,7 +139,7 @@ func (s *Server) scanOneApp(ctx context.Context, acct, app string, budget, inter |
| // one budget ever finishes. Only a COMPLETED sweep waits out the interval, so |
| // LastComplete stays stable long enough to be quoted in an audit answer |
| // instead of being rebuilt from scratch every hour. |
| -func (s *Server) storeIsDue(ctx context.Context, acct, app string, interval time.Duration) (bool, error) { |
| +func (s *Server) storeIsDue(ctx context.Context, rec *objstore.Record, acct, app string, interval time.Duration) (bool, error) { |
| side, _, err := objstore.GetSidecar(ctx, s.cfg.Releases, acct, app) |
| if err != nil { |
| // A sidecar we cannot read is not a reason to re-sweep blindly: it is |
| @@ -154,9 +154,16 @@ func (s *Server) storeIsDue(ctx context.Context, acct, app string, interval time |
| return true, nil // mid-sweep: keep going |
| } |
| if side.NotApplicable { |
| - // Client-encrypted. Re-checking hourly costs a listing and can never |
| - // change until the member re-attests, which rewrites the record. |
| - return false, nil |
| + // Client-encrypted — but the RECORD decides that, not the sidecar. |
| + // |
| + // A member can re-attest from --client-encrypted to --plaintext, which |
| + // rewrites store.json and leaves the stale not_applicable sidecar |
| + // behind. Reading only the sidecar here meant that store was never |
| + // swept again by any tick, for as long as it existed, while |
| + // `store status` cheerfully reported "scanning: clamav". Only the |
| + // on-demand sweep verb could unstick it, and nothing told anyone to |
| + // run it. |
| + return recordSaysEncrypted(rec) == false, nil |
| } |
| if side.LastComplete == nil { |
| return true, nil |
| @@ -204,3 +211,10 @@ func (s *Server) sweepNow(ctx context.Context, acct, app string) (*objstore.Scan |
| } |
| return objstore.RunSweep(ctx, s.cfg.Releases, lister, s.cfg.StoreScanner, acct, app, s.scanBudget()) |
| } |
| + |
| +// recordSaysEncrypted is the single reading of the member's attestation. The |
| +// record is the authority; the sidecar is a cache of a decision made from it. |
| +func recordSaysEncrypted(rec *objstore.Record) bool { |
| + return rec != nil && rec.Scanning != nil && |
| + rec.Scanning.Requested != nil && rec.Scanning.Requested.Encrypted |
| +} |
diff --git a/internal/console/scanpass_test.go b/internal/console/scanpass_test.go| index 77339ad87..dee14afdc 100644 |
| --- a/internal/console/scanpass_test.go |
| +++ b/internal/console/scanpass_test.go |
| @@ -205,7 +205,11 @@ func TestScanPassAlwaysContinuesAnInFlightSweep(t *testing.T) { |
| t.Fatal(err) |
| } |
| - due, err := fx.srv.storeIsDue(ctx, fixtureAcct, fixtureApp, fx.srv.scanInterval()) |
| + rec, _, err := objstore.Get(ctx, fx.rel, fixtureAcct, fixtureApp) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + due, err := fx.srv.storeIsDue(ctx, rec, fixtureAcct, fixtureApp, fx.srv.scanInterval()) |
| if err != nil { |
| t.Fatal(err) |
| } |
| @@ -225,3 +229,70 @@ func TestSweepNowRefusesWithoutScanner(t *testing.T) { |
| t.Fatalf("err = %v, want errNoScanner", err) |
| } |
| } |
| + |
| +// A store re-attested from --client-encrypted back to --plaintext must be |
| +// swept again. |
| +// |
| +// The stale not_applicable sidecar outlives the record change, and reading |
| +// only the sidecar meant such a store was never swept by any tick again — for |
| +// as long as it existed — while `store status` reported "scanning: clamav". |
| +// The RECORD is the authority on the attestation; the sidecar is a cache of a |
| +// decision made from it. |
| +func TestScanPassResweepsAfterReattestationToPlaintext(t *testing.T) { |
| + ctx := context.Background() |
| + fx := newConsoleFixture(t) |
| + fx.srv.cfg.StoreScanner = objstore.NewMemScanner() |
| + enableStore(t, fx, fixtureAcct, fixtureApp, true) // record says plaintext |
| + |
| + // The sidecar still carries the answer from when it was encrypted. |
| + if _, err := objstore.PutSidecar(ctx, fx.rel, fixtureAcct, fixtureApp, |
| + &objstore.ScanSidecar{NotApplicable: true}, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + l := &countingLister{keys: []string{"a.txt"}} |
| + withLister(fx, l) |
| + if err := fx.srv.scanPass(ctx); err != nil { |
| + t.Fatal(err) |
| + } |
| + if l.calls == 0 { |
| + t.Fatal("a store re-attested as plaintext was never swept again") |
| + } |
| + side, _, err := objstore.GetSidecar(ctx, fx.rel, fixtureAcct, fixtureApp) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if side.NotApplicable { |
| + t.Fatal("the stale not_applicable flag survived a sweep of a plaintext store") |
| + } |
| + if side.LastComplete == nil || side.LastComplete.Objects != 1 { |
| + t.Fatalf("coverage = %+v", side.LastComplete) |
| + } |
| +} |
| + |
| +// The mirror: a store the record still says is encrypted stays skipped, and |
| +// costs no listing at all. |
| +func TestScanPassStillSkipsEncryptedStore(t *testing.T) { |
| + ctx := context.Background() |
| + fx := newConsoleFixture(t) |
| + fx.srv.cfg.StoreScanner = objstore.NewMemScanner() |
| + enableStore(t, fx, fixtureAcct, fixtureApp, true) |
| + |
| + rec, etag, err := objstore.Get(ctx, fx.rel, fixtureAcct, fixtureApp) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + rec.Scanning.Requested.Encrypted = true |
| + if _, err := objstore.Put(ctx, fx.rel, fixtureAcct, fixtureApp, rec, etag); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := objstore.PutSidecar(ctx, fx.rel, fixtureAcct, fixtureApp, |
| + &objstore.ScanSidecar{NotApplicable: true}, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + withLister(fx, &refusingLister{t: t}) |
| + if err := fx.srv.scanPass(ctx); err != nil { |
| + t.Fatal(err) |
| + } |
| +} |
diff --git a/internal/objstore/clamav.go b/internal/objstore/clamav.go| index fc4fd6e75..67c4643d0 100644 |
| --- a/internal/objstore/clamav.go |
| +++ b/internal/objstore/clamav.go |
| @@ -2,6 +2,7 @@ package objstore |
| import ( |
| "context" |
| + "errors" |
| "fmt" |
| "io" |
| "strings" |
| @@ -9,6 +10,7 @@ import ( |
| awss3 "github.com/aws/aws-sdk-go-v2/service/s3" |
| s3types "github.com/aws/aws-sdk-go-v2/service/s3/types" |
| + "github.com/aws/smithy-go" |
| "github.com/carlosframework/platform/internal/clamd" |
| ) |
| @@ -116,6 +118,15 @@ func (c *clamavScanner) Verdict(ctx context.Context, bucket, key, etag string) ( |
| Key: &key, |
| }) |
| if err != nil { |
| + if isGone(err) { |
| + return VerdictUnknown, ErrObjectGone |
| + } |
| + if isDenied(err) { |
| + // We cannot read this object's tags and never will without a |
| + // permissions change. That is a fact about the object, not a |
| + // transport failure, and it must not fail the pass. |
| + return VerdictDenied, nil |
| + } |
| return VerdictUnknown, fmt.Errorf("objstore: read tags %s/%s: %w", bucket, key, err) |
| } |
| @@ -154,6 +165,31 @@ func (c *clamavScanner) Scan(ctx context.Context, bucket, key, etag string, size |
| obj, err := c.s3.GetObject(ctx, &awss3.GetObjectInput{Bucket: &bucket, Key: &key}) |
| if err != nil { |
| + // An object we are not allowed to read is DENIED — a permanent fact |
| + // about this object that a permissions fix repairs, not a retry. |
| + // |
| + // This classification is load-bearing, not tidiness. Returning a plain |
| + // error here fails the whole page, unwritten; the next tick re-lists |
| + // the same page, meets the same object and fails identically, and the |
| + // bucket's sweep never advances past it. One SSE-KMS object the |
| + // console's key policy does not cover, or one object owned by another |
| + // account in an adopted bucket, would stall that store's coverage |
| + // forever while every surface reported a sweep merely "in progress". |
| + if isDenied(err) { |
| + if terr := c.tag(ctx, bucket, key, etag, VerdictDenied); terr != nil { |
| + // Tagging it is best effort: if we cannot read the object we |
| + // may well not be able to tag it either. The verdict still |
| + // stands for this sweep's count. |
| + return VerdictDenied, nil |
| + } |
| + return VerdictDenied, nil |
| + } |
| + // Deleted between the listing and the fetch. Routine on a bucket |
| + // anyone is using, and not a failure of anything — the sweep drops it |
| + // rather than counting an object that no longer exists. |
| + if isGone(err) { |
| + return VerdictUnknown, ErrObjectGone |
| + } |
| return VerdictUnknown, fmt.Errorf("objstore: get %s/%s: %w", bucket, key, err) |
| } |
| defer obj.Body.Close() |
| @@ -238,3 +274,43 @@ func aws2s(p *string) string { |
| } |
| func s2aws(s string) *string { return &s } |
| + |
| +// ErrObjectGone is an object that vanished between the listing page and the |
| +// fetch. The sweep drops it rather than counting it: it is neither covered nor |
| +// a gap, it simply is not there any more. |
| +var ErrObjectGone = errors.New("objstore: object no longer exists") |
| + |
| +// deniedCodes are the S3 and KMS answers that mean "not allowed, and a retry |
| +// will not change that". Matched on the API error code rather than the message |
| +// so a wording change upstream does not silently reclassify them as failures. |
| +var deniedCodes = map[string]bool{ |
| + "AccessDenied": true, |
| + "AccessDeniedException": true, // KMS, on an SSE-KMS object |
| + "Forbidden": true, |
| + "AllAccessDisabled": true, |
| + "KMS.AccessDeniedException": true, |
| + "InvalidObjectState": true, // GLACIER / DEEP_ARCHIVE: unreadable until restored |
| +} |
| + |
| +func isDenied(err error) bool { |
| + var ae smithy.APIError |
| + if errors.As(err, &ae) { |
| + return deniedCodes[ae.ErrorCode()] |
| + } |
| + return false |
| +} |
| + |
| +// isGone is an object that is not there — deleted between the listing and the |
| +// fetch, which happens routinely on a bucket in use. |
| +func isGone(err error) bool { |
| + var nsk *s3types.NoSuchKey |
| + if errors.As(err, &nsk) { |
| + return true |
| + } |
| + var ae smithy.APIError |
| + if errors.As(err, &ae) { |
| + code := ae.ErrorCode() |
| + return code == "NoSuchKey" || code == "NotFound" |
| + } |
| + return false |
| +} |
diff --git a/internal/objstore/clamav_test.go b/internal/objstore/clamav_test.go| index 2a1cd44e6..5c89494f6 100644 |
| --- a/internal/objstore/clamav_test.go |
| +++ b/internal/objstore/clamav_test.go |
| @@ -8,6 +8,8 @@ import ( |
| "strings" |
| "testing" |
| + "github.com/aws/smithy-go" |
| + |
| awss3 "github.com/aws/aws-sdk-go-v2/service/s3" |
| s3types "github.com/aws/aws-sdk-go-v2/service/s3/types" |
| @@ -348,3 +350,95 @@ func TestClamAVProvenanceCarriesSignatureVersion(t *testing.T) { |
| t.Fatalf("version = %q, want the signature database in it", version) |
| } |
| } |
| + |
| +// apiErr is an AWS API error with a given code, which is what isDenied and |
| +// isGone classify on. |
| +func apiErr(code string) error { |
| + return &smithy.GenericAPIError{Code: code, Message: code} |
| +} |
| + |
| +// An object we are not allowed to read is DENIED, and the sweep goes on. |
| +// |
| +// This is not tidiness. A plain error here fails the whole page unwritten; the |
| +// next tick re-lists the same page, meets the same object and fails |
| +// identically, and the bucket's sweep never advances past it. One SSE-KMS |
| +// object outside the console's key policy would stall a store's coverage |
| +// forever while every surface said "in progress". |
| +func TestClamAVUnreadableObjectIsDeniedNotAPassFailure(t *testing.T) { |
| + for _, code := range []string{"AccessDenied", "AccessDeniedException", "Forbidden", "InvalidObjectState"} { |
| + t.Run(code, func(t *testing.T) { |
| + var wrote map[string]string |
| + s3 := &stubScanS3{t: t} |
| + s3.getObject = func(*awss3.GetObjectInput) (*awss3.GetObjectOutput, error) { |
| + return nil, apiErr(code) |
| + } |
| + s3.putTagging = func(in *awss3.PutObjectTaggingInput) (*awss3.PutObjectTaggingOutput, error) { |
| + wrote = tagsOf(in) |
| + return &awss3.PutObjectTaggingOutput{}, nil |
| + } |
| + c := &clamavScanner{s3: s3, engine: &fakeEngine{}, maxBytes: 1 << 20} |
| + |
| + v, err := c.Scan(context.Background(), "b", "k", "e", 5) |
| + if err != nil { |
| + t.Fatalf("an unreadable object failed the pass: %v", err) |
| + } |
| + if v != VerdictDenied { |
| + t.Fatalf("verdict = %q, want denied", v) |
| + } |
| + if wrote[TagVerdict] != string(VerdictDenied) { |
| + t.Fatalf("tags = %v", wrote) |
| + } |
| + }) |
| + } |
| +} |
| + |
| +// An object deleted between the listing and the fetch is GONE — neither |
| +// covered nor a gap. Routine on any bucket in use, and it must not stall the |
| +// page either. |
| +func TestClamAVDeletedObjectIsGone(t *testing.T) { |
| + s3 := &stubScanS3{t: t} // putTagging unset: nothing may be written |
| + s3.getObject = func(*awss3.GetObjectInput) (*awss3.GetObjectOutput, error) { |
| + return nil, apiErr("NoSuchKey") |
| + } |
| + c := &clamavScanner{s3: s3, engine: &fakeEngine{}, maxBytes: 1 << 20} |
| + |
| + if _, err := c.Scan(context.Background(), "b", "k", "e", 5); !errors.Is(err, ErrObjectGone) { |
| + t.Fatalf("err = %v, want ErrObjectGone", err) |
| + } |
| +} |
| + |
| +// Unreadable TAGS are denied too, by the same argument: Verdict is called for |
| +// every object on every page, so an error there stalls just as hard. |
| +func TestClamAVUnreadableTagsAreDenied(t *testing.T) { |
| + s3 := &stubScanS3{t: t} |
| + s3.getTagging = func(*awss3.GetObjectTaggingInput) (*awss3.GetObjectTaggingOutput, error) { |
| + return nil, apiErr("AccessDenied") |
| + } |
| + c := &clamavScanner{s3: s3, engine: &fakeEngine{}} |
| + |
| + v, err := c.Verdict(context.Background(), "b", "k", "e") |
| + if err != nil { |
| + t.Fatalf("an unreadable tag set failed the pass: %v", err) |
| + } |
| + if v != VerdictDenied { |
| + t.Fatalf("verdict = %q, want denied", v) |
| + } |
| +} |
| + |
| +// A genuine transport failure is still a pass failure — the classification |
| +// must not swallow everything. |
| +func TestClamAVTransportErrorStillFailsThePass(t *testing.T) { |
| + s3 := &stubScanS3{t: t} |
| + s3.getObject = func(*awss3.GetObjectInput) (*awss3.GetObjectOutput, error) { |
| + return nil, apiErr("InternalError") |
| + } |
| + c := &clamavScanner{s3: s3, engine: &fakeEngine{}, maxBytes: 1 << 20} |
| + |
| + v, err := c.Scan(context.Background(), "b", "k", "e", 5) |
| + if err == nil { |
| + t.Fatal("a transport failure was recorded as a verdict") |
| + } |
| + if v != VerdictUnknown { |
| + t.Fatalf("verdict = %q, want unknown", v) |
| + } |
| +} |
diff --git a/internal/objstore/sidecar.go b/internal/objstore/sidecar.go| index 2655433c1..ebe0c2d3a 100644 |
| --- a/internal/objstore/sidecar.go |
| +++ b/internal/objstore/sidecar.go |
| @@ -49,7 +49,15 @@ type Sweep struct { |
| Denied int64 `json:"denied"` |
| Failed int64 `json:"failed"` |
| Unscanned int64 `json:"unscanned"` |
| - Bytes int64 `json:"bytes"` |
| + // Fresh and Reused split Scanned by WHEN the verdict was reached. A clean |
| + // tag is reused for as long as the object's ETag matches, so a completed |
| + // sweep can be built almost entirely from judgements made months ago, |
| + // against a signature database that is not the one named above. Without |
| + // this split, Signature reads as a claim about every object in the rollup, |
| + // and it is not one. |
| + Fresh int64 `json:"fresh"` |
| + Reused int64 `json:"reused"` |
| + Bytes int64 `json:"bytes"` |
| // After is the last key of the last written page — the resume point. |
| // A key, not a ListObjectsV2 continuation token: a token's lifetime across |
| // processes is unspecified and, on Ceph, unmeasured, while a key is |
diff --git a/internal/objstore/sweep.go b/internal/objstore/sweep.go| index edce0db1b..4da9e8a02 100644 |
| --- a/internal/objstore/sweep.go |
| +++ b/internal/objstore/sweep.go |
| @@ -158,12 +158,22 @@ func RunSweep(ctx context.Context, s release.Store, lister ObjectLister, sc Scan |
| if err != nil { |
| return nil, err |
| } |
| + // Verdicts reached on THIS page versus verdicts carried forward from an |
| + // earlier sweep. A clean tag is reused whenever the ETag still matches, |
| + // so a completed rollup can consist almost entirely of judgements made |
| + // months ago — against signatures that are not the ones the sidecar now |
| + // names. Counting the two apart is what stops "engine database" being |
| + // read as "every object below was judged against this". |
| + var fresh, reused int64 |
| for _, o := range objs { |
| if err := ctx.Err(); err != nil { |
| return nil, err |
| } |
| v, err := sc.Verdict(ctx, bucket, o.Key, o.ETag) |
| + if errors.Is(err, ErrObjectGone) { |
| + continue // deleted since the listing; not ours to count |
| + } |
| if err != nil { |
| return nil, err |
| } |
| @@ -173,13 +183,29 @@ func RunSweep(ctx context.Context, s release.Store, lister ObjectLister, sc Scan |
| // without writing. Nothing is lost but the listing — every |
| // verdict already reached is a tag in the bucket, so the |
| // re-run reads them back without re-scanning a byte. |
| - if v, err = sc.Scan(ctx, bucket, o.Key, o.ETag, o.Size); err != nil { |
| + // |
| + // Per-object facts do NOT arrive here as errors. An object we |
| + // are not allowed to read comes back as VerdictDenied and one |
| + // that has been deleted as ErrObjectGone, precisely so that |
| + // neither can stall the page: an error here abandons the page |
| + // unwritten, and a page that fails identically every tick |
| + // means the bucket's sweep never advances past it. |
| + v, err = sc.Scan(ctx, bucket, o.Key, o.ETag, o.Size) |
| + if errors.Is(err, ErrObjectGone) { |
| + continue |
| + } |
| + if err != nil { |
| return nil, err |
| } |
| + fresh++ |
| + } else { |
| + reused++ |
| } |
| side.Current.count(v, o.Size) |
| } |
| + side.Current.Fresh += fresh |
| + side.Current.Reused += reused |
| side.Current.At = time.Now().UTC() |
| if len(objs) > 0 { |
| side.Current.After = objs[len(objs)-1].Key |
diff --git a/internal/objstore/sweep_test.go b/internal/objstore/sweep_test.go| index 4d86bbd2c..0b7975a5a 100644 |
| --- a/internal/objstore/sweep_test.go |
| +++ b/internal/objstore/sweep_test.go |
| @@ -348,3 +348,126 @@ func TestSweepIgnoresStoreWithoutScanning(t *testing.T) { |
| t.Fatalf("a store that never opted in was swept: %+v", side) |
| } |
| } |
| + |
| +// The CAS retry guard, which the pre-write re-read does NOT cover. |
| +// |
| +// Two writes are in play. The FIRST write of a fresh sweep is create-only, and |
| +// the pre-write re-read is what protects that one. A LATER page's write is |
| +// conditional on an etag, and losing that CAS means someone else moved the |
| +// sidecar — another driver (the on-demand sweep verb races the hourly tick by |
| +// design), or a teardown deleting it. A conditional write against a DELETED |
| +// key also reads as ErrCASConflict, so a blind retry falls back to create-only |
| +// and resurrects the object teardown just removed. |
| +// |
| +// Here the sidecar is deleted between this sweep's page-two write attempt and |
| +// its conflict re-read. The sweep must stand down, not resurrect. |
| +func TestSweepStandsDownWhenSidecarVanishesMidRetry(t *testing.T) { |
| + ctx := context.Background() |
| + s, p, sc, _ := sweepFixture(t, 5) |
| + l := &memLister{p: p, pageSize: 2} |
| + |
| + // One page, so a real sidecar with a Current exists and the next write is |
| + // a conditional replace rather than a create. |
| + if _, err := RunSweep(ctx, s, l, sc, "bab", "titogo", 0); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, _, err := s.Get(ctx, ScanKey("bab", "titogo")); err != nil { |
| + t.Fatalf("setup: no sidecar to conflict against: %v", err) |
| + } |
| + |
| + // Now make the next write lose its CAS *and* find the key gone. Writing a |
| + // different body under a new etag invalidates the caller's etag; deleting |
| + // it afterwards is the teardown. |
| + l.before = func() { |
| + l.before = nil // once: only the second page races |
| + if _, err := PutSidecar(ctx, s, "bab", "titogo", |
| + &ScanSidecar{Current: &Sweep{After: "somewhere-else"}}, ""); err == nil { |
| + t.Fatal("expected the create-only write to conflict with the existing sidecar") |
| + } |
| + // Delete it outright — the teardown case. |
| + if err := s.Delete(ctx, ScanKey("bab", "titogo")); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + |
| + side, err := RunSweep(ctx, s, l, sc, "bab", "titogo", 0) |
| + if err != nil { |
| + t.Fatalf("standing down is not an error: %v", err) |
| + } |
| + _ = side |
| + if _, _, err := s.Get(ctx, ScanKey("bab", "titogo")); !errors.Is(err, os.ErrNotExist) { |
| + t.Fatalf("the sweep resurrected a sidecar that had been deleted: %v", err) |
| + } |
| +} |
| + |
| +// Fresh and reused verdicts are counted apart. A clean tag is reused for as |
| +// long as the ETag matches, so a completed rollup can be built almost entirely |
| +// from judgements made months ago — against a different signature database |
| +// than the one the sidecar names. Without this split, "engine database" reads |
| +// as a claim about every object in the rollup, and it is not one. |
| +func TestSweepCountsFreshAndReusedVerdictsApart(t *testing.T) { |
| + ctx := context.Background() |
| + s, p, sc, _ := sweepFixture(t, 3) |
| + |
| + first, err := RunSweep(ctx, s, &memLister{p: p}, sc, "bab", "titogo", time.Minute) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if first.LastComplete.Fresh != 3 || first.LastComplete.Reused != 0 { |
| + t.Fatalf("first sweep: fresh=%d reused=%d, want 3/0", |
| + first.LastComplete.Fresh, first.LastComplete.Reused) |
| + } |
| + |
| + // Nothing changed in the bucket, so the second sweep reaches no new |
| + // verdicts at all — it reads back the tags the first one wrote. |
| + second, err := RunSweep(ctx, s, &memLister{p: p}, sc, "bab", "titogo", time.Minute) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if second.LastComplete.Fresh != 0 || second.LastComplete.Reused != 3 { |
| + t.Fatalf("second sweep: fresh=%d reused=%d, want 0/3 — every verdict was carried forward", |
| + second.LastComplete.Fresh, second.LastComplete.Reused) |
| + } |
| + if second.LastComplete.Scanned != 3 { |
| + t.Fatalf("scanned = %d, want 3 — reused verdicts are still coverage", |
| + second.LastComplete.Scanned) |
| + } |
| +} |
| + |
| +// An object that vanishes between the listing and the fetch is dropped, not |
| +// counted. It is neither covered nor a gap — it is not there. |
| +func TestSweepDropsObjectsThatDisappearMidPage(t *testing.T) { |
| + ctx := context.Background() |
| + s, p, _, _ := sweepFixture(t, 3) |
| + sc := &goneScanner{gone: map[string]bool{objectName(1): true}} |
| + |
| + side, err := RunSweep(ctx, s, &memLister{p: p}, sc, "bab", "titogo", time.Minute) |
| + if err != nil { |
| + t.Fatalf("a deleted object failed the sweep: %v", err) |
| + } |
| + if side.LastComplete.Objects != 2 { |
| + t.Fatalf("objects = %d, want 2 — the deleted one must not be counted", |
| + side.LastComplete.Objects) |
| + } |
| +} |
| + |
| +// goneScanner answers ErrObjectGone for named keys and clean for the rest. |
| +type goneScanner struct { |
| + MemScanner |
| + gone map[string]bool |
| +} |
| + |
| +func (g *goneScanner) Verdict(context.Context, string, string, string) (Verdict, error) { |
| + return VerdictUnknown, nil |
| +} |
| + |
| +func (g *goneScanner) Scan(_ context.Context, _, key, _ string, _ int64) (Verdict, error) { |
| + if g.gone[key] { |
| + return VerdictUnknown, ErrObjectGone |
| + } |
| + return VerdictClean, nil |
| +} |
| + |
| +func (g *goneScanner) Provenance(context.Context) (string, string, error) { |
| + return "clamav", "test/0/never", nil |
| +} |
diff --git a/scan.go b/scan.go| index bb4e4004d..49e3b2491 100644 |
| --- a/scan.go |
| +++ b/scan.go |
| @@ -209,16 +209,28 @@ func printScanStatus(w io.Writer, res apiScanReply) { |
| c := colorFor(w) |
| fmt.Fprintf(w, "%s %s\n", c.head("object scanning for"), res.App) |
| - if res.Scanning == nil || res.Scanning.Requested == nil { |
| + if res.Scanning == nil || (res.Scanning.Requested == nil && res.Scanning.Enabled == nil) { |
| mutedf(w, "not requested — carlos store scan request --app %s --plaintext", res.App) |
| return |
| } |
| + |
| + // An engine can be enabled with no request on record — it should not be, |
| + // and the console now refuses it, but a record written before that refusal |
| + // existed can still look like this. Returning early on a nil Requested |
| + // would print "not requested" while an engine was actively reading these |
| + // objects, which is the worst sentence this command could produce. |
| req := res.Scanning.Requested |
| - fmt.Fprintf(w, " requested: by %s at %s\n", req.By, req.At.Format(time.RFC3339)) |
| + switch { |
| + case req == nil: |
| + warnf(w, "no attestation is on record for this store, but an engine is enabled — "+ |
| + "run carlos store scan request --app %s to say whether these objects are readable", res.App) |
| + default: |
| + fmt.Fprintf(w, " requested: by %s at %s\n", req.By, req.At.Format(time.RFC3339)) |
| + } |
| // Rule 2, and it returns: an encrypted store gets no coverage line at all, |
| // not a zeroed one. |
| - if req.Encrypted { |
| + if req != nil && req.Encrypted { |
| mutedf(w, " attested client-encrypted — a scanner cannot read these objects,") |
| mutedf(w, " so NO coverage is claimed for this store and none can be.") |
| return |