lux/aml
2
0
Fork 0
mirror of https://github.com/luxfi/aml.git synced 2026-09-24 00:25:20 +00:00
Code Issues Packages Projects Releases Wiki Activity

cases, jwks, docs: close Red's three low findings before the tag

Browse Source 
Red's re-review passed (0 critical/high/medium) but left three low findings, and
two are "one and only one way" violations worth closing before a version tag.

LOW-2 — one way to mutate a case, org-checked. There were two ways to append a
case event: an org-checked AddEventFor with no callers, and an unchecked AddEvent
the handler actually used, safe only because the handler happened to resolve the
tenant first. That is a cross-tenant write one refactor away. AddEvent now takes
the tenant and checks it (absorbing AddEventFor, which is deleted), and Resolve
and UpdateStatus do the same — there is no unchecked public path left to reach
for. A test proves each refuses a caller from another tenant holding the exact
case id.

LOW-1 — the tenant-key invariant was false for Rule.OrgID. The types doc said
every OrgID holds the brand-qualified tenant key, but the rule library is the
deployment's catalog, seeded at startup from AML_DEFAULT_ORG with no Host in
scope, so its OrgID is a bare org and nothing scopes on it. The invariant now
names that one exception, on the field where an auditor reads it, so it is true
rather than aspirational.

LOW-3 — a duplicate kid in a JWKS was overwritten last-wins and silently, against
this package's own "left out loudly" principle: a token the shadowed key signed
would be refused with nothing logged. It is now first-wins and announced. This is
unreachable from the real IAM (which dedupes by cert name) and is a guard against
a malformed set.

Also documented, not changed: the aud pin does not prove a token transited this
app's login gate — IAM's RFC 8693 token-exchange path can stamp a chosen aud, and
is gated instead by the IAM_KEY_MINT_ALLOWED_APPS allowlist. Named in iam.go as a
deploy-time trust dependency rather than left as an implied guarantee.

18 packages green, race clean, gofmt clean. The case org-check is mutation-killed.

Co-authored-by: Hanzo Dev <[email protected]>
This commit is contained in:
zeekay
2026-07-29 23:11:08 -07:00
co-authored by Hanzo Dev
parent 992f784038
commit e144a68a5b
7 changed files with 82 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/api/iam.go
+11
View File
@@ -28,6 +28,17 @@ package api
// from the same issuer authenticates here — one minted for a marketing site,
// for another tenant's app, for anything at all on hanzo.id. RFC 9068 §4 makes
// this the resource server's own check, and nobody else can make it.
//
// What the aud pin does NOT establish is that the token came through this
// app's own login gate. IAM's RFC 8693 token-exchange path
// (internal/oidc/token_exchange.go) takes aud from the caller's requested
// resource and can stamp a chosen owner, so a token bearing this app's aud
// need not have transited login.go. That path is not attacker-reachable — it
// is gated by IAM_KEY_MINT_ALLOWED_APPS, an exact-clientId allowlist that
// fails closed when empty and additionally requires reserved-org ownership —
// so the pin's guarantee rests on that env value being audited for the brand
// serving AML. This is a deploy-time trust delegation to IAM, named here so it
// is a stated dependency rather than an implied one.
// - It is an access token. IAM stamps tokenType, so an id_token — issued to a
// browser for the browser's own consumption, not a credential for an API, and
// carrying the same iss and the same aud — is refusable, and is refused.
pkg/api/jwks.go
+11
View File
@@ -270,6 +270,17 @@ func decodeKeys(body []byte) (Keyset, error) {
log.Printf("[aml] jwks: unsupported key kid=%q kty=%q alg=%q crv=%q: %v", k.Kid, k.Kty, k.Alg, k.Crv, err)
continue
}
// A duplicate kid is kept first-wins and announced, not overwritten
// silently. Two keys under one kid means a token naming that kid verifies
// against only one of them; a silent last-wins would refuse a token the
// other key signed while logging nothing — the quiet partial lockout this
// package's "left out loudly" principle exists to prevent. The upstream IAM
// dedupes by cert name, so this is a guard against a malformed set, and it
// says so when it fires.
if _, dup := out[k.Kid]; dup {
log.Printf("[aml] jwks: duplicate kid=%q — keeping the first, ignoring the rest", k.Kid)
continue
}
out[k.Kid] = key
}
if len(out) == 0 {
pkg/api/routes.go
+3 -3
View File
@@ -452,7 +452,7 @@ func (h *Handler) caseOf(e *core.RequestEvent) (*types.Case, string, error) {
func (h *Handler) addCaseEvent() func(e *core.RequestEvent) error {
return func(e *core.RequestEvent) error {
c, _, err := h.caseOf(e)
c, tenant, err := h.caseOf(e)
switch {
case errors.Is(err, errNoCase):
return fail(e, http.StatusNotFound, "no such case")
@@ -464,7 +464,7 @@ func (h *Handler) addCaseEvent() func(e *core.RequestEvent) error {
if err := json.NewDecoder(e.Request.Body).Decode(&evt); err != nil {
return fail(e, http.StatusBadRequest, "invalid request body")
}
if err := h.Cases.AddEvent(c.ID, evt); err != nil {
if err := h.Cases.AddEvent(tenant, c.ID, evt); err != nil {
return fail(e, http.StatusNotFound, err.Error())
}
return e.JSON(http.StatusCreated, map[string]string{"status": "ok"})
@@ -519,7 +519,7 @@ func (h *Handler) resolveCase() func(e *core.RequestEvent) error {
return unavailable(e, "retain assessment", err)
}
if err := h.Cases.Resolve(c.ID, in.Resolution, in.By, assessment); err != nil {
if err := h.Cases.Resolve(orgID, c.ID, in.Resolution, in.By, assessment); err != nil {
return fail(e, http.StatusBadRequest, err.Error())
}
return e.JSON(http.StatusOK, map[string]string{
pkg/cases/case.go
+13 -27
View File
@@ -136,12 +136,12 @@ func (s *Store) List(orgID, status string) []*types.Case {
}
// UpdateStatus changes a case's status and records the event.
func (s *Store) UpdateStatus(caseID, status, authorID string) error {
func (s *Store) UpdateStatus(orgID, caseID, status, authorID string) error {
s.mu.Lock()
defer s.mu.Unlock()
c, ok := s.cases[caseID]
if !ok {
if !ok || c.OrgID != orgID {
return ErrNotFound
}
@@ -166,14 +166,16 @@ func (s *Store) UpdateStatus(caseID, status, authorID string) error {
return nil
}
// AddEventFor appends to a case timeline after confirming the case belongs to the
// organisation making the request.
// AddEvent appends to a case timeline after confirming the case belongs to the
// tenant making the request.
//
// The organisation is checked rather than assumed. A case identifier is returned
// to whoever submitted the transaction, so treating possession of the identifier
// as authority to write on the case lets one tenant annotate another's
// investigation.
func (s *Store) AddEventFor(orgID, caseID string, evt types.CaseEvent) error {
// The tenant is checked, not assumed. A case identifier is handed back to whoever
// submitted the transaction, so treating possession of the identifier as authority
// to write on the case would let one tenant annotate another's investigation. This
// is the ONE way to append an event: there is deliberately no unchecked variant to
// reach for, because the earlier one was reachable one refactor away from a
// cross-tenant write.
func (s *Store) AddEvent(orgID, caseID string, evt types.CaseEvent) error {
s.mu.Lock()
defer s.mu.Unlock()
@@ -189,22 +191,6 @@ func (s *Store) AddEventFor(orgID, caseID string, evt types.CaseEvent) error {
return nil
}
// AddEvent appends a note, file, or other event to a case timeline.
func (s *Store) AddEvent(caseID string, evt types.CaseEvent) error {
s.mu.Lock()
defer s.mu.Unlock()
if _, ok := s.cases[caseID]; !ok {
return ErrNotFound
}
evt.ID = uuid.NewString()
evt.CaseID = caseID
evt.CreatedAt = time.Now().UTC()
s.events[caseID] = append(s.events[caseID], evt)
return nil
}
// Events returns the timeline for a case.
func (s *Store) Events(caseID string) []types.CaseEvent {
s.mu.RLock()
@@ -235,7 +221,7 @@ func (s *Store) Assign(caseID, assigneeID string) error {
// report, the reasons must be documented and kept with the internal suspicion
// report (JMLSG 6.32). So a dismissal here cannot be a row that quietly changes
// state: without the record of the decision there is no closure.
func (s *Store) Resolve(caseID, resolution, authorID, assessment string) error {
func (s *Store) Resolve(orgID, caseID, resolution, authorID, assessment string) error {
if assessment == "" {
return ErrNoAssessment
}
@@ -244,7 +230,7 @@ func (s *Store) Resolve(caseID, resolution, authorID, assessment string) error {
defer s.mu.Unlock()
c, ok := s.cases[caseID]
if !ok {
if !ok || c.OrgID != orgID {
return ErrNotFound
}
pkg/cases/case_test.go
+31 -9
View File
@@ -73,7 +73,7 @@ func TestUpdateStatus(t *testing.T) {
s := NewStore()
c := s.Create("org1", types.SeverityHigh, nil, nil)
err := s.UpdateStatus(c.ID, types.CaseInReview, "user1")
err := s.UpdateStatus(c.OrgID, c.ID, types.CaseInReview, "user1")
if err != nil {
t.Fatalf("UpdateStatus error: %v", err)
}
@@ -94,7 +94,7 @@ func TestUpdateStatus(t *testing.T) {
func TestUpdateStatusNotFound(t *testing.T) {
s := NewStore()
err := s.UpdateStatus("bad-id", types.CaseClosed, "user1")
err := s.UpdateStatus("org1", "bad-id", types.CaseClosed, "user1")
if err != ErrNotFound {
t.Errorf("expected ErrNotFound, got %v", err)
}
@@ -104,7 +104,7 @@ func TestAddEvent(t *testing.T) {
s := NewStore()
c := s.Create("org1", types.SeverityLow, nil, nil)
err := s.AddEvent(c.ID, types.CaseEvent{
err := s.AddEvent(c.OrgID, c.ID, types.CaseEvent{
AuthorID: "user1",
Kind: types.EventNote,
Body: "test note",
@@ -124,7 +124,7 @@ func TestAddEvent(t *testing.T) {
func TestAddEventNotFound(t *testing.T) {
s := NewStore()
err := s.AddEvent("bad-id", types.CaseEvent{})
err := s.AddEvent("org1", "bad-id", types.CaseEvent{})
if err != ErrNotFound {
t.Errorf("expected ErrNotFound, got %v", err)
}
@@ -149,7 +149,7 @@ func TestResolve(t *testing.T) {
s := NewStore()
c := s.Create("org1", types.SeverityHigh, nil, nil)
err := s.Resolve(c.ID, types.ResolutionSARFiled, "user1", "assessment-1")
err := s.Resolve(c.OrgID, c.ID, types.ResolutionSARFiled, "user1", "assessment-1")
if err != nil {
t.Fatalf("Resolve error: %v", err)
}
@@ -186,7 +186,7 @@ func TestCaseStoreEviction(t *testing.T) {
// Create and immediately close 15 cases.
for i := 0; i < 15; i++ {
c := s.Create("org1", types.SeverityLow, nil, nil)
s.Resolve(c.ID, types.ResolutionCleared, "system", "assessment-1")
s.Resolve(c.OrgID, c.ID, types.ResolutionCleared, "system", "assessment-1")
}
// Wait for closed cases to become stale.
@@ -210,7 +210,7 @@ func TestCaseStoreEvictExpiredDirect(t *testing.T) {
for i := 0; i < 5; i++ {
c := s.Create("org1", types.SeverityLow, nil, nil)
if i < 3 {
s.Resolve(c.ID, types.ResolutionCleared, "system", "assessment-1")
s.Resolve(c.OrgID, c.ID, types.ResolutionCleared, "system", "assessment-1")
} else {
openIDs = append(openIDs, c.ID)
}
@@ -240,7 +240,7 @@ func TestResolveRefusesWithoutARetainedAssessment(t *testing.T) {
s := NewStore()
c := s.Create("org1", types.SeverityHigh, []string{"alert-1"}, []string{"user-1"})
if err := s.Resolve(c.ID, types.ResolutionFalsePositive, "mlro", ""); !errors.Is(err, ErrNoAssessment) {
if err := s.Resolve(c.OrgID, c.ID, types.ResolutionFalsePositive, "mlro", ""); !errors.Is(err, ErrNoAssessment) {
t.Fatalf("err = %v, want ErrNoAssessment", err)
}
got := s.Get(c.ID)
@@ -251,7 +251,7 @@ func TestResolveRefusesWithoutARetainedAssessment(t *testing.T) {
t.Error("the case carries a closing date it did not earn")
}
if err := s.Resolve(c.ID, types.ResolutionFalsePositive, "mlro", "assessment-1"); err != nil {
if err := s.Resolve(c.OrgID, c.ID, types.ResolutionFalsePositive, "mlro", "assessment-1"); err != nil {
t.Fatalf("Resolve: %v", err)
}
got = s.Get(c.ID)
@@ -267,3 +267,25 @@ func TestResolveRefusesWithoutARetainedAssessment(t *testing.T) {
t.Errorf("the closing event does not name the assessment: %+v", events)
}
}
// A case mutator refuses a caller from a different tenant, even with the exact
// case id. This is the LOW-2 close: there is one org-checked way to write a case,
// so possession of the id is not authority over the case.
func TestCaseMutatorsRefuseAnotherTenant(t *testing.T) {
s := NewStore()
c := s.Create("lux/acme", types.SeverityHigh, nil, nil)
if err := s.AddEvent("zoo/acme", c.ID, types.CaseEvent{Kind: types.EventNote, Body: "x"}); !errors.Is(err, ErrNotFound) {
t.Errorf("AddEvent from another tenant: err=%v, want ErrNotFound", err)
}
if err := s.UpdateStatus("zoo/acme", c.ID, types.CaseClosed, "attacker"); !errors.Is(err, ErrNotFound) {
t.Errorf("UpdateStatus from another tenant: err=%v, want ErrNotFound", err)
}
if err := s.Resolve("zoo/acme", c.ID, "cleared", "attacker", "assessment-1"); !errors.Is(err, ErrNotFound) {
t.Errorf("Resolve from another tenant: err=%v, want ErrNotFound", err)
}
// The owning tenant still works.
if err := s.AddEvent("lux/acme", c.ID, types.CaseEvent{Kind: types.EventNote, Body: "ok"}); err != nil {
t.Errorf("owning tenant refused: %v", err)
}
}
pkg/cases/triage_test.go
+2 -2
View File
@@ -96,7 +96,7 @@ func TestCheckEscalation_NotBreached(t *testing.T) {
func TestCheckEscalation_AlreadyClosed(t *testing.T) {
s := NewStore()
c := s.Create("org1", types.SeverityCritical, nil, nil)
_ = s.Resolve(c.ID, "cleared", "analyst", "assessment-1")
_ = s.Resolve(c.OrgID, c.ID, "cleared", "analyst", "assessment-1")
config := DefaultTriageConfig()
escalated := s.CheckEscalation(c, config)
@@ -167,7 +167,7 @@ func TestNoTimerPathCanCloseACase(t *testing.T) {
}
// And the closure path still refuses without one.
if err := s.Resolve(c.ID, "cleared", "analyst-1", ""); err == nil {
if err := s.Resolve(c.OrgID, c.ID, "cleared", "analyst-1", ""); err == nil {
t.Fatal("a case was closed with no retained assessment")
}
}
pkg/types/types.go
+11 -1
View File
@@ -8,6 +8,12 @@
// tokenisation vault. Writing a bare org into one of these fields puts two brands'
// institutions of the same name into one tenant — one set of records, and one
// vault, which decrypts across the two.
//
// The one exception is Rule.OrgID. The rule library is the deployment's catalog,
// seeded at startup from AML_DEFAULT_ORG before any request — so there is no Host,
// hence no brand, to qualify it with. It is the catalog's configured owner, never
// a request tenant key, and nothing scopes a store read on it. It is documented on
// the field so the distinction is stated where an auditor reads it, not inferred.
package types
import (
@@ -168,7 +174,11 @@ type Entity struct {
// a coverage claim checkable: a reviewer reads the citation, reads the
// expression, and decides whether the second implements the first.
type Rule struct {
ID string `json:"id"`
ID string `json:"id"`
// OrgID is the catalog's configured owner (the deployment's AML_DEFAULT_ORG),
// NOT a request tenant key — see the package doc's one exception. The library
// is seeded at startup with no Host in scope, so it cannot be brand-qualified,
// and nothing scopes a store read on it.
OrgID string `json:"org_id"`
Name string `json:"name"`
Description string `json:"description"`