feat(machineid): add shared machine id resolution and persistence - #771
Draft
basti-snyk wants to merge 8 commits into
Draft
basti-snyk wants to merge 8 commits into
basti-snyk wants to merge 8 commits into
Conversation
Adds MACHINE_ID, MACHINE_ID_SOURCE and CLIENT_MACHINE_ID so the CLI and the Language Server address one machine identifier through the same keys.
Resolves one machine identifier per machine in precedence order: stored value, external channel, OS identifier, opt-in legacy file, generated UUIDv4. Persists the value and its source to a shared file and mirrors it into snyk.json. A stored value is never overwritten; Reset is the only way to replace one.
Registers the resolver as a configuration default so every GAF consumer gets the same machine identifier from one config lookup.
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
The format was never agreed by the product owner: it was asserted from a planning-doc reference to an "alignment document" that does not exist, while the authoritative Confluence page lists format, length and allowed characters as undefined. The owner has confirmed no format validation should apply. Drop machineIDPattern, Validate and ErrInvalidMachineID, the brace-stripping normalisation, and the 128-character cap. A candidate value from any source is now adopted exactly as supplied, byte for byte, with no charset, length, or brace transformation. The precedence chain still needs to tell "a source supplied something" from "a source supplied nothing", so a new unexported hasValue helper treats an empty or whitespace-only value as absent and falls through to the next source; this is presence detection, not validation. Tests are updated to match: TestValidate becomes TestHasValue and asserts presence rather than format, and previously-rejected shapes (brace-wrapped GUIDs, over-length values, control characters) are now proven to round-trip unchanged through Resolve. No exported symbol besides Validate and ErrInvalidMachineID needed removal; neither had any caller outside this package.
The legacy device-id file format typically ends with a trailing newline as an artifact of how it was written, not part of the identifier itself. Trim trailing whitespace when the legacy file supplies a value so a stored newline does not leak into the resolved machine id. This is scoped to the legacy file path only. The existing stored value, shared file, external channel, and OS identifier sources continue to be adopted byte for byte, including any trailing whitespace they carry, since this is a parsing detail specific to one known file format rather than a general validation or cleanup rule.
basti-snyk
commented
Sep 21, 2026
basti-snyk
commented
Sep 21, 2026
mirrorIntoStorage could leave MACHINE_ID durably stored without its MACHINE_ID_SOURCE if the process crashed or the storage write failed between the two Set calls. resolve() treats a stored MACHINE_ID alone as proof that resolution already completed and never re-checks the source, so a partial write in the old order stranded future runs on an id with no recorded source. MACHINE_ID_SOURCE is now written first, and a failure to persist it falls back to in-memory-only persistence for the current process instead of writing MACHINE_ID at all. Reset() had the mirror-image race: it cleared storage before the shared file, leaving a window where a concurrent resolve() could re-adopt the about-to-be-cleared value from the shared file and re-persist it into storage after Reset's own clear finished. The shared file is now cleared first, and within storage MACHINE_ID is deleted before MACHINE_ID_SOURCE, mirroring the write order above. Also corrects the package and Resolve() doc comments, which omitted the shared-file precedence step that actually runs second, right after the existing-stored-value check.
The shared file can be written by any Snyk product on the machine, so its identifier_source field is untrusted cross-process input. resolve() cast it directly to Source without checking it against the known constants, letting a tampered or buggy writer's value flow straight into MACHINE_ID_SOURCE. A value that isn't one of the known Source constants now falls back to SourceProvided. The machine id value itself is intentionally left unvalidated, since it has no defined format by design.
writeSharedFileValue wrote the shared machine-id file with mode 0666, making it writable by any local user on the machine even though it is also read as a trust input for MACHINE_ID_SOURCE. It is now written with mode 0644 (owner read/write, everyone else read-only) via a new FILEPERM_644 constant. The probe file used to test directory writability and the lock file are unaffected.
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
This was referenced Sep 22, 2026
| @@ -0,0 +1,256 @@ | |||
| // Package machineid resolves and persists a single machine identifier shared by every Snyk | |||
Contributor
There was a problem hiding this comment.
Question: why is the machineid package in pkg and thereby exported ? It seems to be only useed as plumbing and could easily be internal for now so that we don't extend the public API contract more the necessary
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
2 participants
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.
Description
Adds
pkg/machineid, which resolves one machine identifier per machine so that the CLI and the Language Server report the same value. Today snyk-ls derives a per-process id frommachineid.ProtectedID("Snyk-LS")that the CLI cannot reproduce, and the CLI only relays an environment variable without storing anything.Resolution runs as a configuration default registered in
pkg/app.initConfiguration, so every GAF consumer gets the same algorithm from one config lookup. Precedence: existing stored value, external channel (INTERNAL_SNYK_CLIENT_MACHINE_ID), OS identifier, opt-in legacy device-id file, then a generated lowercase UUIDv4. The winning source is stored next to the value. A stored value is never overwritten;Resetis the only way to replace one. The machine identifier has no defined format. No validation is applied. A value from any source is stored and reported exactly as supplied, whether that is a hardware serial, a hostname, a JAMF UDID or an Intune GUID.Non-OS values are persisted to a product-neutral shared file and mirrored into
snyk.json. OS-derived values are not written, since every product recomputes them identically. Writes take the storage lock, refresh, re-check, then set, so two cold starts converge on one value. Storage failures are non-fatal.Eight commits: config keys, the package itself, app wiring, removal of format validation, trimming trailing whitespace from the legacy device-id file, and three follow-up fixes from review (crash-safe persistence ordering, shared-file source validation, and tighter shared-file permissions).
Design doc: https://snyksec.atlassian.net/wiki/spaces/EE/pages/5182357599
Notes for the reviewer
Six things worth your attention:
DefaultValueFunctionhas no logger parameter, so there is nowhere to log to without changing that signature.config.GetStringinside a default-value function recurses, because GAF re-runs a key's default on every lookup. The re-check goes through a scratchconfiguration.NewInMemory()instead. Worth a second opinion on whether that reads the storage you would expect.sharedFilePathsandosMachineIDare package-level vars purely as test seams.Emission of these fields on analytics events is not in this PR. It depends on the analytics-service API version in https://github.com/snyk/analytics-service/pull/181
Checklist
go test -race ./pkg/machineid/...passes)make generate) — no generated code affectedmake lint) — 0 issues🤖 Generated with Claude Code