feat(devicepolicy): enforce npm secure-registry policy via managed ~/.npmrc block - #174
Draft
raysubham wants to merge 12 commits into
Draft
feat(devicepolicy): enforce npm secure-registry policy via managed ~/.npmrc block#174raysubham wants to merge 12 commits into
raysubham wants to merge 12 commits into
Conversation
….npmrc block
Add the package_config#npm device-policy lane: converge a StepSecurity-owned
block in the console user's ~/.npmrc so npm (and the pnpm / yarn v1 / bun tools
that read the same file) resolve packages through the tenant's secure registry,
alongside the existing VS Code ide_extension lane.
The target is a file the agent may touch as root (macOS LaunchDaemon) against a
user-controlled home, so every file operation goes through Go 1.26 os.Root with
explicit symlink-chain resolution, post-open identity re-checks, and metadata
changes on open handles (never by path) to defeat symlink/entry-swap races.
Supporting pieces:
- an INI classifier that mirrors npm's own key/value parsing (spaced forms,
inline comments, single/double-quoted and JSON-escaped keys) and fails
closed on INI sections, bare CRs, and coercible non-string quoted keys it
cannot safely reason about;
- a cross-process reconciliation lock (flock / LockFileEx) serializing
scheduled and manual cycles per guarded file;
- a per-uid ownership state store kept out of the shared unlocked cache;
- reconciler seams (Converged / Render / ProbeExpected / RestoreSnapshot /
OwnsByMarker / State / ...) that leave the settings.json path byte-identical
when unset;
- a content-aware MDM probe, transactional write/clear with snapshot rollback,
and bounded, identity-checked backups.
Gated behind the existing device-policy feature gate.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
… convergence Remove the bespoke cross-process reconciliation lock (npmrc_lock.go, AcquireReconcileLock via flock/LockFileEx) from the package_config#npm enforce path. The process-wide singleton lock (internal/lock, acquired inside telemetry.Run) already serializes the scan phase across every invocation and privilege mode; only the enforce step runs outside it, and that overlap is atomic-write-safe: each ~/.npmrc write is a temp+rename of a deterministically rendered block, so concurrent cycles never observe a torn file. While a policy is stable both cycles render identical bytes; only a policy transition (a key rotation, or an enforce racing a clear) that interleaves with a concurrent cycle can briefly leave the superseded value, reconverged on the next cycle (eventual consistency). This matches the VS Code ide_extension lane, which enforces the same way with no lock. - delete npmrc_lock.go and the flock / LockFileEx helpers in the platform files; - set the reconciler seams directly in runPackageConfigEnforce; - correct the state-store comments that credited the removed lock; - drop the four lock lifecycle / contention tests. Also clear the residual gosec findings in npmrc.go the way the file already handles cleanup errors: _ = on error-path Close() calls, and a #nosec G115 justification on the POSIX uid conversion.
TestFileStateStoreRoundTrip asserted the state file's mode is exactly 0600, but Windows has no POSIX permission bits: os.Chmod only toggles the write bit and Stat reports 0666 for any writable file, so the check could never hold on Windows (the store's own logic is unaffected — the state file round-trips fine). Guard the assertion with runtime.GOOS != "windows", matching the existing settings_writer_test.go idiom; the rest of the round-trip still runs on every platform.
The package_config#npm enforcement lane is ready to ship, so remove its dedicated feature gate and run it unconditionally. The gate was default-on already, so runtime behavior is unchanged; the lane stays dormant in production until the backend returns a package_config policy (an absent policy is a no-op, never a wipe). The IDE-extension lane keeps its own FeatureDevicePolicy gate. Removes the FeaturePackageConfigPolicy const, its enabled-map entry, and the gate check in runPackageConfigEnforce.
There was a problem hiding this comment.
Pull request overview
Adds a new device-policy enforcement lane for package_config/npm that converges a StepSecurity-managed secure-registry block in the console user’s ~/.npmrc, reusing the existing devicepolicy reconciler via new seams and a separate per-user/per-mode applied-state store.
Changes:
- Introduces an
~/.npmrcmanaged-block writer (cross-platform + race-resistant I/O) and wires it into enterprise telemetry cycles. - Extends
devicepolicy.Reconcilerwith optional seams (rendering, convergence/probing, rollback, marker-based ownership, state store injection) to support non-settings.jsontargets without changing IDE behavior. - Adds a dedicated
StateStoreimplementation forpackage_configand hardens the policy fetcher against category/target mismatches.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/devicepolicy/verify.go | Adds package_config / npm identifiers for policy fetch/report scoping. |
| internal/devicepolicy/statestore.go | Implements a per-target state-store abstraction and a file-backed store for package-config applied state. |
| internal/devicepolicy/statestore_test.go | Unit tests for state-store placement, schema handling, corruption behavior, and mode expectations. |
| internal/devicepolicy/reconcile.go | Adds reconciler seams (render/probe/converged/rollback/state store) to support npmrc lane while preserving IDE defaults. |
| internal/devicepolicy/reconcile_npm_test.go | Exercises reconciler behavior with npm-specific seams and marker-based ownership semantics. |
| internal/devicepolicy/npmrc.go | Adds NPMRCWriter: secure managed-block convergence for ~/.npmrc, including rendering/validation, probing, backups, and rollback. |
| internal/devicepolicy/npmrc_unix.go | Unix-specific metadata enforcement (mode/owner), nonblocking open flag, and owner reader implementation. |
| internal/devicepolicy/npmrc_windows.go | Windows-specific session/identity guard and no-op metadata enforcement/owner checks. |
| internal/devicepolicy/npmrc_unix_test.go | On-disk Unix tests for writer behavior, symlink handling, metadata enforcement, and rollback semantics. |
| internal/devicepolicy/npmrc_test.go | Pure tests for rendering, INI classifier behavior, rewrite/clear transforms, and probe logic. |
| internal/devicepolicy/api.go | Rejects run-config responses whose category/target don’t match the request to prevent mis-scoped enforcement/clear. |
| internal/devicepolicy/api_identity_test.go | Tests the new category/target identity checks in the fetch path. |
| cmd/stepsecurity-dev-machine-guard/main.go | Wires package-config enforcement into enterprise telemetry cycles (including when telemetry fails) and install-time behavior. |
Comments suppressed due to low confidence (1)
internal/devicepolicy/npmrc.go:1243
- Function name has a likely typo ("unquoteININToken"), which makes the intent harder to read/search for. Consider renaming to "unquoteINIToken" to match the INI terminology used throughout this file.
// unquoteININToken mirrors npm ini unsafe()'s quoted branch, which JSON-parses a
// quoted token rather than merely stripping the quotes. A double-quoted token is
// JSON-decoded whole, so string escapes resolve — `"registry"` decodes to
// `registry`, an active override we must recognize — and falls back to the
// ORIGINAL quoted string when it is not a valid JSON string (npm keeps the quoted
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+270
to
+273
| if err := os.Chmod(tmpPath, cacheFileMode); err != nil { | ||
| return err | ||
| } | ||
| return os.Rename(tmpPath, s.path) |
Comment on lines
+1206
to
+1210
| func npmUnsafe(s string) string { | ||
| s = strings.TrimSpace(s) | ||
| if inner, ok := unquoteININToken(s); ok { | ||
| return inner | ||
| } |
Brings in the merged IDE-extension policy work (step-security#178) that the npm lane's reconciler, probe, and api layers build on. reconcile.go conflicted because both lanes changed the enforce ladder; resolved by keeping both paths on the shared seams - marker-based clearing for the ~/.npmrc block writer, the multi-key managed path for VS Code settings.
…pm lane
Enforcement already selects a channel per cycle for IDE-extension policy. This
extends the same model to package_config#npm: "mdm" is verify-only — the agent
reads the effective ~/.npmrc and reports what it observed, and never writes,
patches, or clears the file (an external MDM owns the block).
- ProbeContentNPM is the verify-only reader, bound in main.go because it needs
the writer's identity-checked read path. Presence is an MDM marker outside
every agent-owned block, so a marker planted inside one of ours cannot pass as
MDM management. It reports the observed bag {ecosystem, registry_url,
auth_token_status} computed over the whole file's last-wins precedence, so an
override below the MDM block surfaces as drift instead of being hidden. No
token, hash, or fingerprint is ever returned or logged. With no writer the
reconciler's category-aware fallback reports verification_failed rather than
probing VS Code policy for an npm category.
- EffectivePolicy.Policy is kept RAW (json.RawMessage) instead of a settings
map: the compiled shape is per-category — a settings map for ide_extension, a
registry object for package_config — and each category decodes what it needs.
Values are still written verbatim, never re-serialized, so the backend's
byte-exact applied==desired check holds.
- Ownership collapses onto WrittenSettings for every lane; the single-value
WrittenValue field is retired. A single-value path records exactly one entry
under its own OwnershipKey (the ~/.npmrc block, or the allowlist setting id for
a degraded VS Code writer). A state file carrying the retired key decodes as
"owns nothing" and the next enforce re-converges it.
- ProbeManagedContent takes the expected value so a content probe can decide
presence against what the policy actually compiled to.
Hardening on the npm lane:
- The verify-only reader transmits an observed plaintext http:// registry as
drift evidence instead of refusing it — discarding it would hide the most
security-relevant drift the channel exists to catch. The observed value is
bounded instead: an absolute http(s) URL with a host, 2048 bytes, and no
userinfo, query, fragment, or control bytes.
- Fail closed on npm's `key[]=` array-append form on a managed key. npm's ini
reader folds it into the same key as the block's scalar assignment, so both
orders leave npm resolving to a comma-joined list containing the injected
value while a last-wins scalar scan still reported converged. Scoped to
`registry` and the tenant token key, so unrelated array config (`omit[]=dev`,
another registry's token) is still enforceable.
- Fail closed when clearing a file whose only line breaks are bare CRs. The
managed block could not be located, so the clear reported the nothing-to-do
success and ownership state was dropped while the token stayed on disk.
- Purge our own backups once a clear leaves nothing managed. Removing the block
from the live file while leaving a readable sibling copy of it revokes nothing.
Best-effort with a retry on later no-op clears: a sibling we cannot unlink must
not report a revocation that already happened as failed.
- Log a loose file mode in the verify-only reader rather than failing it. Perms
are outside the observed contract and that lane cannot remediate by writing, so
a hard failure would be permanent and would hide the real registry and auth
status. Ownership is still enforced on every read.
…locked state file The npm lane had its own per-mode/per-user ownership store (statestore.go, Reconciler.State) so its record could not share the IDE file's unlocked read-modify-write. That solved the lost update by splitting the file, which costs the property the state file exists to have: one place that answers "what has this agent written". A root cycle and a user cycle could each hold a different partial truth, and nothing could enumerate ownership across categories. Inverted: every category now reads and writes device-policy-state.json keyed by (category, target), and the lost update is prevented by locking instead of by separation. - StateStore, fileStateStore, NewStateStoreFor, Reconciler.State and NPMRCWriter.TargetUser are deleted. readState/persistState/dropState call the package-level accessors directly; the writeState/clearState test seams stay. The IDE wiring set no State, so its behavior is byte-identical. - statelock.go adds the cross-process lock the shared file now needs: an advisory lock on a sibling device-policy-state.json.lock, held only for the read-modify-write. The lock file is a rendezvous point, never a record — created empty, left empty, and deliberately not unlinked on release, since unlinking would let a peer holding the old inode take a lock nobody else can see. flock(2) on POSIX, LockFileEx on Windows; both are owned by the open file description, so a crashed agent cannot wedge state. - Acquisition fails closed. A lock that cannot be taken fails the operation rather than proceeding unlocked: "busy" is precisely the state in which a concurrent writer is proven to exist, and running anyway is the one case that can actually drop another category's record. The caller reports write_failed and the next cycle retries; for npm the enforce preflight persists through the same locked accessor before the settings file is touched, so contention is normally caught with nothing written and nothing to roll back. The wait budget is 10s — the critical section is microseconds, so the budget only has to absorb a rename serialized behind on-access antivirus, a slow network home, or a descheduled peer without mistaking one for a wedged process. The single waiver is a filesystem that does not implement locking at all (ENOTSUP/EOPNOTSUPP, ERROR_NOT_SUPPORTED/ERROR_INVALID_FUNCTION): there no peer can hold a lock either, so there is no race to lose, while failing closed would leave state permanently unpersistable on a network or FUSE home. Deliberately excluded are two errnos that look like they belong there and do not — ENOLCK is exhausted kernel lock records on a mount where peers may already HOLD locks, and EINVAL is an invalid request; both fail closed. - An unreadable state file is no longer treated as absent. readStateFile is now three-valued: absent-or-corrupt (nothing interpretable to lose, so a write recreates and a clear's remove already takes ENOENT as success) versus unreadable — present, but its bytes were never obtained. The mutators refuse the latter and return the read error, because on POSIX a file we cannot read is still rewritable and unlinkable through a writable parent: an npm clear would have removed live IDE ownership, and both paths returned nil, reporting the loss as success. The realistic trigger is not a mode-000 toy but a root-owned 0600 state file inside a user-owned directory. ReadAppliedState keeps its no-error contract and still reads unreadable as "owns nothing", which is the safe direction: the reconciler re-applies rather than clearing what it has no record of writing. - settings.json writes now preserve a leading UTF-8 BOM. load splits it off before both the blank check and the parse, and store re-prepends it. A BOM is neither whitespace to bytes.TrimSpace nor a valid start of a JSON value, so a seeded file carrying one read as unparseable and stayed that way permanently: the only way to remove the bytes was the write the never-clobber contract then refused to attempt. PowerShell 5.1's Set-Content -Encoding UTF8 and editors set to utf8bom both emit it. stripBOM is now shared with the ~/.npmrc writer. Also corrects the enforceSingle adoption comment, which credited a per-mode store for the "converged on disk but not in our record" case. With one file the remaining causes are a stale or hand-removed record, or a cycle that resolved a different home for the state file.
…ument its exclusions lockUnavailable is the one place the state lock's fail-closed rule is waived: an error classified there runs the read-modify-write unlocked. The POSIX list has a table test pinning it. The Windows list had none on any platform — a _windows_test.go file does not compile on a POSIX host, and no such file existed for the native job to run — so a widened carve-out would have silently reintroduced the lost update the lock exists to prevent. The plausible way in is a field fix: a managed volume answers LockFileEx with "Access is denied.", and the error gets added to the waiver to stop the noise. - statelock_windows_test.go mirrors the POSIX table, including the three near misses that correspond to ENOLCK and EINVAL. ERROR_NO_SYSTEM_RESOURCES and ERROR_NOT_ENOUGH_MEMORY are a lock record the kernel could not allocate on a volume where peers may already hold locks; ERROR_INVALID_PARAMETER is a malformed request, which says nothing about whether the volume can lock. ERROR_LOCK_VIOLATION is asserted as well, even though tryLockHandle filters contention before it can reach the function: it is the one error that proves a peer holds the lock, making it the worst possible thing to waive, and the assertion is what keeps the two classifications from being merged. The test runs in the existing windows-latest devicepolicy job, so there is no CI change. - The Windows doc comment gains the "here is what looks like it belongs and does not" paragraph the POSIX one already carried. That asymmetry was the real rot surface, and it is the half that acts first: the comment sits where the edit happens, whereas a test can only redden afterwards in CI, named in the failure and mirroring the line just changed. The assertions have not executed yet. There is no Windows host on the dev box, so this was verified by cross-compiling the test binary (GOOS=windows go test -c) and by swapping one constant for a bogus name to confirm the file is genuinely in the Windows test build rather than tag-excluded. First real run is CI.
…owing gosec flags int(f.Fd()) in tryLockHandle and unlockHandle as an integer overflow conversion, which reddened the PR's code-scanning gate with two alerts it cannot resolve on its own. Fd() returns a live *os.File descriptor: a small non-negative int widened to uintptr, so narrowing it back cannot overflow. The one edge value is the ^uintptr(0) a closed file returns, and that converts to -1, which flock rejects with EBADF — an error tryLockHandle already fails closed on. Annotated with the #nosec idiom the package already uses rather than reworked to SyscallConn: int(f.Fd()) is the canonical argument form for syscall.Flock, and routing it through a RawConn.Control callback would change how the descriptor is obtained purely to satisfy a static-analysis false positive. Verified with gosec v2.26.1, the version the workflow pins: devicepolicy now reports no findings, and none of the findings remaining elsewhere in the tree sits in a file this PR touches.
…s removed handleClearByMarker logged "cleared managed block at <path>" on every clear, including the ones that removed nothing. Clear itself is deliberately unconditional there — marker ownership is intrinsic to the writer, so a lost or corrupt ownership record must never be the reason a token-bearing block survives an unassignment — but the log inherited that unconditionality and claimed a removal regardless. An unassigned device therefore announced a clear on every cycle forever, which in fleet logs reads as a device being remediated over and over when it has been clean for weeks. "Did anything change" cannot be inferred at the call site: the whole point of calling Clear unconditionally is that the caller does not know. Both writers already computed it and discarded it — NPMRCWriter has two no-op paths (absent file, no block and no prefixed lines) and settingsWriter has two (absent file, key not present) — so Clear now returns it. The call stays unconditional; only the description of what happened changed. - Writer.Clear becomes Clear() (changed bool, err error), documented so a future caller does not read the bool as permission to skip the call. - handleClearByMarker logs a removal or a nothing-to-remove, matching the distinction the settings lane already drew between "cleared N agent-owned key(s)" and "holds no agent-owned value; leaving it". - clearSingle no longer claims a removal when Clear turned out to be a no-op, which is reachable if the value disappears between the read and the clear. - rollbackWrite ignores the bool: a rollback to "absent" is complete whether or not there was anything to remove. Both lanes get the three-way contract pinned — absent file, present-but-nothing- of-ours, and a real removal — because only the last may be reported as a removal, and the middle case is the one that regressed silently. Verified the assertions bite by inverting the success return and watching the live-block subtest fail.
… relies on The AppliedTargetState comment described how a file carrying the retired written_value key decodes, and asserted that "the next enforce re-converges and re-records it". That is not true on the settings lane: a device whose target is already converged with an unchanged hash short-circuits before persisting anything, so there is no next write to repopulate the record. Left in place, the sentence invites someone to add a legacy decode believing it repairs itself, when the actual outcome would be a value-based clear that silently declines to remove keys the agent did write. The shape it describes exists on no device, and the baseline is the next release, so there is nothing to migrate and no reason to document a migration. Replaced with the property that actually makes the short-circuit safe: AppliedHash and WrittenSettings are persisted as one struct in one write, so a record whose hash matches the freshly-fetched hash necessarily carries the complete ownership map for that policy — a matching hash cannot coexist with partial ownership. Also names the one way to break the pairing, a hand-edited state file, and says plainly that it is outside the supported inputs and what it would cost. Comment only; no behavior change.
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.
Summary
Adds the
package_config#npmdevice-policy enforcement lane: the agent converges a StepSecurity-owned managed block in the console user's~/.npmrcso npm (and the pnpm / yarn v1 / bun tools that read the same file) resolve packages through the tenant's secure registry. It runs alongside the existing VS Codeide_extensionlane and reuses the same reconciler, driven through nil-able seams so the settings.json path stays byte-identical.What's included
~/.npmrcmanaged-block writer (npmrc.go,npmrc_unix.go,npmrc_windows.go) — operates on a user-owned tree the agent may touch as root (macOS LaunchDaemon). Every file op goes through Go 1.26os.Rootwith explicit symlink-chain resolution, post-open identity re-checks (Lstat + SameFile), and metadata changes on open handles only — never by path — to defeat symlink / directory-entry-swap races. Transactional write/clear with snapshot rollback, and bounded, identity-checked, 0600 token-bearing backups.[section]headers, bare\rline breaks, and single-quoted non-string JSON keys npm would coerce into an override.telemetry.Runreleases the process-wide singleton lock, so two cycles can overlap only in the write step. Each write is an atomic temp+rename of a deterministically rendered block: identical bytes while the policy is stable, and only a self-healing stale-value window if a policy transition (key rotation / enforce-vs-clear) interleaves with a concurrent cycle, reconverged next cycle. Same model as the lock-free VS Code lane.statestore.go) kept out of the shared device-policy cache so an IDE reconcile in another process can't drop the npm record — separate files make that cross-process lost update structurally impossible without a lock.reconcile.go:Converged/Render/ProbeExpected/RestoreSnapshot/OwnsByMarker/State/WriterInitErr) — every seam at its zero value reproduces the settings.json behavior byte-for-byte.~/.npmrcis user-writable, a bare marker isn't proof; the probe verifies the MDM lane's block is present, effective (last-wins), and correctly owned.Security model
A user who can plant symlinks or swap directory entries mid-operation must not be able to steer a root-owned write, a root read, or a token-bearing backup outside their own regular
~/.npmrc. Token material (<api_key>::dev:<serial>) is never logged, and offboarding removes every managed block — including duplicates — so no live token survives a clear.Testing
go test ./...— all packages pass, including-race.go vet+gofmtclean; builds green on darwin / linux / windows.Notes
package_configpolicy (which ships after this agent release), so the reconciler no-ops until then — an absent or transient policy is always a no-op, never a wipe. The IDE lane keeps its ownFeatureDevicePolicygate.npmrc_lock.go, flock / LockFileEx); it was removed (commit62d9e78) once it was clear the telemetry singleton lock already serializes the scan phase and the enforce overlap is atomic-write-safe. Accepted residual: a narrow, self-healing stale-value window when a policy transition overlaps a concurrent cycle — bounded to one lost cycle, never a corrupt file. Removing the lock also cleared its gosec findings; the remainingnpmrc.gocleanup-error and uid-conversion findings are handled in-convention (_ =/#nosec G115).Opening as a draft for review.