feat(credentials): add credential inventory scanning - #186
Open
raysubham wants to merge 8 commits into
Open
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
raysubham
force-pushed
the
feat/credential-inventory
branch
from
August 5, 2026 19:55
4a34d19 to
cf3d5a3
Compare
raysubham
changed the title
raysubham
changed the title
Adds a credential-location inventory phase: which developer tools on this machine hold credentials, where, and how well guarded each location is. It reports locations and protection, never the credential — no value, substring, digest, fingerprint, whole-file hash, or any category derived from the secret's characters reaches the wire, a row, a log, or an error. - internal/detector/credentials (new): sixteen sources across cloud, source control, containers, package registries, infrastructure and AI/MCP config. Exact catalog paths only, never a directory walk; every read byte-capped and the parse stops at the cap; a capped read is recorded as incomplete rather than as a clean file. Findings carry a tokenized location ($HOME/$APPDATA/ $XDG_CONFIG_HOME/$ABS), protection state, mode, size, mtime and git context. Locations moved by an environment variable are resolved per user, and a variable that is set but unresolvable marks only the sources that read it. - internal/safepath (new): the resolver every read in the phase goes through. Per-component os.Lstat plus os.Readlink rather than EvalSymlinks, an openat chain with O_NOFOLLOW|O_DIRECTORY and O_NONBLOCK on the leaf, the consent guard consulted before each syscall, and containment against the OS user record checked at every hop — so a path that leaves the user's roots is refused before the components below it are stat'd. - internal/model/credentials.go (new): the wire types, with a committed golden fixture and a strict round-trip test. The reader on the other end discards fields it does not know, so a renamed field would stop arriving silently rather than fail; the fixture exercises every protection state, every reason code, both host-report shapes, both authentication and storage vocabularies in full, and all four location roots. - GitHub CLI hosts: authentication status, token storage and observed scopes per configured host. The tool's own verdict strings are translated to this inventory's vocabulary in one place rather than forwarded, and gh output is never logged. - detector/mcp.go: DetectKnownUserConfigs returns the known user-level MCP config paths with no discovery and no directory walk, resolved against the developer's home and roaming profile rather than the service account's. - scan + telemetry wiring, a one-minute phase budget, and the Windows-native test job extended to cover the two new packages. A nil section means the phase did not run; a section with zero findings is the positive assertion that no known location holds a credential, so the pointer is passed through untouched rather than defaulted.
The job covers three packages, not only devicepolicy, so the identifier now matches the display name it already carried. No dependent job or required status check references the old identifier.
None of the table-driven format parsers can report a protected credential; that verdict comes only from key classification, which asserts through its own table. The shorthand therefore had no caller and tripped the unused check.
An environment override was expanded ahead of the catalog defaults instead of in place of them, and every one of these tools stops reading its default path once the variable is set. Two ways that surfaced on a live host: an override naming a real file reported that file and the default, and an override naming a path that does not exist fell through to the default entirely, because stopping at the first match keyed off the first candidate that produced a finding and an absent one produces none. Both are a row naming a real file at a real path that its tool has stopped opening. Take the first override that is set, in declaration order, expand it and stop: no later override and no catalog default, whatever the expansion came to. Being set is what displaces the default, not naming somewhere real. kubectl consults its own default only when KUBECONFIG is empty; any non-empty value is split and its empty elements are then dropped without the default coming back, so a value of nothing but separators names nowhere to look. A missing target is likewise the answer rather than a fallback, and neither is an error, because nothing failed. Only an override kind with no expansion stands aside for the default, where an over-report is the better way for an inventory to be wrong. Tokenisation also learns the filesystem's spelling of the home, which containment already admits: a home reached through a symlink is still the home, and giving its resolved form the opaque root hands one file two identities and hides the only root a reader can interpret. It is applied by respelling the path onto the account record's home and matching the roots again, not by carrying the resolved home as a second root. As a root it would match at the home and stop, taking every path below it away from the more specific roots that live there, so a redirected profile's roaming file would come back under the home token. A configuration directory spelled with the resolved home but otherwise at its default is the default, not a move, and keeps the home token too. Two claims are corrected rather than implemented. The bounded key read does not keep key material out of the process: for a file smaller than the cap the body is joined and decoded whole, private section included, so the comments now state the guarantee that holds, which is that nothing read is serialised, logged, fingerprinted, counted or retained past the classification. Lowering the cap does not recover the stronger claim either, since the hardware-key check reads past a public key an RSA modulus does not fit inside. And the consent test is renamed to say what it covers: which resolver each half of the delegated source reaches, asserted in-process against a temporary tree, which is not evidence about a scheduled run under a system account with no session to prompt in.
raysubham
force-pushed
the
feat/credential-inventory
branch
from
August 6, 2026 14:14
8d4f375 to
81ae6d7
Compare
Two are conversions gosec cannot see are bounded, and both read better without the conversion: the SSH length check now compares in int64, which a uint32 length and a slice length both widen into without wrapping, and the Windows final-path call passes its buffer size as a constant rather than converting the slice length back. The rest are annotated where they sit. Seven are the identifier-name heuristic firing on constants whose names carry "credential" or "token" and whose literals are wire vocabulary: source identifiers, a path root, reason and outcome codes, and the CLI's own word for where it keeps a token. One is the probe's subprocess, whose program is the path a PATH lookup returned for the CLI or the literal sudo, and whose arguments are package constants plus that path and the enumerated account name. The last is the descriptor openat returned, which the success path this line sits on has already established is non-negative.
There was a problem hiding this comment.
Pull request overview
Adds a new credential-location inventory capability to Dev Machine Guard, including a hardened file-open/resolution layer and new wire types, then wires the resulting snapshot into both community scan output and enterprise telemetry.
Changes:
- Introduces
internal/detector/credentialsto inventory known credential locations (fixed paths + bounded reads) and optionally enrich GitHub CLI host entries. - Adds
internal/safepathto safely resolve/open paths through verified components with root containment checks (and platform-specific open verification). - Wires the new scan section into
scanandtelemetry, adds a phase budget, extends model JSON-shape tests, and updates CI to run Windows-native tests for the new Windows-specific code paths.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/telemetry/telemetry.go | Runs the new credentials_scan phase and includes credential_scan in the enterprise telemetry payload. |
| internal/telemetry/phase_deadline.go | Adds a 1-minute budget for the new credentials_scan phase. |
| internal/scan/scanner.go | Runs credential inventory in community-mode scans and attaches it to model.ScanResult. |
| internal/safepath/safepath.go | New safe path resolver that resolves per-component and enforces containment; provides bounded reads and bounded directory listings. |
| internal/safepath/open_unix.go | Unix openat-chain implementation to prevent symlink-swapped component traversal. |
| internal/safepath/open_windows.go | Windows handle-based open verification using FILE_FLAG_OPEN_REPARSE_POINT + GetFinalPathNameByHandle. |
| internal/safepath/safepath_test.go | Unit tests covering containment, guard behavior, truncation semantics, and open verification behavior. |
| internal/model/model.go | Adds CredentialScan *CredentialScanInfo to ScanResult with omitempty semantics. |
| internal/model/credentials.go | New credential inventory wire types and closed vocabularies (categories, protection states, reason codes, GitHub host report enums). |
| internal/model/testdata/credential_scan_golden.json | Golden credential-scan payload fixture exercising the schema and vocabularies. |
| internal/model/credentials_golden_test.go | Golden round-trip + vocabulary coverage tests for credential-scan payloads. |
| internal/model/scanresult_jsonshape_test.go | Ensures credential_scan is omitted when nil (distinguishes “did not run” vs “ran and found nothing”). |
| internal/detector/mcp.go | Adds DetectKnownUserConfigs to return known user-level MCP config paths without discovery/walk. |
| internal/detector/credentials/catalog.go | Credential-source catalog (fixed locations, relocation overrides, caps, and matching policy). |
| internal/detector/credentials/detector.go | Orchestrates per-source collection, applies safepath, enforces caps, and builds findings/errors. |
| internal/detector/credentials/location.go | Path tokenization, relocation handling, git repo/tracked checks, and Windows SDDL parsing helpers. |
| internal/detector/credentials/parse.go | INI-ish parsers and core “observation” folding logic for protection/count inference. |
| internal/detector/credentials/parse_doc.go | JSON/TOML/YAML/JSONC parsing for Docker/Terraform/Kubeconfig/MCP/GCP/GH config shapes. |
| internal/detector/credentials/parse_ssh.go | SSH private key classification (OpenSSH/PKCS8/legacy/PuTTY) with header-only reading. |
| internal/detector/credentials/probe.go | GitHub CLI auth status --json hosts probe runner + parsing and env-probe helpers. |
| internal/detector/credentials/*_test.go | Unit/fuzz tests for parsers, token/env fences, and Windows-specific behavior. |
| .github/workflows/tests.yml | Extends Windows-native CI job to run tests for devicepolicy, detector/credentials, and safepath. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The probe builds its own exec.Cmd because it needs three things the shared executor cannot express: standard error discarded at the pipe rather than returned as a string, standard output collected through a bounded buffer, and an environment with the CLI's token variables removed. What it was missing is orthogonal to all three -- console-window suppression on Windows and a cancellation that reaches the process group, so a CLI left running under sudo does not survive the timeout. Those two are now HardenCommand in the executor package, which is what its own Run and RunInDir already did inline. The probe calls it and keeps building the rest of the command itself. Its wait delay stays stated locally: the group teardown is a no-op on Windows, and the bound the phase depends on has to hold on every platform. Also documents why a skipped_no_user error carries no source id. Nothing was attempted when no account resolves, so there is no source to name, and a value invented for the field would look like a catalog source and match none. The reader accepts an empty source only for this reason code, so the shape is the contract rather than an omission.
Folding Run and RunInDir onto the new helper edited the lines directly above their exec.CommandContext calls, and the subprocess findings already standing against those calls were re-reported as this branch's on the strength of that. The findings are the executor's own and predate this work; silencing them here would be suppressing a live one to unblock an unrelated change. So the helper moves to its own file and the two call sites keep their inline pair. Nothing in executor.go changes, which is what keeps its findings attributed where they belong. Folding those call sites in is worth doing when that file is being changed for its own reasons.
A file holding one function is not worth the name. The helper belongs with the other command construction in this package, and it goes at the end so the file's existing lines keep their positions and the subprocess findings already standing against them stay attributed where they belong.
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.
What does this PR do?
Adds a credential-location inventory phase: which developer tools on this machine hold credentials, where, and how well guarded each location is. It reports locations and protection, never the credential — no value, substring, digest, fingerprint, whole-file hash, or any category derived from the secret's characters reaches the wire, a row, a log, or an error.
Sixteen sources across cloud (AWS, GCP, Azure), source control (SSH keys, git credential store,
gh), containers (Docker, kubeconfig), package registries (npm, PyPI), infrastructure (Vault, Terraform) and AI/MCP configuration.New packages
internal/detector/credentials— exact catalog paths only, never a directory walk. Every read is byte-capped and the parse stops at the cap; a capped read is recorded as incomplete rather than as a clean file, so a credential sitting past the cap can never render as "read, empty, complete". Findings carry a tokenized location ($HOME/$APPDATA/$XDG_CONFIG_HOME/$ABS), protection state, mode, size, mtime and git context. Locations moved by an environment variable are resolved per user, and a variable that is set but unresolvable marks only the sources that read it — not the whole run.internal/safepath— the resolver every read in the phase goes through:os.Lstat+os.Readlinkrather thanfilepath.EvalSymlinks, so no resolution happens behind the guard's backopenatchain withO_NOFOLLOW|O_DIRECTORY, andO_NONBLOCKon the leaf (a FIFO in a credential path cannot hang the phase)~/.aws -> /Volumes/network-share/confignever touches the volumeinternal/model/credentials.go— the wire types, plus a committed golden fixture and a strict round-trip test. The reader on the other end discards fields it does not know, so a renamed field would stop arriving silently rather than fail. The fixture exercises every protection state, every reason code, both host-report shapes, both GitHub-CLI vocabularies in full, and all four location roots.Other changes
ghoutput is never logged.detector/mcp.go:DetectKnownUserConfigsreturns the known user-level MCP config paths with no discovery and no directory walk, resolved against the developer's home and roaming profile rather than the service account's. The no-walk guarantee is in the signature so a caller cannot opt out of it by accident.scan+telemetry, a one-minute phase budget, and the Windows-native test job extended to cover the two new packages (both have Windows-only code — the security descriptor behindbroad_read_allow_ace_present, the per-account registry environment read, and the reparse-point-safe leaf open).A nil section means the phase did not run. A section with zero findings is the positive assertion that no known location holds a credential — and it replaces the stored inventory wholesale, so the pointer is passed through untouched rather than defaulted.
Cross-repo contract
The consumer side is already merged in
agent-api. The payload from this branch — both the committed golden fixture and a live scan of a real machine — was validated through that repo's ownvalidateCredentialScanwith no decode failure and no rejection reason. Three items need the reader to move, and none of them block this:skipped_no_useris still rejected for an emptysource_id; the code describes the run, not a source.broad_read_allow_ace_presentis tri-state here (*bool) and collapses tobool,omitemptythere, merging "evaluated, nothing broad" into "not evaluated".duration_msis unmodelled there; it is carried by every scan section in this repo, so it stays.Type of change
Testing
./stepsecurity-dev-machine-guard --verbose./stepsecurity-dev-machine-guard --json | python3 -m json.toolmake lint— 0 issuesmake testAdditional:
make smoke— 44/44-racegreen on all seven touched packages; 153 top-level tests / 234 subtests acrosscredentials,safepathandmodelCGO_ENABLED=0builds for all six targets (linux, darwin, windows × amd64, arm64) andGOOS=windows go vet ./...cleanscan_complete=trueNote on the
--verbosebox: that path dispatches to enterprise telemetry and was short-circuited by the run gate on this box (offline_cache_skip) — it exited cleanly but did not execute the phases. The credential phase itself was exercised through--json, which is where the 13-finding result above comes from.Related Issues