Repository navigation
Make TPM dirstore cache size configurable - #1070
Merged
Merged
Conversation
NewDirstore hardcoded the diskv in-memory read cache at 1 MiB, so a store handle would keep serving a key/AK blob it had already read even after a separate handle deleted that blob out from under it. This forces callers that mutate the backing directory out of band to rebuild their store handle to observe the change. Add a WithCacheSize functional option to NewDirstore (the constructor is now variadic and backward compatible; the default stays 1 MiB). Setting it to 0 disables caching entirely so every read reflects the current on-disk state. Thread it through tpmkms via the storage-cache-size URI parameter, parsed in both ParseTPMOptions and New so the default and URI-provided stores both honor it. New now parses the URI before constructing the default store so the setting applies regardless of where the storage directory comes from. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
darkfronza
force-pushed
the
diego/eff-89-dirstore-cache-option
branch
from
July 7, 2026 17:06
4cab1ed to
d017c2b
Compare
hslatman
reviewed
Jul 7, 2026
Per review: a negative storage-cache-size now clamps to 0 (cache disabled) rather than being ignored and silently falling back to the default cache, which is less surprising for the caller. Change-Type: patch Release-Note: no Audience: developer Impact: none Breaking: false Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hslatman
reviewed
Jul 7, 2026
Per review: verify New resolves the correct storage directory and cache size. Extract the directory-precedence and dirstore-options logic New uses into resolveStorageDirectory (URI storage-directory > opts.StorageDirectory > default "tpm"), which also removes the previous "two WithStore, last wins" construction. Add TestResolveStorageDirectory covering the directory precedence and cache-size threading, and factor the shared test helpers (parseTestURI, assertCacheDisabled). Change-Type: patch Release-Note: no Audience: developer Impact: none Breaking: false Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
TestResolveStorageDirectory only covered the helper. Add TestNew_storeConstruction, which drives New and asserts the storage directory and exact cache size the backing dirstore is built with, via a newDirstore test seam. Add storage.Dirstore.CacheSize() so the cache budget is observable. Change-Type: patch Release-Note: no Audience: developer Impact: none Breaking: false Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hslatman
reviewed
Jul 8, 2026
New previously replicated ParseTPMOptions' device + store-from-URI logic (via resolveStorageDirectory plus inline device handling). Consolidate both into a single tpmOptionsForURI(u, defaultStorageDirectory) builder: ParseTPMOptions delegates with an empty default, New with its resolved storage directory (opts.StorageDirectory, else "tpm"; a URI storage-directory still wins). The store is built through the newDirstore seam, and storage-cache-size threads through parseDirstoreOptions in that one place. Drop TestResolveStorageDirectory (the helper is gone) and fold its default-directory case into TestNew_storeConstruction, which already drives New end to end. Change-Type: patch Release-Note: no Audience: developer Impact: none Breaking: false Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hslatman
changed the title
hslatman
approved these changes
Jul 14, 2026
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
NewDirstorehardcoded the diskv in-memory read cache at 1 MiB. Because a store handle serves a key/AK blob from that cache after its first read, a handle keeps returning a blob even after a separate handle deletes it from the backing directory. Callers that mutate the directory out of band therefore have to rebuild their store handle to observe the change.This adds a way to disable (or size) that cache.
Changes
tpm/storage:NewDirstore(directory string, opts ...DirstoreOption)is now variadic (backward compatible; existing callers keep the 1 MiB default). NewWithCacheSize(bytes uint64)option;0disables caching so every read reflects current on-disk state.kms/tpmkms: thread the setting through thestorage-cache-sizeURI parameter, parsed in bothParseTPMOptionsandNew.Newnow parses the URI before constructing the default store, so the setting is honored regardless of whether the storage directory comes from the URI orapiv1.Options.StorageDirectory.Motivation
The smallstep agent hit this on Windows: it recovers from an "orphaned AK" (blob present, backing PCP/CNG key deleted) by deleting the stale blob and letting the KMS recreate the AK. Its probe TPM handle had already cached the dead blob, forcing a handle rebuild after deletion. With
storage-cache-size=0the probe handle reflects the deletion directly and the rebuild goes away. (Loading a key into the TPM dwarfs a blob read from disk, so caching that read buys little here.)Testing
TestNewDirstore_cacheSize— default / custom / disabled map to the rightCacheSizeMax.TestDirstore_noCacheReflectsOutOfBandDelete— a cached store serves a stale blob after an out-of-band delete; an uncached store reflects the delete.TestParseDirstoreOptions— URIs (nil, absent,=0,=4096, negative) thread through to the expected cache behavior via the public store API.🤖 Generated with Claude Code