Improve ADCSESC6 post-processing #1655
Conversation
WalkthroughThe changes introduce a new property, Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant esc6.go (PostADCSESC6a/b)
participant EnterpriseCA Node
participant CertTemplate
participant Enroller
participant JobQueue
Caller->>esc6.go (PostADCSESC6a/b): Call with EnterpriseCA node
esc6.go (PostADCSESC6a/b)->>EnterpriseCA Node: Check IsUserSpecifiesSanEnabledCollected
alt Property missing or false
esc6.go (PostADCSESC6a/b)-->>Caller: Return early
else Property true
esc6.go (PostADCSESC6a/b)->>CertTemplate: For each published template
esc6.go (PostADCSESC6a/b)->>Enroller: For each enroller of template and CA
esc6.go (PostADCSESC6a/b)->>Enroller: Filter via filterUserDNSResults
esc6.go (PostADCSESC6a/b)->>JobQueue: Submit post relationship job per enroller and domain
end
Poem
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
cmd/api/src/test/integration/harnesses/esc6a-template1.json (1)
112-114: Prefer JSON boolean over string literal for new flagAll existing boolean-style properties in these harness files are already stored as strings, so the addition is internally consistent.
However, JSON supports the native literalstrue/false; adopting them for any new flags avoids one more layer of string/boolean conversion in future test utilities and makes hand-inspection less error-prone.Consider:
- "IsUserSpecifiesSanEnabledCollected": "True", + "IsUserSpecifiesSanEnabledCollected": true,cmd/api/src/test/integration/harnesses/esc6a-template2.json (1)
112-114: Same boolean-literal suggestion as in template 1For the newly introduced flag, using raw JSON booleans (
true/false) will make future parsing simpler and more idiomatic.No action needed if string encoding is required for legacy reasons.
cmd/api/src/test/integration/harnesses/esc6a-eca.json (1)
130-134: Consistent flag encoding across all EnterpriseCA nodesThe new
IsUserSpecifiesSanEnabledCollectedproperty is added uniformly (with one intentionalFalsecase).
If backward-compatibility doesn’t force string values, prefer the JSON boolean literal form (true/false) for all instances to avoid confusion later.Otherwise, looks good.
Also applies to: 228-232, 329-333, 429-433, 529-533
cmd/api/src/test/integration/harnesses.go (4)
5355-5356: Consider maintaining consistent property ordering.The properties are set in a different order compared to earlier hunks. For consistency, consider setting
IsUserSpecifiesSanEnabledbeforeIsUserSpecifiesSanEnabledCollectedto match the pattern used in hunks 1-3.The logic is correct and functional - this is purely a style consistency suggestion:
- s.EnterpriseCA0.Properties.Set(ad.IsUserSpecifiesSanEnabledCollected.String(), true) - s.EnterpriseCA0.Properties.Set(ad.IsUserSpecifiesSanEnabled.String(), true) + s.EnterpriseCA0.Properties.Set(ad.IsUserSpecifiesSanEnabled.String(), true) + s.EnterpriseCA0.Properties.Set(ad.IsUserSpecifiesSanEnabledCollected.String(), true)Apply similar reordering to the other EnterpriseCA nodes in this section.
Also applies to: 5361-5362, 5366-5367, 5371-5372, 5376-5377
5482-5483: Consider maintaining consistent property ordering.Same inconsistency as noted in the previous section - consider setting
IsUserSpecifiesSanEnabledbeforeIsUserSpecifiesSanEnabledCollectedfor consistency.
7228-7233: Consider maintaining consistent property ordering.Same inconsistency as noted in previous sections - consider setting
IsUserSpecifiesSanEnabledbeforeIsUserSpecifiesSanEnabledCollectedfor consistency.
7351-7354: Consider maintaining consistent property ordering.Same inconsistency as noted in previous sections - consider setting
IsUserSpecifiesSanEnabledbeforeIsUserSpecifiesSanEnabledCollectedfor consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
cmd/api/src/test/integration/harnesses/esc6a-eca.svgis excluded by!**/*.svgcmd/api/src/test/integration/harnesses/esc6a-template1.svgis excluded by!**/*.svgcmd/api/src/test/integration/harnesses/esc6a-template2.svgis excluded by!**/*.svg
📒 Files selected for processing (5)
cmd/api/src/test/integration/harnesses.go(7 hunks)cmd/api/src/test/integration/harnesses/esc6a-eca.json(5 hunks)cmd/api/src/test/integration/harnesses/esc6a-template1.json(1 hunks)cmd/api/src/test/integration/harnesses/esc6a-template2.json(1 hunks)packages/go/analysis/ad/esc6.go(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
cmd/api/src/test/integration/harnesses/esc6a-eca.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.
cmd/api/src/test/integration/harnesses.go (1)
undefined
<retrieved_learning>
Learnt from: mvlipka
PR: #1615
File: cmd/api/src/test/integration/harnesses/AZPIMRolesHarness.json:190-194
Timestamp: 2025-06-26T20:50:03.695Z
Learn
8000
ing: 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.
</retrieved_learning>
packages/go/analysis/ad/esc6.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.
⏰ 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 (8)
cmd/api/src/test/integration/harnesses.go (3)
3615-3616: LGTM! Property setting is correct.The addition of
IsUserSpecifiesSanEnabledCollectedproperty aligns with the updated analysis logic requirements.
5069-5070: LGTM! Property setting is correct.The addition of
IsUserSpecifiesSanEnabledCollectedproperty aligns with the updated analysis logic requirements.
5187-5188: LGTM! Property setting is correct.The addition of
IsUserSpecifiesSanEnabledCollectedproperty aligns with the updated analysis logic requirements.packages/go/analysis/ad/esc6.go (5)
39-43: Good addition of data collection check.The new check for
IsUserSpecifiesSanEnabledCollectedensures that post-processing only runs when the necessary data has been collected. The error handling and early return pattern are implemented correctly.
50-51: Good performance optimization.Moving the
enterpriseCAEnrollerscache lookup outside the loop avoids redundant lookups for each certificate template, improving performance when processing multiple templates.
60-76: Excellent refactoring for clarity and resilience.The simplified logic directly processes enrollers without intermediate result accumulation, making the code more readable and maintainable. The error handling pattern with
slog.WarnContextandcontinueensures that processing continues for other certificate templates even if one encounters an error, improving overall resilience.
84-88: Consistent implementation with PostADCSESC6a.The data collection check is implemented consistently across both ESC6 variants, maintaining code uniformity.
105-123: Consistent refactoring with ESC6b-specific logic preserved.The simplified processing logic matches the pattern in
PostADCSESC6awhile correctly maintaining the ESC6b-specific check forHasUPNCertMappingInForest. This ensures consistent code structure across both variants while preserving their unique behaviors.
Description
Add IsUserSpecifiesSanEnabledCollected check in post-processing function for ADCSESC6
Refactor post-processing of ADCSESC6 to improve readability and performance
Motivation and Context
Resolves BED-6144
How Has This Been Tested?
By running tests
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor