From 86d3fc9018197116c046786eb0e22686b057e4af Mon Sep 17 00:00:00 2001 From: Jayson Grace Date: Mon, 3 Aug 2026 13:13:58 -0700 Subject: [PATCH] feat: add lab config referential integrity linting and fix overlay drift **Added:** - Cross-file integrity checker for merged lab configs (`cli/internal/labconfig/integrity.go`) that validates every referenced principal, group, domain, host, and vulns_vars entry resolves after RFC 7386 overlay merges, accounting for AD builtins, well-known SIDs, cross-domain forest trusts, and distinguished names - `CheckOverlayDrops` to catch capability silently lost when an overlay redeclares a host array (`vulns`, `scripts`, `vulns_adcs_templates`) without a base entry, a class of defect invisible to referential checks since the merged document stays internally consistent - `adcs_esc10_case1` vuln to dc03 across GOAD base config, dev, staging, and test overlays so ESC6/ESC9 become exploitable in every environment - Regression test suite (`integrity_test.go`) covering all lab configs per environment against a shrink-only baseline, the three shipped GOAD overlay defects, silent-drop detection, deleted-host tolerance, and false-positive guards for builtins and cross-domain refs - Accepted-findings baseline (`testdata/known_findings.txt`) documenting the deliberately rehomed GOAD dev share and the deferred GOAD-variant-1 missing vulns_vars defect - Developer docs explaining that overlay arrays replace rather than merge and that new vulns must be added to every redeclaring overlay (`docs/mkdocs/docs/developers/add_lab.md`) **Changed:** - Reassigned `managed_by` from the nonexistent `goadmin` account to `Administrator` across GOAD and GOAD-variant-1 domain groups to fix dangling principal references **Removed:** - Stale `shares` vuln from GOAD-Light and GOAD-variant-1 host vulns lists - Redundant `greatmaster` group, `Domain Admins`/`Dragons`/`QueenProtector` managed_by, and dc03 `local_groups` overrides from GOAD dev, staging, and test overlays --- ad/GOAD-Light/data/config.json | 2 +- ad/GOAD-variant-1/data/config.json | 9 +- ad/GOAD/data/config.json | 8 +- ad/GOAD/data/dev-overlay.json | 35 +- ad/GOAD/data/staging-overlay.json | 13 +- ad/GOAD/data/test-overlay.json | 15 +- cli/internal/labconfig/integrity.go | 667 ++++++++++++++++++ cli/internal/labconfig/integrity_test.go | 378 ++++++++++ .../labconfig/testdata/known_findings.txt | 26 + docs/mkdocs/docs/developers/add_lab.md | 12 + 10 files changed, 1108 insertions(+), 57 deletions(-) create mode 100644 cli/internal/labconfig/integrity.go create mode 100644 cli/internal/labconfig/integrity_test.go create mode 100644 cli/internal/labconfig/testdata/known_findings.txt diff --git a/ad/GOAD-Light/data/config.json b/ad/GOAD-Light/data/config.json index 51b04d07..e60a6a3e 100644 --- a/ad/GOAD-Light/data/config.json +++ b/ad/GOAD-Light/data/config.json @@ -67,7 +67,7 @@ "gpo_abuse.ps1", "rdp_scheduler.ps1" ], - "vulns" : ["disable_firewall", "directory", "credentials", "autologon", "files", "ntlmdowngrade", "enable_llmnr", "enable_nbt_ns", "shares"], + "vulns" : ["disable_firewall", "directory", "credentials", "autologon", "files", "ntlmdowngrade", "enable_llmnr", "enable_nbt_ns"], "vulns_vars" : { "directory": { "setup": "C:\\setup" diff --git a/ad/GOAD-variant-1/data/config.json b/ad/GOAD-variant-1/data/config.json index 45fe6813..a85637fb 100644 --- a/ad/GOAD-variant-1/data/config.json +++ b/ad/GOAD-variant-1/data/config.json @@ -68,8 +68,7 @@ "autologon", "files", "enable_llmnr", - "enable_nbt_ns", - "shares" + "enable_nbt_ns" ], "vulns_vars": { "directory": { @@ -290,18 +289,18 @@ "path": "CN=Users,DC=vortexindustries,DC=local" }, "AdministrationGroup": { - "managed_by": "goadmin", + "managed_by": "Administrator", "path": "CN=Users,DC=vortexindustries,DC=local" }, "Services": { - "managed_by": "goadmin", + "managed_by": "Administrator", "path": "CN=Users,DC=vortexindustries,DC=local", "members": [ "VORTEXIND\\AdministrationGroup" ] }, "Domain Admins": { - "managed_by": "goadmin", + "managed_by": "Administrator", "path": "CN=Users,DC=vortexindustries,DC=local", "members": [ "VORTEXIND\\Services" diff --git a/ad/GOAD/data/config.json b/ad/GOAD/data/config.json index 3f006a92..b439393e 100644 --- a/ad/GOAD/data/config.json +++ b/ad/GOAD/data/config.json @@ -185,7 +185,7 @@ ] }, "scripts" : ["asrep_roasting2.ps1"], - "vulns" : ["ntlmdowngrade", "disable_firewall", "adcs_esc7", "adcs_esc13", "adcs_esc15"], + "vulns" : ["ntlmdowngrade", "disable_firewall", "adcs_esc10_case1", "adcs_esc7", "adcs_esc13", "adcs_esc15"], "vulns_adcs_templates": ["ESC1", "ESC2", "ESC3", "ESC3-CRA", "ESC4", "ESC9"], "vulns_vars" : { "adcs_esc7": { @@ -271,16 +271,16 @@ "path" : "CN=Users,DC=essos,DC=local" }, "Dragons":{ - "managed_by" : "goadmin", + "managed_by" : "Administrator", "path" : "CN=Users,DC=essos,DC=local" }, "QueenProtector":{ - "managed_by" : "goadmin", + "managed_by" : "Administrator", "path" : "CN=Users,DC=essos,DC=local", "members" : ["ESSOS\\Dragons"] }, "Domain Admins":{ - "managed_by" : "goadmin", + "managed_by" : "Administrator", "path" : "CN=Users,DC=essos,DC=local", "members" : ["ESSOS\\QueenProtector"] } diff --git a/ad/GOAD/data/dev-overlay.json b/ad/GOAD/data/dev-overlay.json index 7c13a112..ea3f6eb5 100644 --- a/ad/GOAD/data/dev-overlay.json +++ b/ad/GOAD/data/dev-overlay.json @@ -4,22 +4,6 @@ "essos.local": { "acls": { "GenericWrite_missandei_viserys": null - }, - "groups": { - "global": { - "Domain Admins": { - "managed_by": "Administrator" - }, - "Dragons": { - "managed_by": "Administrator" - }, - "QueenProtector": { - "managed_by": "Administrator" - } - }, - "universal": { - "greatmaster": null - } } } }, @@ -52,17 +36,24 @@ "enable_nbt_ns", "shares", "anonymous_enum" - ] + ], + "vulns_vars": { + "shares": { + "thewall": { + "path": "C:\\thewall", + "list": "yes", + "full": "NORTH\\Stark", + "change": "NORTH\\jon.snow,NORTH\\samwell.tarly", + "read": "Users" + } + } + } }, "dc03": { - "local_groups": { - "Administrators": [ - "essos\\daenerys.targaryen" - ] - }, "vulns": [ "ntlmdowngrade", "disable_firewall", + "adcs_esc10_case1", "adcs_esc7", "adcs_esc13", "adcs_esc15" diff --git a/ad/GOAD/data/staging-overlay.json b/ad/GOAD/data/staging-overlay.json index 3d8ac6ba..84dac893 100644 --- a/ad/GOAD/data/staging-overlay.json +++ b/ad/GOAD/data/staging-overlay.json @@ -2,12 +2,7 @@ "lab": { "domains": { "essos.local": { - "domain_password": "-cwuyGW494yZnC_M8wLN", - "groups": { - "universal": { - "greatmaster": null - } - } + "domain_password": "-cwuyGW494yZnC_M8wLN" }, "north.sevenkingdoms.local": { "domain_password": "moydNed_wEKuP8KN6rUx" @@ -50,14 +45,10 @@ }, "dc03": { "local_admin_password": "-cwuyGW494yZnC_M8wLN", - "local_groups": { - "Administrators": [ - "essos\\daenerys.targaryen" - ] - }, "vulns": [ "ntlmdowngrade", "disable_firewall", + "adcs_esc10_case1", "adcs_esc7", "adcs_esc13", "adcs_esc15" diff --git a/ad/GOAD/data/test-overlay.json b/ad/GOAD/data/test-overlay.json index 22bd1158..314ea9ea 100644 --- a/ad/GOAD/data/test-overlay.json +++ b/ad/GOAD/data/test-overlay.json @@ -1,14 +1,5 @@ { "lab": { - "domains": { - "essos.local": { - "groups": { - "universal": { - "greatmaster": null - } - } - } - }, "hosts": { "dc01": { "vulns": [ @@ -40,14 +31,10 @@ ] }, "dc03": { - "local_groups": { - "Administrators": [ - "essos\\daenerys.targaryen" - ] - }, "vulns": [ "ntlmdowngrade", "disable_firewall", + "adcs_esc10_case1", "adcs_esc7", "adcs_esc13", "adcs_esc15" diff --git a/cli/internal/labconfig/integrity.go b/cli/internal/labconfig/integrity.go new file mode 100644 index 00000000..c88ae23a --- /dev/null +++ b/cli/internal/labconfig/integrity.go @@ -0,0 +1,667 @@ +package labconfig + +import ( + "encoding/json" + "fmt" + "sort" + "strings" +) + +// Finding is one referential-integrity problem in a lab config. +type Finding struct { + // Path locates the offending value, e.g. + // `domains["essos.local"].groups.universal.greatmaster`. + Path string + // Ref is the reference that failed to resolve, when the finding is about + // a dangling reference rather than a missing pairing. + Ref string + // Msg explains the invariant that was violated. + Msg string +} + +func (f Finding) String() string { + if f.Ref == "" { + return fmt.Sprintf("%s: %s", f.Path, f.Msg) + } + return fmt.Sprintf("%s: %s (%q)", f.Path, f.Msg, f.Ref) +} + +// Options configures the cross-file invariants CheckIntegrity enforces. +type Options struct { + // VulnsRequiringVars names vulns whose Ansible role dereferences + // vulns_vars, meaning a host that lists the vuln must also carry a + // matching vulns_vars entry. + // + // This is worth linting precisely because Ansible will not complain: + // vulnerabilities.yml passes `vulns_vars[vuln] | default({})`, so a + // missing entry leaves the role iterating an empty dict and provisioning + // nothing, with the vuln still listed as if it had been applied. + // + // Callers derive this from the role sources rather than hardcoding it + // here, so the set cannot drift from the roles it describes. A nil map + // disables the pairing check. + VulnsRequiringVars map[string]bool + + // VulnsVarsGroupRefs maps a vuln name to the vulns_vars leaf keys that + // hold a group name, e.g. adcs_esc13 -> ["adcs_esc13_group"]. These are + // the references that a per-env overlay can silently orphan by deleting + // the group while leaving the vuln in place. + VulnsVarsGroupRefs map[string][]string + + // VulnsVarsPrincipalRefs is the same idea for leaf keys holding a + // DOMAIN\user reference, e.g. adcs_esc7 -> ["ca_manager"]. + VulnsVarsPrincipalRefs map[string][]string + + // KnownVulnRoles is the set of vuln names that have a corresponding + // ansible/roles/vulns_ role. vulnerabilities.yml interpolates the + // name straight into include_role, so a typo here is a provision-time + // "role not found" rather than anything the data itself reveals. Upstream + // GOAD-Light shipped `enable_nbt-ns` against a `vulns_enable_nbt_ns` role + // for exactly this reason. A nil map disables the check. + KnownVulnRoles map[string]bool +} + +// DefaultOptions returns the reference map for the vulns currently shipped in +// ad/*/data. VulnsRequiringVars is deliberately left nil; see Options. +func DefaultOptions() Options { + return Options{ + VulnsVarsGroupRefs: map[string][]string{ + "adcs_esc13": {"adcs_esc13_group"}, + }, + VulnsVarsPrincipalRefs: map[string][]string{ + "adcs_esc7": {"ca_manager"}, + }, + } +} + +// CheckIntegrity validates that every entity a merged lab config references +// still exists in that config. +// +// It exists because the per-env `{env}-overlay.json` files are RFC 7386 merge +// patches, where a null deletes a key and an array replaces its base wholesale. +// A one-line overlay can therefore remove a group, user, or vulns_vars entry +// that something else in the config still points at, and nothing else in the +// pipeline notices: Ansible only fails later, on the host, when a role +// dereferences the missing object. +// +// Pass the already-merged document, not the base config. Validating the base +// alone is what lets overlay-introduced breakage through. +func CheckIntegrity(merged []byte, opts Options) ([]Finding, error) { + var doc struct { + Lab struct { + Domains map[string]domain `json:"domains"` + Hosts map[string]host `json:"hosts"` + } `json:"lab"` + } + if err := json.Unmarshal(merged, &doc); err != nil { + return nil, fmt.Errorf("parse merged lab config: %w", err) + } + + idx := newIndex(doc.Lab.Domains, doc.Lab.Hosts) + var out []Finding + + for name, d := range doc.Lab.Domains { + out = append(out, idx.checkDomain(name, d)...) + } + for id, h := range doc.Lab.Hosts { + out = append(out, idx.checkHost(id, h, opts)...) + } + + sort.Slice(out, func(i, j int) bool { + if out[i].Path != out[j].Path { + return out[i].Path < out[j].Path + } + return out[i].Ref < out[j].Ref + }) + return out, nil +} + +// overlayDropKeys names the per-host arrays whose loss silently removes lab +// capability. Each is provisioned by looping over the array, so an entry the +// overlay drops is never applied to the host. +var overlayDropKeys = []struct { + name string + get func(host) []string +}{ + {"vulns", func(h host) []string { return h.Vulns }}, + {"scripts", func(h host) []string { return h.Scripts }}, + {"vulns_adcs_templates", func(h host) []string { return h.VulnsADCSTemplates }}, +} + +// CheckOverlayDrops reports capability the base config grants a host that the +// merged, post-overlay config does not. +// +// CheckIntegrity cannot see this class, by construction. Under RFC 7386 an +// array in an overlay replaces its base counterpart wholesale rather than +// merging into it, so an overlay that redeclares `vulns` without one of the +// base's entries yields a document that is internally consistent and silently +// smaller. Nothing dangles, so a referential check reports nothing, and +// vulnerabilities.yml simply never includes the absent role: the play recap +// reads ok=N failed=0 and the lab is quietly wrong in that environment alone. +// +// This shipped. Adding adcs_esc10_case1 to dc03 in config.json left the dev, +// staging and test overlays redeclaring dc03.vulns without it, so ESC6 and +// ESC9 stayed unexploitable in all three, while prod, which carries no dc03 +// override and therefore inherited the base, was correct. +// +// Pass the same base and merged pair the provisioner resolves. A host the +// overlay deletes outright with an explicit null is not reported, since that +// removal is already legible in the overlay. +func CheckOverlayDrops(base, merged []byte) ([]Finding, error) { + baseHosts, err := hostsOf(base) + if err != nil { + return nil, fmt.Errorf("parse base lab config: %w", err) + } + mergedHosts, err := hostsOf(merged) + if err != nil { + return nil, fmt.Errorf("parse merged lab config: %w", err) + } + + var out []Finding + for id, bh := range baseHosts { + mh, ok := mergedHosts[id] + if !ok { + continue + } + for _, key := range overlayDropKeys { + kept := map[string]bool{} + for _, v := range key.get(mh) { + kept[v] = true + } + for _, v := range key.get(bh) { + if !kept[v] { + out = append(out, Finding{ + Path: fmt.Sprintf("hosts[%q].%s", id, key.name), + Ref: v, + Msg: "overlay drops an entry the base config declares, so it is never provisioned in this environment", + }) + } + } + } + } + + sort.Slice(out, func(i, j int) bool { + if out[i].Path != out[j].Path { + return out[i].Path < out[j].Path + } + return out[i].Ref < out[j].Ref + }) + return out, nil +} + +// hostsOf decodes just the host table from a lab config document. +func hostsOf(doc []byte) (map[string]host, error) { + var parsed struct { + Lab struct { + Hosts map[string]host `json:"hosts"` + } `json:"lab"` + } + if err := json.Unmarshal(doc, &parsed); err != nil { + return nil, err + } + return parsed.Lab.Hosts, nil +} + +type domain struct { + DC string `json:"dc"` + NetbiosName string `json:"netbios_name"` + // Trust is a single partner FQDN, empty when the domain trusts nobody. + // Every lab in ad/ uses this scalar form rather than a list. + Trust string `json:"trust"` + Users map[string]labUser `json:"users"` + Groups map[string]map[string]group `json:"groups"` + ACLs map[string]acl `json:"acls"` + OUs map[string]any `json:"organisation_units"` + MultiDomainGroupsMember map[string][]string `json:"multi_domain_groups_member"` + LAPSReaders []string `json:"laps_readers"` + GMSA map[string]gmsa `json:"gmsa"` +} + +type labUser struct { + Groups []string `json:"groups"` +} + +type group struct { + ManagedBy string `json:"managed_by"` + Members []string `json:"members"` +} + +type acl struct { + For string `json:"for"` + To string `json:"to"` +} + +type gmsa struct { + Name string `json:"gMSA_Name"` + HostNames []string `json:"gMSA_HostNames"` +} + +type host struct { + Hostname string `json:"hostname"` + Domain string `json:"domain"` + LocalGroups map[string][]string `json:"local_groups"` + Vulns []string `json:"vulns"` + Scripts []string `json:"scripts"` + VulnsADCSTemplates []string `json:"vulns_adcs_templates"` + VulnsVars map[string]any `json:"vulns_vars"` + MSSQL *struct { + Sysadmins []string `json:"sysadmins"` + ExecuteAsLogin map[string]string `json:"executeaslogin"` + ExecuteAsUser map[string]struct { + User string `json:"user"` + } `json:"executeasuser"` + } `json:"mssql"` +} + +// index answers "does this principal exist?" across the whole lab. +type index struct { + // byDomain maps a domain FQDN to its principals. + byDomain map[string]*domainPrincipals + // domainKey resolves a netbios name or FQDN (both lowercased) to the + // domain's FQDN, since references use either form interchangeably. + domainKey map[string]string + // computers and gmsaAccounts are lab-wide: a "name$" reference is not + // scoped to a domain in the config's own notation. + computers map[string]bool + gmsaAccounts map[string]bool + hostnames map[string]bool + hostIDs map[string]bool +} + +type domainPrincipals struct { + users map[string]bool + groups map[string]bool +} + +func newIndex(domains map[string]domain, hosts map[string]host) *index { + idx := &index{ + byDomain: map[string]*domainPrincipals{}, + domainKey: map[string]string{}, + computers: map[string]bool{}, + gmsaAccounts: map[string]bool{}, + hostnames: map[string]bool{}, + hostIDs: map[string]bool{}, + } + + for fqdn, d := range domains { + p := &domainPrincipals{users: map[string]bool{}, groups: map[string]bool{}} + for u := range d.Users { + p.users[strings.ToLower(u)] = true + } + for _, scope := range d.Groups { + for g := range scope { + p.groups[strings.ToLower(g)] = true + } + } + idx.byDomain[fqdn] = p + + idx.domainKey[strings.ToLower(fqdn)] = fqdn + if d.NetbiosName != "" { + idx.domainKey[strings.ToLower(d.NetbiosName)] = fqdn + } + // A domain's leftmost label is also used as a prefix in local_groups + // (e.g. "north\\eddard.stark" for north.sevenkingdoms.local). + if label := strings.SplitN(strings.ToLower(fqdn), ".", 2)[0]; label != "" { + if _, taken := idx.domainKey[label]; !taken { + idx.domainKey[label] = fqdn + } + } + + for _, g := range d.GMSA { + if g.Name != "" { + idx.gmsaAccounts[strings.ToLower(g.Name)] = true + } + } + } + + for id, h := range hosts { + idx.hostIDs[strings.ToLower(id)] = true + if h.Hostname != "" { + idx.hostnames[strings.ToLower(h.Hostname)] = true + idx.computers[strings.ToLower(h.Hostname)] = true + } + } + return idx +} + +// builtinUsers and builtinGroups are principals AD creates itself, so the lab +// config references them without declaring them. +var builtinUsers = map[string]bool{ + "administrator": true, "guest": true, "krbtgt": true, "defaultaccount": true, + "ansible": true, "ssm-user": true, "vagrant": true, +} + +var builtinGroups = map[string]bool{ + "domain admins": true, "domain users": true, "domain guests": true, + "domain computers": true, "domain controllers": true, + "enterprise admins": true, "schema admins": true, + "group policy creator owners": true, "protected users": true, + "cert publishers": true, "read-only domain controllers": true, + "enterprise read-only domain controllers": true, + "dnsadmins": true, "dnsupdateproxy": true, "ras and ias servers": true, + "allowed rodc password replication group": true, + "denied rodc password replication group": true, + "cloneable domain controllers": true, "key admins": true, + "enterprise key admins": true, "account operators": true, + "administrators": true, "backup operators": true, "server operators": true, + "print operators": true, "remote desktop users": true, "users": true, + "guests": true, "iis_iusrs": true, "performance log users": true, + "performance monitor users": true, "distributed com users": true, + "event log readers": true, "certificate service dcom access": true, + "cryptographic operators": true, "network configuration operators": true, + "incoming forest trust builders": true, + "pre-windows 2000 compatible access": true, + "windows authorization access group": true, + "terminal server license servers": true, "remote management users": true, + "access control assistance operators": true, + "system managed accounts group": true, "storage replica administrators": true, + "hyper-v administrators": true, +} + +// wellKnownPrefixes mark references to SIDs AD resolves on its own. +var wellKnownPrefixes = []string{ + "nt authority\\", "builtin\\", "nt service\\", "everyone", + "authenticated users", "creator owner", +} + +// resolve reports whether ref names something that exists. domainCtx is the +// FQDN a bare (unprefixed) reference is interpreted against. +// +// It accepts the four notations the configs actually use: a bare name, a +// DOMAIN\name pair where DOMAIN is a netbios name or an FQDN, a "name$" +// machine or gMSA account, and a distinguished name. +func (idx *index) resolve(ref, domainCtx string) bool { + r := strings.TrimSpace(ref) + if r == "" { + return true // nothing referenced; emptiness is checked by the callers that care + } + lower := strings.ToLower(r) + + for _, p := range wellKnownPrefixes { + if strings.HasPrefix(lower, p) || lower == strings.TrimSuffix(p, "\\") { + return true + } + } + + // A distinguished name is structural rather than a principal. Validate the + // domain it claims to live in and leave the leaf alone: CN/OU leaves are + // created by roles this config does not enumerate. + if strings.Contains(r, "=") { + return idx.resolveDN(lower) + } + + if i := strings.Index(r, "\\"); i >= 0 { + dom, leaf := lower[:i], r[i+1:] + fqdn, ok := idx.domainKey[dom] + if !ok { + return false + } + return idx.resolveLeaf(leaf, fqdn) + } + return idx.resolveLeaf(r, domainCtx) +} + +func (idx *index) resolveDN(lowerDN string) bool { + var labels []string + for _, part := range strings.Split(lowerDN, ",") { + part = strings.TrimSpace(part) + if strings.HasPrefix(part, "dc=") { + labels = append(labels, strings.TrimPrefix(part, "dc=")) + } + } + if len(labels) == 0 { + return true // a relative DN carries no domain claim to check + } + _, ok := idx.domainKey[strings.Join(labels, ".")] + return ok +} + +func (idx *index) resolveLeaf(leaf, domainCtx string) bool { + l := strings.ToLower(strings.TrimSpace(leaf)) + if l == "" { + return true + } + if base, isAccount := strings.CutSuffix(l, "$"); isAccount { + // Machine and gMSA accounts. Trust accounts are the partner domain's + // netbios name, which the domainKey index already covers. + if idx.computers[base] || idx.gmsaAccounts[base] { + return true + } + _, isTrust := idx.domainKey[base] + return isTrust + } + if builtinUsers[l] || builtinGroups[l] { + return true + } + if p, ok := idx.byDomain[domainCtx]; ok && (p.users[l] || p.groups[l]) { + return true + } + // Cross-domain references are common in this lab (forest trusts), so a + // bare name that resolves in any domain is accepted rather than reported. + for _, p := range idx.byDomain { + if p.users[l] || p.groups[l] { + return true + } + } + return false +} + +func (idx *index) resolveGroup(ref, domainCtx string) bool { + l := strings.ToLower(strings.TrimSpace(ref)) + if l == "" { + return true + } + if builtinGroups[l] { + return true + } + if p, ok := idx.byDomain[domainCtx]; ok && p.groups[l] { + return true + } + for _, p := range idx.byDomain { + if p.groups[l] { + return true + } + } + return false +} + +// refChecker accumulates findings for a single domain or host. It carries the +// path prefix and the domain context that bare references resolve against, so +// the per-section helpers below stay small enough to read at a glance. +type refChecker struct { + idx *index + prefix string // e.g. `domains["essos.local"].` + ctx string // domain FQDN that unprefixed references resolve against + out []Finding +} + +func (c *refChecker) at(format string, a ...any) string { + return c.prefix + fmt.Sprintf(format, a...) +} + +func (c *refChecker) add(path, ref, msg string) { + c.out = append(c.out, Finding{Path: path, Ref: ref, Msg: msg}) +} + +// ref reports ref unless it resolves to any principal. +func (c *refChecker) ref(path, ref, what string) { + if ref != "" && !c.idx.resolve(ref, c.ctx) { + c.add(path, ref, "unresolved "+what) + } +} + +// group reports ref unless it resolves to a group specifically. +func (c *refChecker) group(path, ref string) { + if ref != "" && !c.idx.resolveGroup(ref, c.ctx) { + c.add(path, ref, "unresolved group") + } +} + +func (idx *index) checkDomain(fqdn string, d domain) []Finding { + c := &refChecker{idx: idx, prefix: fmt.Sprintf("domains[%q].", fqdn), ctx: fqdn} + c.domainTopology(d) + c.domainGroups(d) + c.domainUsers(d) + c.domainACLs(d) + c.domainCrossRefs(d) + return c.out +} + +func (c *refChecker) domainTopology(d domain) { + if d.DC != "" && !c.idx.hostIDs[strings.ToLower(d.DC)] { + c.add(c.at("dc"), d.DC, "domain controller is not a declared host") + } + if d.Trust == "" { + return + } + if _, ok := c.idx.domainKey[strings.ToLower(d.Trust)]; !ok { + c.add(c.at("trust"), d.Trust, "trust partner is not a declared domain") + } +} + +func (c *refChecker) domainGroups(d domain) { + for scope, groups := range d.Groups { + for name, g := range groups { + c.ref(c.at("groups.%s.%s.managed_by", scope, name), g.ManagedBy, "managed_by principal") + for _, m := range g.Members { + c.ref(c.at("groups.%s.%s.members", scope, name), m, "group member") + } + } + } +} + +func (c *refChecker) domainUsers(d domain) { + for name, u := range d.Users { + for _, g := range u.Groups { + c.group(c.at("users.%s.groups", name), g) + } + } +} + +func (c *refChecker) domainACLs(d domain) { + for key, a := range d.ACLs { + c.ref(c.at("acls.%s.for", key), a.For, "ACL principal") + c.ref(c.at("acls.%s.to", key), a.To, "ACL target") + } +} + +func (c *refChecker) domainCrossRefs(d domain) { + for g, members := range d.MultiDomainGroupsMember { + c.group(c.at("multi_domain_groups_member"), g) + for _, m := range members { + c.ref(c.at("multi_domain_groups_member.%s", g), m, "cross-domain member") + } + } + for _, r := range d.LAPSReaders { + c.ref(c.at("laps_readers"), r, "LAPS reader") + } + for key, g := range d.GMSA { + for _, hn := range g.HostNames { + if !c.idx.hostnames[strings.ToLower(hn)] { + c.add(c.at("gmsa.%s.gMSA_HostNames", key), hn, "gMSA host is not a declared host") + } + } + } +} + +func (idx *index) checkHost(id string, h host, opts Options) []Finding { + c := &refChecker{idx: idx, prefix: fmt.Sprintf("hosts[%q].", id), ctx: h.Domain} + c.hostDomain(h) + c.hostLocalGroups(h) + c.hostMSSQL(h) + c.hostVulns(h, opts) + return c.out +} + +func (c *refChecker) hostDomain(h host) { + if h.Domain == "" { + return + } + if _, ok := c.idx.domainKey[strings.ToLower(h.Domain)]; !ok { + c.add(c.at("domain"), h.Domain, "host joins an undeclared domain") + } +} + +func (c *refChecker) hostLocalGroups(h host) { + for lg, members := range h.LocalGroups { + for _, m := range members { + c.ref(c.at("local_groups.%s", lg), m, "local group member") + } + } +} + +func (c *refChecker) hostMSSQL(h host) { + if h.MSSQL == nil { + return + } + for _, s := range h.MSSQL.Sysadmins { + c.ref(c.at("mssql.sysadmins"), s, "MSSQL sysadmin") + } + for login := range h.MSSQL.ExecuteAsLogin { + // Values are SQL logins (e.g. "sa"), which are not AD principals; + // only the granted-to key is an AD identity. + c.ref(c.at("mssql.executeaslogin"), login, "MSSQL login") + } + for key, e := range h.MSSQL.ExecuteAsUser { + c.ref(c.at("mssql.executeasuser.%s.user", key), e.User, "MSSQL user") + } +} + +// hostVulns covers the pairing that broke ESC13: a vuln stays in the list while +// an overlay deletes the vulns_vars entry, or the group that entry points at. +func (c *refChecker) hostVulns(h host, opts Options) { + for _, v := range h.Vulns { + c.vulnHasRole(v, opts) + c.vulnHasVars(h, v, opts) + c.vulnVarsRefs(h, v, opts) + } +} + +func (c *refChecker) vulnHasRole(v string, opts Options) { + if opts.KnownVulnRoles != nil && !opts.KnownVulnRoles[v] { + c.add(c.at("vulns"), v, "no ansible/roles/vulns_ role exists for this vuln") + } +} + +func (c *refChecker) vulnHasVars(h host, v string, opts Options) { + if !opts.VulnsRequiringVars[v] { + return + } + if _, ok := h.VulnsVars[v]; !ok { + c.add(c.at("vulns"), v, "vuln has no vulns_vars entry, so its role provisions nothing") + } +} + +func (c *refChecker) vulnVarsRefs(h host, v string, opts Options) { + for _, leaf := range opts.VulnsVarsGroupRefs[v] { + for _, ref := range vulnsVarsLeaves(h.VulnsVars, v, leaf) { + c.group(c.at("vulns_vars.%s.*.%s", v, leaf), ref) + } + } + for _, leaf := range opts.VulnsVarsPrincipalRefs[v] { + for _, ref := range vulnsVarsLeaves(h.VulnsVars, v, leaf) { + c.ref(c.at("vulns_vars.%s.*.%s", v, leaf), ref, "principal") + } + } +} + +// vulnsVarsLeaves pulls every value of leaf out of vulns_vars[vuln], which is +// shaped as a map of arbitrary case names to option objects. +func vulnsVarsLeaves(vars map[string]any, vuln, leaf string) []string { + entry, ok := vars[vuln].(map[string]any) + if !ok { + return nil + } + var out []string + for _, caseRaw := range entry { + c, ok := caseRaw.(map[string]any) + if !ok { + continue + } + if v, ok := c[leaf].(string); ok && v != "" { + out = append(out, v) + } + } + sort.Strings(out) + return out +} diff --git a/cli/internal/labconfig/integrity_test.go b/cli/internal/labconfig/integrity_test.go new file mode 100644 index 00000000..90fe6e99 --- /dev/null +++ b/cli/internal/labconfig/integrity_test.go @@ -0,0 +1,378 @@ +package labconfig + +import ( + "encoding/json" + "os" + "path/filepath" + "regexp" + "strings" + "testing" + + "github.com/dreadnode/dreadgoad/internal/jsonmerge" +) + +// repoRoot walks up from the package directory to the checkout root, which is +// where ad/ and ansible/ live. +func repoRoot(t *testing.T) string { + t.Helper() + dir, err := filepath.Abs("../../..") + if err != nil { + t.Fatalf("resolve repo root: %v", err) + } + if _, err := os.Stat(filepath.Join(dir, "ad")); err != nil { + t.Fatalf("repo root %s has no ad/ directory: %v", dir, err) + } + return dir +} + +// testOptions builds Options with VulnsRequiringVars derived from the role +// sources, so adding a vulns_vars-consuming role automatically extends the +// pairing check instead of quietly falling outside it. +func testOptions(t *testing.T, root string) Options { + t.Helper() + opts := DefaultOptions() + opts.VulnsRequiringVars = vulnsRequiringVars(t, root) + if len(opts.VulnsRequiringVars) == 0 { + t.Fatal("derived no vulns_vars-consuming roles; the role scan is broken") + } + return opts +} + +func vulnsRequiringVars(t *testing.T, root string) map[string]bool { + t.Helper() + entries, err := os.ReadDir(filepath.Join(root, "ansible", "roles")) + if err != nil { + t.Fatalf("read roles dir: %v", err) + } + + out := map[string]bool{} + for _, e := range entries { + if !e.IsDir() || !strings.HasPrefix(e.Name(), "vulns_") { + continue + } + tasks := filepath.Join(root, "ansible", "roles", e.Name(), "tasks") + if uses, err := treeMentions(tasks, "vulns_vars"); err == nil && uses { + out[strings.TrimPrefix(e.Name(), "vulns_")] = true + } + } + return out +} + +func treeMentions(dir, needle string) (bool, error) { + found := false + err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { + if err != nil || d.IsDir() || found { + return err + } + b, err := os.ReadFile(path) + if err != nil { + return err + } + if strings.Contains(string(b), needle) { + found = true + } + return nil + }) + return found, err +} + +// labDataDirs returns every ad//data directory that holds a config.json. +func labDataDirs(t *testing.T, root string) []string { + t.Helper() + matches, err := filepath.Glob(filepath.Join(root, "ad", "*", "data", "config.json")) + if err != nil { + t.Fatalf("glob lab configs: %v", err) + } + if len(matches) == 0 { + t.Fatal("found no ad/*/data/config.json") + } + dirs := make([]string, 0, len(matches)) + for _, m := range matches { + dirs = append(dirs, filepath.Dir(m)) + } + return dirs +} + +var overlayRE = regexp.MustCompile(`^(.+)-overlay\.json$`) + +// mergedVariants returns the config as each environment actually resolves it: +// the base alone (which is what an env with no overlay provisions) plus one +// merged document per {env}-overlay.json. +func mergedVariants(t *testing.T, dataDir string) map[string][]byte { + t.Helper() + base, err := os.ReadFile(filepath.Join(dataDir, "config.json")) + if err != nil { + t.Fatalf("read base config: %v", err) + } + + out := map[string][]byte{"base (no overlay)": base} + + entries, err := os.ReadDir(dataDir) + if err != nil { + t.Fatalf("read %s: %v", dataDir, err) + } + for _, e := range entries { + m := overlayRE.FindStringSubmatch(e.Name()) + if m == nil { + continue + } + patch, err := os.ReadFile(filepath.Join(dataDir, e.Name())) + if err != nil { + t.Fatalf("read overlay %s: %v", e.Name(), err) + } + merged, err := jsonmerge.MergePatchBytes(base, patch) + if err != nil { + t.Fatalf("merge overlay %s: %v", e.Name(), err) + } + out[m[1]] = merged + } + return out +} + +// baselinePath holds the findings that are known and accepted, so this gate can +// be green without pretending the lab data is clean. See the file's own header. +const baselinePath = "testdata/known_findings.txt" + +// loadBaseline reads accepted findings keyed as "||". +func loadBaseline(t *testing.T) map[string]bool { + t.Helper() + b, err := os.ReadFile(baselinePath) + if err != nil { + t.Fatalf("read baseline %s: %v", baselinePath, err) + } + out := map[string]bool{} + for _, line := range strings.Split(string(b), "\n") { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + out[line] = true + } + return out +} + +// TestLabConfigIntegrity is the regression gate: every lab, in every +// environment, must reference only entities that survive the overlay merge. +// +// Findings listed in the baseline are reported as logs instead of failures. +// Anything new fails, and a baseline entry that stops occurring also fails, so +// the accepted set can only shrink. +func TestLabConfigIntegrity(t *testing.T) { + root := repoRoot(t) + opts := testOptions(t, root) + baseline := loadBaseline(t) + seen := map[string]bool{} + + for _, dataDir := range labDataDirs(t, root) { + lab := filepath.Base(filepath.Dir(dataDir)) + t.Run(lab, func(t *testing.T) { + base, err := os.ReadFile(filepath.Join(dataDir, "config.json")) + if err != nil { + t.Fatalf("read base config: %v", err) + } + for env, merged := range mergedVariants(t, dataDir) { + t.Run(env, func(t *testing.T) { + findings, err := CheckIntegrity(merged, opts) + if err != nil { + t.Fatalf("CheckIntegrity: %v", err) + } + // Drops are only meaningful against the base, so this is a + // no-op for the base pseudo-env and needs no special case. + drops, err := CheckOverlayDrops(base, merged) + if err != nil { + t.Fatalf("CheckOverlayDrops: %v", err) + } + findings = append(findings, drops...) + for _, f := range findings { + key := lab + "|" + env + "|" + f.String() + if baseline[key] { + seen[key] = true + t.Logf("known: %s", f) + continue + } + t.Errorf("%s\n\tif this is intended, add to %s with a reason:\n\t%s", + f, baselinePath, key) + } + }) + } + }) + } + + for key := range baseline { + if !seen[key] { + t.Errorf("baseline entry no longer occurs, delete it from %s:\n\t%s", baselinePath, key) + } + } +} + +// TestCheckIntegrityCatchesOverlayRegressions pins the three defects that +// shipped in ad/GOAD's per-env overlays, each expressed as the minimal merge +// patch that reintroduces it. These are the cases a base-config-only check +// cannot see. +func TestCheckIntegrityCatchesOverlayRegressions(t *testing.T) { + root := repoRoot(t) + opts := testOptions(t, root) + base, err := os.ReadFile(filepath.Join(root, "ad", "GOAD", "data", "config.json")) + if err != nil { + t.Fatalf("read GOAD config: %v", err) + } + + tests := []struct { + name string + patch string + wantPath string + wantRef string + wantInMsg string + }{ + { + name: "overlay deletes the group ESC13 targets", + // What dev/staging/test-overlay.json did: drop greatmaster while + // leaving adcs_esc13 and its vulns_vars pointing at it. + patch: `{"lab":{"domains":{"essos.local":{"groups":{"universal":{"greatmaster":null}}}}}}`, + wantPath: `hosts["dc03"].vulns_vars.adcs_esc13.*.adcs_esc13_group`, + wantRef: "greatmaster", + wantInMsg: "unresolved group", + }, + { + name: "managed_by names an account the lab never creates", + patch: `{"lab":{"domains":{"essos.local":{"groups":{"global":{"Dragons":{"managed_by":"goadmin"}}}}}}}`, + wantPath: `domains["essos.local"].groups.global.Dragons.managed_by`, + wantRef: "goadmin", + wantInMsg: "unresolved managed_by principal", + }, + { + name: "overlay strips a vulns_vars entry the vuln still needs", + patch: `{"lab":{"domains":{},"hosts":{"dc03":{"vulns_vars":{"adcs_esc13":null}}}}}`, + wantPath: `hosts["dc03"].vulns`, + wantRef: "adcs_esc13", + wantInMsg: "provisions nothing", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + merged, err := jsonmerge.MergePatchBytes(base, []byte(tc.patch)) + if err != nil { + t.Fatalf("merge patch: %v", err) + } + findings, err := CheckIntegrity(merged, opts) + if err != nil { + t.Fatalf("CheckIntegrity: %v", err) + } + for _, f := range findings { + if f.Path == tc.wantPath && f.Ref == tc.wantRef && strings.Contains(f.Msg, tc.wantInMsg) { + return + } + } + t.Errorf("no finding matched path=%q ref=%q msg~=%q; got %v", + tc.wantPath, tc.wantRef, tc.wantInMsg, findings) + }) + } +} + +// TestCheckOverlayDropsCatchesSilentRemoval pins the defect that shipped on +// 2026-07-30: adcs_esc10_case1 was added to dc03 in config.json, but the dev, +// staging and test overlays each redeclared dc03.vulns without it, so the role +// never ran and ESC6/ESC9 stayed unexploitable in exactly those environments. +// +// The merged document is internally consistent in that state, which is why +// CheckIntegrity reports nothing and this check has to exist separately. +func TestCheckOverlayDropsCatchesSilentRemoval(t *testing.T) { + root := repoRoot(t) + base, err := os.ReadFile(filepath.Join(root, "ad", "GOAD", "data", "config.json")) + if err != nil { + t.Fatalf("read GOAD config: %v", err) + } + + // Redeclare dc03.vulns without adcs_esc10_case1, exactly as the overlays did. + patch := `{"lab":{"hosts":{"dc03":{"vulns":["ntlmdowngrade","disable_firewall","adcs_esc7","adcs_esc13","adcs_esc15"]}}}}` + merged, err := jsonmerge.MergePatchBytes(base, []byte(patch)) + if err != nil { + t.Fatalf("merge patch: %v", err) + } + + if findings, err := CheckIntegrity(merged, testOptions(t, root)); err != nil { + t.Fatalf("CheckIntegrity: %v", err) + } else if len(findings) > 0 { + t.Fatalf("CheckIntegrity unexpectedly reports the drop, so this test no longer\n"+ + "proves CheckOverlayDrops is load-bearing; got %v", findings) + } + + drops, err := CheckOverlayDrops(base, merged) + if err != nil { + t.Fatalf("CheckOverlayDrops: %v", err) + } + for _, f := range drops { + if f.Path == `hosts["dc03"].vulns` && f.Ref == "adcs_esc10_case1" { + return + } + } + t.Errorf(`no finding for hosts["dc03"].vulns ref=adcs_esc10_case1; got %v`, drops) +} + +// TestCheckOverlayDropsIgnoresDeletedHost keeps the check quiet about removals +// that are already legible in the overlay: a host deleted outright with a null +// is an explicit choice, not a silent loss of capability. +func TestCheckOverlayDropsIgnoresDeletedHost(t *testing.T) { + base := []byte(`{"lab":{"hosts":{"dc03":{"vulns":["adcs_esc10_case1"]}}}}`) + merged, err := jsonmerge.MergePatchBytes(base, []byte(`{"lab":{"hosts":{"dc03":null}}}`)) + if err != nil { + t.Fatalf("merge patch: %v", err) + } + drops, err := CheckOverlayDrops(base, merged) + if err != nil { + t.Fatalf("CheckOverlayDrops: %v", err) + } + if len(drops) != 0 { + t.Errorf("deleting a host should report nothing; got %v", drops) + } +} + +// TestCheckIntegrityAcceptsBuiltinsAndCrossDomainRefs guards against the +// opposite failure: a validator noisy enough that people stop reading it. +func TestCheckIntegrityAcceptsBuiltinsAndCrossDomainRefs(t *testing.T) { + doc := map[string]any{ + "lab": map[string]any{ + "domains": map[string]any{ + "a.local": map[string]any{ + "dc": "dc01", + "netbios_name": "A", + "trust": "b.local", + "users": map[string]any{"alice": map[string]any{"groups": []string{"Domain Admins", "Protected Users"}}}, + "groups": map[string]any{"global": map[string]any{"Team": map[string]any{"managed_by": "Administrator", "members": []string{"B\\bob"}}}}, + "laps_readers": []string{"alice"}, + "organisation_units": map[string]any{}, + "acls": map[string]any{ + "dn_target": map[string]any{"for": "alice", "to": "CN=AdminSDHolder,CN=System,DC=a,DC=local", "right": "GenericAll"}, + "anon": map[string]any{"for": "NT AUTHORITY\\ANONYMOUS LOGON", "to": "DC=a,DC=local", "right": "ReadProperty"}, + "machine": map[string]any{"for": "alice", "to": "host01$", "right": "GenericAll"}, + "trust_acct": map[string]any{"for": "B$", "to": "alice", "right": "GenericAll"}, + "cross_domain": map[string]any{"for": "bob", "to": "alice", "right": "GenericAll"}, + }, + }, + "b.local": map[string]any{ + "dc": "dc02", + "netbios_name": "B", + "users": map[string]any{"bob": map[string]any{}}, + "groups": map[string]any{}, + }, + }, + "hosts": map[string]any{ + "dc01": map[string]any{"hostname": "host01", "domain": "a.local", + "local_groups": map[string]any{"Administrators": []string{"A\\alice", "a\\Team"}}}, + "dc02": map[string]any{"hostname": "host02", "domain": "b.local"}, + }, + }, + } + raw, err := json.Marshal(doc) + if err != nil { + t.Fatalf("marshal: %v", err) + } + findings, err := CheckIntegrity(raw, DefaultOptions()) + if err != nil { + t.Fatalf("CheckIntegrity: %v", err) + } + for _, f := range findings { + t.Errorf("false positive: %s", f) + } +} diff --git a/cli/internal/labconfig/testdata/known_findings.txt b/cli/internal/labconfig/testdata/known_findings.txt new file mode 100644 index 00000000..059dc5e4 --- /dev/null +++ b/cli/internal/labconfig/testdata/known_findings.txt @@ -0,0 +1,26 @@ +# Accepted findings for TestLabConfigIntegrity. +# +# One line per finding, formatted "||". A finding that is not +# listed here fails the test; a line here that no longer occurs also fails, so +# this list can only shrink. Every entry needs a reason and an intended fix. +# +# Only add an entry when a finding is understood and deliberately deferred, never +# to quiet a failure you have not diagnosed. +# +# The GOAD|dev entry is correct rather than deferred. dev moves the `shares` vuln +# off srv02 and onto dc02, carrying vulns_vars.thewall with it and nulling the +# srv02 vulns_vars.shares, so CheckOverlayDrops sees srv02 lose a vuln the base +# grants it. The capability is not lost, only rehomed, and the null is the +# overlay stating that intent explicitly. Delete this line if dev ever stops +# moving the share. +GOAD|dev|hosts["srv02"].vulns: overlay drops an entry the base config declares, so it is never provisioned in this environment ("shares") + +# The GOAD-variant-1|dev entry is a real defect, deferred because fixing it means +# picking share definitions for a generated lab rather than changing checker +# behavior. The dev overlay adds `shares` to dc02's vulns but supplies no +# vulns_vars.shares to drive it, so the role provisions nothing and dev is +# quietly missing the share the vuln implies. GOAD's dev overlay does this +# correctly, adding the vuln together with its vulns_vars block. Fix by giving +# dc02 a vulns_vars.shares entry in ad/GOAD-variant-1/data/dev-overlay.json, +# then delete this line. +GOAD-variant-1|dev|hosts["dc02"].vulns: vuln has no vulns_vars entry, so its role provisions nothing ("shares") diff --git a/docs/mkdocs/docs/developers/add_lab.md b/docs/mkdocs/docs/developers/add_lab.md index 2a830d47..22f6a169 100644 --- a/docs/mkdocs/docs/developers/add_lab.md +++ b/docs/mkdocs/docs/developers/add_lab.md @@ -78,6 +78,18 @@ overlay files (`{env}-overlay.json`) that contain only the fields that differ from the base `config.json`. The CLI merges them at runtime using RFC 7386 JSON Merge Patch. See `docs/cli.md` in the repository for the overlay format and resolution order. +Arrays replace, they do not merge. If an overlay redeclares a host's `vulns`, +that list becomes the complete set for the environment and the base list is +discarded, so adding a vuln to `config.json` alone does nothing wherever an +overlay names that host. The failure is silent in both directions: nothing +dangles, so a referential check sees a valid document, and `vulnerabilities.yml` +just never includes the missing role, leaving `PLAY RECAP` reporting `failed=0`. + +After adding a vuln to any `config.json`, add it to every `{env}-overlay.json` +that redeclares that host. `TestLabConfigIntegrity` enforces this via +`CheckOverlayDrops`; a deliberate removal goes in +`cli/internal/labconfig/testdata/known_findings.txt` with a reason. + ## Inventory files The `data/inventory` file is an Ansible inventory that defines host groups and connection variables (WinRM settings, credentials). Each provider also has its own `inventory` file under `providers//` that overrides connection-specific values (IP addresses, ports) for that provider.