feat: add lab config integrity validation and fix overlay vuln drops - #399
Merged
Merged
Conversation
**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
dreadnode-renovate-bot
Bot
added
lab/GOAD
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Key Changes:
labconfigpackage that validates referential integrity of merged lab configs and detects silently dropped vulns in per-env overlaysadcs_esc10_case1was added todc03in base config but omitted from dev/staging/test overlay redeclarationsmanaged_byreferences from the non-existentgoadminaccount toAdministratoracross GOAD and GOAD-variant-1 configsgreatmastergroup and orphanedlocal_groupsentries that overlays were silently droppingAdded:
cli/internal/labconfig/integrity.goimplementingCheckIntegrity, which validates that every referenced principal, group, DC, trust partner, and vulns_vars entity in a merged lab config actually exists, accounting for AD built-ins, cross-domain forest-trust references, machine/gMSA accounts, and distinguished namesCheckOverlayDropscatches the RFC 7386 array-replacement class of bug thatCheckIntegritycannot see, reporting anyvulns,scripts, orvulns_adcs_templatesentry the base grants a host but the merged overlay silently omitsintegrity_test.gowith a regression gate (TestLabConfigIntegrity) that runs every lab across every environment, plus targeted tests pinning the three shipped overlay defects and guarding against false positives on built-ins and cross-domain references;VulnsRequiringVarsis derived from actual role sources so the pairing check cannot drifttestdata/known_findings.txtdocumenting the two deliberately-deferred findings (GOAD dev share rehoming and GOAD-variant-1 dev missing vulns_vars) with reasons and intended fixes, structured so the accepted set can only shrinkdocs/mkdocs/docs/developers/add_lab.mdexplaining that arrays replace rather than merge, and that adding a vuln toconfig.jsonrequires updating every overlay that redeclares that hostChanged:
adcs_esc10_case1todc03.vulnsin GOAD's dev, staging, and test overlays so ESC6/ESC9 are actually exploitable in those environments, matching the base configgoadminwithAdministratorasmanaged_byfor the Dragons, QueenProtector, Domain Admins, AdministrationGroup, and Services groups in GOAD and GOAD-variant-1 configssharesvuln from srv02 to dc02 in GOAD's dev overlay, carrying itsvulns_vars.thewalldefinition along and nulling the srv02 entry to state the intent explicitlyRemoved:
sharesvuln fromvulnslists in GOAD-Light and GOAD-variant-1 configsgreatmasteruniversal group and redundant groupmanaged_byoverrides from GOAD's dev, staging, and test overlayslocal_groupsAdministrators entries fordc03that overlays were carrying but that were no longer needed