Skip to content

Added entry parameter for preserving certificate Tags #64

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
spbsoluble merged 8 commits into from
Oct 1, 2025
Merged

Added entry parameter for preserving certificate Tags #64

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a808ec9
Added additional null check for certificate tags
joevanwanzeeleKF Apr 23, 2025
fad860b
Added entry parameter to indicate whether existing Tags should be re-…
joevanwanzeeleKF Sep 19, 2025
f671487
Update generated docs
Sep 19, 2025
641b134
additional logging; fixed unresolving AzureCloud value
joevanwanzeeleKF Sep 23, 2025
6bb0e69
changelog
joevanwanzeeleKF Sep 23, 2025
c22733d
Update generated docs
Sep 23, 2025
cdd4568
Fix duplicate entry in CHANGELOG for version 3.1.10
spbsoluble Sep 25, 2025
ee418c1
Merge pull request #65 from Keyfactor/government_cloud_debug
spbsoluble Sep 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions AzureKeyVault/AkvProperties.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ internal protected string VaultEndpoint
{
return PrivateEndpoint.TrimStart('.');
}
switch (AzureCloud)
switch (AzureCloud?.Trim()?.ToLowerInvariant())
{
case "china":
return "vault.azure.cn";
Expand All @@ -58,6 +58,6 @@ internal protected string VaultEndpoint
}
}
}
internal protected string VaultURL => $"https://{VaultName}.{VaultEndpoint}/";
public string VaultURL => $"https://{VaultName}.{VaultEndpoint}/";
}
}
36 changes: 14 additions & 22 deletions AzureKeyVault/AzureClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,11 @@
using Azure.ResourceManager.KeyVault;
using Azure.ResourceManager.KeyVault.Models;
using Azure.ResourceManager.Resources;
using Azure.ResourceManager.Resources.Models;
using Azure.Security.KeyVault.Certificates;
using Keyfactor.Logging;
using Keyfactor.Orchestrators.Common.Enums;
using Keyfactor.Orchestrators.Extensions;
using Microsoft.Extensions.Logging;
using Newtonsoft.Json;

namespace Keyfactor.Extensions.Orchestrator.AzureKeyVault
{
Expand All @@ -34,16 +32,19 @@ private Uri AzureCloudEndpoint
{
get
{
switch (VaultProperties.AzureCloud?.ToLower())
logger.LogTrace($"the AzureCloud is {VaultProperties.AzureCloud}, so we will use the following endpoint for authentication: ");
switch (VaultProperties.AzureCloud?.Trim()?.ToLowerInvariant())
{

case "china":
logger.LogTrace(AzureAuthorityHosts.AzureChina.ToString());
return AzureAuthorityHosts.AzureChina;
//case "germany":
// return AzureAuthorityHosts.AzureGermany; // germany is no longer a valid azure authority host as of 2021
case "government":
logger.LogTrace(AzureAuthorityHosts.AzureGovernment.ToString());
return AzureAuthorityHosts.AzureGovernment;
default:
logger.LogTrace(AzureAuthorityHosts.AzurePublicCloud.ToString());
return AzureAuthorityHosts.AzurePublicCloud;
}
}
Expand Down Expand Up @@ -94,6 +95,8 @@ internal protected virtual ArmClient getArmClient(string tenantId)
{
TokenCredential credential;
var credentialOptions = new DefaultAzureCredentialOptions { AuthorityHost = AzureCloudEndpoint, AdditionallyAllowedTenants = { "*" } };
logger.LogTrace($"creating an ARM client for management operations with authorityhost {AzureCloudEndpoint.ToString()}");

if (this.VaultProperties.UseAzureManagedIdentity)
{
logger.LogTrace("getting management client for a managed identity");
Expand Down Expand Up @@ -196,7 +199,7 @@ public virtual async Task<KeyVaultResource> CreateVault()
}
}

public virtual async Task<KeyVaultCertificateWithPolicy> ImportCertificateAsync(string certName, string contents, string pfxPassword, string tags = null)
public virtual async Task<KeyVaultCertificateWithPolicy> ImportCertificateAsync(string certName, string contents, string pfxPassword, Dictionary<string,string> tags)
Copy link

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method signature change from string tags = null to Dictionary<string,string> tags is a breaking change. Consider adding method overloads to maintain backward compatibility or make the parameter nullable with Dictionary<string,string>? tags = null.

Copilot uses AI. Check for mistakes.
{
try
{
Expand All @@ -217,25 +220,14 @@ public virtual async Task<KeyVaultCertificateWithPolicy> ImportCertificateAsync(

logger.LogTrace($"calling ImportCertificateAsync on the KeyVault certificate client to import certificate {certName}");

var tagDict = new Dictionary<string, string>();
var options = new ImportCertificateOptions(certName, p12bytes);

if (!string.IsNullOrEmpty(tags))
if (tags.Any())
{
if (!tags.IsValidJson())
foreach (var tag in tags)
{
logger.LogError($"the entry parameter provided for Certificate Tags: \" {tags} \", does not seem to be valid JSON.");
throw new Exception($"the string \" {tags} \" is not a valid json string. Please enter a valid json string for CertificateTags in the entry parameter or leave empty for no tags to be applied.");
options.Tags.Add(tag);
}
logger.LogTrace($"converting the json value provided for tags into a Dictionary<string,string>");
tagDict = JsonConvert.DeserializeObject<Dictionary<string, string>>(tags);
logger.LogTrace($"{tagDict.Count} tag(s) will be associated with the certificate in Azure KeyVault");
}

var options = new ImportCertificateOptions(certName, p12bytes);

foreach (var tag in tagDict.Keys)
{
options.Tags.Add(tag, tagDict[tag]);
}

var cert = await CertClient.ImportCertificateAsync(options);
Expand Down Expand Up @@ -396,9 +388,9 @@ public virtual (List<string>, List<string>) GetVaults()
var warning = $"Exception thrown performing discovery on tenantId {searchTenantId} and subscription ID {searchSubscription}. Exception message: {ex.Message}";

logger.LogWarning(warning);
warnings.Add(warning);
//throw;
warnings.Add(warning);
}

return (vaultNames, warnings);
}
}
Expand Down
5 changes: 5 additions & 0 deletions AzureKeyVault/Constants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ static class AzureKeyVaultConstants
public const string STORE_TYPE_NAME = "AKV";
}

static class EntryParameters {
public const string TAGS = "CertificateTags";
public const string PRESERVE_TAGS = "PreserveExistingTags";
}

static class JobTypes
{
public const string CREATE = "Create";
Expand Down
11 changes: 6 additions & 5 deletions AzureKeyVault/Jobs/AzureKeyVaultJob.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,8 @@ namespace Keyfactor.Extensions.Orchestrator.AzureKeyVault
public abstract class AzureKeyVaultJob<T> : IOrchestratorJobExtension
{
public string ExtensionName => AzureKeyVaultConstants.STORE_TYPE_NAME;

internal protected virtual AzureClient AzClient { get; set; }
internal protected virtual AkvProperties VaultProperties { get; set; }

internal protected IPAMSecretResolver PamSecretResolver { get; set; }
internal protected ILogger logger { get; set; }

Expand Down Expand Up @@ -96,12 +94,15 @@ public void InitializeStore(dynamic config)
VaultProperties.SubscriptionId = properties.SubscriptionId ?? VaultProperties.SubscriptionId;
VaultProperties.ResourceGroupName = !string.IsNullOrEmpty(properties.ResourceGroupName as string) ? properties.ResourceGroupName : VaultProperties.ResourceGroupName;
VaultProperties.VaultName = properties.VaultName ?? VaultProperties.VaultName; // check the field in case of legacy paths.

VaultProperties.TenantId = !string.IsNullOrEmpty(VaultProperties.TenantId) ? VaultProperties.TenantId : config.CertificateStoreDetails?.ClientMachine; // Client Machine could be null in the case of managed identity. That's ok.
VaultProperties.AzureCloud = !string.IsNullOrEmpty(properties.AzureCloud as string) ? properties.AzureCloud : VaultProperties.AzureCloud;
VaultProperties.PrivateEndpoint = !string.IsNullOrEmpty(properties.PrivateEndpoint as string) ? properties.PrivateEndpoint : VaultProperties.PrivateEndpoint;

VaultProperties.AzureCloud = properties.AzureCloud;
logger.LogTrace($"Azure Cloud: {VaultProperties.AzureCloud}");
VaultProperties.PrivateEndpoint = properties.PrivateEndpoint;
logger.LogTrace($"Private Endpoint: {VaultProperties.PrivateEndpoint}");
string skuType = !string.IsNullOrEmpty(properties.SkuType as string) ? properties.SkuType : null;
VaultProperties.PremiumSKU = skuType?.ToLower() == "premium";

VaultProperties.VaultRegion = !string.IsNullOrEmpty(properties.VaultRegion as string) ? properties.VaultRegion : VaultProperties.VaultRegion;
VaultProperties.VaultRegion = VaultProperties.VaultRegion?.ToLower();
}
Expand Down
2 changes: 1 addition & 1 deletion AzureKeyVault/Jobs/Inventory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public JobResult ProcessJob(InventoryJobConfiguration config, SubmitInventoryUpd

inventoryItems = AzClient.GetCertificatesAsync().Result?.ToList();

logger.LogTrace($"Found {inventoryItems.Count()} Total Certificates in Azure Key Vault.");
logger.LogTrace($"Found {inventoryItems.Count} Total Certificates in Azure Key Vault.");
}

catch (Exception ex)
Expand Down
67 changes: 60 additions & 7 deletions AzureKeyVault/Jobs/Management.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
using Keyfactor.Orchestrators.Extensions;
using Microsoft.Extensions.Logging;
using Keyfactor.Orchestrators.Extensions.Interfaces;
using System.Collections.Generic;
using Newtonsoft.Json;

namespace Keyfactor.Extensions.Orchestrator.AzureKeyVault
{
Expand All @@ -38,8 +40,18 @@ public JobResult ProcessJob(ManagementJobConfiguration config)
Result = OrchestratorJobStatusJobResult.Failure,
FailureMessage = "Invalid Management Operation"
};
object tagsObj;
object preserveTagsObj;
string tagsJSON;
bool preserveTags;

var tagsJSON = config.JobProperties["CertificateTags"]?.ToString();
config.JobProperties.TryGetValue("CertificateTags", out tagsObj);

config.JobProperties.TryGetValue(EntryParameters.PRESERVE_TAGS, out preserveTagsObj);

preserveTags = (bool)preserveTagsObj;
Copy link

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direct casting to bool can throw InvalidCastException if preserveTagsObj is null or not a boolean. Use safe casting with as bool? and provide a default value, or check for null first.

Suggested change
preserveTags = (bool)preserveTagsObj;
preserveTags = preserveTagsObj as bool? ?? false;

Copilot uses AI. Check for mistakes.

tagsJSON = tagsObj as string;

switch (config.OperationType)
{
Expand All @@ -49,8 +61,8 @@ public JobResult ProcessJob(ManagementJobConfiguration config)
break;
case CertStoreOperationType.Add:
logger.LogDebug($"Begin Management > Add...");
complete = PerformAddition(config.JobCertificate.Alias, config.JobCertificate.PrivateKeyPassword, config.JobCertificate.Contents, tagsJSON, config.JobHistoryId, config.Overwrite);

complete = PerformAddition(config.JobCertificate.Alias, config.JobCertificate.PrivateKeyPassword, config.JobCertificate.Contents, tagsJSON, config.JobHistoryId, config.Overwrite, preserveTags);
break;
case CertStoreOperationType.Remove:
logger.LogDebug($"Begin Management > Remove...");
Expand Down Expand Up @@ -92,10 +104,25 @@ protected async Task<JobResult> PerformCreateVault(long jobHistoryId)
#endregion

#region Add
protected virtual JobResult PerformAddition(string alias, string pfxPassword, string entryContents, string tagsJSON, long jobHistoryId, bool overwrite)
protected virtual JobResult PerformAddition(string alias, string pfxPassword, string entryContents, string tagsJSON, long jobHistoryId, bool overwrite, bool preserveTags)
{
var complete = new JobResult() { Result = OrchestratorJobStatusJobResult.Failure, JobHistoryId = jobHistoryId };

var tagDict = new Dictionary<string, string>();

if (!string.IsNullOrEmpty(tagsJSON))
{
if (!tagsJSON.IsValidJson())
{
logger.LogError($"the entry parameter provided for Certificate Tags: \" {tagsJSON} \", does not seem to be valid JSON.");
throw new Exception($"the string \" {tagsJSON} \" is not a valid json string. Please enter a valid json string for CertificateTags in the entry parameter or leave empty for no tags to be applied.");
}
else
{
tagDict = JsonConvert.DeserializeObject<Dictionary<string, string>>(tagsJSON);
}
}

if (!string.IsNullOrWhiteSpace(pfxPassword)) // This is a PFX Entry
{
if (string.IsNullOrWhiteSpace(alias))
Expand All @@ -106,11 +133,27 @@ protected virtual JobResult PerformAddition(string alias, string pfxPassword, st

try
{
var existingTags = new Dictionary<string, string>();
logger.LogTrace($"checking for an existing cert with the alias {alias}");
var existing = AzClient.GetCertificate(alias).Result;
Copy link

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using .Result on async operations can cause deadlocks and blocks the thread. Consider making this method async and using await, or use GetAwaiter().GetResult() if async conversion is not possible.

Copilot uses AI. Check for mistakes.
if (existing != null)
{
logger.LogTrace($"there is an existing cert..");
}

existingTags = existing?.Properties.Tags as Dictionary<string, string> ?? new Dictionary<string, string>();
Copy link

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cast as Dictionary<string, string> may fail silently if Properties.Tags is not actually a Dictionary<string, string>. Consider using a safer conversion method or checking the type first.

Suggested change
existingTags = existing?.Properties.Tags as Dictionary<string, string> ?? new Dictionary<string, string>();
if (existing?.Properties.Tags is IDictionary<string, string> tagsDict)
{
existingTags = new Dictionary<string, string>(tagsDict);
}
else
{
existingTags = new Dictionary<string, string>();
}

Copilot uses AI. Check for mistakes.

logger.LogTrace("existing cert tags: ");
if (!existingTags.Any()) logger.LogTrace("(none)");

foreach (var tag in existingTags)
{
logger.LogTrace(tag.Key + " : " + tag.Value);
}

// if overwrite is unchecked, check for an existing cert first
if (!overwrite)
{
logger.LogTrace($"checking for an existing cert with the alias {alias}");
var existing = AzClient.GetCertificate(alias).Result;
if (existing != null)
{
var message = $"A certificate named {alias} already exists and the overwrite checkbox was unchecked. No action was taken.";
Expand All @@ -120,8 +163,18 @@ protected virtual JobResult PerformAddition(string alias, string pfxPassword, st
return complete;
}
}
else if (preserveTags)
{ // if preserveTags is true, we want to get the existing tags before replacing the cert
foreach (var existingTag in existingTags)
{
if (!tagDict.ContainsKey(existingTag.Key)) // if it's not being overwritten by what was provided..
{
tagDict[existingTag.Key] = existingTag.Value; // then we include it
}
}
}

var cert = AzClient.ImportCertificateAsync(alias, entryContents, pfxPassword, tagsJSON).Result;
var cert = AzClient.ImportCertificateAsync(alias, entryContents, pfxPassword, tagDict).Result;

// Ensure the return object has a AKV version tag, and Thumbprint
if (!string.IsNullOrEmpty(cert.Properties.Version) &&
Expand Down
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
- 3.1.9
- Added optional entry parameter to indicate that existing tags should be preserved if certificate is replaced
- bug fix for government cloud host name resolution

- 3.1.8
- Fixed bug where enrollment would fail if the CertificateTags field was not defined as an entry parameter
- Convert to .net6/8 dual build
Expand Down
Loading
Loading