[bug] Fix key material node collision across accounts with empty Identifier (#7)#8
Merged
Merged
Conversation
…#7) When `kc.Identifier` is empty, `keyNodeId` previously evaluated to `".<NodeKind>"` and collapsed every empty-Identifier credential of the same key type into a single shared graph node, falsely linking unrelated accounts as sharing key material. Fall back to the per-credential `keyCredentialNodeId` (which itself includes the owning account's DN) so the key material node is unique per credential when no Identifier is available.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked Issue
Closes #7
Root Cause
ParseResultsalready handled an emptykc.Identifierfor the KeyCredential node by falling back to a unique id derived from the owning account ("Unknown-<n>." + distinguishedName, seeparse.go:52–57). The matching code path for the KeyMaterial node atparse.go:163did not have this fallback:When
kc.Identifier == "", every key material of a given type collapsed onto the same id (".KeyCredentialRSAPublicKey", etc.). The next line then reuses any existing node with that id, so every unrelated account producing an empty-Identifier credential of the same key type ended up wired (viaHasKeyMaterial) to the single first node. The README's "shared key material" Cypher query therefore reported false positives for these credentials.Fix Description
Use the same fallback shape as the KeyCredential branch: when
kc.Identifieris empty, derivekeyNodeIdfrom the credential-scopedkeyCredentialNodeId(which already encodes the owning DN) rather than the global identifier. The non-empty path is unchanged so legitimate sharing of key material across accounts — when two credentials genuinely have the same Identifier — continues to produce a single shared node, as the README's Cypher query intends.How Verified
Static: the patched
parse.go:163–168mirrors the empty-Identifier handling atparse.go:52–57, so:kc.Identifier:keyNodeId == kc.Identifier + "." + nodeKind(unchanged).kc.Identifier:keyNodeId == keyCredentialNodeId + "." + nodeKind == "Unknown-<idx>." + distinguishedName + "." + nodeKind, which is unique per credential per account.Runtime:
go build -o KeyCredentialHound .builds cleanly; help output unchanged; the binary's runtime path up to the LDAP step is unaffected.Test Coverage
None: the repository has no Go test suite. The change is a small, deterministic adjustment to a single string construction; the static analysis above covers both branches.
Scope of Change
parse.goRisk and Rollout
Local to
ParseResults. The only behavioral difference is that empty-Identifier key materials are no longer merged. Existing exported graphs from previous versions may show fewer falseHasKeyMaterialoverlaps after re-running the collector; consumers re-running their saved Cypher queries should see fewer spurious shared-key-material findings.