Add store-location=machine URI option for Windows machine key store#1006
Open
darkfronza wants to merge 5 commits intomasterfrom
Open
Add store-location=machine URI option for Windows machine key store#1006darkfronza wants to merge 5 commits intomasterfrom
darkfronza wants to merge 5 commits intomasterfrom
Conversation
Introduces WithMachineKey() TPM option wired to the store-location=machine URI parameter in tpmkms. On Windows, this causes NCRYPT_MACHINE_KEY_FLAG to be passed to NCrypt key operations, directing key creation and access to the local machine key store rather than the current user key store. Has no effect on non-Windows platforms. Also bumps go-attestation from pre-release v0.4.4-... to released v0.4.5, which introduces the MachineKey field used by this option. Change-Type: feature Release-Note: yes Audience: operator, admin Impact: low Breaking: false Co-Authored-By: Claude <noreply@anthropic.com>
Three fixes for Windows PCP key management during reset: 1. tpm/tpm.go: initializeCommandChannel() was dropping MachineKey when creating new attest.OpenConfig structs in both branches. Even though WithMachineKey() correctly set o.attestConfig.MachineKey=true, the TPM was always opened with openPCP(false), placing keys in the current-user key store rather than the machine key store. Session-0 (agent service) and other sessions (reset command) see different user key stores, so NCryptOpenKey could not find keys. Fix: copy MachineKey in both branches. 2. tpm/key.go: DeleteKey() returned early on PCP failure without calling t.store.DeleteKey(), leaving stale file-store entries. Subsequent ListKeys() calls returned the same key, causing reset to fail repeatedly on the same key. Fix: save the PCP error, always clean up the file store, return PCP error only after successful file-store cleanup. 3. tpm/ak.go: same file-store cleanup issue as key.go for DeleteAK(). Also bumps github.com/smallstep/go-attestation to v0.4.6, which fixes LoadKeyByName to return the actual NCrypt HRESULT instead of GetLastError(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Added MachineKey bool to CreateConfig and threaded it from TPM.CreateKey down to the internal/key Windows PCP layer. NCryptCreatePersistedKey now passes NCRYPT_MACHINE_KEY_FLAG when MachineKey is true, so unattested endpoint keys land in the machine key store when store-location=machine is set, matching the behaviour of attested AKs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
hslatman
reviewed
Apr 28, 2026
| return nil | ||
| } | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
This option will affect operations on all keys going through the specific tpm.TPM instance it is applied on. It might be OK, if separate tpm.TPM instances are used, but I was wondering if the alternative was considered that passes this config for specific keys.
| t.attestConfig = &attest.OpenConfig{ | ||
| TPMVersion: t.options.attestConfig.TPMVersion, | ||
| CommandChannel: t.commandChannel, | ||
| MachineKey: t.options.attestConfig.MachineKey, |
Member
There was a problem hiding this comment.
The option probably shouldn't sit on attestConfig. That's used for higher level config of opening a TPM in a specific mode / configuration. Can probably sit on options directly, since it's just passed around.
| } | ||
|
|
||
| return | ||
| return attestErr |
Member
There was a problem hiding this comment.
Is the goal to be able to act on this error case? A typed error might help.
| } | ||
|
|
||
| return | ||
| return attestErr |
Member
There was a problem hiding this comment.
See the remark for the other case
hslatman
reviewed
Apr 28, 2026
| // key store rather than the current user key store. | ||
| // The Caller is expected to call Close() when they are done. | ||
| func openPCP() (*winPCP, error) { | ||
| func openPCP(machineKey bool) (*winPCP, error) { |
Member
There was a problem hiding this comment.
Instead of passing this via openPCP, it looks better to me if passed via the KeyConfig.
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
3 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.
Introduces WithMachineKey() TPM option wired to the store-location=machine URI parameter in tpmkms. On Windows, this causes NCRYPT_MACHINE_KEY_FLAG to be passed to NCrypt key operations, directing key creation and access to the local machine key store rather than the current user key store. Has no effect on non-Windows platforms.
Also bumps go-attestation from pre-release v0.4.4-... to released v0.4.5, which introduces the MachineKey field used by this option.