Repository navigation
Mid-cycle reissue mock data, live sync repro, and a RequestManager build fix - #37
Open
bhillkeyfactor wants to merge 12 commits into
Open
bhillkeyfactor wants to merge 12 commits into
bhillkeyfactor wants to merge 12 commits into
Conversation
* Add custom field support * changelog * support cname return from enrollment * Update generated docs * feat: release 1.1.1 * Update generated docs * Fix for issues with * Test * Added template parameter configuration via REST gateway. Fixed bug with email used for verification. Changed docs and enrollment field/template parameter names. See changelog. * Update generated docs * Fixed broken logging. * Incremental sync support added using csc date filter so sync timing can run faster that default full sync periods * Update generated docs * Fixes for Incremental Sync * Update CHANGELOG.md --------- Co-authored-by: Mikey Henderson <4452096+fiddlermikey@users.noreply.github.com> Co-authored-by: Sean <1661003+spbsoluble@users.noreply.github.com> Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io> Co-authored-by: Brian Hill <bhill@keyfactor.com> Co-authored-by: Brian Hill <76450501+bhillkeyfactor@users.noreply.github.com> * Fix NullReferenceException in GetEnrollmentResult for null DCV email (#9) * Fix NullReferenceException in GetEnrollmentResult for null DCV email The condition for adding DCV email entries to the cnames dictionary was inverted (string.IsNullOrEmpty instead of !string.IsNullOrEmpty), causing cnames.Add(null, null) and an ArgumentNullException on every enrollment where CSC returned a DcvDetail with email=null (typical for EMAIL DCV orders that have actionNeeded=N, and for CNAME-only DCV). Inverts the condition and adds a ContainsKey guard to mirror the existing CName branch. * Update generated docs --------- Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io> * Feature/dns plugins (#12) * 200 day renewal fixes * Update generated docs * Improved logging .net 10 support * Removed Template Sync Logic * Update generated docs * fixed template mapping issue * product fixes * Update generated docs * fixed renewal issue * documentation fixes * Update generated docs * DNS Changes * Update generated docs * dns code updates * Update generated docs * change type * Update generated docs * fixed mismatch * Use 'cname' validation type for CSC CNAME DCV CSC DCV requires a CNAME record, so resolve a DNS provider advertising the 'cname' validation type (e.g. GoDaddyCnameDomainValidator) rather than the ACME 'dns-01'/TXT variant. Docs updated to call out the CNAME validator and warn against selecting the TXT validator for CSC domains. * Update generated docs * added polling to grab cert * Update generated docs * Update integration-manifest.json * Update CSCGlobalCAPlugin.csproj * Update CSCGlobalCAPlugin.csproj * Update keyfactor-bootstrap-workflow-v3.yml * docs: auto-generate README and documentation [skip ci] * Add Enabled CA connection flag Adds the standard Enabled boolean field to the CA Connection settings, matching the pattern used by every other Keyfactor CA plugin (SSL Store, Digicert, HydrantId, GCP CAS, Idnomic, etc.). Purpose: allow ops to create the CA record before all API credentials are available. When Enabled=false, the plugin short-circuits: - Initialize: skips CscGlobalClient construction (no valid creds needed) - Ping: no-op, logs a warning - ValidateCAConnectionInfo / ValidateProductInfo: skip validation - Synchronize: completes the buffer immediately - Enroll: returns FAILED with a clear message - Revoke: throws InvalidOperationException with a clear message Default is true so existing deployments that don't set the key continue to function without change. Reads the value from incoming connectionInfo in the Validate* methods so an operator editing the CA sees consistent behavior with the current form state. * docs: auto-generate README and documentation [skip ci] --------- Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Update integration-manifest.json (#14) * Update integration-manifest.json (#29) * docs: auto-generate README and documentation [skip ci] * Port 1.2.0 product rename and fixes onto DNS plugin support (release-2.0) (#34) * 200 day renewal fixes * Update generated docs * Improved logging .net 10 support * Removed Template Sync Logic * Update generated docs * fixed template mapping issue * product fixes * Update generated docs * fixed renewal issue * documentation fixes * Update generated docs * DNS Changes * Update generated docs * dns code updates * Update generated docs * change type * Update generated docs * fixed mismatch * Use 'cname' validation type for CSC CNAME DCV CSC DCV requires a CNAME record, so resolve a DNS provider advertising the 'cname' validation type (e.g. GoDaddyCnameDomainValidator) rather than the ACME 'dns-01'/TXT variant. Docs updated to call out the CNAME validator and warn against selecting the TXT validator for CSC domains. * Update generated docs * added polling to grab cert * Update generated docs * Update integration-manifest.json * Update CSCGlobalCAPlugin.csproj * Update CSCGlobalCAPlugin.csproj * Update keyfactor-bootstrap-workflow-v3.yml * docs: auto-generate README and documentation [skip ci] * Add Enabled CA connection flag Adds the standard Enabled boolean field to the CA Connection settings, matching the pattern used by every other Keyfactor CA plugin (SSL Store, Digicert, HydrantId, GCP CAS, Idnomic, etc.). Purpose: allow ops to create the CA record before all API credentials are available. When Enabled=false, the plugin short-circuits: - Initialize: skips CscGlobalClient construction (no valid creds needed) - Ping: no-op, logs a warning - ValidateCAConnectionInfo / ValidateProductInfo: skip validation - Synchronize: completes the buffer immediately - Enroll: returns FAILED with a clear message - Revoke: throws InvalidOperationException with a clear message Default is true so existing deployments that don't set the key continue to function without change. Reads the value from incoming connectionInfo in the Validate* methods so an operator editing the CA sees consistent behavior with the current form state. * docs: auto-generate README and documentation [skip ci] * Port product rename, new certificate types, and bug fixes from feature/ev-ov-dv-multiname-certs Ports the following from feature/ev-ov-dv-multiname-certs, adapted to this branch's existing patterns (ProductIdToCodeMap/CodeToProductIdMap, structured logging style) rather than overwriting them: - Renamed all certificate product IDs to CSC's current certificate type names, with pre-1.2.0 legacy names still accepted (added as additional entries in the existing product-id maps, not a separate alias layer) so existing Certificate Templates in Command keep working. - Added the 3 new certificate products: CSC TrustedSecure EV, Multiple Names; OV Wildcard, Multiple Names; DV Wildcard, Multiple Names (types 7/8/9). - Replaced the hardcoded certificateType == "2"/"3" checks with MultiNameCertificateTypes/EvCertificateTypes sets covering all applicable types. - ValidateProductInfo now calls RequestManager.IsKnownProductId instead of checking a separate list, so accepted names can't drift out of sync with what GetCertificateType actually resolves. - Fixed the "Addtl Sans Comma Separated DCV Emails" field never being read (typo'd lookup key: "DVC" instead of "DCV"), and added a fallback to the primary CN's DCV email when no per-domain SAN email override matches (CSC Global rejects requests with a SAN missing domainControlValidation). - Fixed a case-sensitivity bug ("priorcertsn" vs "PriorCertSN") that silently prevented PriorCertSN from ever being read during Renew/Reissue. - Made Price.Total nullable to fix a JSON deserialization crash when CSC Global returns "price.total": null. - Updated integration-manifest.json and docsource/configuration.md to match. Explicitly NOT ported (per discussion - these don't fit this branch): - FlowLogger changes/redesign - this branch's FlowLogger has an incompatible tree-based design already wired into DNS-01 CNAME auto-publish call sites; left untouched. - The EnrollmentContext "Flow Summary" UX feature - this branch's TryPublishCnameDcvAsync treats EnrollmentContext entries as real DNS records to auto-publish; adding non-DNS entries there would be actively harmful. - .NET 6/8 multi-targeting - this branch already moved to net10.0-only with newer package versions; not reintroducing the older targets. - The Renew/Reissue GENERATED-vs-EXTERNALVALIDATION fix - already present independently on this branch. Also adds a new xUnit test project (this branch had none), with fresh tests written against this branch's actual code shape rather than adapted from the other branch's now-incompatible test suite: 40 tests covering certificate type/SAN/EV routing for all 10 canonical + 7 legacy product names, IsKnownProductId, the DCV email fallback fix, Price.Total null deserialization, and Renew/Reissue status codes. * docs: auto-generate README and documentation [skip ci] * Raise unit test coverage to ~95% line coverage Adds testability seams (matching the pattern already used for HTTP mocking in feature/ev-ov-dv-multiname-certs): - CSCGlobalCAPlugin.CscGlobalClient property is now internal instead of private, so tests can inject a mock ICscGlobalClient. - CscGlobalClient gets an internal HttpMessageHandler-accepting constructor overload, so tests can supply a fake handler instead of making real HTTP calls. - AssemblyInfo.cs adds InternalsVisibleTo("CSCGlobalCAPlugin.Tests"). Expands the test suite from 40 to 233 tests, covering: - RequestManager: all certificate-type/SAN/EV routing (canonical + legacy names), IsKnownProductId, DCV email fallback, GetRevokeResult, MapReturnStatus/MapCertificateTypeToProductId, custom fields, Price.Total null deserialization. - CscGlobalClient: all 7 API methods (success/400/error paths) via a fake HttpMessageHandler, plus constructor validation. - FlowLogger: Step/StepAsync/Fail/Skip/Branch/Dispose. - CSCGlobalCAPlugin: Initialize, GetSingleRecord, Synchronize/ SyncCertificates, Revoke, Ping, ValidateCAConnectionInfo/ ValidateProductInfo, Enroll (New and RenewOrReissue - renewal vs. reissue decision, DNS-01 CNAME auto-publish via IDomainValidatorFactory, synchronous issuance polling via DcvPollTimeoutSeconds, and the various failure branches), and the PEM/leaf-certificate parsing helpers (GetEndEntityCertificate/ExtractCertificates/FindLeaf). Line coverage: 15.57% -> 95.62%. Remaining gaps are dead code (ExportCollectionToPem, RetryCountExceededException - both already unused before this change) and a few defensive catch blocks for exception types (AggregateException, arbitrary X509 export failures) that aren't reachable through normal async/mock-based testing. * Raise unit test branch coverage from ~75% to ~80% Added targeted tests for previously-uncovered conditional branches across RequestManager, CSCGlobalCAPlugin, and CscGlobalClient - optional-field mapping in the registration/renewal/reissue request builders, null-valued config keys, null-object constructor inputs, and a few edge cases in the renewal/reissue decision and DNS auto-publish paths. Left purely diagnostic logging ternaries and practically-unreachable defensive branches alone. * Sync ProductID directly from CSC's certificateType, drop reverse-mapping table Synced DV/EV/UC/Premium certs weren't mapping to their Certificate Profile in Command because CodeToProductIdMap resolved them back to the old pre-1.2.0 legacy product names, which no longer match the canonical Template Short Names configured in Command. Follow the same fix already proven on feature/ev-ov-dv-multiname-certs: pass CSC's certificateType straight through as ProductID during sync instead of remapping it, since CSC's API already returns the current/canonical product name directly. * Fix Enroll failures reporting INPROCESS instead of FAILED, surface flow summary in StatusMessage Every early-return failure path in Enroll (plus the catch-all exception handler) set Status = 30 (INPROCESS) instead of FAILED, so Command treated these as still-pending rather than errors and never surfaced them in the UI. Follow the same fix already proven on feature/ev-ov-dv-multiname-certs: use EndEntityStatus.FAILED throughout, and add FlowLogger.GetSummary() (a concise step list, distinct from the ASCII-tree RenderFlow() used for Trace logs) to prepend what the plugin actually attempted ahead of the terse error text. Also fixes the reissue "one click not available" message incorrectly saying "Renew" instead of "Reissue". * Attach flow summary to EnrollmentContext on success, not just StatusMessage Command's enrollment UI doesn't surface StatusMessage on a successful/pending result at all - only EnrollmentContext is shown, so the flow summary added for failures was invisible on the success path. Add AttachFlowSummary (following the same fix already proven on feature/ev-ov-dv-multiname-certs) to also attach a "Flow Summary" entry to EnrollmentContext for success/pending results, alongside whatever DCV instructions came back, and use it for the FAILED results returned from GetEnrollmentResult/GetRenewResponse/ GetReIssueResult, which the earlier Status=30 fix didn't cover since they're already FAILED coming out of RequestManager. Called after TryPublishCnameDcvAsync in the New enrollment path so the "Flow Summary" entry is never present yet when DNS auto-publish walks EnrollmentContext looking for real CNAME records to publish. * Render flow summary as one bullet per step instead of one blob Matches the same fix already proven on feature/ev-ov-dv-multiname-certs: add FlowLogger.GetSummaryEntries(), which returns one dictionary entry per step (plus a header entry) instead of a single multi-line block. AttachFlowSummary now merges these into EnrollmentContext directly so Command's bulleted rendering shows a readable line per step, rather than one run-on entry with embedded newlines that don't render as separate bullets. * Rename PublishCnameDcv step and explain why it's a no-op for non-CNAME methods The DcvAutoPublish step (formerly PublishCnameDcv) ran unconditionally on every New enrollment and, for EMAIL-validated certs, silently no-op'd while still showing a bare [OK] under a CNAME-sounding step name in the flow summary - confusing when read back in Command's UI. Add a FlowLogger.StepAsync overload that uses the action's own return value as the step detail (the existing overload's detail parameter is evaluated before the action runs, so it can't reflect what the action decided), and have TryPublishCnameDcvAsync return why it skipped or how many records it published. --------- Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> --------- Co-authored-by: David Galey <dgaley@keyfactor.com> Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io> Co-authored-by: Mark Kachkaev <37276742+mkachk@users.noreply.github.com> Co-authored-by: Mikey Henderson <4452096+fiddlermikey@users.noreply.github.com> Co-authored-by: Sean <1661003+spbsoluble@users.noreply.github.com> Co-authored-by: Brian Hill <bhill@keyfactor.com> Co-authored-by: Brian Hill <76450501+bhillkeyfactor@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
A "Merge branch 'main' into release-2.0" commit mangled RequestManager's
GetCertificateType into a truncated method (missing body/closing brace,
swallowing the next method's signature), and dropped the
MultiNameCertificateTypes/EvCertificateTypes lookup sets it depends on.
That broken state then flowed into main via the 2.0.2 merge. The actual
published 2.0.2 release tag predates this and is unaffected.
Also fixes a test file truncated by the same bad merge (a test body cut
off mid-assertion, swallowing the next test's attribute/signature), two
tests using the wrong ("TrustedSecure" vs "Trusted Secure") spelling for
two legacy product names, and removes a stale duplicate GetProductIds
test left over from before the product rename.
…ssueCertificate CSC's API already returns these fields but the model silently dropped them. isLifeCycleReIssueCertificate in particular flags when CSC has mid-cycle reissued a certificate under the same uuid with a new key pair CSC generated itself - not yet acted on anywhere in the plugin, but needed to write any test against that scenario. See the mid-cycle-reissue mock fixtures.
CSC can reissue a certificate partway through its order term under the same uuid, generating a new key pair itself rather than from our CSR, and flagging isLifeCycleReIssueCertificate on the next sync. Keyfactor never holds a matching private key for that reissued certificate. Adds MidCycleReissueFixtures with two real-world scenarios (single-name DV and wildcard multi-name DV, metadata taken from actual CSC API responses) each producing a baseline record and its mid-cycle-reissue counterpart, plus Synchronize tests pinning down current behavior: both records get queued independently under the same CARequestID with different certificate content, with nothing today reflecting that the second one is a reissue rather than an ordinary sync. This is a repro/test harness only - no handling of the flag has been implemented yet.
Moves MidCycleReissueFixtures into a new shared CSCGlobalCAPlugin.MockData project so it can be used outside the test project too. Adds CSCGlobalCAPlugin.MockServer, a runnable console app that starts a real HttpListener implementing CSC's list/get certificate endpoints (backed by the mid-cycle-reissue fixtures), then runs the real, unmodified CSCGlobalCAPlugin.Synchronize() against it over an actual HTTP connection - no mocked ICscGlobalClient. Confirms live what the xUnit tests already showed: both the baseline and its mid-cycle reissue come back under the same CARequestID with distinct certificate content, and nothing distinguishes the reissue from an ordinary re-sync. Run with: dotnet run --project cscglobal-caplugin.MockServer
…e-reissue-into-release-2.0 # Conflicts: # cscglobal-caplugin/RequestManager.cs
…esting A separate local mock HTTP server works, but networking (Docker, remote Command instances) makes it awkward to point a real CA connection at it. This is simpler: a CA connection property that makes a real Synchronize() against the real CSC API also inject a synthetic mid-cycle-reissue duplicate for every ACTIVE certificate it gets back, using a freshly generated key (we never hold CSC's real key for an unsolicited reissue). Test/diagnostic only - defaults to false, logs a warning when enabled, and is documented as never-for-production in its connector annotation.
…w row AnyGatewayCore's CertificateService.CreateOrUpdateCertificate only writes a cert into a row the first time it is populated; once a row has a cert it logs "The existing certificate will be preserved" and leaves it alone, with no config to override that. Reusing the real cert's CARequestID for the synthetic duplicate collided with the real cert's row and was silently dropped - confirmed live, where a sync with MockReissueRun enabled queued 62 certs but the gateway database still held exactly the 31 real ones. Derive the mock duplicate's CARequestID from the real one plus the mock cert's serial number, so it gets its own row and that row differs on every sync run (a fixed suffix would collide with its own prior mock row on the next sync and hit the same preserve problem again). Diverges from CSC's real same-UUID reissue continuity, but makes the injected cert actually observable for testing. Rollback: git tag mock-reissue-same-careqid-backup points at the prior same-CARequestID behavior (a7ce54a).
… window Deriving the mock duplicate's CARequestID from its own serial number made every sync create a brand-new row, since a freshly generated self-signed cert gets a new random serial each call - confirmed live, where repeated syncs ballooned the gateway database to 4000+ records instead of one duplicate per real cert. Go back to a fixed, derived CARequestID (uuid + "-mock-reissue") so a sync only creates the mock row once; AnyGatewayCore's existing-cert-preserve behavior then does the right thing on every later sync - it finds the row and leaves it alone, instead of inserting a new one. Also set the mock cert's NotBefore/NotAfter from the real cert's own validity window instead of DateTimeOffset.UtcNow, since the real cert's dates don't drift between syncs the way "now" does, and that may be a factor in whether a downstream consumer treats a cert as the same vs. a replacement.
Pulled two real CSC Certificates payloads (a renewal and an isLifeCycleReIssueCertificate case) and confirmed CSC never sends two records for one order - it's always a single record per UUID with the certificate field (and dates/flags) swapped in place. Replace the dual-queue, derived-CARequestID approach with an in-place swap: the first sync for a UUID passes the real cert through untouched; every sync after that substitutes a synthetic reissue cert for the same UUID/CARequestID, matching CSC's actual behavior exactly. This also means MockReissueRun now faithfully reproduces the 'existing certificate will be preserved' gateway behavior a real mid-cycle reissue would hit, instead of working around it - that's the point, not a bug in the mock. Tracking state (which UUIDs have already been shown once, and the cached replacement cert) has to be static since the plugin is reconstructed and re-initialized on every sync call.
…ehavior MockReissueRun's cert-swap always lands under the same CARequestID, which hits AnyGatewayCore's existing-certificate-preserve behavior every time - useful for proving that behavior, but it means there's no way to also see what the gateway does with a genuinely new row for the same UUID. Add a second, independent test/diagnostic CA connection setting: AppendThumbprintToCaId. When true, Synchronize appends the incoming certificate's SHA1 thumbprint (matching AnyGatewayCore's own thumbprint convention) to every CARequestID it hands to the gateway. Combined with MockReissueRun, this lets the same scenario be synced both ways - new cert visible as its own row (true) vs. the real-world preserved-row behavior (false, default) - without changing any other code.
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.
Brings the mid-cycle-reissue mock data/repro work (previously PR'd into
main) intorelease-2.0as well, sincerelease-2.0independently inherited the same bad merge-conflict damage toRequestManager.GetCertificateTypethatmainhad (see the separate fix already merged there).What's included
RequestManager.GetCertificateType- truncated by the same bad merge that hitmain, plus theMultiNameCertificateTypes/EvCertificateTypeslookup sets it depends onorderValidityStartDate/orderValidityEndDate,isRenewed,isLifeCycleReIssueCertificate- CSC already sends these, the model was silently dropping themCSCGlobalCAPlugin.MockData- shared fixtures reproducing CSC's mid-cycle certificate reissue (same uuid, re-signed from the same key pair, new serial,isLifeCycleReIssueCertificate = true), built from real API sample dataCSCGlobalCAPlugin.MockServer- a runnable console app that starts a real local HTTP server implementing CSC's list/get certificate endpoints and runs the actual, unmodifiedCSCGlobalCAPlugin.Synchronize()against it - confirms live, not just in xUnit, that both the baseline and its reissue get queued under the sameCARequestIDwith distinct certificate content, with nothing today distinguishing the reissue from an ordinary re-syncSynchronizetests pinning down that same behaviorNo handling of
isLifeCycleReIssueCertificatehas been implemented yet - this is the repro/test harness to make that decision against.Test plan
dotnet build- 0 errors (whole solution, including new MockData/MockServer projects)dotnet test- 209/209 passingdotnet run --project cscglobal-caplugin.MockServer- confirmed live against this branch