Conversation
check for duplicate PEMs
change default start sync date for first incremental sync
removing caching of product type list
change default incremental sync range
shorten incremental sync if it is too long
* add duplicate support * Update generated docs --------- Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io>
There was a problem hiding this comment.
Pull request overview
This PR merges the release-2.2 branch to main, incorporating changes from three releases (2.1.1, 2.1.2, and 2.2.0). The primary purpose is to add certificate duplication functionality to the DigiCert CertCentral plugin, along with several bug fixes and enhancements.
Changes:
- Added certificate duplication support allowing users to duplicate existing certificate orders via an enrollment field
- Added temporary support for including Client Authentication EKU in SSL certificates (planned for deprecation by DigiCert in May 2026)
- Improved SMIME certificate profile type handling to use DigiCert product defaults instead of hardcoded 'strict' value
- Fixed incremental sync to use a 6-day window when no previous sync exists
- Removed caching of product ID lookups and added workaround for DigiCert API duplicate PEM issue
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| integration-manifest.json | Added IncludeClientAuthEKU parameter and updated ProfileType description |
| docsource/configuration.md | Added documentation for certificate duplication feature |
| README.md | Added IncludeClientAuthEKU parameter documentation and certificate duplication instructions |
| CHANGELOG.md | Added version history for 2.1.1, 2.1.2, and 2.2.0 releases |
| digicert-certcentral-caplugin.csproj | Updated assembly version to 2.1.2 and file encoding |
| Constants.cs | Added DUPLICATE and INCLUDE_CLIENT_AUTH constants |
| CertCentralCertType.cs | Removed static caching of certificate types |
| OrderCertificate.cs | Added ProfileOption property for client auth EKU support |
| Duplicate.cs | New file implementing duplicate certificate request/response models |
| CertCentralClient.cs | Added DuplicateCertificate method |
| CertCentralCAPlugin.cs | Implemented duplicate functionality, client auth EKU support, sync window fix, and duplicate PEM handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| orderRequest.Certificate.ProfileType = profileType; | ||
| orderRequest.Certificate.ProfileType = profile; | ||
| } |
There was a problem hiding this comment.
This line has trailing whitespace after the closing brace. Consider removing the trailing whitespace to maintain code cleanliness and consistency.
| } | |
| } |
| if (typeOfCert.Equals("ssl") && Convert.ToBoolean(productInfo.ProductParameters[CertCentralConstants.Config.INCLUDE_CLIENT_AUTH])) | ||
| { | ||
| orderRequest.Certificate.ProfileOption = "server_client_auth_eku"; | ||
| _logger.LogWarning($"{CertCentralConstants.Config.INCLUDE_CLIENT_AUTH}: Ability to include client auth EKU in SSL certs is currently planned to cease in May 2026. Make sure any workflows that depend on this feature are updated before then to avoid interruptions."); | ||
| } |
There was a problem hiding this comment.
The code calls 'Convert.ToBoolean(productInfo.ProductParameters[CertCentralConstants.Config.INCLUDE_CLIENT_AUTH])' without first checking if the key exists in the dictionary. This will throw a KeyNotFoundException if the INCLUDE_CLIENT_AUTH parameter is not present. Since this is an OPTIONAL parameter (as documented), consider using 'ContainsKey' to check for existence first, or use 'TryGetValue', before attempting the conversion.
| /// <param name="request">The <see cref="OrderRequest"/>.</param> | ||
| /// <param name="enrollmentProductInfo">Information about the DigiCert product this certificate uses.</param> | ||
| /// <returns></returns> |
There was a problem hiding this comment.
The parameter documentation mentions 'request' with type 'OrderRequest', but this parameter does not exist in the method signature. The method actually has individual parameters like 'enrollmentProductInfo', 'caRequestId', 'commonName', etc. Update the documentation to accurately reflect the actual parameters or remove the incorrect parameter reference.
| /// <param name="request">The <see cref="OrderRequest"/>.</param> | |
| /// <param name="enrollmentProductInfo">Information about the DigiCert product this certificate uses.</param> | |
| /// <returns></returns> | |
| /// <param name="enrollmentProductInfo">Information about the DigiCert product this certificate uses.</param> | |
| /// <param name="caRequestId">The CA request identifier from which to derive the original order ID.</param> | |
| /// <param name="commonName">The common name (CN) for the certificate.</param> | |
| /// <param name="csr">The certificate signing request (CSR) for the new duplicate certificate.</param> | |
| /// <param name="dnsNames">Additional DNS names (SANs) to include in the duplicate certificate.</param> | |
| /// <param name="signatureHash">The signature hash algorithm to use.</param> | |
| /// <param name="caCertId">The DigiCert CA certificate identifier to use for issuance.</param> | |
| /// <returns>The result of the duplicate enrollment operation.</returns> |
| DuplicateRequest duplicateRequest = new DuplicateRequest(orderId) | ||
| { | ||
| Certificate = new CertificateDuplicateRequest | ||
| { | ||
| CommonName = commonName, | ||
| CSR = csr, | ||
| DnsNames = dnsNames, | ||
| SignatureHash = signatureHash, | ||
| CACertID = caCertId | ||
| } | ||
| }; |
There was a problem hiding this comment.
The DuplicateRequest is created without setting 'SkipApproval', which will default to false. The similar Reissue method explicitly sets 'SkipApproval = true' with a comment explaining it allows the certificate ID to return a value (see line 1479). Consider whether the Duplicate method should also set 'SkipApproval = true' for consistency and to ensure the certificate ID is returned, unless there's a specific reason for the different behavior.
| //Another check for duplicate PEMs to get arround issue with DigiCert API returning incorrect data sometimes on reissued/duplicate certs | ||
| if (pemList.Contains(certificate)) | ||
| { | ||
| _logger.LogWarning($"Found duplicate PEM for ID {caReqId}. Skipping..."); | ||
| continue; | ||
| } | ||
| pemList.Add(certificate); |
There was a problem hiding this comment.
The 'certificate' variable could be null if the certificate status is not GENERATED or REVOKED (line 1639-1650), but it's added to 'pemList' unconditionally at line 1658. This means null values could be added to the list, and the duplicate check at line 1653 would match multiple null values incorrectly. Consider only adding to pemList when certificate is not null, or restructure the logic to handle null certificates appropriately.
| //Another check for duplicate PEMs to get arround issue with DigiCert API returning incorrect data sometimes on reissued/duplicate certs | |
| if (pemList.Contains(certificate)) | |
| { | |
| _logger.LogWarning($"Found duplicate PEM for ID {caReqId}. Skipping..."); | |
| continue; | |
| } | |
| pemList.Add(certificate); | |
| //Another check for duplicate PEMs to get around issue with DigiCert API returning incorrect data sometimes on reissued/duplicate certs | |
| if (certificate != null) | |
| { | |
| if (pemList.Contains(certificate)) | |
| { | |
| _logger.LogWarning($"Found duplicate PEM for ID {caReqId}. Skipping..."); | |
| continue; | |
| } | |
| pemList.Add(certificate); | |
| } |
| using Newtonsoft.Json; | ||
|
|
||
| using Org.BouncyCastle.Asn1.X509; | ||
| using Org.BouncyCastle.Pqc.Crypto.Falcon; |
There was a problem hiding this comment.
The import 'using Org.BouncyCastle.Pqc.Crypto.Falcon;' appears to be unused. This namespace is typically used for post-quantum cryptography (Falcon signature scheme), but there are no references to Falcon types anywhere in this file. This unused import should be removed to keep the code clean and avoid confusion.
| using Org.BouncyCastle.Pqc.Crypto.Falcon; |
| _logger.MethodEntry(LogLevel.Trace); | ||
|
|
||
| lastSync = lastSync.HasValue ? lastSync.Value.AddHours(-7) : DateTime.MinValue; // DigiCert issue with treating the timezone as mountain time. -7 to accomodate DST | ||
| // DigiCert issue with treating the timezone as mountain time. -7 hours to accomodate DST |
There was a problem hiding this comment.
The comment contains a spelling error: 'accomodate' should be spelled 'accommodate' (with two 'm's).
| // DigiCert issue with treating the timezone as mountain time. -7 hours to accomodate DST | |
| // DigiCert issue with treating the timezone as mountain time. -7 hours to accommodate DST |
| throw new Exception($"Unexpected error downloading certificate {certId} for order {orderId}: {certificateChainResponse.Errors.FirstOrDefault()?.message}"); | ||
| } | ||
| } | ||
| //Another check for duplicate PEMs to get arround issue with DigiCert API returning incorrect data sometimes on reissued/duplicate certs |
There was a problem hiding this comment.
The comment contains a spelling error: 'arround' should be spelled 'around' (with one 'r').
| //Another check for duplicate PEMs to get arround issue with DigiCert API returning incorrect data sometimes on reissued/duplicate certs | |
| //Another check for duplicate PEMs to get around issue with DigiCert API returning incorrect data sometimes on reissued/duplicate certs |
| // If no last sync, use a 6 day window for the sync range (only relevant for incremental syncs) | ||
| lastSync = lastSync.HasValue ? lastSync.Value.AddHours(-7) : DateTime.UtcNow.AddDays(-5); | ||
| DateTime? utcDate = DateTime.UtcNow.AddDays(1); | ||
| if ((utcDate.Value - lastSync.Value).Days > 6) |
There was a problem hiding this comment.
The date calculation uses '.Days' property which returns only the whole number of days in the TimeSpan, potentially losing precision. For example, if the difference is 6.5 days, this will evaluate to 6 and the condition won't trigger. Consider using '.TotalDays' instead and comparing with a double value, or ensure this behavior is intentional for the sync window logic.
| if ((utcDate.Value - lastSync.Value).Days > 6) | |
| if ((utcDate.Value - lastSync.Value).TotalDays > 6.0) |
| else | ||
| { | ||
| profile = null; | ||
| } |
There was a problem hiding this comment.
The else block at lines 1788-1791 that sets 'profile = null' is redundant. The variable 'profile' is already initialized to null at line 1774, and the only assignment inside the if block (line 1777) converts the parameter to a string. If that string is empty, it won't pass the validation at line 1780-1786 and profile would remain as set. This else block appears unnecessary and could be removed for clarity.
| else | |
| { | |
| profile = null; | |
| } |
3 participants
Merge release-2.2 to main - Automated PR