tpm/storage: make dirstore cache size configurable by darkfronza · Pull Request #1070 · smallstep/crypto

darkfronza · 2026-07-07T17:01:15Z

Summary

NewDirstore hardcoded 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). New WithCacheSize(bytes uint64) option; 0 disables caching so every read reflects current on-disk state.
kms/tpmkms: thread the setting through the storage-cache-size URI parameter, parsed in both ParseTPMOptions and New. New now parses the URI before constructing the default store, so the setting is honored regardless of whether the storage directory comes from the URI or apiv1.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=0 the 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 right CacheSizeMax.
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

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>

hslatman · 2026-07-07T19:13:38Z

+		{"no-cache-param", "tpmkms:storage-directory=x", false},
+		{"cache-disabled", "tpmkms:storage-cache-size=0", true},
+		{"cache-custom", "tpmkms:storage-cache-size=4096", false},
+		{"negative-ignored", "tpmkms:storage-cache-size=-1", false},


Maybe change this so that the negative size becomes 0, and thus disabled. I believe that'll be less surprising.

Good call — done in ea21c43. A negative storage-cache-size now clamps to 0 (cache disabled) instead of being ignored, and the negative-disables test case asserts the disabled behavior.

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 · 2026-07-07T20:07:21Z

+	// Parse the URI up front so the default store below can honor any storage
+	// options it carries (e.g. storage-cache-size).
+	if opts.URI != "" {
+		if u, err = uri.ParseWithScheme(Scheme, opts.URI); err != nil {
+			return nil, fmt.Errorf("failed parsing %q as URI: %w", opts.URI, err)
+		}
+		uriOptions = ParseOptions(u)
+	}


Could you add a case or two to verify that the correct storage directory and cache size ends up being used when New is called?

Added in 9f13682. I pulled the directory + cache resolution New performs into resolveStorageDirectory (URI storage-directory > opts.StorageDirectory > default tpm) — which also drops the earlier "two WithStore, last wins" construction — and added TestResolveStorageDirectory covering the directory precedence and that storage-cache-size threads through. Since New now delegates to that helper, the cases exercise its actual resolution logic.

Follow-up in f6cff88: the previous test only exercised the helper. Added TestNew_storeConstruction, which drives New itself and asserts the storage directory and the exact CacheSize() the backing dirstore is built with (0 and 4096 cases, plus default-enabled), via a newDirstore test seam. Also added storage.Dirstore.CacheSize() so the cache budget is observable.

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 · 2026-07-08T10:08:48Z

 	}
 	if storageDirectory := u.Get("storage-directory"); storageDirectory != "" {
-		opts = append(opts, tpm.WithStore(storage.NewDirstore(storageDirectory)))
+		opts = append(opts, tpm.WithStore(storage.NewDirstore(storageDirectory, parseDirstoreOptions(u)...)))


Suggested change

opts = append(opts, tpm.WithStore(storage.NewDirstore(storageDirectory, parseDirstoreOptions(u)...)))

opts = append(opts, tpm.WithStore(newDirstore(storageDirectory, parseDirstoreOptions(u)...)))

Addressed by the same consolidation (eec0a45): store construction now lives solely in tpmOptionsForURI, which uses the newDirstore seam — so this line went through that helper rather than a direct storage.NewDirstore call.

hslatman · 2026-07-08T10:41:48Z

+		if device := u.Get("device"); device != "" {
+			tpmOpts = append(tpmOpts, tpm.WithDeviceName(device))
+		}
+	}


This now seems to replicate the logic from ParseTPMOptions. Not sure if the cache size can be incorporated there instead, so that the options are handled in a single place?

Good call — consolidated in eec0a45. New no longer replicates ParseTPMOptions: both now delegate to a single tpmOptionsForURI(u, defaultStorageDirectory) that builds the device + store (with storage-cache-size applied) in one place. ParseTPMOptions passes an empty default; New passes its resolved storage directory (opts.StorageDirectory, else tpm), and a URI storage-directory still wins. Removed resolveStorageDirectory accordingly.

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>

github-actions Bot added the needs triage label Jul 7, 2026

darkfronza force-pushed the diego/eff-89-dirstore-cache-option branch from 4cab1ed to d017c2b Compare July 7, 2026 17:06

hslatman reviewed Jul 7, 2026

View reviewed changes

darkfronza and others added 2 commits July 7, 2026 19:19

hslatman reviewed Jul 8, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

tpm/storage: make dirstore cache size configurable#1070

tpm/storage: make dirstore cache size configurable#1070
darkfronza wants to merge 5 commits into
masterfrom
diego/eff-89-dirstore-cache-option

darkfronza commented Jul 7, 2026

Uh oh!

hslatman Jul 7, 2026 •

edited

Loading

Uh oh!

darkfronza Jul 7, 2026

Uh oh!

hslatman Jul 7, 2026 •

edited

Loading

Uh oh!

darkfronza Jul 7, 2026

Uh oh!

darkfronza Jul 7, 2026 •

edited

Loading

Uh oh!

hslatman Jul 8, 2026

Uh oh!

darkfronza Jul 8, 2026

Uh oh!

hslatman Jul 8, 2026 •

edited

Loading

Uh oh!

darkfronza Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	opts = append(opts, tpm.WithStore(storage.NewDirstore(storageDirectory, parseDirstoreOptions(u)...)))
	opts = append(opts, tpm.WithStore(newDirstore(storageDirectory, parseDirstoreOptions(u)...)))

Uh oh!

Conversation

darkfronza commented Jul 7, 2026

Summary

Changes

Motivation

Testing

Uh oh!

hslatman Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darkfronza Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

hslatman Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darkfronza Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

darkfronza Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hslatman Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

darkfronza Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

hslatman Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darkfronza Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hslatman Jul 7, 2026 •

edited

Loading

hslatman Jul 7, 2026 •

edited

Loading

darkfronza Jul 7, 2026 •

edited

Loading

hslatman Jul 8, 2026 •

edited

Loading