[BED-5838] NTLM registry processing updates - #1440
Conversation
…y don't; skip CoerceAndRelayNTLMToSMB edge gen to computers where RestrictOutboundNTLM is enabled
3fabc0d to
f6f8d46
Compare
There was a problem hiding this comment.
What is going on that's causing this file to drop libraries?
There was a problem hiding this comment.
Nothing exciting going on in this folder - mostly copy/paste from a CitrixRDPConfiguration component that affects the same behavior.
This does show how difficult it is though to add new BH configuration toggles, so I'll want to simplify the configutation toggle components to make it easier to add a new one in the future.
WalkthroughA new configuration parameter, "Restrict Outbound NTLM Default Value," was introduced across backend, database, and frontend layers. The change enables administrators to control how missing NTLM registry values are interpreted during NTLM relay analysis. Supporting logic, tests, and UI components were added or updated to propagate and manage this setting throughout the system. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant API
participant DB
User->>UI: Toggles "Restrict Outbound NTLM Default Value" switch
UI->>UI: Show confirmation dialog
User->>UI: Confirms change
UI->>API: PUT /configuration with new value
API->>DB: Update parameter in database
DB-->>API: Ack
API-->>UI: Success response
UI->>User: Update switch state
sequenceDiagram
participant Daemon
participant API
participant DB
Daemon->>API: Request NTLM post-processing
API->>DB: Get "Restrict Outbound NTLM Default Value"
DB-->>API: Return config value
API->>API: Use config to interpret missing NTLM registry values
API->>DB: Perform NTLM relay analysis with config
DB-->>API: Analysis results
API-->>Daemon: Return results
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (8)
✨ 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 (
|
![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 (9)
cmd/api/src/test/integration/harnesses.go (1)
8827-8827: Function signature change aligns with the PR objectives.The updated signature now accepts a nullable boolean parameter for
restrictOutboundNTLMComputerProperty, which supports testing scenarios where the RestrictOutboundNTLM property can be explicitly set or omitted. This change is aligned with the PR objective of making NTLM registry data nullable.Consider adding a doc comment to explain the purpose and expected values of this parameter:
+// Setup initializes the test harness with the necessary nodes and relationships. +// If restrictOutboundNTLMComputerProperty is non-nil, the RestrictOutboundNTLM property +// of the Computer node is set to the provided value. If nil, the property is not set. func (s *CoerceAndRelayNTLMtoADCS) Setup(graphTestContext *GraphTestContext, restrictOutboundNTLMComputerProperty *bool) {packages/javascript/bh-shared-ui/src/components/RestrictOutboundNTLMDefaultValueConfiguration/RestrictOutboundNTLMDefaultValueConfirmDialog.tsx (1)
51-55: Avoid mixing Markdown‐style back-ticks with HTML<b>tagsBack-ticks render as literal characters in HTML, so users will literally see “`Confirm`”.
If you intend to emphasise the words, drop the back-ticks (or use<code>if you really want monospace).- <Typography sx={{ fontSize: '0.75rem' }}> - Select <b>`Confirm`</b> to proceed. Changes will be reflected upon completion of next analysis. - </Typography> + <Typography sx={{ fontSize: '0.75rem' }}> + Select <b>Confirm</b> to proceed. Changes will be reflected upon completion of next analysis. + </Typography> ... - <Typography sx={{ fontSize: '0.75rem' }}> - Select <b>`Cancel`</b> to return to previous configuration. - </Typography> + <Typography sx={{ fontSize: '0.75rem' }}> + Select <b>Cancel</b> to return to previous configuration. + </Typography>packages/javascript/bh-shared-ui/src/components/RestrictOutboundNTLMDefaultValueConfiguration/RestrictOutboundNTLMDefaultValueConfiguration.tsx (1)
40-50: Minor readability tweak – considerhasPendingRequestsThe boolean describes pending operations rather than unsettled ones.
A clearer name improves self-documentation.No code change required – optional.
packages/javascript/bh-shared-ui/src/components/RestrictOutboundNTLMDefaultValueConfiguration/RestrictOutboundNTLMDefaultValueConfiguration.test.tsx (2)
123-130: Potential flake – wait for switch state after async mutation
user.click(confirmButton)triggers an asyncmutateAsynccall.
Immediately assertingpanelSwitchis checked can race the network mock (especially on slower CI).- await waitFor(() => expect(panelDialogTitle).not.toBeInTheDocument()); - expect(panelDialogDescription).not.toBeInTheDocument(); - expect(panelSwitch).toBeChecked(); + await waitFor(() => { + expect(panelDialogTitle).not.toBeInTheDocument(); + expect(panelDialogDescription).not.toBeInTheDocument(); + expect(panelSwitch).toBeChecked(); + });Apply the same pattern to the “disable” scenario below (lines 178-185).
28-35: Shared mutableserverStaterisks inter-test coupling
serverStateis mutated by each handler and reused across tests. If a test fails before itsbeforeEachruns, later tests could inherit a polluted state.Consider moving
serverStateinto eachbeforeEachblock or replacing it with MSWctx.jsonthat derives its value from the current request payload.cmd/api/src/analysis/ad/ntlm_integration_test.go (2)
45-75: Enable sub-test concurrency for faster suitesThese sub-tests are completely independent – adding
t.Parallel()at the top of eacht.Runbody speeds execution and matches Go’s testing idioms.t.Run("NTLMCoerceAndRelayNTLMToADCS Success - Restrict Outbound NTLM: false", func(t *testing.T) { t.Parallel() // 💡 run sub-test concurrently ... })Repeat for the remaining sub-tests where harness isolation is guaranteed.
199-218: Deferredoperation.Done()without error inspection
operation.Done()returns an error you correctly ignore earlier withrequire.NoError, but if a panic happens betweenSubmitReadercompletion and here, the call may be skipped.A safer pattern is:
defer func() { require.NoError(t, operation.Done()) }()Not critical, yet increases robustness.
packages/go/analysis/ad/ntlm.go (2)
62-66: Prefer an options struct over another boolean flag
NewNTLMCachenow takes a fifth boolean parameter, making the call-site hard to read (true/falseconveys no intent). Consider introducing a smallNTLMOptionsstruct (or functional options) so that future behaviour toggles can be added without turning the signature into a “Boolean-parameter soup”.type NTLMOptions struct { TreatMissingRestrictOutboundNTLMAsRestricting bool } func NewNTLMCache(ctx context.Context, db graph.Database, groupExpansions impact.PathAggregator, opts NTLMOptions) (NTLMCache, error) { ... }Call sites become self-documenting:
cache, err := NewNTLMCache(ctx, db, expansions, NTLMOptions{TreatMissingRestrictOutboundNTLMAsRestricting: cfg.FailClosed})
140-150:isRestrictingOutboundNTLMreturns both a value andErrPropertyNotFoundWhen the property is missing you already synthesise the boolean according to
missingPropertyMeansRestricting. Returning theErrPropertyNotFoundas well forces every caller to remember to special-case it (and several already don’t).
Consider swallowing that specific error inside the helper and return(bool, nil)instead:- return missingPropertyMeansRestricting, err + if errors.Is(err, graph.ErrPropertyNotFound) { + return missingPropertyMeansRestricting, nil + } + return missingPropertyMeansRestricting, errThis localises the conditional logic and simplifies callers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
cmd/api/src/analysis/ad/ntlm_integration_test.go(5 hunks)cmd/api/src/analysis/ad/post.go(2 hunks)cmd/api/src/daemons/datapipe/analysis.go(1 hunks)cmd/api/src/database/migration/migrations/schema.sql(1 hunks)cmd/api/src/database/migration/migrations/v7.5.0.sql(1 hunks)cmd/api/src/database/parameters_test.go(2 hunks)cmd/api/src/model/appcfg/parameter.go(4 hunks)cmd/api/src/test/integration/harnesses.go(2 hunks)cmd/ui/src/views/BloodHoundConfiguration/BloodHoundConfiguration.tsx(2 hunks)packages/go/analysis/ad/ntlm.go(6 hunks)packages/go/ein/ad.go(1 hunks)packages/go/ein/ad_test.go(2 hunks)packages/go/ein/incoming_models.go(1 hunks)packages/javascript/bh-shared-ui/src/components/RestrictOutboundNTLMDefaultValueConfiguration/RestrictOutboundNTLMDefaultValueConfiguration.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/RestrictOutboundNTLMDefaultValueConfiguration/RestrictOutboundNTLMDefaultValueConfiguration.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/RestrictOutboundNTLMDefaultValueConfiguration/RestrictOutboundNTLMDefaultValueConfirmDialog.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/RestrictOutboundNTLMDefaultValueConfiguration/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/components/index.ts(1 hunks)packages/javascript/js-client-library/src/utils/config.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (9)
packages/javascript/bh-shared-ui/src/components/RestrictOutboundNTLMDefaultValueConfiguration/index.ts (1)
packages/javascript/js-client-library/src/utils/config.ts (1)
RestrictOutboundNTLMDefaultValueConfiguration(65-70)
cmd/ui/src/views/BloodHoundConfiguration/BloodHoundConfiguration.tsx (1)
packages/javascript/js-client-library/src/utils/config.ts (1)
RestrictOutboundNTLMDefaultValueConfiguration(65-70)
cmd/api/src/database/parameters_test.go (1)
cmd/api/src/model/appcfg/parameter.go (2)
RestrictOutboundNTLMDefaultValue(42-42)Parameter(57-64)
cmd/api/src/test/integration/harnesses.go (3)
cmd/api/src/test/integration/graph.go (1)
GraphTestContext(46-50)packages/go/graphschema/ad/ad.go (2)
Computer(31-31)RestrictOutboundNTLM(233-233)packages/go/ein/incoming_models.go (1)
Computer(318-338)
packages/javascript/bh-shared-ui/src/components/RestrictOutboundNTLMDefaultValueConfiguration/RestrictOutboundNTLMDefaultValueConfiguration.tsx (3)
packages/javascript/js-client-library/src/utils/config.ts (2)
RestrictOutboundNTLMDefaultValueConfiguration(65-70)parseRestrictOutboundNTLMDefaultValueConfiguration(126-133)packages/javascript/bh-shared-ui/src/providers/NotificationProvider/hooks.ts (1)
useNotifications(22-38)packages/javascript/bh-shared-ui/src/providers/NotificationProvider/actions.ts (1)
addNotification(31-44)
packages/go/ein/ad.go (2)
packages/go/graphschema/ad/ad.go (9)
RestrictOutboundNTLM(233-233)RestrictReceivingNTLMTraffic(251-251)RequireSecuritySignature(249-249)EnableSecuritySignature(250-250)NTLMMinClientSec(253-253)NTLMMinServerSec(252-252)LMCompatibilityLevel(254-254)UseMachineID(255-255)ClientAllowedNTLMServers(256-256)packages/go/dawgs/graph/kind.go (1)
String(27-29)
packages/javascript/js-client-library/src/utils/config.ts (2)
cmd/api/src/model/appcfg/parameter.go (1)
RestrictOutboundNTLMDefaultValue(42-42)packages/javascript/js-client-library/src/responses.ts (2)
GetConfigurationResponse(277-277)ConfigurationWithMetadata(270-275)
packages/go/ein/incoming_models.go (1)
packages/go/graphschema/ad/ad.go (4)
RequireSecuritySignature(249-249)EnableSecuritySignature(250-250)RestrictReceivingNTLMTraffic(251-251)ClientAllowedNTLMServers(256-256)
cmd/api/src/analysis/ad/ntlm_integration_test.go (4)
cmd/api/src/test/integration/harnesses.go (2)
HarnessDetails(9868-9980)CoerceAndRelayNTLMToSMB(8878-8899)packages/go/dawgs/ops/ops.go (2)
FetchRelationships(364-374)FetchRelationshipNodes(416-436)packages/go/analysis/post_operation.go (1)
NewPostRelationshipOperation(39-70)packages/go/analysis/ad/ntlm.go (3)
NewNTLMCache(62-109)PostCoerceAndRelayNTLMToADCS(395-483)PostCoerceAndRelayNTLMToSMB(574-611)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: run-tests
- GitHub Check: build-ui
🔇 Additional comments (26)
cmd/ui/src/views/BloodHoundConfiguration/BloodHoundConfiguration.tsx (2)
18-18: Import for RestrictOutboundNTLMDefaultValueConfiguration added correctlyThe import has been properly added alongside the existing imports from 'bh-shared-ui'.
35-35: RestrictOutboundNTLMDefaultValueConfiguration component added properlyThe component has been correctly added to the Box container alongside the existing CitrixRDPConfiguration component, maintaining consistent UI layout.
packages/javascript/bh-shared-ui/src/components/index.ts (1)
159-161: RestrictOutboundNTLMDefaultValueConfiguration exports added correctlyBoth the named exports and default export for the new component are properly added, following the established pattern used for other components in this file.
cmd/api/src/database/parameters_test.go (2)
130-141: Test for RestrictOutboundNTLMDefaultValue parameter added correctlyThe test verifies that the new parameter can be retrieved and has the expected key, name, and description. The test structure follows the established pattern of other parameter tests in this file.
151-151: Updated parameter count in TestParameters_GetAllConfigurationParameterThe assertion has been properly updated to expect 8 parameters instead of 7, accounting for the new RestrictOutboundNTLMDefaultValue parameter.
cmd/api/src/database/migration/migrations/v7.5.0.sql (1)
21-27: SQL migration for RestrictOutboundNTLMDefaultValue parameter implemented correctlyThe INSERT statement properly adds the new configuration parameter with appropriate key, name, description, and default value. The conditional WHERE clause prevents duplicate entries, and the default value of
{ "enabled": false }aligns with the PR objective of defaulting missing NTLM registry values to null or false.cmd/api/src/daemons/datapipe/analysis.go (1)
79-79: Added parameter for NTLM default value configuration looks good!The change adds
appcfg.GetRestrictOutboundNTLMDefaultValue(ctx, db)as a new parameter to thead.Postfunction call, which aligns with the PR objective of making NTLM registry data nullable and controlling how missing Restrict Outbound NTLM registry values are treated. This is a clean way to propagate the configuration through the analysis pipeline.packages/javascript/bh-shared-ui/src/components/RestrictOutboundNTLMDefaultValueConfiguration/index.ts (1)
17-19: Clean component export implementationThis module follows standard React/TypeScript patterns by re-exporting the component, allowing for cleaner imports throughout the codebase. The component being exported handles the configuration for Restrict Outbound NTLM default values, which aligns with the PR objectives.
packages/go/ein/ad_test.go (1)
213-214: Test properly updated to use pointer type for NTLM registry fieldThe test has been correctly updated to use a pointer to the
restrictSendingNtlmTrafficvariable, reflecting the change of the field's type fromuintto*uintin the model. This change supports the PR objective of making NTLM registry data nullable, allowing explicit representation of missing registry values.Also applies to: 226-226
cmd/api/src/database/migration/migrations/schema.sql (1)
719-723: Well-defined configuration parameter for NTLM default behaviorThe addition of the
analysis.restrict_outbound_ntlm_default_valueparameter is well-implemented with a clear description that explains its purpose. The default value of{ "enabled": false }is sensible, as it maintains backward compatibility by treating missing registry values as not restricting by default.cmd/api/src/analysis/ad/post.go (2)
29-29: Function signature update aligns with PR objectiveThis change adds a new boolean parameter
treatMissingRestrictOutboundNTLMPropertyAsRestrictingFlagto control how missing RestrictOutboundNTLM registry values are handled, matching the PR's goal of making NTLM registry data nullable with configurable default behavior.
45-45: Parameter correctly passed to PostNTLM functionThe new configuration parameter is properly passed to the PostNTLM function, ensuring that the NTLM relay post-processing logic can access the configured behavior for missing RestrictOutboundNTLM values.
cmd/api/src/test/integration/harnesses.go (1)
8867-8869: Correctly implements conditional property setting.The conditional logic properly handles the nullable behavior by only setting the RestrictOutboundNTLM property when a non-nil value is provided. This directly supports the PR's objective of skipping CoerceAndRelayNTLM* edge creation for computers where the RestrictOutboundNTLM value is either null or false.
packages/go/ein/ad.go (3)
84-93: Clear initialization of NTLM registry fields.Explicitly setting all NTLM properties to nil before conditional assignment is a good approach to ensure properties are properly cleared when registry values don't exist.
102-128: Proper null-safety handling with pointer fields.This implementation correctly handles the nullable NTLM registry values by checking for nil before dereferencing. Each property is only set when the corresponding field exists, which aligns with the goal of making these fields nullable.
93-93:Details
❓ Verification inconclusive
Existing comment about revisiting code.
There's a previous comment indicating this code should be revisited. Consider addressing this comment or documenting why it's being kept.
🏁 Script executed:
#!/bin/bash # Check for related comments or issues about this line git log -p -- packages/go/ein/ad.go | grep -A 3 -B 3 "ClientAllowedNTLMServers"Length of output: 0
Verify rationale for the
ClientAllowedNTLMServersnil assignmentI wasn’t able to find any prior commit or in‐code comment about revisiting this line. Please double-check whether this
nilassignment is still required or if there was an earlier review note or issue:• File: packages/go/ein/ad.go line 93
itemProps[ad.ClientAllowedNTLMServers.String()] = nilIf this really needs to stay, add a brief comment (e.g.
// TODO: explain why this must remain) or link to the relevant ticket. Otherwise, consider removing it.packages/go/ein/incoming_models.go (1)
307-315: Appropriate use of pointer types for nullable registry values.Converting all fields in the
NTLMRegistryInfostruct to pointer types is the right approach for making these values nullable. This allows explicit representation of missing values (nil) versus zero values (0 or empty array).packages/javascript/js-client-library/src/utils/config.ts (4)
37-37: Configuration key follows naming convention.The new configuration key follows the existing naming pattern and is properly added to the enum.
65-70: Clean type definition for configuration value.The type definition follows the established pattern for boolean configuration parameters.
91-91: Configuration type correctly added to union.The new configuration type is properly added to the union type for ConfigurationPayload.
126-133: Parsing function follows established pattern.The parser function follows the same pattern as other configuration parsers in the file, providing a consistent API.
cmd/api/src/model/appcfg/parameter.go (5)
42-42: Configuration key defined with proper naming.The new configuration parameter key follows the established naming convention for analysis parameters.
79-79: Key correctly added to valid keys list.The new configuration key is properly added to the list of valid keys checked by the IsValidKey method.
108-109: Parameter validation added for new configuration.The Validate method is properly extended to handle the new configuration type.
231-233: Simple and clear configuration struct.The RestrictOutboundNTLMDefault struct follows the pattern used for other boolean configurations, with a simple Enabled field.
235-245: Consistent getter function implementation.The GetRestrictOutboundNTLMDefaultValue function follows the established pattern for configuration getters, including proper error handling and logging.
|
Heads up: This PR swaps the constant types #1478 for some type safety. Will likely conflict if merged after. |
Co-authored-by: mistahj67 <26472282+mistahj67@users.noreply.github.com>
2 participants
Description
Making NTLM registry data for computer nodes nullable, defaulting values to null if not exists; skipping creation of CoerceAndRelayNTLM* edges for computers where RestrictOutboundNTLM is null or false.
Motivation and Context
Resolves BED-5838
How Has This Been Tested?
Localhost, importing Computers collections files generated on ESC 10 Lab and manually edited to check expected behaviors of ntlm registry fields.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit