d4fea635

test(store): a security action that cannot be recorded must not happen

a73x 2026-08-23 11:11

Commit message

test(store): a security action that cannot be recorded must not happen

The two ...Atomically tests only proved the audit row existed on the
success path: moving the write to after tx.Commit() left them green. A
SQLite trigger that aborts every audit_log insert supplies the fault
the concrete *sql.DB gives no seam for, and the rollback is asserted end
to end — no host row, token unconsumed, generation unmoved, and the
operation still works once the trigger is dropped. The success-path
claims keep their own tests under honest names.

RemoveHost's refusal was asserted as a bare error, which the vms.host_id
foreign key satisfies on its own — the alarm rang for the wrong reason,
and an ON DELETE CASCADE added for some other purpose would turn the
refusal into a silent delete of the host and its VM rows. The curated
"not drained" text is what the console shows an operator, so that is
what the three refusal sites now assert.

internal/server/store/decommission_test.go

Old		New
@@ -84,7 +84,11 @@ func TestRemoveHostRefusesWhileVMsRemain(t *testing.T) {
84	require.NoError(t, s.DecommissionHost(h.ID))	84	require.NoError(t, s.DecommissionHost(h.ID))
85	// VM row still present (not yet reaped) — removal must refuse.	85	// VM row still present (not yet reaped) — removal must refuse.
86	err := s.RemoveHost(h.ID)	86	err := s.RemoveHost(h.ID)
87	assert.Error(t, err, "RemoveHost must refuse while VM rows remain")	87	// The curated message, not a bare error: the vms.host_id foreign key would
		88	// also refuse, so an error alone proves nothing about the drain check. The
		89	// operator sees this text in the console, and a schema edit that turned the
		90	// FK into ON DELETE CASCADE would silently take the VM rows with the host.
		91	assert.ErrorContains(t, err, "not drained", "RemoveHost must refuse by counting VM rows, with the message the console shows the operator")
88	}	92	}
89		93
90	// TestRemoveHostDrainSequence pins the same VM-row-counting behavior the	94	// TestRemoveHostDrainSequence pins the same VM-row-counting behavior the
@@ -101,12 +105,12 @@ func TestRemoveHostDrainSequence(t *testing.T) {
101	s2 := newStore(t)	105	s2 := newStore(t)
102	h2 := enrollHost(t, s2)	106	h2 := enrollHost(t, s2)
103	vm := makeVM(t, s2, h2, "vm-a")	107	vm := makeVM(t, s2, h2, "vm-a")
104	assert.Error(t, s2.RemoveHost(h2.ID), "live VM row: RemoveHost must refuse")	108	assert.ErrorContains(t, s2.RemoveHost(h2.ID), "not drained", "live VM row: RemoveHost must refuse by its own count, not by the foreign key underneath it")
105		109
106	// DecommissionHost tombstones the host's VMs but leaves the rows in place	110	// DecommissionHost tombstones the host's VMs but leaves the rows in place
107	// until the agent acks their destroy — still not drained.	111	// until the agent acks their destroy — still not drained.
108	require.NoError(t, s2.DecommissionHost(h2.ID))	112	require.NoError(t, s2.DecommissionHost(h2.ID))
109	assert.Error(t, s2.RemoveHost(h2.ID), "tombstoned but not reaped: RemoveHost must still refuse")	113	assert.ErrorContains(t, s2.RemoveHost(h2.ID), "not drained", "tombstoned but not reaped: a row the agent has not acked still counts")
110		114
111	require.NoError(t, s2.HardDeleteVM(vm.ID))	115	require.NoError(t, s2.HardDeleteVM(vm.ID))
112	require.NoError(t, s2.RemoveHost(h2.ID), "reaped: RemoveHost should now succeed")	116	require.NoError(t, s2.RemoveHost(h2.ID), "reaped: RemoveHost should now succeed")

internal/server/store/store_test.go

Old		New
@@ -977,10 +977,51 @@ func TestListVMEvents(t *testing.T) {
977	assert.Equal(t, "vm.delete", limited[0].Action)	977	assert.Equal(t, "vm.delete", limited[0].Action)
978	}	978	}
979		979
980	// TestRedeemWritesAuditRowAtomically pins audit durability: the host.enroll	980	// failAuditWrites installs a trigger that aborts every audit_log insert, and
981	// audit row is written inside the SAME transaction as the redeem, so an	981	// returns the function that removes it. Injecting the failure at the SQLite
982	// enrolled host can never exist without its durable audit record.	982	// level is the only seam available: *sql.DB is concrete, so the fault has to
983	func TestRedeemWritesAuditRowAtomically(t *testing.T) {	983	// come from the database, not from Go.
		984	func failAuditWrites(t testing.T, s Store) (drop func()) {
		985	t.Helper()
		986	_, err := s.db.Exec(`CREATE TRIGGER fail_audit BEFORE INSERT ON audit_log
		987	BEGIN SELECT RAISE(ABORT, 'injected audit failure'); END`)
		988	require.NoError(t, err)
		989	return func() {
		990	_, err := s.db.Exec(`DROP TRIGGER fail_audit`)
		991	require.NoError(t, err)
		992	}
		993	}
		994
		995	// TestRedeemRefusesToEnrollUnaudited proves the host.enroll audit row is
		996	// written INSIDE the redeem transaction. With the audit write failing, the
		997	// whole redeem must roll back: no host row, and the token still unused. An
		998	// audit write moved after tx.Commit() would leave an enrolled host with no
		999	// record of who enrolled it — and this test is the only evidence that a host
		1000	// cannot join the fleet unrecorded.
		1001	func TestRedeemRefusesToEnrollUnaudited(t *testing.T) {
		1002	s := newStore(t)
		1003	tok, err := s.CreateEnrollmentToken(testTenant)
		1004	require.NoError(t, err)
		1005
		1006	drop := failAuditWrites(t, s)
		1007	_, err = s.RedeemEnrollmentToken(tok, EnrollFacts{Name: "host-a", OS: "linux", Arch: "amd64", Provisioner: "cloudhv", Remote: "192.0.2.9"})
		1008	require.Error(t, err, "a redeem whose audit row cannot be written must fail, not enroll silently")
		1009
		1010	hosts, err := s.ListHosts()
		1011	require.NoError(t, err)
		1012	assert.Empty(t, hosts, "no host may exist without its audit row: the enroll and the audit write share one transaction, so a failed audit rolls the host row back")
		1013
		1014	// The rollback must be complete, not partial: a token marked used by the
		1015	// failed attempt would burn a one-shot enrollment credential for nothing.
		1016	drop()
		1017	h, err := s.RedeemEnrollmentToken(tok, EnrollFacts{Name: "host-a", OS: "linux", Arch: "amd64", Provisioner: "cloudhv", Remote: "192.0.2.9"})
		1018	require.NoError(t, err, "the rolled-back redeem must not have consumed the token")
		1019	assert.NotEmpty(t, h.ID)
		1020	}
		1021
		1022	// TestRedeemWritesAuditRow pins what the host.enroll row carries: the host it
		1023	// created and the IP that presented the token, and never the raw token itself.
		1024	func TestRedeemWritesAuditRow(t *testing.T) {
984	s := newStore(t)	1025	s := newStore(t)
985	tok, err := s.CreateEnrollmentToken(testTenant)	1026	tok, err := s.CreateEnrollmentToken(testTenant)
986	require.NoError(t, err)	1027	require.NoError(t, err)
@@ -1020,9 +1061,31 @@ func TestCredGenerationLifecycle(t *testing.T) {
1020	assert.Error(t, err, "unknown host must error")	1061	assert.Error(t, err, "unknown host must error")
1021	}	1062	}
1022		1063
1023	// TestBumpCredGenerationAuditsAtomically pins that the revoke audit row is	1064	// TestBumpCredGenerationRefusesUnaudited proves the host.credential.revoke row
1024	// written in the same transaction as the bump.	1065	// is written INSIDE the bump transaction. With the audit write failing, the
1025	func TestBumpCredGenerationAuditsAtomically(t *testing.T) {	1066	// generation must stay put: a revoke that lands without its audit row is a
		1067	// credential invalidated by nobody, with no record of who did it or when.
		1068	func TestBumpCredGenerationRefusesUnaudited(t *testing.T) {
		1069	s := newStore(t)
		1070	h := enrollHost(t, s)
		1071
		1072	drop := failAuditWrites(t, s)
		1073	_, err := s.BumpCredGeneration(h.ID, "192.0.2.7")
		1074	require.Error(t, err, "a revoke whose audit row cannot be written must fail, not revoke silently")
		1075
		1076	got, err := s.GetHost(h.ID)
		1077	require.NoError(t, err)
		1078	assert.Equal(t, int64(1), got.CredGeneration, "no credential may be revoked without its audit row: the bump and the audit write share one transaction, so a failed audit rolls the bump back")
		1079
		1080	drop()
		1081	gen, err := s.BumpCredGeneration(h.ID, "192.0.2.7")
		1082	require.NoError(t, err)
		1083	assert.Equal(t, int64(2), gen, "the rolled-back bump must not have consumed a generation")
		1084	}
		1085
		1086	// TestBumpCredGenerationAudits pins what the host.credential.revoke row
		1087	// carries: the host whose credentials died and the IP that ordered it.
		1088	func TestBumpCredGenerationAudits(t *testing.T) {
1026	s := newStore(t)	1089	s := newStore(t)
1027	h := enrollHost(t, s)	1090	h := enrollHost(t, s)
1028	_, err := s.BumpCredGeneration(h.ID, "192.0.2.7")	1091	_, err := s.BumpCredGeneration(h.ID, "192.0.2.7")