Create-on-404 for file-format stores (HCVKVPFX/JKS/P12) + net10 target + tests#40
Open
spbsoluble wants to merge 7 commits into
Open
Create-on-404 for file-format stores (HCVKVPFX/JKS/P12) + net10 target + tests#40spbsoluble wants to merge 7 commits into
spbsoluble wants to merge 7 commits into
Conversation
InitProps was declared as `async void`, so exceptions propagated via Task.ThrowAsync as unhandled exceptions and crashed the .NET process. Changed to `async Task` and updated all three Initialize overloads to block synchronously via .GetAwaiter().GetResult(), so errors surface as job failures rather than process restarts. Also wraps GetTokenPoliciesAsync in try-catch (best-effort diagnostic logging; token may lack lookup-self capability).
The trailing slash required by HcvKeyValueClient was only appended when StorePath came from the store's custom Properties dict. When it came from config.CertificateStoreDetails.StorePath (the normal case for stores defined without an inline StorePath property), the slash was skipped and path concatenation produced /certspath instead of /certs/path. Move the normalize+trailing-slash block outside the ContainsKey guard so it applies regardless of where the value originated.
For Management-Add on a fresh file store (no existing keystore in Vault), the cert read returns 404. The old code threw DirectoryNotFoundException before ever reading the passphrase, failing the job. Split cert and passphrase reads into separate try-catch blocks. A 404 on the cert path is now treated as an empty store — certContent stays empty so AddCertificate creates a new keystore. Passphrase read continues and is still required. Also guards against null values in both fields (null value in a KV secret no longer NPEs on .ToString()).
UO 25.5.x ships with the .NET 10 runtime. Loading a plugin compiled against net8.0 produces System.NotSupportedException at HcvKeyfactorClient.GetTokenPoliciesAsync() when System.Net.Requests v10.0.0.0 is substituted for the v8.0.0.0 the plugin was built against. Adding net10.0 to TargetFrameworks lets the plugin be built against the same runtime UO 25.5.x supplies. net6.0 and net8.0 are retained for backward compatibility with older UO releases.
Make Management-Add against an empty or never-seeded file-format store
succeed by auto-creating the store on first use, rather than failing
with 404s for a missing cert blob and passphrase.
Previous behaviour: Command issues exactly one Management/Create job
per store at registration time. If that Create failed for any reason
(plugin bug, transient Vault error, etc.), Command never retries Create
— so every subsequent Management-Add hit GetCertificateAndPassphrase,
got back a 404 for the (non-existent) cert and passphrase secrets, and
threw. The store was permanently broken without operator intervention.
Five related fixes, all on the same code path:
1. PutCertificateIntoFileStore: when GetCertificateAndPassphrase
returns empty cert AND empty passphrase, call CreateFileStore() to
seed an empty store + random passphrase, then re-read and proceed.
Mirrors how HCVKVPEM's CreateCertStore writes an empty PEM secret
up-front. When only the passphrase is missing (orphaned store) we
refuse with a clear error rather than silently overwriting.
2. GetCertificateAndPassphrase: passphrase read is now tolerant of a
404 in the same way the cert read already was — a missing passphrase
on a fresh store is a signal to create-if-not-exist, not a fatal
error. Catch pattern switched from `catch (VaultApiException) when
(ex.StatusCode == 404)` to a plain catch with explicit StatusCode
test inside: the `when` filter has been observed not to fire
reliably for exceptions raised from inside the async state machine
on .NET 10 + VaultSharp 1.17, sending the flow into the generic
`catch (Exception)` branch even for 404s. The same pattern is
applied to the cert-read catch for symmetry.
3. PatchSecretAutoAsync (KV v2): catches a 404 from PatchSecretAsync
and falls back to WriteSecretAutoAsync. KV v2 Patch returns 404 when
the secret doesn't yet exist; falling back to Write makes the call
idempotent ("create-or-merge") rather than requiring callers to
pre-create every path. Same robust catch pattern.
4. CreateFileStore: pre-existing bugs that surfaced once the path
started getting exercised by the auto-seed code:
- Cert/passphrase writes were targeting the PARENT path instead of
the full secret path. KV v2 only accepts writes at the leaf, so
these previously failed.
- The passphrase write unconditionally used PatchSecretAutoAsync,
which 404s on first write. Now uses Write for non-JSON layouts
(whole-secret-is-passphrase) and Patch for JSON layouts (shared
secret with a passphrase property to merge).
- PutCertificateIntoFileStore had the same Patch-only bug for the
cert blob write — fixed the same way.
5. Expose hooks for unit testing without a real Vault: _storeType is
now `protected` and GetKVVersionAsync / ReadSecretAutoAsync /
WriteSecretAutoAsync / PatchSecretAutoAsync are now `virtual`, so
the new TestableHcvKeyValueClient subclass can stub Vault I/O.
Validated end-to-end against a Keyfactor Command 25.5.x + UO 25.5.x +
OpenBao lab: a Management-Add against an HCVKVPFX, HCVKVJKS, and
HCVKVP12 store that had never had a successful Create now succeeds on
the first try (auto-creates empty store + passphrase, then adds the
new cert). 15/15 file-format Management jobs returned Success across
five algorithms (RSA-2048/4096, ECDSA P-256/384/521) × three formats.
Add a hashicorp-vault-orchestrator.Tests xUnit project covering the
file-format CreateFileStore path that the previous commit fixed.
The TestableHcvKeyValueClient subclass stubs all Vault I/O so path-
level behaviour can be asserted without a real Vault connection:
ReadSecretAutoAsync returns canned responses; WriteSecretAutoAsync
and PatchSecretAutoAsync record their calls for assertion.
7 tests cover:
- cert + passphrase written at the expected leaf paths
- non-JSON layout writes the whole secret (no JSON property
indirection)
- JSON layout writes the value under the configured property name
- 404 from Patch falls back to Write (create-or-merge)
- random passphrase actually gets recorded in the Write call
- unrecognized store-type values throw InvalidOperationException
InternalsVisibleTo("hashicorp-vault-orchestrator.Tests") added via
hashicorp-vault-orchestrator/AssemblyInfo.cs so the tests can reach
the protected/internal members of HcvKeyValueClient.
.gitignore extended to exclude the Tests project's bin/obj plus
.idea (Rider workspace files).
$ dotnet test
Passed! - Failed: 0, Passed: 7, Skipped: 0, Total: 7
spbsoluble
force-pushed
the
fix/create-on-404-file-stores
branch
from
303d4c2 to
526a9af
Compare
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
1 participant
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
Makes the file-format Vault stores (HCVKVPFX, HCVKVJKS, HCVKVP12) self-healing: a
Management-Addagainst a store that was never successfullyCreatednow auto-seeds the empty store + passphrase on first use and proceeds with the Add, instead of failing with a 404. Also addsnet10.0toTargetFrameworks(UO 25.5.x runtime), and a new xUnit test project coveringCreateFileStore.Why
Command issues exactly one
Management/Createjob per store at registration time. If that one Create failed — plugin bug, transient Vault error, deferred policy change, anything — Command never retries Create. Every subsequentManagement-Addthen hitGetCertificateAndPassphrase, got 404s for the missing cert blob and passphrase, and threw. The store was permanently broken without operator intervention (re-register, hand-seed Vault, or PUTCreateIfMissingto trigger a new Create).The fix treats "Add against a store that doesn't exist yet" as a valid state and seeds it inline, matching how
HCVKVPEMalready behaves (CreatePemStorewrites an empty PEM secret as part of registration).What's in this PR
build: add net10.0 to TargetFrameworks— UO 25.5.x runs .NET 10. A net8.0 plugin throwsSystem.NotSupportedExceptionatHcvKeyfactorClient.GetTokenPoliciesAsync()because the substitutedSystem.Net.Requestsv10 assembly rejects the v8 API the plugin compiles against. net6.0 and net8.0 are retained.fix: create-on-404 for file-format stores— five interlocking changes:PutCertificateIntoFileStoreauto-seeds (random passphrase + empty PFX/JKS/P12 file) when both the cert blob and the passphrase are missing at the configured Store Path. Orphaned stores (passphrase missing but cert present) raise a clear error rather than silently overwriting.GetCertificateAndPassphrase: passphrase read tolerates 404 the same way the cert read already did — both empty is the create-if-not-exist signal. Catch pattern changed fromcatch (VaultApiException) when (ex.StatusCode == 404)to a plain catch with explicit StatusCode test: thewhenfilter does not fire reliably for exceptions raised inside the async state machine on .NET 10 + VaultSharp 1.17 (observed end-to-end on a live UO 25.5.x lab). Applied to the cert-read catch too for symmetry.PatchSecretAutoAsync(KV v2): catches 404 and falls back toWriteSecretAutoAsync. KV v2 Patch returns 404 when the secret doesn't exist; falling back to Write makes the operation idempotent (create-or-merge).CreateFileStorefixes that surfaced once the auto-seed code started exercising it: cert/passphrase writes were targeting the parent KV path instead of the leaf; passphrase write unconditionally used Patch (404s on first write). Now uses Write for non-JSON layouts and Patch for JSON layouts. Same Patch-only bug fixed for the cert blob write inPutCertificateIntoFileStore._storeType→protected, plusvirtualonGetKVVersionAsync/ReadSecretAutoAsync/WriteSecretAutoAsync/PatchSecretAutoAsyncso the new test project can stub Vault I/O.test: xUnit project for CreateFileStore behaviour— newhashicorp-vault-orchestrator.Testsproject. 7 tests covering the leaf-path writes, JSON vs non-JSON layouts, Patch→Write 404 fallback, passphrase generation, and the unrecognised-store-type error.InternalsVisibleToadded viaAssemblyInfo.cs.The PR also picks up three pre-existing fixes for the same .NET 10 / cert-store stability effort that hadn't been pushed:
prevent process crash when InitProps throws on misconfigured store,always normalize StorePath trailing slash for PEM/PKI store types,handle missing cert gracefully in GetCertificateAndPassphrase.Test plan
dotnet build -c Release— clean (0 errors, 29 warnings, all pre-existing)dotnet test— 7/7 pass on net8.0Management-Addsucceeded on every store, auto-seed traces fired exactly once per storeSuccess(5 algorithms × 3 formats: RSA-2048/4096, ECDSA P-256/P-384/P-521)