Merge 3.2.1 to main#43
Merged
Merged
Conversation
* separating the writing on passphrase and cert; including parameters for passphrase path and cert and passphrase prop names * added additional logic to build request based on how the secret is stored. * updated remaining cert stores * Update generated docs * Updated password write to preserve existing secret content if any. * Update generated docs * documentation and manifest updates * Update generated docs * Trimming leading slashes from passphrase secret name * Update generated docs * added additional logging and null checking on response data. * Update generated docs * added null check in catch block for reading the cert and passphrase; removed newtonsoft.json from the solution. Added additional logging. * implemented check for KV version, format requests depending on result * Update generated docs * updated read/write operations on kv-v1 to reflect that the secrets are _always_ stored as JSON in v1. --------- Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io>
…t + tests (#40) * fix: prevent process crash when InitProps throws on misconfigured store 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). * fix: always normalize StorePath trailing slash for PEM/PKI store types 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. * fix: handle missing cert gracefully in GetCertificateAndPassphrase 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()). * build: add net10.0 to TargetFrameworks 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. * fix: create-on-404 for file-format stores (HCVKVPFX/JKS/P12) 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. * test: xUnit project for CreateFileStore behaviour 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 * Update generated docs * Added a fix for an issue that caused jobs to fail if the KV version could not be determined. Additional unit tests. license headers * Update generated docs * Update starter workflow to version 5 * docs: auto-generate README and documentation [skip ci] * docs: auto-generate README and documentation [skip ci] --------- Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io> Co-authored-by: Joe VanWanzeele <jvanwanzeele@keyfactor.com> Co-authored-by: Joe VanWanzeele <76071503+joevanwanzeeleKF@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.
Merge release-3.2 to main - Automated PR