Skip to content

feat: remove ADCSESC9a/b edges: BED-8269#2769

Open
rvazarkar wants to merge 3 commits into
mainfrom
BED-8269
Open

feat: remove ADCSESC9a/b edges: BED-8269#2769
rvazarkar wants to merge 3 commits into
mainfrom
BED-8269

Conversation

@rvazarkar
Copy link
Copy Markdown
Contributor

@rvazarkar rvazarkar commented May 13, 2026

Description

Windows now enforces strong certificate mapping for Kerberos with no option to opt out.

The StrongCertificateBindingEnforcement property we collect from domain controller computers used to tell if strong mapping was enforced or in compatibility mode, indicating opportunity of attacks. The value now has no impact on the system’s behavior.

We should therefore remove the property from the product and the affected components that uses the property.

Motivation and Context

https://specterops.atlassian.net/browse/BED-8269

How Has This Been Tested?

Screenshots (optional):

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • Database Migrations

Checklist:

Summary by CodeRabbit

  • Bug Fixes

    • Removed ADCS ESC9a/ESC9b relationship detection and analysis.
    • Updated certificate binding evaluation to use UPN certificate mapping.
  • Chores

    • Cleaned up database migrations and schema entries for ESC9.
    • Removed exported property/relationship definitions tied to ESC9.
  • Tests

    • Removed ESC9 integration test harnesses and JSON fixtures.
  • New Features

    • Enhanced ESC10a with dedicated path detection logic.

@rvazarkar rvazarkar self-assigned this May 13, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 13, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 29da42ea-84f2-4c2d-8bc8-aa4b265b2b42

📥 Commits

Reviewing files that changed from the base of the PR and between 0643032 and 60b9ad6.

📒 Files selected for processing (2)
  • cmd/api/src/test/integration/harnesses.go
  • packages/go/cypher/test/cases/positive_tests.json

📝 Walkthrough

Walkthrough

Removes ADCS ESC9a/ESC9b kinds, processing, tests, and fixtures; adds a DB migration to delete existing ESC9 relationships; and replaces forest-level "weak certificate binding" tracking with "UPN certificate mapping" tracking in ADCS cache logic.

Changes

Remove ADCS ESC9 support and switch forest certificate-mapping detection

Layer / File(s) Summary
Schema kind removal and reversible migration
cmd/api/src/database/migration/extensions/ad_graph_schema.sql, cmd/api/src/database/migration/migrations/20260512162651_v9_remove_adcs_esc9_schema.sql
Deleted upsert calls that registered ADCSESC9a/ADCSESC9b in extension SQL. Added a Goose migration with Up that deletes schema_findings_subtypes/schema_findings and schema_relationship_kinds for ESC9a/ESC9b, and Down that re-inserts those relationship kinds and findings (with upserts).
Runtime graph cleanup migration
cmd/api/src/migrations/manifest.go
Added Version_930_Migration and registered 9.3.0; the migration batches through and deletes all graph relationships whose kind is ADCSESC9a or ADCSESC9b.
Edge composition & post-processing removal
packages/go/analysis/ad/ad.go, packages/go/analysis/ad/adcs.go, packages/go/analysis/ad/esc10.go
Removed switch cases mapping ADCSESC9a/ADCSESC9b to their composition functions and removed submission of PostADCSESC9a/PostADCSESC9b jobs. Introduced adcsESC10APath2Pattern and wired ESC10 composition to use the new ESC10a-specific path-2 pattern instead of reusing ESC9 pattern.
Forest-level certificate-mapping cache refactor
packages/go/analysis/ad/adcscache.go
Replaced hasWeakCertBindingInForest bitmap with hasUPNCertMappingInForest, updated NewADCSCache/BuildCache to populate UPN-mapping bitmap, removed HasWeakCertBindingInForest and added HasUPNCertMappingInForest, and replaced internal helper logic to detect UPN certificate mapping in DC registry/config.
Public schema/property cleanup
packages/csharp/graphschema/PropertyNames.cs, packages/cue/bh/ad/ad.cue
Removed exported property name constants StrongCertificateBindingEnforcementRaw and StrongCertificateBindingEnforcement. Removed ADCSESC9a/ADCSESC9b relationship-kind declarations and removed them from RelationshipKinds, SharedRelationshipKinds, EdgeCompositionRelationships, and PostProcessedRelationships in CUE.
Tests, fixtures, and harnesses removal/updates
cmd/api/src/test/integration/harnesses.go, cmd/api/src/test/integration/harnesses/*.json, packages/go/analysis/ad/adcs_integration_test.go, cmd/api/src/test/fixtures/fixtures/expected_ingest_adcs.go, packages/go/cypher/test/cases/positive_tests.json
Deleted ESC9a/ESC9b harness types and their Setup methods from Go harnesses, removed multiple ESC9 JSON harness fixtures, deleted TestADCSESC9a/TestADCSESC9b integration tests, updated ADCS expected fixture to assert CertificateMappingMethods registry-not-present values instead of StrongCertificateBindingEnforcement*, and removed ADCSESC9a/ADCSESC9b from a Cypher positive test query's relationship label alternation.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: removal of ADCSESC9a/b edges with reference to the associated Jira ticket BED-8269.
Description check ✅ Passed The PR description provides clear motivation (Windows now enforces strong certificate mapping), explains the change (removing StrongCertificateBindingEnforcement property), and includes the associated Jira ticket and checklist items.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch BED-8269

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@urangel urangel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-15k lines 😌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants