TEST ADCS update #1578
TEST ADCS update #1578
Conversation
JonasBK
added
enhancement
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces the detection, analysis, and UI support for the ADCS ESC16 vulnerability in Active Directory environments. It adds new schema properties and relationship kinds, implements backend logic for ESC16 edge calculation and post-processing, and expands test harnesses and integration tests. The frontend gains explanatory help texts, abuse scenarios, and visualization support for ESC16 and related privilege relationships. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test
participant DB as Graph Database
participant ADCS as ADCS Post-Processor
participant ESC16 as ESC16 Logic
Test->>DB: Load ADCSESC16Harness.json (without ESC16 edges)
Test->>ADCS: Trigger post-processing for EnterpriseCA
ADCS->>ESC16: PostADCSESC16(EnterpriseCA, Domains, Cache)
ESC16->>DB: Analyze CA and CertTemplate properties
ESC16->>DB: Traverse graph to find eligible users/groups
ESC16->>DB: Create ADCSESC16 edges for valid paths
Test->>DB: Query for ADCSESC16 edges
Test->>Test: Assert edge count and correctness
Poem
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 (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (1)
packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
260-277: 🛠️ Refactor suggestionEdge registry growth – consider modularisation
EdgeInfoComponentsis now ~280 lines long and growing. Continuing to append entries scales poorly and invites merge conflicts.Suggestion: split into thematic maps (e.g.
ADCS,AZ,Write*, etc.) and compose withObject.assign:export default { ...ADCSEdges, ...WriteEdges, ...AzureEdges, // ... } as const;
🧹 Nitpick comments (28)
packages/javascript/bh-shared-ui/src/edgeTypes.tsx (1)
92-101: Maintain alphabetical ordering for clarityWithin the “Advanced Object Manipulation” array, the new entries break the existing alphabetical order (
WriteAccountRestrictions,WriteAltSecurityIdentities,WriteGPLink,WritePublicInformation,WriteSPN).
Keeping this list sorted makes future diffs easier to review.- ActiveDirectoryRelationshipKind.WriteAccountRestrictions, - ActiveDirectoryRelationshipKind.WriteAltSecurityIdentities, - ActiveDirectoryRelationshipKind.WriteGPLink, - ActiveDirectoryRelationshipKind.WritePublicInformation, - ActiveDirectoryRelationshipKind.WriteSPN, + ActiveDirectoryRelationshipKind.WriteAccountRestrictions, + ActiveDirectoryRelationshipKind.WriteAltSecurityIdentities, + ActiveDirectoryRelationshipKind.WriteGPLink, + ActiveDirectoryRelationshipKind.WritePublicInformation, + ActiveDirectoryRelationshipKind.WriteSPN,(No functional change—pure readability.)
packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/References.tsx (2)
23-26: Optional: Enhance link security attributes
For added security and privacy, includenoreferreralongsidenoopenerto prevent the new page from accessing the referrer:- rel='noopener' + rel='noopener noreferrer'Also applies to: 30-33, 37-40, 44-47, 51-54, 58-61
22-64: Optional: Use semantic list or Stack for spacing
Rather than inserting<br/>between links, consider using MUI’s<Stack spacing={1}>or a semantic<ul>/<li>pattern to manage spacing and improve accessibility:- <Box sx={{ overflowX: 'auto' }}> - <Link ...>...</Link> - <br/> - <Link ...>...</Link> + <Stack direction="column" spacing={1} sx={{ overflowX: 'auto' }}> + {links.map(({ href, label }) => ( + <Link key={href} href={href} target="_blank" rel="noopener noreferrer"> + {label} + </Link> + ))}packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/Opsec.tsx (1)
22-30: Nitpick: Remove unnecessary fragment
The React fragment wrapping<Typography>is redundant. You can return the element directly:- return ( - <> - <Typography variant='body2'>…</Typography> - </> - ); + return <Typography variant='body2'>…</Typography>;packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/Opsec.tsx (2)
20-30: Remove redundant fragment wrapper
Since the component returns only a single<Typography>element, the<>…</>wrapper is unnecessary. Simplify to:- return ( - <> - <Typography variant="body2"> - …text… - </Typography> - </> - ); + return ( + <Typography variant="body2"> + …text… + </Typography> + );
23-24: Use double quotes for JSX props
For consistency with typical JSX conventions, changevariant='body2'tovariant="body2".cmd/api/src/test/integration/harnesses.go (1)
3615-3617: Repetitive property-setting boilerplate – extract a helperEach harness now repeats the same two-line pattern to flag SAN collection.
A tiny helper would de-duplicate ~15 lines, improve readability, and reduce future merge noise.Example:
func markSANCollected(ca *graph.Node, enabled bool) { ca.Properties.Set(ad.IsUserSpecifiesSanEnabledCollected.String(), true) ca.Properties.Set(ad.IsUserSpecifiesSanEnabled.String(), enabled) }Then replace the two setters with
markSANCollected(s.EnterpriseCA1, true/false).Also applies to: 5069-5071, 5187-5189, 5481-5483
packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/ADCSESC16.tsx (1)
24-31: Consider freezing the descriptor object
ADCSESC16is effectively a constant lookup table – declaring it withas const(orreadonlyRecord type) prevents accidental mutation and gives the compiler stricter exhaustiveness checks.-const ADCSESC16 = { +const ADCSESC16 = { general: General, windowsAbuse: WindowsAbuse, linuxAbuse: LinuxAbuse, opsec: Opsec, references: References, composition: Composition, -}; +} as const;packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
22-23: Import placement is slightly off-alphabeticalImports in this block appear to be sorted numerically/alphabetically; inserting
ADCSESC16afterADCSESC13keeps numeric order but breaks lexical ordering (13<16, so fine) – no functional issue, just mentioning for consistency.packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/References.tsx (1)
20-96: UI readability nitpickUsing
<br />repeatedly produces uneven spacing. Consider a<Stack>or<List>for semantic grouping and automatic spacing.-<Box sx={{ overflowX: 'auto' }}> +<Stack sx={{ overflowX: 'auto' }} spacing={1}> ... -</Box> +</Stack>packages/go/graphschema/common/common.go (1)
43-47: Consider auto-generating the ever-growing kind slices
InboundRelationshipKinds/OutboundRelationshipKindskeep being manually appended (now withADCSESC16,WriteAltSecurityIdentities,WritePublicInformation).
A generated map or a small unit-test asserting uniqueness would prevent accidental duplicates and shorten future diffs.packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/References.tsx (1)
23-95: Addnoreferrerto external linksEvery
<Link>opens a new tab withtarget="_blank"but only setsrel="noopener".
Missingnoreferrerbreaks the recommendation against reverse-tab-nabbing in some security guides.-<Link target='_blank' rel='noopener' href='https://attack.mitre.org/techniques/T1098/'> +<Link target='_blank' rel='noopener noreferrer' href='https://attack.mitre.org/techniques/T1098/'>Apply the same addition to every link in the component.
packages/go/lab/loader.go (1)
166-185: Gracefully handle emptySTRLIST:and tighten decimal parsing
STRLIST:with no elements currently returns[]string{""}.
Trim the prefix first and check for empty string.
DECIMAL:silently acceptsNaN,Inf, etc. Consider validating withmath.IsFinite(Go 1.22) or a simple range check if those values would break the DB layer.packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/General.tsx (1)
25-37: Add missing verbs & fix minor grammar issuesTwo small omissions make the text read awkwardly:
- “…
sourcethe privileges to perform …” → should read “sourcehas the privileges…”.- “This setup allow an attacker…” → should be “allows”.
- {groupSpecialFormat(sourceType, sourceName)} the privileges to perform the ADCS ESC16 attack against the + {groupSpecialFormat(sourceType, sourceName)} **has** the privileges to perform the ADCS ESC16 attack against the @@ - any published certificate template. Also, the CA has the SID security extension disabled. This setup - allow an attacker principal to obtain a malicious certificate as any AD forest user or computer and use + any published certificate template. Also, the CA has the SID security extension disabled. This setup + **allows** an attacker principal to obtain a malicious certificate as any AD forest user or computer and usepackages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/Composition.tsx (1)
38-44: UX: Add empty-state messageWhen
nodesArrayis empty the list renders nothing, leaving users wondering if loading failed. Consider a short placeholder:- ) : ( - <VirtualizedNodeList nodes={nodesArray} /> + ) : nodesArray.length === 0 ? ( + <Typography variant="body2" color="text.secondary"> + No composition items were found for this edge. + </Typography> + ) : ( + <VirtualizedNodeList nodes={nodesArray} /> )}packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/General.tsx (1)
25-28: Sentence missing a verbAdd “has” to keep the sentence grammatical.
- {groupSpecialFormat(sourceType, sourceName)} the ability to write to the AltSecurityIdentities attribute + {groupSpecialFormat(sourceType, sourceName)} **has** the ability to write to the AltSecurityIdentities attributepackages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/LinuxAbuse.tsx (1)
49-50: Maintain formatting consistency for code blocks
Step 2shows a raw<pre>block while the rest usesCodeController, leading to inconsistent styling (copy-buttons, syntax colours, etc.).- <Typography component={'pre'}>{'certipy auth -pfx administrator.pfx -dc-ip 172.16.126.128'}</Typography> + <CodeController> +{`certipy auth -pfx administrator.pfx -dc-ip 172.16.126.128`} +</CodeController>packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/General.tsx (1)
31-35: Improve grammatical flow and semantic HTML.
The sentence structure is a bit awkward and<i>isn’t semantic for emphasis. Consider this diff:- <Typography variant='body2'> - The Public-Information property set includes the altSecurityIdentities attribute allows you to specify{' '} - <i>explicit certificate mappings</i> for a principal. An explicit certificate mapping is a reference to - a certificate, which can be used to authenticate as the principal. - </Typography> + <Typography variant='body2'> + The Public-Information property set, which includes the <code>altSecurityIdentities</code> attribute, + allows you to specify <em>explicit certificate mappings</em> for a principal. An explicit certificate + mapping is a reference to a certificate, which can be used to authenticate as the principal. + </Typography>This adds commas for clarity, wraps the attribute in
<code>, and uses<em>for semantic emphasis.cmd/api/src/analysis/ad/adcs_integration_test.go (1)
3676-3685: Drop the nested read-only transactionWrapping
graphDB.ReadTransactioninside anotherReadTransactionadds no value, obscures the intent, and may dead-lock on some drivers.-err = graphDB.ReadTransaction(testCtx.Context(), func(tx graph.Transaction) error { - if err = graphDB.ReadTransaction(context.Background(), func(tx graph.Transaction) error { - ... - }); err != nil { - ... - } - return nil -}) +err = graphDB.ReadTransaction(testCtx.Context(), func(tx graph.Transaction) error { + ... + return nil +})packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/LinuxAbuse.tsx (2)
34-40: Minor wording & nesting nitsTypo and invalid HTML nesting:
- “The certificate must meet the following requirements”.
<p>tags inside a<Typography>(which itself renders a<p>) violate HTML-spec; browsers forgive it but React DevTools will warn.
UseTypography component="div"or drop the inner<p>.-<Typography variant='body2'> - The certificate must the following requirements: +<Typography variant='body2' component='div'> + The certificate must meet the following requirements:Same applies to the later “Verify the that mapping” line – drop the extra the.
Also applies to: 102-103
22-22: Unused props – add placeholder to keep TS happyThe component is typed as
FC<EdgeInfoProps>but doesn’t accept a param.
While structurally assignable, enablingstrictFunctionTypeswill flag this.
Recommend accepting but ignoring the props:-const LinuxAbuse: FC<EdgeInfoProps> = () => { +const LinuxAbuse: FC<EdgeInfoProps> = (_props) => {packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/WindowsAbuse.tsx (2)
34-40: Duplicate wording issues (same as Linux variant)“must the following requirements” → “must meet …”.
Also remove nested<p>inside<Typography>as noted earlier.Refactor once and reuse between Windows/Linux variants to avoid copy-paste drift.
Also applies to: 110-111
22-22: Accept but ignore props to silencestrictFunctionTypesSame as Linux file.
-const WindowsAbuse: FC<EdgeInfoProps> = () => { +const WindowsAbuse: FC<EdgeInfoProps> = (_props) => {packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/LinuxAbuse.tsx (2)
34-40: Replicate wording / HTML fixesThe issues called out in the AltSecurityIdentities Linux file also exist here.
Please apply the same corrections.Also applies to: 102-103
22-22: Props unusedSame fix as other files.
packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/WindowsAbuse.tsx (2)
34-40: Wording / HTML duplicationSee previous comments – identical corrections needed here.
Also applies to: 110-111
22-22: Props unusedSame fix.
packages/go/graphschema/ad/ad.go (1)
263-271: Properties list updated but consider ordering for diff hygiene.
RPCEncryption*andDisabledExtensions*are appended to the very end ofAllProperties().
Keeping alphabetical order makes future diffs smaller and helps readers locate fields quickly.
📜 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 (47)
cmd/api/src/analysis/ad/adcs_integration_test.go(2 hunks)cmd/api/src/test/integration/harnesses.go(7 hunks)cmd/api/src/test/integration/harnesses/ADCSESC16Harness.json(1 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/cue/bh/ad/ad.cue(10 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/esc6.go(1 hunks)packages/go/analysis/ad/post.go(1 hunks)packages/go/cypher/test/cases/positive_tests.json(1 hunks)packages/go/ein/ad.go(1 hunks)packages/go/ein/incoming_models.go(1 hunks)packages/go/graphschema/ad/ad.go(7 hunks)packages/go/graphschema/common/common.go(1 hunks)packages/go/lab/loader.go(3 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/WriteAltSecurityIdentities/General.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/LinuxAbuse.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/Opsec.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/References.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/WindowsAbuse.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/WriteAltSecurityIdentities.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/General.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/LinuxAbuse.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/Opsec.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/References.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/WindowsAbuse.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/WritePublicInformation.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx(4 hunks)packages/javascript/bh-shared-ui/src/edgeTypes.tsx(2 hunks)packages/javascript/bh-shared-ui/src/graphSchema.ts(9 hunks)packages/javascript/bh-shared-ui/src/views/TierManagement/Details/Details.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/TierManagement/Details/DetailsList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/TierManagement/Details/EntityInfo/SelectorList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/TierManagement/Details/MembersList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/TierManagement/Details/utils.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/TierManagement/Summary/SummaryList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/TierManagement/TierManagement.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/javascript/bh-shared-ui/src/views/TierManagement/Details/utils.tsx
🧰 Additional context used
🧬 Code Graph Analysis (17)
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 (4)
IsUserSpecifiesSanEnabled(141-141)RoleSeparationEnabled(143-143)RPCEncryptionEnforced(263-263)DisabledExtensions(265-265)
packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
packages/go/graphschema/ad/ad.go (3)
ADCSESC16(114-114)WriteAltSecurityIdentities(125-125)WritePublicInformation(126-126)
packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/WritePublicInformation.tsx (1)
packages/go/graphschema/ad/ad.go (1)
WritePublicInformation(126-126)
packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/ADCSESC16.tsx (1)
packages/go/graphschema/ad/ad.go (1)
ADCSESC16(114-114)
packages/go/lab/loader.go (2)
packages/go/dawgs/graph/kind.go (1)
StringsToKinds(154-162)packages/go/graphschema/ad/ad.go (2)
NodeKinds(1134-1136)Entity(28-28)
packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/General.tsx (1)
packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
EdgeInfoProps(142-151)
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(110-231)
packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/WriteAltSecurityIdentities.tsx (1)
packages/go/graphschema/ad/ad.go (1)
WriteAltSecurityIdentities(125-125)
packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/General.tsx (1)
packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
EdgeInfoProps(142-151)
packages/go/ein/ad.go (2)
packages/go/ein/incoming_models.go (3)
CARegistryData(148-155)RPCEncryptionEnforced(138-141)DisabledExtensions(143-146)packages/go/graphschema/ad/ad.go (2)
RPCEncryptionEnforced(263-263)DisabledExtensions(265-265)
packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/General.tsx (1)
packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
EdgeInfoProps(142-151)
packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/LinuxAbuse.tsx (1)
packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
EdgeInfoProps(142-151)
packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/LinuxAbuse.tsx (1)
packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
EdgeInfoProps(142-151)
packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/WindowsAbuse.tsx (1)
packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
EdgeInfoProps(142-151)
packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/WindowsAbuse.tsx (1)
packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
EdgeInfoProps(142-151)
packages/go/graphschema/ad/ad.go (2)
packages/go/dawgs/graph/kind.go (1)
StringKind(145-152)packages/go/ein/incoming_models.go (2)
RPCEncryptionEnforced(138-141)DisabledExtensions(143-146)
🪛 Gitleaks (8.26.0)
packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/LinuxAbuse.tsx
99-99: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
121-121: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/WindowsAbuse.tsx
99-99: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
121-121: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/WindowsAbuse.tsx
107-107: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
129-129: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 golangci-lint (1.64.8)
cmd/api/src/analysis/ad/adcs_integration_test.go
[error] 30-30: ST1019(related information): other import of "github.com/specterops/bloodhound/graphschema"
(stylecheck)
🔇 Additional comments (35)
packages/javascript/bh-shared-ui/src/views/TierManagement/Summary/SummaryList.tsx (1)
1-15: License header addition
The new Apache 2.0 license header with SPDX identifier is correctly added. Ensure the wording and formatting are consistent with other files in the codebase (e.g., inclusion of “(the "License")” suffix if that’s your standard).packages/javascript/bh-shared-ui/src/views/TierManagement/TierManagement.tsx (1)
22-33: Import-order tweak LGTMPure re-ordering of route constants improves grouping consistency without altering behavior. No action needed.
cmd/api/src/test/integration/harnesses/esc6a-template2.json (1)
113-113: Test harness update: SAN collection flag added.The new
"IsUserSpecifiesSanEnabledCollected": "True"property on theEnterpriseCAnode aligns with other ESC6a templates and supports the updated post-processing logic.packages/javascript/bh-shared-ui/src/views/TierManagement/Details/Details.tsx (1)
24-24: Import reorder: TierActionBar moved for consistency.The
TierActionBarimport has been repositioned to group related fragments together, matching the import ordering patterns used elsewhere in this module.cmd/api/src/test/integration/harnesses/esc6a-template1.json (1)
113-113: Test harness update: SAN collection flag added.Adding
"IsUserSpecifiesSanEnabledCollected": "True"on theEnterpriseCAnode maintains consistency across ESC6a templates and ensures test data supports the new analysis logic.packages/javascript/bh-shared-ui/src/views/TierManagement/Details/EntityInfo/SelectorList.tsx (1)
23-23: Consolidate imports from utils.Combining
getTagUrlValueanditemSkeletonsinto a single import from../../utilssimplifies the code without affecting functionality.packages/javascript/bh-shared-ui/src/views/TierManagement/Details/DetailsList.tsx (1)
31-31: Adjusted import grouping for utilities.Swapping the relative import order to bring
itemSkeletonsbeforeSelectedHighlight/getListHeightkeeps related utilities organized together.packages/javascript/bh-shared-ui/src/views/TierManagement/Details/MembersList.tsx (1)
27-29: Verify relocated helper import
SelectedHighlightandgetListHeighthave been moved from../utilsto./utils.
Double-check that:
./utils/index.ts(or equiv.) actually re-exports both symbols.- The previous
../utilsexport has been removed to avoid duplicate definitions.If either is missing, the TS build will fail at compile time.
packages/go/analysis/ad/ad.go (1)
574-576: ESC16 case wired correctlyThe new switch branch cleanly delegates to
GetADCSESC16EdgeComposition, matching the pattern used for ESC1/3/4/6/9/10/13. No issues spotted.packages/go/analysis/ad/post.go (1)
55-60: Relationship list updated
ad.ADCSESC16is now part ofPostProcessedRelationships(), ensuring it’s included in post-processing orchestration. Looks good.packages/javascript/bh-shared-ui/src/edgeTypes.tsx (1)
104-118: Edge kind added – confirm schema export
ActiveDirectoryRelationshipKind.ADCSESC16enters the ADCS bucket. Ensure the enum value is exported fromgraphSchema.ts(the import at line 17) and that all consumer code paths (filters, legend, help text mapping) are updated; otherwise the UI may blow up at runtime.cmd/api/src/test/integration/harnesses/esc6a-eca.json (1)
128-134: Property naming consistency
"IsUserSpecifiesSanEnabledCollected"is introduced alongside the existing"IsUserSpecifiesSanEnabled".
Confirm the parser expects this exact camel-cased key; a typo or case mismatch will silently skip the value in Go’s struct tags.Also, all values are string literals
"True"/"False", matching the surrounding convention—so no action needed on typing.Also applies to: 228-232, 325-331, 427-433, 527-533
packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/References.tsx (1)
20-66: Approve: References component implementation
The component correctly renders a horizontally scrollable container with the curated list of static links.packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/WritePublicInformation.tsx (1)
23-31: Approve: Aggregation object for WritePublicInformation
This mapping cleanly aggregates all subcomponents for theWritePublicInformationhelp text set.packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/Opsec.tsx (1)
20-31: Approve: Opsec component content
The operational security guidance is accurate and theTypographyusage aligns with design guidelines.packages/go/ein/ad.go (1)
1167-1175: Approve: ParseCARegistryProperties extended
The addition ofRPCEncryptionEnforcedandDisabledExtensionsfollows the existing pattern and correctly populates the property map when collected.packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/WriteAltSecurityIdentities.tsx (1)
23-31: Approve: Aggregation object for WriteAltSecurityIdentities
The component correctly imports and aggregates all subcomponents for theWriteAltSecurityIdentitieshelp text set.packages/go/cypher/test/cases/positive_tests.json (1)
693-699:✅ Verification successful
Test updated to include new ADCS relationship types
The extended relationship list now includesADCSESC16,WriteAltSecurityIdentities, andWritePublicInformation, which aligns with the schema and analysis logic additions. Ensure these kinds are correctly registered in the graph schema and parser.Run this to verify registration in your Go schema packages:
🏁 Script executed:
#!/bin/bash # Verify new relationship kinds are present in graphschema rg -nE '(ADCSESC16|WriteAltSecurityIdentities|WritePublicInformation)' packages/go/graphschemaLength of output: 195
Let's try a fixed-string search to avoid the regex parsing error:
🏁 Script executed:
#!/bin/bash # Search for new relationship kinds in graphschema using fixed strings rg -nF -e ADCSESC16 -e WriteAltSecurityIdentities -e WritePublicInformation packages/go/graphschemaLength of output: 8697
Approved: New ADCS relationship types registered in graphschema
The constantsADCSESC16,WriteAltSecurityIdentities, andWritePublicInformationare defined inpackages/go/graphschema/ad/ad.goand included in all relevant[]graph.Kindreturn lists. No further changes required.packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/Opsec.tsx (1)
1-16: License header and SPDX tag are correctly applied
The Apache-2.0 header and SPDX identifier conform to project standards.cmd/api/src/test/integration/harnesses.go (1)
5359-5363:❓ Verification inconclusive
Comment/doc inconsistency – CA1
IsUserSpecifiesSanEnabledis not leftnilThe inline comment says the property should remain unset, yet the very next lines explicitly set it to
false.
This breaks the intended test scenario and can hide bugs that rely on thenil(vsfalse) distinction.-// leave ca1 isUserSpecifiesSanEnabled as nil s.EnterpriseCA1 = graphTestContext.NewActiveDirectoryEnterpriseCA("EnterpriseCA1", domainSid1) -s.EnterpriseCA1.Properties.Set(ad.IsUserSpecifiesSanEnabledCollected.String(), true) -s.EnterpriseCA1.Properties.Set(ad.IsUserSpecifiesSanEnabled.String(), false) +# Keep IsUserSpecifiesSanEnabled unset (nil) – only mark it as collected +s.EnterpriseCA1.Properties.Set(ad.IsUserSpecifiesSanEnabledCollected.String(), true)Verify whether the test logic actually needs the property to be
nilorfalse, then adjust either the comment or the code accordingly.
Mismatch between inline comment and code for
IsUserSpecifiesSanEnabledThe comment in
cmd/api/src/test/integration/harnesses.go(lines 5359–5363) says to leaveIsUserSpecifiesSanEnabledunset, but the code immediately marks it collected and sets it tofalse. Please confirm which behavior is intended:
- If the test should treat the flag as unset (nil), remove the
.Set(ad.IsUserSpecifiesSanEnabled.String(), false)call.- If the test should explicitly set it to false, update the inline comment to reflect that.
File:
cmd/api/src/test/integration/harnesses.go
Lines: 5359–5363- // leave ca1 isUserSpecifiesSanEnabled as nil s.EnterpriseCA1 = graphTestContext.NewActiveDirectoryEnterpriseCA("EnterpriseCA1", domainSid1) s.EnterpriseCA1.Properties.Set(ad.IsUserSpecifiesSanEnabledCollected.String(), true) - s.EnterpriseCA1.Properties.Set(ad.IsUserSpecifiesSanEnabled.String(), false) + // (adjusted to either remove this line to leave nil or update the comment to explain it’s explicitly set false)packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/Opsec.tsx (1)
20-29: Component looks good – no action requiredThe functional component is concise, uses FC typing, and applies MUI correctly.
No correctness, security, or styling issues spotted.packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
131-140: Security nitpick: missingnoreferreron external linksFor
WriteAltSecurityIdentitiesandWritePublicInformationhelp-text files (imported here) ensure any<Link target="_blank">also setsrel="noopener noreferrer"(some existing components only usenoopener). This prevents potential reverse-tab-napping.packages/go/ein/incoming_models.go (1)
138-146: Struct additions correctly follow existing pattern
RPCEncryptionEnforcedandDisabledExtensionsembedAPIResultlike the neighboring types and introduce the expectedValue/Datafields. No functional or stylistic issues spotted, and the fields are picked up inCARegistryData.Also applies to: 153-155
packages/go/lab/loader.go (1)
24-32:slicesimport pins the codebase to Go 1.21+The std-lib
slicespackage arrived in Go 1.21. If CI or downstream users are still on 1.20, builds will fail.packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/Composition.tsx (1)
24-29: Possible typo: use enum/member access instead of string-indexing
EdgeInfoItems['composition']works only ifEdgeInfoItemsis an object literal, but in the rest of the codebase it’s treated like an enum (e.g.,EdgeInfoItems.composition).
Using bracket notation here introduces an implicit string key and bypasses type-checking.- type: EdgeInfoItems['composition'], + type: EdgeInfoItems.composition,If
EdgeInfoItemsis indeed aconst enum, the current code won’t compile. Please confirm the intended shape.packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/General.tsx (5)
1-16: License header is correctly applied.
Apache 2.0 license and SPDX identifier are present and properly formatted.
18-21: Import statements look good.
All required modules (Typography,FC,EdgeInfoProps,groupSpecialFormat, andtypeFormat) are imported succinctly.
23-24: Component signature is clear and typed.
UsingFC<EdgeInfoProps>provides strong typing, and destructuring only the needed props keeps the signature clean.
26-29: Correct use of formatting helpers in the first paragraph.
groupSpecialFormatandtypeFormatinject the dynamic entity names/types appropriately.
38-40: Export is correctly defined.
export default General;cleanly exposes the component for consumption.packages/cue/bh/ad/ad.cue (2)
973-999: Confirm backend / frontend handling for newly-added properties
RPCEncryption{Enforced|Collected}andDisabledExtensions{,Collected}are correctly declared and appended toProperties, but the Go ingestion layer (incoming_models.go,ad.go) and the JS graph schema still need corresponding string constants (or generated code) to avoid silent drops during unmarshalling / rendering.Please double-check that:
- These four strings are present in the
const (…)blocks inpackages/go/graphschema/ad.- They are added to
PropertyEnum→id<>namemappings used by the REST API.- The front-end
graphSchema.tsexport is regenerated.Skipping any of the above will produce “unknown property” warnings at runtime.
Also applies to: 1133-1136
1598-1602: Edge-kind additions look good – verify traversal / abuse helpers
ADCSESC16,WriteAltSecurityIdentities, andWritePublicInformationhave been wired into all the major kind collections 🎉.
Just ensure:
- Post-processing functions (
esc16.go, etc.) emit these exact strings.- UI
edgeTypes.tsx& help-text indexes were regenerated (looks like they were).- Unit / integration tests cover path-finding traversal on the new kinds (not only existence).
Also applies to: 1653-1661, 1735-1748, 1780-1782, 1828-1839, 1862-1866
packages/go/graphschema/ad/ad.go (1)
1112-1119: Enum slices stay consistent – good work.
ADCSESC16,WriteAltSecurityIdentities, andWritePublicInformationwere added everywhere necessary (Relationships/ACL/Pathfinding).
No issues spotted.packages/go/analysis/ad/esc6.go (1)
39-43: Early exit correctly guards on collection flag – LGTM.packages/javascript/bh-shared-ui/src/graphSchema.ts (1)
480-483: Display strings present – looks good.Also applies to: 749-756
...s/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/References.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (15)
cmd/api/src/test/integration/harnesses.go (2)
7228-7233: Missing UpdateNode calls after setting properties on EnterpriseCA nodes
TheIsUserSpecifiesSanEnabledCollectedand original SAN-enabled flags are set on EnterpriseCA0/1/2 but never persisted. Add the following lines after line 7233 to ensure the changes are applied:graphTestContext.UpdateNode(s.EnterpriseCA0) graphTestContext.UpdateNode(s.EnterpriseCA1) graphTestContext.UpdateNode(s.EnterpriseCA2)
7351-7355: Missing UpdateNode call for EnterpriseCA1
EnterpriseCA1is missing an update after its flags are set. Extend the update sequence:graphTestContext.UpdateNode(s.EnterpriseCA0) +graphTestContext.UpdateNode(s.EnterpriseCA1)packages/go/analysis/ad/adcs.go (1)
210-217: Post-processing fan-out still duplicatedThe new
PostADCSESC16block follows the same copy-paste pattern previously flagged. Re-iterating earlier feedback: collapse these 12 nearly identicalSubmitReadercalls into a table-driven loop to cut maintenance overhead.packages/go/lab/loader.go (1)
95-103: 🛠️ Refactor suggestionStill mutating
nodeKindswhile ranging – duplicates & brittle logic remain
The loop appends tonodeKindsduring iteration and thenbreaks. This is exactly the issue highlighted in the earlier review and can still introduce duplicated kinds (Base/AZBase) as well as the general “modify-while-iterate” foot-gun.- for _, kind := range nodeKinds { - if slices.Contains(ad.NodeKinds(), kind) { - nodeKinds = append(nodeKinds, ad.Entity) - break - } else if slices.Contains(azure.NodeKinds(), kind) { - nodeKinds = append(nodeKinds, azure.Entity) - break - } - } + 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) + } + + nodeKinds = slices.Compact(nodeKinds) // remove accidental dupsThis keeps the code clear, avoids slice mutation during range, and guarantees deduplication.
packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/References.tsx (1)
23-95: Addnoreferrerto every external link
AllLinkcomponents userel="noopener"but omitnoreferrer, leaving the referrer header exposed and enabling reverse tab-napping.-<Link target='_blank' rel='noopener' +<Link target='_blank' rel='noopener noreferrer'Apply to every occurrence in this file.
packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/WindowsAbuse.tsx (1)
25-32:⚠️ Potential issueTemplate name typo – should be
/template:ESC16
The example command still referencesESC6, which contradicts the ESC16 context and will confuse readers.-.\\Certify.exe ... /template:ESC6 ... +.\\Certify.exe ... /template:ESC16 ...Please update the sample command.
cmd/api/src/test/integration/harnesses/ADCSESC16Harness.json (1)
114-169:⚠️ Potential issueRelationship IDs still collide with node IDs
Previous feedback pointed this out; the current harness still re-uses the
"nX"pattern for relationships ("id": "n0","id": "n1"…).
Some graph loaders require IDs to be unique across the whole set and may drop or overwrite data.Rename relationship IDs to a distinct namespace (e.g.
r0,rel-0, etc.):- "id": "n0", + "id": "r0", … - "id": "n1", + "id": "r1", … - "id": "n2", + "id": "r2",cmd/api/src/analysis/ad/adcs_integration_test.go (3)
29-32: Duplicate import alias still presentThe aliased re-import of
graphschema(schema) was already flagged in a previous review. It violatesST1019and risks symbol collisions.🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 29-29: ST1019: package "github.com/specterops/bloodhound/graphschema" is being imported more than once
(stylecheck)
[error] 30-30: ST1019(related information): other import of "github.com/specterops/bloodhound/graphschema"
(stylecheck)
3671-3673: Calloperation.Done()once, after the loopClosing the operation inside the
forloop terminates internal channels before all readers are submitted – same issue noted in earlier tests.- err = operation.Done() - require.Nil(t, err) - } + } + + err = operation.Done() + require.NoError(t, err)
3651-3653:⚠️ Potential issueHandle
FetchADCSPrereqserror immediately
erris discarded, so the test will keep running with undefined pre-requisites and will panic later.- groupExpansions, enterpriseCertAuthorities, _, domains, cache, err := FetchADCSPrereqs(graphDB) + groupExpansions, enterpriseCertAuthorities, _, domains, cache, err := FetchADCSPrereqs(graphDB) + require.NoError(t, err)packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/LinuxAbuse.tsx (2)
22-22: Consider removing unused props or documenting their purpose.Similar to
WindowsAbuse.tsx, this component acceptsEdgeInfoPropsbut doesn't use any of the properties.
34-34:⚠️ Potential issueFix the grammatical error.
Missing word "meet" in the sentence.
- The certificate must the following requirements: + The certificate must meet the following requirements:packages/go/analysis/ad/esc16.go (3)
244-246:⚠️ Potential issueRestore critical CA-level predicates
The
IsUserSpecifiesSanEnabledandDisabledExtensionsfilters are still commented out, so anyEnroll → CAedge is treated as ESC16. This re-opens the false-positive hole called out earlier.
266-274:⚠️ Potential issueRe-enable template-validation clauses
The manager-approval, auth-enabled and schema/authorized-signatures checks remain commented. Without them, virtually every published template matches, massively over-reporting ESC16 paths.
257-258: 🛠️ Refactor suggestionRemove unused
edgeKindparameter
edgeKindis passed toADCSESC16Path2Patternbut never referenced inside the function. It’s dead code that confuses readers and triggers linters.-func ADCSESC16Path2Pattern(domainId graph.ID, enterpriseCAs cardinality.Duplex[uint64], edgeKind graph.Kind) traversal.PatternContinuation { +func ADCSESC16Path2Pattern(domainId graph.ID, enterpriseCAs cardinality.Duplex[uint64]) traversal.PatternContinuation {…and drop the argument at all call-sites.
Also applies to: 197-198
🧹 Nitpick comments (25)
packages/javascript/bh-shared-ui/src/views/TierManagement/TierManagement.tsx (1)
21-33: Import order tidy-up acknowledged, but enforce linter/auto-sort to avoid churnThe three
ROUTE_TIER_MANAGEMENT_SUMMARY*imports were merely moved upward for grouping. While harmless, manual shuffling easily drifts from whatever canonical ordering the rest of the codebase follows (alphabetical, path-depth, eslint-plugin-import, etc.). Consider configuring / re-running an import-sorting linter (e.g.eslint-plugin-import/order) so these re-orderings are automatic and consistent project-wide, preventing noise in future diffs.packages/javascript/bh-shared-ui/src/views/TierManagement/Details/MembersList.tsx (1)
27-29: Two separateutilspaths may cause accidental drift
ItemSkeletonis still imported from../utils, whereasSelectedHighlightandgetListHeightare now imported from./utils.
Unless the intent is to have two different utility modules, consider consolidating them into a single location to avoid duplicated helpers and confusing maintenance later on.packages/go/analysis/ad/post.go (1)
57-61: Duplicate kinds remain in the slice – clean-up recommended
OwnsandWriteOwnerare each listed twice inPostProcessedRelationships().
While unrelated to the newADCSESC16entry, the duplicates add minor overhead and may confuse future readers.- ad.Owns, - ad.WriteOwner, ad.ExtendedByPolicy, - ad.Owns, - ad.WriteOwner, + ad.ExtendedByPolicy,packages/go/cypher/test/cases/positive_tests.json (1)
694-697: Long edge-type list getting unwieldyThe test query now carries 40 + relationship kinds in a single pattern. Consider extracting the list to a constant or splitting lines to improve readability and diff-friendliness—but functionally this is fine.
packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/References.tsx (2)
1-16: Update license year to current project standard.The file header lists 2024, but other new modules in this PR use 2025. Please synchronize the copyright year.
23-64: Includenoreferrerin linkrelattributes.For all external
<Link target='_blank'>, addnoreferrerto therelprop to improve security:- rel='noopener' + rel='noopener noreferrer'packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/WriteAltSecurityIdentities.tsx (1)
23-29: Align property key order for readability
Consider reordering the keys inWriteAltSecurityIdentities(e.g., match import order or alphabetical) to make the mapping easier to scan.packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/ADCSESC16.tsx (1)
24-31: Align property key order for readability
For consistency, reorder the keys in theADCSESC16object (e.g.,general,composition,linuxAbuse,opsec,references,windowsAbuse) to mirror imports or alphabetical order.packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/Opsec.tsx (1)
20-30: Remove unnecessary React fragment
Since there's only a single<Typography>element, you can return it directly without the surrounding<>...</>fragment.packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/Opsec.tsx (1)
23-26: Drop duplicate wording for crisper copyRedundant use of “identify” in the same sentence hurts readability. A quick tweak keeps the message tight:
- identify illegitimately issued certificates and identify the principal that requested the certificate, as - well as the target identity the attacker is attempting to impersonate. + identify illegitimately issued certificates and trace the principal that requested the certificate, as well + as the target identity the attacker is attempting to impersonate.packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/References.tsx (1)
23-95: Addnoreferrerfor safer external linksOpening links in a new tab should include both
noopenerandnoreferrerto prevent tab-nabbing and suppress referrer leakage.-<Link target='_blank' rel='noopener' href='https://attack.mitre.org/techniques/T1098/'> +<Link target='_blank' rel='noopener noreferrer' href='https://attack.mitre.org/techniques/T1098/'>Apply the same change to every external
<Link>in this file.packages/go/lab/loader.go (1)
166-185:STRLIST:/DECIMAL:parsers need trimming & empty-value handling
strings.Split(val, ",")returns[]string{""}for an empty input – likely not intended.- Neither branch trims outer spaces before parsing;
" 1.23 "will currently fail forDECIMAL:and leave whitespace in list items.- listVal := strings.Split(val, ",") - for i := range listVal { - listVal[i] = strings.TrimSpace(listVal[i]) - } + val = strings.TrimSpace(val) + if val == "" { + out.Set(k, []string{}) + } else { + listVal := strings.Split(val, ",") + for i := range listVal { + listVal[i] = strings.TrimSpace(listVal[i]) + } + out.Set(k, listVal) + }- if floatVal, err := strconv.ParseFloat(val, 64); err != nil { + if floatVal, err := strconv.ParseFloat(strings.TrimSpace(val), 64); err != nil {These small fixes make the new property types robust against common input quirks.
packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/General.tsx (1)
26-37: Minor grammar & readability polish
- “allow an attacker principal” → “allows an attacker”
- “allowing for authentication” → “that allow authentication”
- “This setup allow” → “This setup allows”
While purely cosmetic, tightening the language improves professionalism of on-screen help text.
packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/Composition.tsx (2)
24-29: Prefer property-access over index-access for runtime constants
EdgeInfoItemsis imported as a value. Using bracket notation (EdgeInfoItems['composition']) works only if the property literally exists at runtime.
IfEdgeInfoItemsis later refactored to anenumor type-only export, this expression collapses toundefinedafter compilation and the hook will receive an invalidtype.- type: EdgeInfoItems['composition'], + type: EdgeInfoItems.composition,
38-44: Guard against empty results
VirtualizedNodeListis rendered whenever the request resolves without error, even whennodesArray.length === 0.
Showing an empty list gives no visual cue that the edge has no composing objects. Consider displaying a short notice instead.- ) : ( - <VirtualizedNodeList nodes={nodesArray} /> + ) : nodesArray.length === 0 ? ( + <Alert severity='info'>No composing objects found for this edge.</Alert> + ) : ( + <VirtualizedNodeList nodes={nodesArray} /> )}packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/General.tsx (1)
26-28: Minor grammar tweak improves readabilityThe sentence currently omits a verb:
“Group1 the ability to write …”
Add “has” (or “has been granted”) for correct English.
- {groupSpecialFormat(sourceType, sourceName)} the ability to write to the AltSecurityIdentities attribute + {groupSpecialFormat(sourceType, sourceName)} has the ability to write to the <code>altSecurityIdentities</code> attributeAlso, the attribute is normally written in lowercase (
altSecurityIdentities) in Microsoft documentation.packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/General.tsx (2)
26-28: Missing verb breaks sentence readabilityThe sentence rendered to the UI currently reads:
“<source>the ability to write …”.
It needs a verb (“has”) to be grammatically correct.- {groupSpecialFormat(sourceType, sourceName)} the ability to write to the Public-Information property set + {groupSpecialFormat(sourceType, sourceName)} has the ability to write to the Public-Information property set
30-33: Run-on sentence & missing commaSmall grammar fix to avoid a run-on sentence:
- The Public-Information property set includes the altSecurityIdentities attribute allows you to specify + The Public-Information property set includes the altSecurityIdentities attribute, which allows you to specifypackages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/LinuxAbuse.tsx (1)
21-23: Consider declaring unused props for API-shape consistencyOther help-text components are typed
FC<EdgeInfoProps>.
Here we useFCwithout props; this is perfectly valid, but creates an inconsistent public API surface.
Either:
- Add
EdgeInfoPropseven if unused, or- Omit the unused import to avoid dead code.
No functional bug—just consistency.
packages/cue/bh/ad/ad.cue (1)
1598-1601: Relationship kind additions are complete but duplicated “ExtendedByPolicy” remainsThe new kinds (
ADCSESC16,WriteAltSecurityIdentities,WritePublicInformation) are wired into every required list – good.Unrelated, but while touching this area:
ExtendedByPolicyis declared twice (lines 1533-1541 & 1538-1546).
This pre-existing duplication can trigger CUE compile errors for duplicate symbol names.If you’re already regenerating schemas, worth cleaning now.
Also applies to: 1653-1661, 1735-1748
packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/LinuxAbuse.tsx (2)
34-39: Grammar – missing verb- The certificate must the following requirements: + The certificate must meet the following requirements:
102-104: Typo: duplicate “the”- Verify the that the mapping was added using ldapsearch: + Verify that the mapping was added using ldapsearch:packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/WindowsAbuse.tsx (2)
34-39: Grammar – missing verbSame phrasing issue as in Linux variant:
- The certificate must the following requirements: + The certificate must meet the following requirements:
110-111: Small wording tweak“Verify the that the mapping …” – appears later in file as well.
Replace duplicate “the”.packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/WindowsAbuse.tsx (1)
22-22: Consider removing unused props or documenting their purpose.The component accepts
EdgeInfoPropsbut doesn't use any of the properties. If these props are required for consistency with other help text components, consider adding a comment explaining this. Otherwise, remove the unused props.-const WindowsAbuse: FC<EdgeInfoProps> = () => { +// EdgeInfoProps maintained for consistency with help text component interface +const WindowsAbuse: FC<EdgeInfoProps> = () => {
📜 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 (47)
cmd/api/src/analysis/ad/adcs_integration_test.go(2 hunks)cmd/api/src/test/integration/harnesses.go(7 hunks)cmd/api/src/test/integration/harnesses/ADCSESC16Harness.json(1 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/cue/bh/ad/ad.cue(10 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/esc6.go(1 hunks)packages/go/analysis/ad/post.go(1 hunks)packages/go/cypher/test/cases/positive_tests.json(1 hunks)packages/go/ein/ad.go(1 hunks)packages/go/ein/incoming_models.go(1 hunks)packages/go/graphschema/ad/ad.go(7 hunks)packages/go/graphschema/common/common.go(1 hunks)packages/go/lab/loader.go(3 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/WriteAltSecurityIdentities/General.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/LinuxAbuse.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/Opsec.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/References.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/WindowsAbuse.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/WriteAltSecurityIdentities.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/General.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/LinuxAbuse.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/Opsec.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/References.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/WindowsAbuse.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/WritePublicInformation.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx(4 hunks)packages/javascript/bh-shared-ui/src/edgeTypes.tsx(2 hunks)packages/javascript/bh-shared-ui/src/graphSchema.ts(9 hunks)packages/javascript/bh-shared-ui/src/views/TierManagement/Details/Details.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/TierManagement/Details/DetailsList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/TierManagement/Details/EntityInfo/SelectorList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/TierManagement/Details/MembersList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/TierManagement/Details/utils.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/TierManagement/Summary/SummaryList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/TierManagement/TierManagement.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/javascript/bh-shared-ui/src/views/TierManagement/Details/utils.tsx
🧰 Additional context used
🧬 Code Graph Analysis (16)
packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/WriteAltSecurityIdentities.tsx (1)
packages/go/graphschema/ad/ad.go (1)
WriteAltSecurityIdentities(125-125)
packages/go/ein/ad.go (2)
packages/go/ein/incoming_models.go (3)
CARegistryData(148-155)RPCEncryptionEnforced(138-141)DisabledExtensions(143-146)packages/go/graphschema/ad/ad.go (2)
RPCEncryptionEnforced(263-263)DisabledExtensions(265-265)
packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/WindowsAbuse.tsx (1)
packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
EdgeInfoProps(142-151)
packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/WindowsAbuse.tsx (1)
packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
EdgeInfoProps(142-151)
packages/go/analysis/ad/post.go (1)
packages/go/graphschema/ad/ad.go (1)
ADCSESC16(114-114)
packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/LinuxAbuse.tsx (1)
packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
EdgeInfoProps(142-151)
packages/go/lab/loader.go (2)
packages/go/dawgs/graph/kind.go (1)
StringsToKinds(154-162)packages/go/graphschema/ad/ad.go (2)
NodeKinds(1134-1136)Entity(28-28)
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(110-231)
packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/WritePublicInformation.tsx (1)
packages/go/graphschema/ad/ad.go (1)
WritePublicInformation(126-126)
packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/General.tsx (1)
packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
EdgeInfoProps(142-151)
packages/go/ein/incoming_models.go (1)
packages/go/graphschema/ad/ad.go (2)
RPCEncryptionEnforced(263-263)DisabledExtensions(265-265)
packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
packages/go/graphschema/ad/ad.go (3)
ADCSESC16(114-114)WriteAltSecurityIdentities(125-125)WritePublicInformation(126-126)
packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/General.tsx (1)
packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
EdgeInfoProps(142-151)
packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/LinuxAbuse.tsx (1)
packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
EdgeInfoProps(142-151)
packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/General.tsx (1)
packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
EdgeInfoProps(142-151)
packages/javascript/bh-shared-ui/src/components/HelpTexts/ADCSESC16/ADCSESC16.tsx (1)
packages/go/graphschema/ad/ad.go (1)
ADCSESC16(114-114)
🪛 Gitleaks (8.26.0)
packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/LinuxAbuse.tsx
99-99: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
121-121: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 golangci-lint (1.64.8)
cmd/api/src/analysis/ad/adcs_integration_test.go
[error] 30-30: ST1019(related information): other import of "github.com/specterops/bloodhound/graphschema"
(stylecheck)
🔇 Additional comments (39)
packages/javascript/bh-shared-ui/src/views/TierManagement/Summary/SummaryList.tsx (1)
1-15: License header addition looks correctHeader matches project convention and includes SPDX identifier.
No functional impact; please ensure the linter / build pipeline has been updated (if necessary) to recognise the new header length to avoid max-lines warnings.packages/javascript/bh-shared-ui/src/views/TierManagement/Details/Details.tsx (1)
24-24: Import re-ordering only – LGTMMoving
TierActionBarhigher keeps related Tier-Management imports grouped. Nothing else changed.packages/javascript/bh-shared-ui/src/views/TierManagement/Details/DetailsList.tsx (1)
31-31: Combined util imports are clearerCollapsing two imports into one reduces noise. 👍
packages/javascript/bh-shared-ui/src/views/TierManagement/Details/EntityInfo/SelectorList.tsx (1)
23-23: Consolidated import – good housekeepingMerging into a single import keeps the file tidy and prevents duplicate bindings.
cmd/api/src/test/integration/harnesses/esc6a-template2.json (1)
112-114: Confirm downstream code reads the newIsUserSpecifiesSanEnabledCollectedflagThe harness now carries both
IsUserSpecifiesSanEnabledand the newIsUserSpecifiesSanEnabledCollectedproperties.
Double-check that analysis / post-processing consumes the Collected variant exclusively; if both exist, remove the legacy key in fixtures to avoid ambiguity.cmd/api/src/test/integration/harnesses/esc6a-template1.json (1)
111-115: LGTM – property addition is consistentAdding
IsUserSpecifiesSanEnabledCollectedaligns the harness with the updated post-processing logic. No concerns.packages/go/analysis/ad/ad.go (1)
572-576: Case forADCSESC16wired in correctlyThe switch now routes
ADCSESC16edges toGetADCSESC16EdgeComposition, matching the other ESC handlers. Looks good.cmd/api/src/test/integration/harnesses/esc6a-eca.json (5)
131-134: IncludeIsUserSpecifiesSanEnabledCollectedflag on EnterpriseCA0 node.Adding this property on the first
EnterpriseCAnode ensures the harness can differentiate between the original SAN-enabled state and the collected state for testing.
227-231: Set collected flag to false for EnterpriseCA1.Explicitly marking
IsUserSpecifiesSanEnabledCollected: "False"here captures the scenario where SAN collection was disabled.
329-332: Include collected flag on EnterpriseCA3 node.Consistently adding
IsUserSpecifiesSanEnabledCollectedsupports the ESC6a post‐processing logic in the integration test.
429-432: Include collected flag on EnterpriseCA4 node.This ensures the harness fully exercises the code path when SAN‐enabled data is collected.
529-532: Include collected flag on EnterpriseCA5 node.Adding this property completes the setup for all
EnterpriseCAnodes in the ESC6a harness.packages/javascript/bh-shared-ui/src/edgeTypes.tsx (2)
96-100: Register new advanced object manipulation edge types.The additions of
WriteAltSecurityIdentitiesandWritePublicInformationunder Advanced Object Manipulation align with the updated schema and backend support.
114-118: AddADCSESC16to ADCS certificate services.Including
ADCSESC16in the Active Directory Certificate Services subcategory reflects the new post‐processing relationship and ensures it appears in the UI.packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/WritePublicInformation.tsx (1)
17-31: ExposeWritePublicInformationhelp texts.Aggregating the
General,WindowsAbuse,LinuxAbuse,Opsec, andReferencescomponents into theWritePublicInformationobject correctly wires up the new UI documentation.packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/Opsec.tsx (1)
20-31: Add operational security guidance for WritePublicInformation.The
Opseccomponent concisely explains defender detection strategies via the issued‐certificates store.packages/go/ein/ad.go (2)
1167-1171: Include RPCEncryptionEnforced registry property
The newRPCEncryptionEnforcedblock correctly parses and adds the boolean topropMap.
1172-1176: Include DisabledExtensions registry property
TheDisabledExtensionsblock properly parses and attaches the string slice topropMap. Ensure downstream consumers handle this raw slice as expected.cmd/api/src/test/integration/harnesses.go (5)
3615-3617: Approve addition of collected property and update call for EnterpriseCA1
The newIsUserSpecifiesSanEnabledCollectedflag is set and the node is persisted withUpdateNode.
5069-5071: Approve addition of collected property and update call for EnterpriseCA
Adding and persistingIsUserSpecifiesSanEnabledCollectedhere is consistent with other harness setups.
5187-5189: Approve collected property on new EnterpriseCA and updateNode call
The harness now correctly tracks and persists the collected SAN-enabled flag.
5355-5379: Approve bulk addition of collected property and update for multiple EnterpriseCA nodes
All EnterpriseCA instances (0,1,3,4,5) now include theIsUserSpecifiesSanEnabledCollectedflag and are updated viaUpdateNode.
5482-5484: Approve collected property and update call for EnterpriseCA in Domain scenario
Consistent with other harness routines, the new property is set and persisted.packages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsx (1)
22-23: Imports and registry update look correctThe new help-text modules are properly imported and registered in
EdgeInfoComponents; no naming or typing inconsistencies spotted.Also applies to: 131-140, 260-277
packages/go/ein/incoming_models.go (1)
138-156: Struct additions align with existing styleNew registry-data wrappers (
RPCEncryptionEnforced,DisabledExtensions) mirror prior patterns and slot cleanly intoCARegistryData. No functional or tag discrepancies detected.packages/go/graphschema/common/common.go (1)
42-47: Lists are hand-edited in a generated file – verify generator alignment
common.gois tagged “Code generated … DO NOT EDIT”, yet the new kinds were appended manually. Ensure the underlying CUE schema / generator is updated; otherwise future regen will overwrite these additions.packages/cue/bh/ad/ad.cue (1)
973-999: Property enum additions look correct – remember to update collectorsThe four new CA registry properties are declared & added to
Properties; string representations follow the established lower-snake convention.
Double-check that:
- The ingest/collector code emits exactly these representation strings.
- There are matching Go
graphschemaconstants (appears so from other diffs).LGTM otherwise.
packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/LinuxAbuse.tsx (1)
56-64: Minor wording & link placement“The last requirement does not have to be met if a DC has UPN mapping disabled (see link).”
Consider moving the link outside the parentheses for cleaner sentence flow, but not critical.packages/javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/WindowsAbuse.tsx (1)
65-131: Well-structured abuse instructions.The step-by-step guide is clear and comprehensive, with proper command examples and explanations for each phase of the attack.
packages/javascript/bh-shared-ui/src/components/HelpTexts/WritePublicInformation/LinuxAbuse.tsx (1)
97-122: Command examples are appropriate.The static analysis warnings about API keys are false positives. The flagged values are LDAP distinguished names and SHA1 hash placeholders used in command examples, not actual secrets.
🧰 Tools
🪛 Gitleaks (8.26.0)
99-99: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
121-121: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
packages/javascript/bh-shared-ui/src/graphSchema.ts (4)
143-143: New relationship kinds properly integrated.The new ADCS ESC16 and write permission relationship kinds are correctly added to the schema.
Also applies to: 154-155
299-300: Display mappings correctly implemented.The display function properly handles the new relationship kinds with appropriate user-friendly names.
Also applies to: 321-324
342-342: Arrays properly updated with new relationships.The new relationship kinds are correctly added to both the edge composition and pathfinding arrays.
Also applies to: 805-805, 814-815
480-483: New properties correctly implemented.The RPC encryption and disabled extensions properties follow the established pattern with both value and collection status flags.
Also applies to: 749-756
packages/go/analysis/ad/esc6.go (2)
38-81: Excellent refactoring of ESC6a post-processing.The changes improve the function by:
- Adding proper collection status validation before checking the actual value
- Simplifying the logic by directly computing enrollers and submitting jobs
- Reducing memory overhead by eliminating intermediate bitmap operations
83-128: Consistent refactoring of ESC6b post-processing.The changes mirror the improvements in ESC6a while preserving the scenario-specific UPNCertMapping validation.
packages/go/graphschema/ad/ad.go (3)
114-114: Generated constants correctly match TypeScript schema.The new relationship kinds and properties in the Go schema are consistent with the TypeScript definitions.
Also applies to: 125-126, 263-266
270-270: Property handling functions properly updated.The generated parsing and utility functions correctly handle the new properties.
Also applies to: 536-543
1112-1112: Relationship arrays comprehensively updated.All relevant relationship arrays include the new kinds, maintaining consistency across the schema.
Also applies to: 1115-1115, 1118-1118, 1121-1121, 1124-1124
...javascript/bh-shared-ui/src/components/HelpTexts/WriteAltSecurityIdentities/WindowsAbuse.tsx
Outdated
Show resolved
Hide resolved
JonasBK
force-pushed
the
adcs-update
branch
3 times, most recently
from
a057a07 to
1a4feb1
Compare
Description
Describe your changes in detail
Motivation and Context
Resolves <TICKET_OR_ISSUE_NUMBER>
Why is this change required? What problem does it solve?
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Enhancements
Documentation
Style