carlos / platform Public

console: disable actually disables

The plan's first draft left this out entirely, and the omission was the
worst kind: an operator would run `carlos system antivirus disable`,
watch the hourly tick go quiet, and `carlos store scan sweep` would
still read tenant bytes. A tick going quiet is not scanning being off.

So the check lives in RunSweep as well as the doors. The sweep verb
reaches RunSweep by its own path, and a check that only ever sits in a
handler is a check the next caller forgets. Disable has to disable
everywhere or it disables nothing.

The two 503s stay distinct, which is subtler than it looks. "There is
no clamd wired on this box" and "an operator switched scanning off"
want different actions — one is a unit file, the other is one command —
so scanNoEngine names CARLOS_CLAMD_ADDR and the new refusal names the
command. Collapsing them would send someone to the box to fix something
a verb would have fixed, and a test asserts the two messages differ.

The member's own verbs keep working while scanning is disabled. Without
that, disabling would strand every enforcing record, with the door that
clears the deny refusing for exactly the reason it needs using — the
deadlock the original design was careful about, which it would have
been easy to reintroduce here.

Roughly twenty-five existing tests arm a scanner with no deployment
record; enableScanning and scanArmed now write one, and the third
fixture that arms a scanner by hand gets it too.

Worth recording: while writing this, a patch script aborted after making
one edit but before writing the file, so the grant-door gate silently
did not land. TestDeploymentDisableStopsEveryPath caught it — /grant
answering 200 while scanning was disabled, which is precisely the bug
this commit exists to close, reproduced by accident.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Paul Campbell pushed by paul@keymail.dev 1 month ago 01dca93e8f180c73eb9d8db295fc10ff6b1cf228 parent f3c3111

4 files changed, +202 −7

internal/console/apiscan.go +39 −3
internal/console/apiscan_test.go +132 −3
internal/objstore/sweep.go +20 −1
internal/objstore/teardown_scan_test.go +11 −0

diff --git a/internal/console/apiscan.go b/internal/console/apiscan.go

index f8e1e0399..a4e34cf54 100644

--- a/internal/console/apiscan.go

+++ b/internal/console/apiscan.go

@@ -38,11 +38,40 @@ import (

// still costs one request rather than a console goroutine.

const scanSweepTimeout = 2 * time.Minute

-// scanNoEngine is the 503 the doors that need an engine give. It names the

+// scanNoEngine is the 503 for a deployment with no clamd WIRED. It names the

// variable, because that is the one thing the person reading it can act on.

+//

+// Distinct from scanDisabled below, and deliberately: "there is no engine on

+// this box" and "an operator turned scanning off" want different actions, and

+// a single message would send someone to edit a unit file when a command would

+// have done.

func scanNoEngine(w http.ResponseWriter) {

jsonError(w, http.StatusServiceUnavailable,

- "no virus-scanning engine is configured for this deployment (CARLOS_STORE_SCANNER)")

+ "no virus-scanning engine is configured for this deployment (CARLOS_CLAMD_ADDR)")

+// scanDisabled is the 503 for a deployment whose engine is wired but whose

+// scanning is switched off. Names the command, not a variable: this one is

+// restartless.

+func scanDisabled(w http.ResponseWriter) {

+ jsonError(w, http.StatusServiceUnavailable,

+ "object scanning is disabled for this deployment — run `carlos system antivirus enable`")

+// scanEnabledForDeployment is the door-side read of the deployment switch. A

+// read error is neither on nor off, so it answers 502 rather than guessing.

+func (s *Server) scanEnabledForDeployment(ctx context.Context, w http.ResponseWriter) bool {

+ on, err := release.AntivirusEnabled(ctx, s.cfg.Releases)

+ if err != nil {

+ log.Printf("console: api scan: read antivirus record: %v", err)

+ jsonError(w, http.StatusBadGateway, "could not read the scanning state — try again")

+ return false

+ }

+ if !on {

+ scanDisabled(w)

+ return false

+ }

+ return true

}

// scanReply is the single shape every door here answers with, so status and

@@ -76,7 +105,7 @@ func (s *Server) scanReply(ctx context.Context, acct *Account, app string, rec *

// 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"

+ "(CARLOS_CLAMD_ADDR) — the record says one is enabled, but no sweep is running"

}

return out, nil

}

@@ -256,6 +285,9 @@ func (s *Server) handleAPIScanGrant(w http.ResponseWriter, r *http.Request) {

ctx, cancel := context.WithTimeout(r.Context(), storeTimeout)

defer cancel()

+ if !s.scanEnabledForDeployment(ctx, w) {

+ return

+ }

rec, _, ok := s.scanRecord(ctx, w, acct, app)

if !ok {

return

@@ -536,6 +568,10 @@ func (s *Server) handleAPIScanSweep(w http.ResponseWriter, r *http.Request) {

// stays "see the log": a backend error carries bucket names, ARNs and

// request ids, and none of that belongs in a client sentence.

switch {

+ case errors.Is(err, objstore.ErrScanningDisabled):

+ // Not an incident — a command away. The sweep verb reaches

+ // RunSweep by its own path, so this is where `disable` stops it.

+ scanDisabled(w)

case errors.Is(err, objstore.ErrEngineUnreachable):

jsonError(w, http.StatusServiceUnavailable,

"the scanning engine is not answering — check clamd on the console box, then re-run")

diff --git a/internal/console/apiscan_test.go b/internal/console/apiscan_test.go

index 7a3d09c12..a28f10b25 100644

--- a/internal/console/apiscan_test.go

+++ b/internal/console/apiscan_test.go

@@ -1,11 +1,15 @@

package console

import (

+ "context"

+ "errors"

"net/http"

"strings"

"testing"

+ "time"

"github.com/carlosframework/platform/internal/objstore"

+ "github.com/carlosframework/platform/internal/release"

)

func scanURL(fx *consoleFixture, app string) string { return storeURL(fx, app) + "/scan" }

@@ -27,6 +31,7 @@ func scanArmed(t *testing.T, code string) (*consoleFixture, *objstore.MemScanner

grantStore(t, fx, token)

sc := objstore.NewMemScanner()

fx.srv.cfg.StoreScanner = sc

+ enableDeploymentScanning(t, fx)

return fx, sc, token

}

@@ -82,8 +87,8 @@ func TestScanGrantRequires503WithoutScanner(t *testing.T) {

if status != http.StatusServiceUnavailable {

t.Fatalf("scan%s with no engine = %d %v, want 503", door, status, out)

}

- if msg := str(t, out, "error"); !strings.Contains(msg, "CARLOS_STORE_SCANNER") {

- t.Fatalf("scan%s error = %q, want the scanner variable named", door, msg)

+ if msg := str(t, out, "error"); !strings.Contains(msg, "CARLOS_CLAMD_ADDR") {

+ t.Fatalf("scan%s error = %q, want the wiring variable named", door, msg)

}

// enforce ON also needs one — the tag denied on comes from the engine.

@@ -205,6 +210,7 @@ func TestScanEnforceWritesTheDenyToThePolicy(t *testing.T) {

fx, fake, token, _ := storeArmed(t, "code-scan-policy")

grantStore(t, fx, token)

fx.srv.cfg.StoreScanner = objstore.NewMemScanner()

+ enableDeploymentScanning(t, fx)

requestScanning(t, fx, token, false)

if status, out := postJSON(t, scanURL(fx, "titogo")+"/grant", map[string]any{}, token); status != http.StatusOK {

@@ -350,7 +356,130 @@ func TestScanStatusReportsMissingEngineOnEnabledRecord(t *testing.T) {

t.Fatalf("scan status = %d %v", status, out)

}

msg, _ := out["engine_error"].(string)

- if !strings.Contains(msg, "CARLOS_STORE_SCANNER") {

+ if !strings.Contains(msg, "CARLOS_CLAMD_ADDR") {

t.Fatalf("engine_error = %q, want it to say no engine is configured", msg)

}

+// `disable` has to disable everywhere, or it disables nothing.

+//

+// The hourly pass going quiet is not the same as scanning being off: the sweep

+// verb and the grant door reach the engine by their own paths. Without these

+// checks an operator could disable scanning, watch the tick stop, and still

+// have `carlos store scan sweep` read tenant bytes.

+func TestDeploymentDisableStopsEveryPath(t *testing.T) {

+ fx, _, token := scanArmed(t, "code-scan-disabled")

+ requestScanning(t, fx, token, false)

+ if s, o := postJSON(t, scanURL(fx, "titogo")+"/grant", map[string]any{}, token); s != http.StatusOK {

+ t.Fatalf("setup grant = %d %v", s, o)

+ }

+ // Now switch the deployment off, the way `carlos system antivirus disable`

+ // will. The engine stays wired — this is policy, not wiring.

+ disableDeploymentScanning(t, fx)

+ for _, door := range []string{"/grant", "/sweep"} {

+ status, out := postJSON(t, scanURL(fx, "titogo")+door, map[string]any{}, token)

+ if status != http.StatusServiceUnavailable {

+ t.Fatalf("scan%s while disabled = %d %v, want 503", door, status, out)

+ }

+ msg := str(t, out, "error")

+ if !strings.Contains(msg, "carlos system antivirus enable") {

+ t.Fatalf("scan%s error = %q, want it to name the command that fixes it", door, msg)

+ }

+ // And NOT the wiring variable: an operator whose engine is wired and

+ // whose record is off must not be sent to edit a unit file.

+ if strings.Contains(msg, "CARLOS_CLAMD_ADDR") {

+ t.Fatalf("scan%s conflated a disabled record with missing wiring: %q", door, msg)

+ }

+// The two 503s stay distinct. Wired-but-off and not-wired want different

+// actions, and a single message would send someone to the wrong one.

+func TestDisabledAndUnwiredAreDifferent503s(t *testing.T) {

+ fx, _, token := scanArmed(t, "code-scan-two-503s")

+ requestScanning(t, fx, token, false)

+ disableDeploymentScanning(t, fx)

+ _, offBody := postJSON(t, scanURL(fx, "titogo")+"/sweep", map[string]any{}, token)

+ offMsg := str(t, offBody, "error")

+ enableDeploymentScanning(t, fx)

+ fx.srv.cfg.StoreScanner = nil

+ _, unwiredBody := postJSON(t, scanURL(fx, "titogo")+"/sweep", map[string]any{}, token)

+ unwiredMsg := str(t, unwiredBody, "error")

+ if offMsg == unwiredMsg {

+ t.Fatalf("both states answer identically: %q", offMsg)

+ }

+ if !strings.Contains(unwiredMsg, "CARLOS_CLAMD_ADDR") {

+ t.Fatalf("the unwired message does not name the wiring: %q", unwiredMsg)

+ }

+// RunSweep itself refuses, not merely the door in front of it. The sweep verb

+// is not the only caller, and a check that lived only in a handler would be a

+// check the next caller forgets.

+func TestRunSweepRefusesWhenDeploymentDisabled(t *testing.T) {

+ ctx := context.Background()

+ fx := newConsoleFixture(t)

+ fx.srv.cfg.StoreScanner = objstore.NewMemScanner()

+ enableStoreRecordOnly(t, fx, fixtureAcct, fixtureApp)

+ // No deployment record at all: absent means not enabled.

+ _, err := objstore.RunSweep(ctx, fx.rel, &countingLister{keys: []string{"a.txt"}},

+ objstore.NewMemScanner(), fixtureAcct, fixtureApp, time.Minute)

+ if !errors.Is(err, objstore.ErrScanningDisabled) {

+ t.Fatalf("RunSweep with no deployment record = %v, want ErrScanningDisabled", err)

+ }

+// The member's own verbs keep working while scanning is disabled. Otherwise

+// disabling would strand every enforcing record, with the door that clears it

+// refusing for the very reason it needs using.

+func TestMemberVerbsSurviveDeploymentDisable(t *testing.T) {

+ fx, _, token := scanArmed(t, "code-scan-disabled-member")

+ requestScanning(t, fx, token, false)

+ if s, o := postJSON(t, scanURL(fx, "titogo")+"/grant", map[string]any{}, token); s != http.StatusOK {

+ t.Fatalf("setup grant = %d %v", s, o)

+ }

+ if s, o := postJSON(t, scanURL(fx, "titogo")+"/enforce", map[string]any{"on": true}, token); s != http.StatusOK {

+ t.Fatalf("setup enforce = %d %v", s, o)

+ }

+ disableDeploymentScanning(t, fx)

+ if s, o := postJSON(t, scanURL(fx, "titogo"), map[string]any{"encrypted": false}, token); s != http.StatusOK {

+ t.Fatalf("request while disabled = %d %v", s, o)

+ }

+ if s, o := postJSON(t, scanURL(fx, "titogo")+"/enforce", map[string]any{"on": false}, token); s != http.StatusOK {

+ t.Fatalf("enforce off while disabled = %d %v", s, o)

+ }

+ if s, o := postJSON(t, scanURL(fx, "titogo")+"/revoke", map[string]any{}, token); s != http.StatusOK {

+ t.Fatalf("revoke while disabled = %d %v", s, o)

+ }

+ if rec := storedRecord(t, fx, "titogo"); rec.Scanning.Enforce != nil {

+ t.Fatal("enforce off left a live deny while scanning was disabled")

+ }

+// disableDeploymentScanning flips the switch off, leaving the engine wired.

+func disableDeploymentScanning(t *testing.T, fx *consoleFixture) {

+ t.Helper()

+ ctx := context.Background()

+ av, etag, err := release.GetAntivirus(ctx, fx.rel)

+ if err != nil {

+ t.Fatal(err)

+ }

+ if av == nil {

+ av = &release.AntivirusSettings{Engine: "clamav"}

+ }

+ av.Enabled = false

+ av.At = time.Now().UTC()

+ av.By = "test"

+ if _, err := release.PutAntivirus(ctx, fx.rel, av, etag); err != nil {

+ t.Fatal(err)

+ }

diff --git a/internal/objstore/sweep.go b/internal/objstore/sweep.go

index 53f589fa0..e3e47df6f 100644

--- a/internal/objstore/sweep.go

+++ b/internal/objstore/sweep.go

@@ -107,6 +107,10 @@ var errStandDown = errors.New("objstore: sweep standing down")

var (

// ErrEngineUnreachable is the engine not answering at all.

ErrEngineUnreachable = errors.New("objstore: the scanning engine is not answering")

+ // ErrScanningDisabled is the deployment switch being off. Its own sentinel

+ // because a door must tell "nobody turned this on" apart from "the engine

+ // is broken": one is a command away, the other is an incident.

+ ErrScanningDisabled = errors.New("objstore: object scanning is disabled for this deployment")

// ErrEngineDrift is a record naming a different engine than the one

// configured. Refusing beats recounting another engine's verdicts as

// unscanned, which would destroy the coverage history.

@@ -161,7 +165,22 @@ func RunSweep(ctx context.Context, s release.Store, lister ObjectLister, sc Scan

if sc == nil {

return nil, fmt.Errorf("objstore: %s/%s has scanning enabled but no engine is configured "+

- "(set CARLOS_STORE_SCANNER)", acct, app)

+ "(set CARLOS_CLAMD_ADDR)", acct, app)

+ }

+ // The DEPLOYMENT switch, checked here and not only in the pass.

+ //

+ // A tick that goes quiet is not the same as scanning being off: the

+ // on-demand sweep verb reaches this function by its own path, so without

+ // this an operator could run `carlos system antivirus disable`, watch the

+ // hourly pass stop, and still have `carlos store scan sweep` read tenant

+ // bytes. Disable has to disable everywhere or it disables nothing.

+ on, err := release.AntivirusEnabled(ctx, s)

+ if err != nil {

+ return nil, err

+ }

+ if !on {

+ return nil, fmt.Errorf("%w (%s/%s)", ErrScanningDisabled, acct, app)

}

engine, signature, err := sc.Provenance(ctx)

diff --git a/internal/objstore/teardown_scan_test.go b/internal/objstore/teardown_scan_test.go

index 5282d5330..159555d46 100644

--- a/internal/objstore/teardown_scan_test.go

+++ b/internal/objstore/teardown_scan_test.go

@@ -27,6 +27,17 @@ func enableScanning(t *testing.T, s release.Store, acct, app string) {

if _, err := Put(ctx, s, acct, app, rec, etag); err != nil {

t.Fatal(err)

}

+ // The DEPLOYMENT switch too. Absent record = not enabled, and every

+ // fixture that arms a scanner and expects a sweep needs both halves.

+ if av, _, err := release.GetAntivirus(ctx, s); err != nil {

+ t.Fatal(err)

+ } else if av == nil {

+ if _, err := release.PutAntivirus(ctx, s, &release.AntivirusSettings{

+ Enabled: true, Engine: "clamav", At: time.Now().UTC(), By: "test",

+ }, ""); err != nil {

+ t.Fatal(err)

+ }

}

// The scanner is released BEFORE the bucket goes, while the record naming it