Conversation
JonasBK
added
enhancement
WalkthroughAdds ESC16 (ADCSESC16) support: schema and property additions, ingestion of DisabledExtensions, new ESC16 analysis and traversal logic, post-processing wiring and integration test with harness, graph IO enhancements, frontend saved searches and help-text components. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Test as Integration Test
participant Loader as arrows
participant DB as Graph DB
participant Post as ADCS Analysis
participant Queue as PostRelationship Queue
Test->>Loader: Load ADCSESC16Harness.json
Loader->>DB: Create nodes/edges (omit ADCSESC16 edges)
Test->>Post: Trigger ADCSESC16 post-processing op
Post->>DB: Fetch prerequisites (CAs, domains, groups, cache)
Post->>Queue: Submit PostADCSESC16 jobs per EnterpriseCA
Queue-->>Post: Execute PostADCSESC16 jobs (create ADCSESC16 edges)
Post->>DB: Persist ADCSESC16 relationships
Test->>DB: Query and verify ADCSESC16 edges exist
Note over DB,Post: Frontend queries/saved-searches can then surface ADCSESC16
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/References.tsx (1)
23-64: Includenoreferrerin linkrelattributeBest practice is to pair
noopenerwithnoreferrerto avoid leaking the referrer header.- rel='noopener' + rel='noopener noreferrer'Apply to every external
<Link>in this component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
cmd/api/src/analysis/ad/adcs_integration_test.go(2 hunks)cmd/api/src/test/integration/harnesses/ADCSESC16Harness.json(1 hunks)packages/cue/bh/ad/ad.cue(6 hunks)packages/go/analysis/ad/ad.go(1 hunks)packages/go/analysis/ad/adcs.go(1 hunks)packages/go/analysis/ad/esc16.go(1 hunks)packages/go/analysis/ad/post.go(1 hunks)packages/go/ein/ad.go(1 hunks)packages/go/ein/incoming_models.go(1 hunks)packages/go/graphschema/ad/ad.go(6 hunks)packages/go/graphschema/common/common.go(1 hunks)packages/go/lab/arrows/graph.go(3 hunks)packages/javascript/bh-shared-ui/src/commonSearchesAGI.ts(1 hunks)packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/ADCSESC16.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/Composition.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/General.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/LinuxAbuse.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/Opsec.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/References.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/WindowsAbuse.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx(2 hunks)packages/javascript/bh-shared-ui/src/edgeTypes.tsx(1 hunks)packages/javascript/bh-shared-ui/src/graphSchema.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (6)
packages/go/graphschema/common/common.go (1)
Learnt from: elikmiller
PR: SpecterOps/BloodHound#1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
packages/go/analysis/ad/adcs.go (1)
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
cmd/api/src/test/integration/harnesses/ADCSESC16Harness.json (1)
Learnt from: mvlipka
PR: SpecterOps/BloodHound#1615
File: cmd/api/src/test/integration/harnesses/AZPIMRolesHarness.json:190-194
Timestamp: 2025-06-26T20:50:03.695Z
Learning: The JSON files in cmd/api/src/test/integration/harnesses/ directory (like AZPIMRolesHarness.json) are used as reference documents for writing Go test harnesses, not as actual data files that get parsed by the application code. Minor formatting issues like trailing spaces in these reference files don't affect functionality.
packages/go/lab/arrows/graph.go (2)
Learnt from: elikmiller
PR: SpecterOps/BloodHound#1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T17:52:33.291Z
Learning: In BloodHound Go code, prefer using explicit slog type functions like slog.Any(), slog.String(), slog.Int(), etc. over simple key-value pairs for structured logging. This provides better type safety and makes key-value pairs more visually distinct. For error types, use slog.Any("key", err) or slog.String("key", err.Error()).
cmd/api/src/analysis/ad/adcs_integration_test.go (2)
Learnt from: elikmiller
PR: SpecterOps/BloodHound#1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
packages/go/analysis/ad/esc16.go (2)
Learnt from: elikmiller
PR: SpecterOps/BloodHound#1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Learnt from: JonasBK
PR: SpecterOps/BloodHound#1434
File: packages/go/analysis/ad/gpos.go:102-126
Timestamp: 2025-06-18T08:27:18.317Z
Learning: In Active Directory's containment hierarchy, each user/computer has exactly one direct parent container, forming a tree structure. When processing GPO edges in packages/go/analysis/ad/gpos.go, the fetchDirectChildUsersAndComputers function only returns direct children, ensuring each user/computer is processed exactly once by its immediate parent container, eliminating the need for deduplication logic.
🧬 Code Graph Analysis (8)
packages/go/analysis/ad/ad.go (2)
packages/go/graphschema/ad/ad.go (1)
ADCSESC16(114-114)packages/go/analysis/ad/esc16.go (1)
GetADCSESC16EdgeComposition(114-239)
packages/go/ein/ad.go (3)
packages/go/ein/incoming_models.go (2)
CARegistryData(143-149)DisabledExtensions(138-141)packages/go/graphschema/ad/ad.go (1)
DisabledExtensions(265-265)packages/go/graphschema/common/common.go (1)
Collected(57-57)
packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
packages/go/graphschema/ad/ad.go (1)
ADCSESC16(114-114)
packages/go/analysis/ad/post.go (1)
packages/go/graphschema/ad/ad.go (1)
ADCSESC16(114-114)
packages/go/ein/incoming_models.go (1)
packages/go/graphschema/ad/ad.go (1)
DisabledExtensions(265-265)
packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/ADCSESC16.tsx (1)
packages/go/graphschema/ad/ad.go (1)
ADCSESC16(114-114)
packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/General.tsx (2)
packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
EdgeInfoProps(144-153)packages/javascript/bh-shared-ui/src/components/HelpTexts/utils.ts (1)
groupSpecialFormat(19-26)
packages/go/analysis/ad/esc16.go (6)
packages/go/analysis/post.go (1)
CreatePostRelationshipJob(151-157)packages/go/analysis/ad/adcscache.go (1)
ADCSCache(32-54)packages/go/graphschema/ad/ad.go (26)
IsUserSpecifiesSanEnabledCollected(144-144)IsUserSpecifiesSanEnabled(143-143)DisabledExtensionsCollected(266-266)DisabledExtensions(265-265)Contains(54-54)RequiresManagerApproval(194-194)SchemaVersion(193-193)AuthorizedSignatures(190-190)AuthenticationEnabled(195-195)EnterpriseCA(40-40)CertTemplate(42-42)User(29-29)MemberOf(49-49)Group(31-31)Enroll(92-92)TrustedForNTAuth(97-97)NTAuthStore(41-41)NTAuthStoreFor(96-96)GenericAll(45-45)AllExtendedRights(51-51)PublishedTo(88-88)IssuedSignedBy(99-99)EnterpriseCAFor(98-98)AIACA(38-38)RootCA(39-39)RootCAFor(86-86)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(401-546)packages/go/analysis/analysis.go (1)
MaximumDatabaseParallelWorkers(36-36)packages/go/analysis/ad/queries.go (1)
FetchAuthUsersAndEveryoneGroups(1830-1840)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-analysis
- GitHub Check: build-ui
🔇 Additional comments (30)
packages/go/analysis/ad/ad.go (1)
574-575: LGTM! Consistent integration of new ADCSESC16 edge kind.The addition follows the established pattern for other ADCS ESC variants and correctly calls the corresponding edge composition function. The implementation is consistent with the existing codebase architecture.
packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (2)
22-22: LGTM! Proper import integration.The import follows the established naming convention and is correctly placed in alphabetical order with other ADCS ESC components.
262-262: LGTM! Consistent component registration.The addition to EdgeInfoComponents follows the established pattern and maintains the existing object structure for help text components.
packages/go/ein/ad.go (1)
1209-1212: LGTM! Proper property handling implementation.The DisabledExtensions property handling follows the established pattern used by other properties in this function. The implementation correctly checks if data was collected before setting the property and uses the appropriate schema constant and data field.
packages/go/ein/incoming_models.go (2)
138-141: LGTM! Consistent struct definition.The DisabledExtensions struct follows the established pattern of embedding APIResult and containing appropriate data fields, consistent with other similar structs in this file.
148-148: LGTM! Proper field integration.The addition of DisabledExtensions field to CARegistryData maintains consistency with the existing structure and enables the ESC16 functionality to access this data during ingestion.
packages/go/graphschema/common/common.go (2)
43-43: LGTM! Correct integration of new relationship kind.The addition of
ad.ADCSESC16to the inbound relationship kinds is appropriate and consistent with other ADCS ESC variants. Since this is generated code from CUE schemas, the change is a legitimate output of the code generation process.
46-46: LGTM! Proper outbound relationship integration.The inclusion of
ad.ADCSESC16in the outbound relationship kinds maintains consistency with the inbound list and follows the established pattern for bidirectional relationship handling.packages/javascript/bh-shared-ui/src/edgeTypes.tsx (1)
104-118: Edge type addition is consistent with existing pattern
ActiveDirectoryRelationshipKind.ADCSESC16is imported correctly and inserted into the AD CS sub-category. No further action needed.packages/go/analysis/ad/post.go (1)
59-65: Verify that re-ordering doesn’t introduce unintended side-effects
ad.Ownsandad.WriteOwnerwere moved belowad.ExtendedByPolicy. If any consumer relies on the slice order (e.g., deterministic post-processing or UI ordering), this could change behaviour. Please confirm order is immaterial, or restore prior ordering.packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/Opsec.tsx (1)
20-28: LGTM – concise and clear help textComponent follows existing HelpText style and uses correct MUI primitives.
packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/ADCSESC16.tsx (1)
24-31: Module wiring looks correctAll sub-components are aggregated and exported in line with other HelpText modules. No issues spotted.
packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/LinuxAbuse.tsx (1)
21-59: LGTM! Well-structured help text component.The component follows React and Material UI best practices with clear, detailed instructions. The step-by-step format and error handling explanations provide comprehensive guidance for the ADCSESC16 attack vector.
packages/cue/bh/ad/ad.cue (3)
973-985: LGTM! Properly structured schema additions.The new
DisabledExtensionsandDisabledExtensionsCollectedproperties follow the established schema patterns with consistent naming, schema assignment, and representation values.
1577-1580: LGTM! Consistent relationship kind definition.The
ADCSESC16kind follows the same pattern as other ADCS ESC relationship kinds with proper schema assignment.
1119-1120: LGTM! Comprehensive integration into schema lists.The new properties and relationship kind are properly added to all relevant lists (
Properties,RelationshipKinds,SharedRelationshipKinds,EdgeCompositionRelationships), ensuring full integration with the schema system.Also applies to: 1724-1724, 1816-1816, 1852-1852
packages/go/analysis/ad/adcs.go (1)
111-137: Excellent refactoring! Eliminates code duplication effectively.The consolidation of repetitive post-processing calls into a loop structure is a great improvement. The slice of structs approach is clean and maintainable, and the consistent error handling with appropriate logging levels is well-implemented. This follows the DRY principle while maintaining clear functionality.
packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/WindowsAbuse.tsx (1)
20-61: LGTM! Comprehensive Windows exploitation guide.The component follows established patterns and provides detailed, step-by-step Windows attack instructions. The use of Material UI components is consistent, and the preformatted command examples are well-presented. The error handling explanations add valuable context.
packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/Composition.tsx (1)
23-50: LGTM! Well-implemented composition component with proper state management.The component demonstrates good React patterns with proper TypeScript typing, comprehensive state handling (loading, error, success), and appropriate Material UI usage. The conditional rendering logic covers all necessary states and provides good user feedback.
packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/General.tsx (1)
22-43: Well-structured component with accurate ESC16 documentation.The component properly implements the help text for the ADCSESC16 attack path. The explanation clearly describes the vulnerability where the CA's disabled security extension (OID 1.3.6.1.4.1.311.25.2) allows attackers to obtain malicious certificates for impersonation.
packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts (1)
192-196: Correctly implements ESC16 vulnerability detection query.The Cypher query properly identifies enrollment rights on certificate templates published to Enterprise CAs that have the security extension (OID "1.3.6.1.4.1.311.25.2") disabled, which is the key characteristic of the ESC16 vulnerability.
packages/javascript/bh-shared-ui/src/commonSearchesAGI.ts (1)
192-196: Maintains parity with commonSearchesAGT.ts as intended.The ESC16 query is correctly duplicated here to maintain consistency between the two common searches files, as noted in the file comment.
packages/go/lab/arrows/graph.go (1)
94-104: Good implementation of base entity kind detection.The logic correctly identifies AD and Azure nodes and appends the appropriate base entity kinds. This ensures proper categorization of nodes in the graph database.
cmd/api/src/test/integration/harnesses/ADCSESC16Harness.json (1)
1-242: Test harness correctly models ESC16 scenario.The JSON structure properly represents an ESC16-vulnerable environment with:
- EnterpriseCA configured with
isuserspecifiessanenabled: trueand the critical OID "1.3.6.1.4.1.311.25.2" indisabledextensions- CertTemplate with authentication enabled and no manager approval requirement
- Appropriate ADCSESC16 relationship connecting the attack path
packages/javascript/bh-shared-ui/src/graphSchema.ts (1)
143-143: Schema updates correctly integrate ADCSESC16 support.All additions follow the established patterns:
- ADCSESC16 relationship kind properly added to enum and display mappings
- New properties DisabledExtensions and DisabledExtensionsCollected correctly defined
- Edge included in composition and pathfinding arrays as expected
Also applies to: 301-302, 348-348, 488-489, 755-758, 806-806
packages/go/graphschema/ad/ad.go (1)
114-114: Generated schema code properly includes ESC16 additions.The code generation correctly added:
- ADCSESC16 relationship kind
- DisabledExtensions and DisabledExtensionsCollected properties
- All necessary function updates to support the new constants
Also applies to: 265-266, 270-270, 536-539, 808-811, 1080-1083, 1100-1100, 1106-1106, 1109-1109, 1112-1112
packages/go/analysis/ad/esc16.go (4)
241-301: Path traversal patterns correctly implement ESC16 logic.The two traversal patterns properly implement the ESC16 detection logic:
- Path1 finds Enterprise CAs with the specific disabled extension
- Path2 finds cert templates meeting ESC16 requirements and connects them to the domain
The patterns match the Cypher query documented in the comments.
1-1: Update copyright year to 2025.The copyright header shows 2023 but should be 2025 to match the current year.
-// Copyright 2023 Specter Ops, Inc. +// Copyright 2025 Specter Ops, Inc.⛔ Skipped due to learnings
Learnt from: superlinkx PR: SpecterOps/BloodHound#1509 File: packages/go/stbernard/command/license/internal/utils.go:77-86 Timestamp: 2025-05-29T18:42:23.137Z Learning: Copyright years in license headers should typically preserve the original year when the work was created, not be updated to the current year when the license is added later. The engineering team (#bhe-engineering, @bhe-ec) should provide guidance on the copyright year policy for the BloodHound project.
207-207: certTemplateValidForUserVictim already defined in esc_shared.goThe function is implemented in packages/go/analysis/ad/esc_shared.go (lines 350–362), and since both files share the same package, the call in esc16.go is valid. No further action required.
73-73: Outdated missing-definition check for filterUserDNSResultsThe filterUserDNSResults function is already implemented in packages/go/analysis/ad/esc_shared.go (starting at line 364), so there’s no missing definition in esc16.go. No further action required.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/go/analysis/ad/esc16.go (1)
65-69: Error handling follows established pattern.The warning logging pattern is consistent with other ADCS ESC implementations in the codebase, as confirmed by the established learning.
🧹 Nitpick comments (1)
packages/go/analysis/ad/esc16.go (1)
42-58: Consider extracting property validation logic.The nested if-else chain for property validation is quite deep and could benefit from extraction into a helper function for better readability and testability.
Consider refactoring like this:
func validateEnterpriseCAForESC16(enterpriseCA *graph.Node) error { if isUserSpecifiesSanEnabledCollected, err := enterpriseCA.Properties.Get(ad.IsUserSpecifiesSanEnabledCollected.String()).Bool(); err != nil { return err } else if !isUserSpecifiesSanEnabledCollected { return fmt.Errorf("IsUserSpecifiesSanEnabledCollected not collected") } if isUserSpecifiesSanEnabled, err := enterpriseCA.Properties.Get(ad.IsUserSpecifiesSanEnabled.String()).Bool(); err != nil { return err } else if !isUserSpecifiesSanEnabled { return fmt.Errorf("IsUserSpecifiesSanEnabled is false") } // ... continue with other validations return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
cmd/api/src/analysis/ad/adcs_integration_test.go(2 hunks)cmd/api/src/test/integration/harnesses/ADCSESC16Harness.json(1 hunks)packages/cue/bh/ad/ad.cue(6 hunks)packages/go/analysis/ad/ad.go(1 hunks)packages/go/analysis/ad/adcs.go(1 hunks)packages/go/analysis/ad/esc16.go(1 hunks)packages/go/analysis/ad/post.go(1 hunks)packages/go/ein/ad.go(1 hunks)packages/go/ein/incoming_models.go(1 hunks)packages/go/graphschema/ad/ad.go(6 hunks)packages/go/graphschema/common/common.go(1 hunks)packages/go/lab/arrows/graph.go(3 hunks)packages/javascript/bh-shared-ui/src/commonSearchesAGI.ts(1 hunks)packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/ADCSESC16.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/ADCSESC16.tsx
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/go/ein/incoming_models.go
- packages/go/analysis/ad/post.go
- packages/go/ein/ad.go
- packages/go/analysis/ad/ad.go
- packages/go/graphschema/common/common.go
- packages/go/lab/arrows/graph.go
- cmd/api/src/test/integration/harnesses/ADCSESC16Harness.json
- packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts
- packages/cue/bh/ad/ad.cue
- packages/go/analysis/ad/adcs.go
- packages/javascript/bh-shared-ui/src/commonSearchesAGI.ts
- packages/go/graphschema/ad/ad.go
- cmd/api/src/analysis/ad/adcs_integration_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
🔇 Additional comments (8)
packages/go/analysis/ad/esc16.go (8)
37-39: LGTM: Well-documented constant definition.The OID constant is correctly defined with clear documentation explaining its purpose in ESC16 exploitation detection.
71-71: Cross-product calculation logic is correct.The use of
CalculateCrossProductNodeSetsto combine cert template enrollers and enterprise CA enrollers follows the expected pattern for ADCS analysis.
94-112: LGTM: Correct ESC16 cert template validation logic.The validation logic correctly implements ESC16 requirements:
- Rejects templates requiring manager approval
- Handles schema version and authorized signatures validation properly
- Ensures authentication is enabled
The function structure and error handling are appropriate.
114-136: Excellent documentation with Cypher query reference.The detailed Cypher query comment provides clear context for understanding the traversal logic implementation.
141-147: Proper concurrent access protection.The mutex lock and cardinality bitmaps are correctly initialized for thread-safe operations during traversal.
241-263: LGTM: Correct Path1 pattern implementation.The traversal pattern correctly implements the first ESC16 path:
- Proper depth constraints for MemberOf relationships
- Correct property filters for Enterprise CA (SAN enabled, disabled extensions)
- Appropriate relationship chain to target domain
The query logic matches the documented Cypher pattern.
265-301: LGTM: Complex but correct Path2 pattern implementation.The traversal pattern correctly implements the second ESC16 path:
- Proper cert template property validation (manager approval, authentication, schema version)
- Correct filtering of Enterprise CAs using Path1 results
- Appropriate certificate chain traversal logic
- Handles multi-tier CA hierarchies correctly
The complex query logic is well-structured and matches the documented requirements.
207-207: Helper function exists – no action neededThe
certTemplateValidForUserVictimfunction is defined inpackages/go/analysis/ad/esc_shared.go(lines 350–362). There’s no missing helper; you can safely ignore this suggestion.Likely an incorrect or invalid review comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/api/src/analysis/ad/adcs_integration_test.go (1)
3827-3867: Strengthen verification: clearer length check and disambiguate nodes by kind to avoid name collisions.
- Length: require.Len gives a better diff on failure than comparing two ints.
- Lookup: guarding the name-based lookup with KindIn reduces false positives if names collide across labels (rare, but cheap to harden).
Note: Using findNodeByID from ad_integration_test.go is correct for this package; no action needed.
- require.Equal(t, len(testEdges), len(results)) + require.Len(t, results, len(testEdges)) for _, testEdge := range testEdges { // Find source and destination nodes fromNode, found := findNodeByID(fixture.Nodes, testEdge.FromID) require.True(t, found, "source node with ID %s not found", testEdge.FromID) toNode, found := findNodeByID(fixture.Nodes, testEdge.ToID) require.True(t, found, "destination node with ID %s not found", testEdge.ToID) // Fetch node IDs from graph - fromGraphNodeId, err := ops.FetchNodeIDs(tx.Nodes().Filterf(func() graph.Criteria { - return query.Equals(query.NodeProperty(common.Name.String()), fromNode.Caption) - })) + fromKinds := graph.StringsToKinds(fromNode.Labels) + fromGraphNodeId, err := ops.FetchNodeIDs(tx.Nodes().Filterf(func() graph.Criteria { + return query.And( + query.Equals(query.NodeProperty(common.Name.String()), fromNode.Caption), + query.KindIn(query.Node(), fromKinds...), + ) + })) require.NoError(t, err, "error fetching node with name %s", fromNode.Caption) require.Len(t, fromGraphNodeId, 1, "expected exactly one node with name %s, found %d", fromNode.Caption, len(fromGraphNodeId)) - toGraphNodeId, err := ops.FetchNodeIDs(tx.Nodes().Filterf(func() graph.Criteria { - return query.Equals(query.NodeProperty(common.Name.String()), toNode.Caption) - })) + toKinds := graph.StringsToKinds(toNode.Labels) + toGraphNodeId, err := ops.FetchNodeIDs(tx.Nodes().Filterf(func() graph.Criteria { + return query.And( + query.Equals(query.NodeProperty(common.Name.String()), toNode.Caption), + query.KindIn(query.Node(), toKinds...), + ) + })) require.NoError(t, err, "error fetching node with name %s", toNode.Caption) require.Len(t, toGraphNodeId, 1, "expected exactly one node with name %s, found %d", toNode.Caption, len(toGraphNodeId))Optional: Consider wrapping each edge verification in a subtest (t.Run(fmt.Sprintf("%s->%s", ...))) to localize failures.
🧹 Nitpick comments (4)
packages/go/analysis/ad/queries.go (2)
2072-2073: Add ADCSESC16 to ADCSEscalations path traversal — OK, but prefer KindIn and de-dup the list
- Functional change (adding ad.ADCSESC16) is correct and aligns with the new relationship kind.
- Style/consistency: elsewhere in this file, multiple-kind filters use query.KindIn(...). Suggest switching for clarity.
- Reduce duplication: the same relationship list appears here and in CreateADCSEscalationsListDelegate; consolidate into a shared slice to avoid future drift.
Apply within-range change:
- return query.Kind(query.Relationship(), ad.GoldenCert, ad.ADCSESC1, ad.ADCSESC3, ad.ADCSESC4, ad.ADCSESC6a, ad.ADCSESC6b, ad.ADCSESC9a, ad.ADCSESC9b, ad.ADCSESC10a, ad.ADCSESC10b, ad.ADCSESC16) + return query.KindIn(query.Relationship(), ad.GoldenCert, ad.ADCSESC1, ad.ADCSESC3, ad.ADCSESC4, ad.ADCSESC6a, ad.ADCSESC6b, ad.ADCSESC9a, ad.ADCSESC9b, ad.ADCSESC10a, ad.ADCSESC10b, ad.ADCSESC16)Additionally, define a shared slice once and reuse (outside of the changed range; shown for clarity):
// near other helpers in this file var adcsEscalationRels = []graph.Kind{ ad.GoldenCert, ad.ADCSESC1, ad.ADCSESC3, ad.ADCSESC4, ad.ADCSESC6a, ad.ADCSESC6b, ad.ADCSESC9a, ad.ADCSESC9b, ad.ADCSESC10a, ad.ADCSESC10b, ad.ADCSESC16, }Then this line can be:
return query.KindIn(query.Relationship(), adcsEscalationRels...)
2082-2083: Add ADCSESC16 to ADCSEscalations list traversal — mirror the path changeSame notes as above: the addition is correct; prefer KindIn and share the relationship list.
Apply within-range change:
- return query.Kind(query.Relationship(), ad.GoldenCert, ad.ADCSESC1, ad.ADCSESC3, ad.ADCSESC4, ad.ADCSESC6a, ad.ADCSESC6b, ad.ADCSESC9a, ad.ADCSESC9b, ad.ADCSESC10a, ad.ADCSESC10b, ad.ADCSESC16) + return query.KindIn(query.Relationship(), ad.GoldenCert, ad.ADCSESC1, ad.ADCSESC3, ad.ADCSESC4, ad.ADCSESC6a, ad.ADCSESC6b, ad.ADCSESC9a, ad.ADCSESC9b, ad.ADCSESC10a, ad.ADCSESC10b, ad.ADCSESC16)Optionally, reuse:
return query.KindIn(query.Relationship(), adcsEscalationRels...)cmd/api/src/analysis/ad/adcs_integration_test.go (2)
3787-3797: Good isolation of ADCSESC16 edges from the fixture; add a sanity assertion.You correctly strip ADCSESC16 edges before ingest to let post-processing create them. To fail fast if the harness ever ships without ADCSESC16 edges, add an explicit assertion after the split.
fixture.Relationships = otherEdges + + // Sanity: the harness should contain at least one ADCSESC16 edge + require.NotEmpty(t, testEdges, "harness contains no ADCSESC16 edges (fixture drift?)")
3798-3825: Minor consistency: prefer require.NoError over require.Nil for errors.Aligns with the rest of this test and produces clearer failure messages.
- err = operation.Done() - require.Nil(t, err) + err = operation.Done() + require.NoError(t, err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
cmd/api/src/analysis/ad/adcs_integration_test.go(2 hunks)cmd/api/src/test/integration/harnesses/ADCSESC16Harness.json(1 hunks)packages/csharp/graphschema/PropertyNames.cs(1 hunks)packages/cue/bh/ad/ad.cue(6 hunks)packages/go/analysis/ad/ad.go(1 hunks)packages/go/analysis/ad/adcs.go(1 hunks)packages/go/analysis/ad/esc16.go(1 hunks)packages/go/analysis/ad/post.go(1 hunks)packages/go/analysis/ad/queries.go(2 hunks)packages/go/ein/ad.go(1 hunks)packages/go/ein/incoming_models.go(1 hunks)packages/go/graphschema/ad/ad.go(6 hunks)packages/go/graphschema/common/common.go(1 hunks)packages/go/lab/arrows/graph.go(3 hunks)packages/javascript/bh-shared-ui/src/commonSearchesAGI.ts(1 hunks)packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/ADCSESC16.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/javascript/bh-shared-ui/src/commonSearchesAGI.ts
- packages/go/analysis/ad/ad.go
- packages/go/ein/ad.go
- packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/ADCSESC16.tsx
- packages/go/ein/incoming_models.go
- cmd/api/src/test/integration/harnesses/ADCSESC16Harness.json
- packages/go/analysis/ad/post.go
- packages/go/graphschema/common/common.go
- packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts
- packages/go/analysis/ad/adcs.go
- packages/go/lab/arrows/graph.go
- packages/go/analysis/ad/esc16.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-10T14:33:20.317Z
Learnt from: JonasBK
PR: SpecterOps/BloodHound#1671
File: cmd/api/src/analysis/ad/adcs_integration_test.go:3687-3687
Timestamp: 2025-07-10T14:33:20.317Z
Learning: When reviewing Go code, functions defined in one file within a package are accessible from other files in the same package. Before flagging missing functions as compilation errors, check if they exist in other files within the same package directory.
Applied to files:
cmd/api/src/analysis/ad/adcs_integration_test.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
PR: SpecterOps/BloodHound#1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Applied to files:
cmd/api/src/analysis/ad/adcs_integration_test.go
🧬 Code graph analysis (4)
packages/go/analysis/ad/queries.go (1)
packages/go/graphschema/ad/ad.go (11)
GoldenCert(100-100)ADCSESC1(104-104)ADCSESC3(105-105)ADCSESC4(106-106)ADCSESC6a(107-107)ADCSESC6b(108-108)ADCSESC9a(109-109)ADCSESC9b(110-110)ADCSESC10a(111-111)ADCSESC10b(112-112)ADCSESC16(114-114)
packages/csharp/graphschema/PropertyNames.cs (2)
packages/go/graphschema/ad/ad.go (2)
DisabledExtensions(270-270)DisabledExtensionsCollected(271-271)packages/go/ein/incoming_models.go (1)
DisabledExtensions(140-143)
packages/go/graphschema/ad/ad.go (2)
packages/go/graphschema/common/common.go (2)
InboundRelationshipKinds(42-44)OutboundRelationshipKinds(45-47)packages/go/ein/incoming_models.go (2)
DisabledExtensions(140-143)GPLink(249-252)
cmd/api/src/analysis/ad/adcs_integration_test.go (8)
cmd/api/src/test/integration/graph.go (1)
NewGraphTestContext(42-49)packages/go/graphschema/schema.go (1)
DefaultGraphSchema(152-162)packages/go/lab/arrows/graph.go (4)
Graph(35-38)LoadGraphFromFile(136-148)Edge(61-75)WriteGraphToDatabase(81-132)cmd/api/src/test/integration/harnesses.go (1)
Harnesses(39-39)packages/go/analysis/post_operation.go (1)
NewPostRelationshipOperation(39-70)packages/go/analysis/post.go (1)
CreatePostRelationshipJob(151-157)packages/go/analysis/ad/esc16.go (1)
PostADCSESC16(41-92)packages/go/analysis/analysis.go (1)
FetchEdgeByStartAndEnd(103-117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: run-tests
- GitHub Check: run-tests
- GitHub Check: build-ui
🔇 Additional comments (13)
packages/csharp/graphschema/PropertyNames.cs (1)
178-180: Verified: DisabledExtensions constants aligned across C#, Go, TS, and CUEAll language bindings expose
DisabledExtensionsandDisabledExtensionsCollectedwith identical lowercase representations:
- C#:
PropertyNames.cslines 178–179 define the two constants as"disabledextensions"and"disabledextensionscollected".- Go graphschema:
packages/go/graphschema/ad/ad.golines 270–271 declare
DisabledExtensions Property = "disabledextensions"and
DisabledExtensionsCollected Property = "disabledextensionscollected".- TypeScript:
packages/javascript/bh-shared-ui/src/graphSchema.tslines 495–496 include the same values, and the display mapping returns the correct human-readable names.- CUE:
packages/cue/bh/ad/ad.cueentries 1005/1012 setrepresentation: "disabledextensions"and"disabledextensionscollected".- Additional Go and JS code (incoming models, analysis logic, and commonSearches) reference the properties consistently, confirming no drift.
No discrepancies detected—approving these autogenerated changes.
packages/cue/bh/ad/ad.cue (5)
1001-1006: New EnterpriseCA properties defined with correct symbol and representation
- DisabledExtensions and DisabledExtensionsCollected are added with expected lowercase representations.
- Matches other language bindings and the PR objective.
Also applies to: 1008-1013
1609-1612: Introduces ADCSESC16 kindKind wiring added; name and schema consistent with existing ESC kinds.
1762-1763: ADCSESC16 added to RelationshipKindsEnsures the edge is recognized as a relationship; aligns with traversal usage elsewhere.
1855-1856: ADCSESC16 added to SharedRelationshipKindsThis inclusion ensures consistent behavior across inbound/outbound/pathfinding sets. Good.
1894-1895: ✅ ADCSESC16 successfully added and propagatedRan a global search for
ADCSESC16across all codegen targets—CUE, Go, and JavaScript—and confirmed a single, consistent definition with no duplicates or collisions. All references have been updated accordingly:• packages/cue/bh/ad/ad.cue
• packages/go/analysis/ad/{ad.go, queries.go, post.go, adcs.go, esc16.go}
• packages/go/graphschema/ad/ad.go
• packages/javascript/bh-shared-ui/{edgeTypes.tsx, graphSchema.ts, components/HelpTexts}No further changes needed.
cmd/api/src/analysis/ad/adcs_integration_test.go (1)
30-30: Importing lab/arrows is appropriate here.Necessary for reading/writing the arrows.app harness and for the Edge/Node helpers used later. No issues.
packages/go/graphschema/ad/ad.go (6)
114-114: New relationship kind ADCSESC16 added.Schema extension is consistent with other ADCSESC* kinds.
549-553: ParseProperty updated for new properties.Covers both sources; error path unchanged. Good.
829-833: Property.String and Property.Name mappings completed.String values and human-readable names look correct and consistent with conventions.
Also applies to: 1109-1113
1129-1130: ADCSESC16 included in Relationships().Ensures persistence and enumeration of the new edge kind.
270-276: Verification complete: DisabledExtensions properties are properly referenced across ingestion and analysis.The
rgoutput confirms that bothDisabledExtensionsandDisabledExtensionsCollectedare used in all expected code paths:
- graphschema/ad/ad.go
• Added to theAllPropertiesslice (lines 270–276)
• Serialization and parsing cases for both properties (lines 829–832, 1109–1111)- ein/ad.go & incoming_models.go
• Ingestion mapper setspropMap[DisabledExtensions]whenCollectedis true (lines 1272–1274)
• Incoming model definesDisabledExtensionsstruct (lines 140–150)- analysis/ad/esc16.go
• Logic reads both properties viaProperties.Get(...)(lines 50–54)
• Query construction referencesDisabledExtensionsOID (line 253)No further action required—approving these changes.
1135-1142: ADCSESC16 properly integrated across the entire codebase.The verification confirms that ADCSESC16 has been comprehensively implemented across all necessary components:
- Go schema: Added to pathfinding, inbound, and outbound relationship collections (as shown in the reviewed code)
- JavaScript/TypeScript UI: Included in graph schema definitions, edge types, and UI components
- CUE schema: Properly defined with symbol and included in relationship collections
- Analysis engine: Full implementation with post-processing functions, edge composition logic, and query integration
- Testing: Integration tests with dedicated harness file
The addition to the relationship kind collections in the reviewed code is consistent with the broader implementation pattern and ensures proper graph traversal functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/go/lab/arrows/graph.go (1)
170-189: Resolved: inconsistent key casing in STRLIST/DECIMAL.Past feedback about STRLIST/DECIMAL using k instead of kLowercase is addressed here; both now set via kLowercase. Thanks for fixing.
🧹 Nitpick comments (7)
packages/go/lab/arrows/graph.go (5)
94-104: Avoid duplicate ADEntity/AzureEntity kinds when augmenting nodeKinds.If a label already resolves to ad.Entity or azure.Entity, the current code can append duplicates. Guard with a contains check.
Apply this diff:
- // Determine node kinds and add Base and AZBase + // Determine node kinds and add ADEntity/AzureEntity if applicable nodeKinds := graph.StringsToKinds(node.Labels) isAD := slices.ContainsFunc(nodeKinds, func(k graph.Kind) bool { return slices.Contains(ad.NodeKinds(), k) }) isAzure := slices.ContainsFunc(nodeKinds, func(k graph.Kind) bool { return slices.Contains(azure.NodeKinds(), k) }) - if isAD { - nodeKinds = append(nodeKinds, ad.Entity) - } - if isAzure { - nodeKinds = append(nodeKinds, azure.Entity) - } + if isAD && !slices.Contains(nodeKinds, ad.Entity) { + nodeKinds = append(nodeKinds, ad.Entity) + } + if isAzure && !slices.Contains(nodeKinds, azure.Entity) { + nodeKinds = append(nodeKinds, azure.Entity) + }Optional micro-optimization: precompute membership sets for ad.NodeKinds()/azure.NodeKinds() outside the node loop to avoid repeated scans.
94-95: Update stale comment wording.“Base and AZBase” doesn’t match the code, which appends ad.Entity and azure.Entity. Adjust for clarity.
- // Determine node kinds and add Base and AZBase + // Determine node kinds and append ad.Entity/azure.Entity when any AD/Azure kind is present
161-169: BOOL: trim value and improve error context.Trim spaces before parsing and include the property key in errors for easier debugging.
case strings.HasPrefix(v, "BOOL:"): _, val, found := strings.Cut(v, "BOOL:") if !found { - return nil, fmt.Errorf("could not process bool value `%s`", v) - } else if boolVal, err := strconv.ParseBool(val); err != nil { - return nil, fmt.Errorf("could not process bool value `%s`: %w", v, err) + return nil, fmt.Errorf("could not process bool value for key `%s`: %q", kLowercase, v) + } + val = strings.TrimSpace(val) + if boolVal, err := strconv.ParseBool(val); err != nil { + return nil, fmt.Errorf("could not process bool value for key `%s`: %q: %w", kLowercase, v, err) } else { out.Set(kLowercase, boolVal) }
170-179: STRLIST: trim, filter empties to avoid accidental empty elements.For inputs like “a, b, , c” this will otherwise include an empty string. Trim first and filter empty elements.
case strings.HasPrefix(v, "STRLIST:"): _, val, found := strings.Cut(v, "STRLIST:") if !found { - return nil, fmt.Errorf("could not process list value `%s`", v) + return nil, fmt.Errorf("could not process list value for key `%s`: %q", kLowercase, v) } - listVal := strings.Split(val, ",") - for i := range listVal { - listVal[i] = strings.TrimSpace(listVal[i]) - } - out.Set(kLowercase, listVal) + val = strings.TrimSpace(val) + parts := strings.Split(val, ",") + listVal := make([]string, 0, len(parts)) + for _, s := range parts { + s = strings.TrimSpace(s) + if s != "" { + listVal = append(listVal, s) + } + } + out.Set(kLowercase, listVal)
181-189: DECIMAL: trim whitespace and reject NaN/Inf.Protect the store from non-finite values, and improve error context.
case strings.HasPrefix(v, "DECIMAL:"): _, val, found := strings.Cut(v, "DECIMAL:") if !found { - return nil, fmt.Errorf("could not process decimal value `%s`", v) + return nil, fmt.Errorf("could not process decimal value for key `%s`: %q", kLowercase, v) } - if floatVal, err := strconv.ParseFloat(val, 64); err != nil { - return nil, fmt.Errorf("could not process decimal value `%s`: %w", v, err) - } else { - out.Set(kLowercase, floatVal) - } + val = strings.TrimSpace(val) + if floatVal, err := strconv.ParseFloat(val, 64); err != nil { + return nil, fmt.Errorf("could not process decimal value for key `%s`: %q: %w", kLowercase, v, err) + } else if math.IsNaN(floatVal) || math.IsInf(floatVal, 0) { + return nil, fmt.Errorf("invalid decimal value for key `%s`: %q", kLowercase, v) + } else { + out.Set(kLowercase, floatVal) + }Add this import:
import "math"packages/go/analysis/ad/queries.go (2)
2072-2073: Include ADCSESC16 in traversal — good; consider de-duplicating the kind listADCSESC16 was correctly added to the inbound ADC escalation path traversal. The full list of ESC kinds is duplicated in this file; suggest centralizing to avoid drift.
Apply this refactor to DRY the kind list:
@@ import ( "context" "fmt" "log/slog" "slices" "time" @@ "github.com/specterops/dawgs/traversal" ) +// adcEscRelKinds centralizes all ADCS escalation relationship kinds to avoid duplication across delegates. +var adcEscRelKinds = []graph.Kind{ + ad.GoldenCert, + ad.ADCSESC1, ad.ADCSESC3, ad.ADCSESC4, + ad.ADCSESC6a, ad.ADCSESC6b, + ad.ADCSESC9a, ad.ADCSESC9b, + ad.ADCSESC10a, ad.ADCSESC10b, + ad.ADCSESC16, +} + func CreateADCSEscalationsPathDelegate(tx graph.Transaction, node *graph.Node) (graph.PathSet, error) { return ops.TraversePaths(tx, ops.TraversalPlan{ Root: node, Direction: graph.DirectionInbound, BranchQuery: func() graph.Criteria { - return query.KindIn(query.Relationship(), ad.GoldenCert, ad.ADCSESC1, ad.ADCSESC3, ad.ADCSESC4, ad.ADCSESC6a, ad.ADCSESC6b, ad.ADCSESC9a, ad.ADCSESC9b, ad.ADCSESC10a, ad.ADCSESC10b, ad.ADCSESC16) + return query.KindIn(query.Relationship(), adcEscRelKinds...) }, }) } @@ func CreateADCSEscalationsListDelegate(tx graph.Transaction, node *graph.Node, skip, limit int) (graph.NodeSet, error) { return ops.AcyclicTraverseNodes(tx, ops.TraversalPlan{ Root: node, Direction: graph.DirectionInbound, BranchQuery: func() graph.Criteria { - return query.KindIn(query.Relationship(), ad.GoldenCert, ad.ADCSESC1, ad.ADCSESC3, ad.ADCSESC4, ad.ADCSESC6a, ad.ADCSESC6b, ad.ADCSESC9a, ad.ADCSESC9b, ad.ADCSESC10a, ad.ADCSESC10b, ad.ADCSESC16) + return query.KindIn(query.Relationship(), adcEscRelKinds...) }, Skip: skip, Limit: limit, }, func(candidate *graph.Node) bool { return candidate.ID != node.ID
2082-2083: Traversal list delegate updated — matches path delegateADCSESC16 addition mirrors the PathDelegate change. After the optional refactor, both will source from the same list.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/cue/bh/ad/ad.cue(6 hunks)packages/go/analysis/ad/queries.go(2 hunks)packages/go/lab/arrows/graph.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
PR: SpecterOps/BloodHound#1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Applied to files:
packages/go/lab/arrows/graph.go
🧬 Code graph analysis (1)
packages/go/analysis/ad/queries.go (1)
packages/go/graphschema/ad/ad.go (11)
GoldenCert(100-100)ADCSESC1(104-104)ADCSESC3(105-105)ADCSESC4(106-106)ADCSESC6a(107-107)ADCSESC6b(108-108)ADCSESC9a(109-109)ADCSESC9b(110-110)ADCSESC10a(111-111)ADCSESC10b(112-112)ADCSESC16(114-114)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-tests
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (8)
packages/go/lab/arrows/graph.go (2)
19-31: Go toolchain version confirmed
The go.mod file specifiesgo 1.24.4, which is ≥ 1.21 and fully supports theslicespackage import. No further action needed.
153-154: I’ve requested the surrounding code snippet to verify the exact behavior of the lowercase normalization. Once we review its full context, we can confirm whether downstream updates or whitespace trimming are necessary.packages/cue/bh/ad/ad.cue (5)
994-1007: New EnterpriseCA properties are well-formed and consistentBoth DisabledExtensions and DisabledExtensionsCollected follow the existing StringEnum pattern (symbol/schema/name/representation). Naming aligns with other ADCS properties. No issues found.
1602-1606: New Kind ADCSESC16 added — matches ESC naming and schemaADCSESC16 kind declaration is consistent with existing ESC kinds. Good.
1755-1755: RelationshipKinds includes ADCSESC16Included alongside other ESC kinds. This ensures the edge is recognized in the global relationship set. Good.
1848-1848: SharedRelationshipKinds includes ADCSESC16Good inclusion for traversal parity (inbound/outbound/pathfinding pull from SharedRelationshipKinds). No concerns.
1144-1145: Properties update validated — please regenerate downstream bindingsVerification script confirms that both
DisabledExtensionsandDisabledExtensionsCollectedappear exactly once astypes.#StringEnuminpackages/cue/bh/ad/ad.cue, with no duplicate declarations. Please proceed to regenerate the schemas and client bindings (Go, C#, and TS) so these new fields are exposed in all layers.packages/go/analysis/ad/queries.go (1)
2072-2083: All ESC kinds are up to date in JS; no C# hard-coded lists detected
- In packages/javascript/bh-shared-ui, ADCSESC16 has been added to:
• edgeTypes.tsx
• graphSchema.ts (enum, switch case, and array lists)
• commonSearchesAGI.ts and commonSearchesAGT.ts
• HelpTexts index and ADCSESC16 component under components/HelpTexts- A search of packages/csharp yielded no references to ADCSESC16 (or any ESC kinds) in enums or code-generated schema files, indicating there are no hard-coded relationship-kind lists to update in C#.
1 participant
Description
Support for ADCS ESC16 attacks.
ESC16 is a certificate-based attack path that allows attackers to impersonate any domain user or computer by exploiting misconfigured Enterprise Certificate Authorities.
This PR implements:
Additional cleanup suggestions by coderabbit:
Motivation and Context
Resolves BED-6176
See ticket for rationale.
How Has This Been Tested?
Using the test harness and locally with this data:
20250611101235_BloodHound_esc16.zip
Screenshots (optional):
MATCH p=()-[r:ADCSESC16]->() RETURN p LIMIT 25
Types of changes
Checklist:
Summary by CodeRabbit