diff --git a/AzureKeyVault.Tests/AzureKeyVault.Tests.csproj b/AzureKeyVault.Tests/AzureKeyVault.Tests.csproj index 9cff98f..e2bfae0 100644 --- a/AzureKeyVault.Tests/AzureKeyVault.Tests.csproj +++ b/AzureKeyVault.Tests/AzureKeyVault.Tests.csproj @@ -32,7 +32,6 @@ - diff --git a/AzureKeyVault.Tests/ManagementTests.cs b/AzureKeyVault.Tests/ManagementTests.cs index c3871c3..d5c5e3a 100644 --- a/AzureKeyVault.Tests/ManagementTests.cs +++ b/AzureKeyVault.Tests/ManagementTests.cs @@ -6,6 +6,7 @@ using System; using System.Collections.Generic; using System.Reflection; +using Azure.ResourceManager.KeyVault; using Azure.Security.KeyVault.Certificates; using FluentAssertions; using Keyfactor.Logging; @@ -42,6 +43,54 @@ public class ManagementTests private const string EmptyTags = ""; private const long JobHistoryId = 42; + // ── Create ──────────────────────────────────────────────────────────── + + /// + /// Regression test for: "The given key 'CertificateTags' was not present in the dictionary." + /// A Create operation does not supply entry parameters (Tags, PreserveTags, NonExportable) + /// in JobProperties. ProcessJob must not throw a KeyNotFoundException before reaching + /// PerformCreateVault. + /// + [Fact] + public void Create_EmptyJobProperties_DoesNotThrow_AndSucceeds() + { + var mockClient = new Mock(); + mockClient + .Setup(c => c.CreateVault()) + .ReturnsAsync(BuildFakeKeyVaultResource("test-vault")); + + var resolverMock = new Mock(); + resolverMock.Setup(r => r.Resolve(It.IsAny())).Returns(s => s); + + var job = new TestableManagement(resolverMock.Object) + { + AzClient = mockClient.Object, + Logger = LogHandler.GetClassLogger() + }; + + var config = new ManagementJobConfiguration + { + OperationType = CertStoreOperationType.Create, + JobHistoryId = JobHistoryId, + JobProperties = new Dictionary(), // no entry parameters — intentionally empty + ServerUsername = "test-client-id", + ServerPassword = "test-client-secret", + CertificateStoreDetails = new CertificateStore + { + StorePath = "00000000-0000-0000-0000-000000000000:test-rg:test-vault", + ClientMachine = "00000000-0000-0000-0000-000000000001", // TenantId + Properties = "{\"AzureCloud\":\"\",\"PrivateEndpoint\":\"\",\"SkuType\":\"Standard\",\"VaultRegion\":\"\",\"SubscriptionId\":\"\",\"ResourceGroupName\":\"\",\"VaultName\":\"\"}" + } + }; + + JobResult result = null; + var act = () => { result = job.ProcessJob(config); }; + + act.Should().NotThrow("ProcessJob must not throw a KeyNotFoundException when entry parameters are absent for a Create operation"); + result.Should().NotBeNull(); + result.Result.Should().Be(OrchestratorJobStatusJobResult.Success); + } + // ── Add: success cases ──────────────────────────────────────────────── [Fact] @@ -238,6 +287,79 @@ public void Remove_DeleteThrows_ReturnsFailWithMessage() result.FailureMessage.Should().Contain("vault unreachable"); } + // ── Add: AKV alias validation (regression for cryptic AKV "invalid name" error) ───── + + /// + /// Regression test: when the AKV SDK rejects an invalid alias with its + /// "The request URI contains an invalid name" error, PerformAddition should + /// translate that into a friendly message listing the AKV naming rules + /// rather than surfacing the raw SDK exception. + /// + [Theory] + [InlineData("linux01.kf.baah.net")] // contains dots + [InlineData("my_cert")] // contains underscore + [InlineData("1starts-with-digit")] // starts with a digit + [InlineData("has spaces")] // contains spaces + public void Add_InvalidAlias_ReturnsFriendlyErrorMessage(string invalidAlias) + { + var job = BuildJob(out var mockClient); + + // Simulate the exact AKV SDK error message we're translating + var akvError = $"One or more errors occurred. (The request URI contains an invalid name: {invalidAlias} " + + $"Status: 400 (Bad Request) ErrorCode: BadParameter Content: " + + $"{{\"error\":{{\"code\":\"BadParameter\",\"message\":\"The request URI contains an invalid name: {invalidAlias}\"}}}})"; + mockClient + .Setup(c => c.ImportCertificateAsync( + invalidAlias, It.IsAny(), It.IsAny(), + It.IsAny>(), It.IsAny())) + .ThrowsAsync(new Exception(akvError)); + + var result = job.CallPerformAddition( + invalidAlias, CertificateFixtures.PfxPassword, CertificateFixtures.Rsa2048Base64, + EmptyTags, JobHistoryId, overwrite: true, preserveTags: false, nonExportable: false); + + result.Result.Should().Be(OrchestratorJobStatusJobResult.Failure); + + // Should NOT contain the raw SDK error noise + result.FailureMessage.Should().NotContain("Status: 400"); + result.FailureMessage.Should().NotContain("BadParameter"); + result.FailureMessage.Should().NotContain("request URI"); + + // Should explain the AKV alias rules + result.FailureMessage.Should().Contain("127", + "the message should mention the 127-character length limit"); + result.FailureMessage.Should().Contain("alphanumeric", + "the message should mention the alphanumeric-only rule"); + result.FailureMessage.Should().Contain("letter", + "the message should mention that the alias must start with a letter"); + } + + /// + /// Make sure the friendly-error branch is specific to the AKV "invalid name" + /// signature - other exceptions should still fall through to the generic + /// failure path so we don't mask unrelated problems. + /// + [Fact] + public void Add_GenericException_DoesNotReturnAliasValidationMessage() + { + var job = BuildJob(out var mockClient); + mockClient + .Setup(c => c.ImportCertificateAsync( + It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny>(), It.IsAny())) + .ThrowsAsync(new Exception("unrelated network failure")); + + var result = job.CallPerformAddition( + Alias, CertificateFixtures.PfxPassword, CertificateFixtures.Rsa2048Base64, + EmptyTags, JobHistoryId, overwrite: true, preserveTags: false, nonExportable: false); + + result.Result.Should().Be(OrchestratorJobStatusJobResult.Failure); + result.FailureMessage.Should().Contain("unrelated network failure"); + // The AKV-naming-rules text should NOT appear for unrelated errors + result.FailureMessage.Should().NotContain("127"); + result.FailureMessage.Should().NotContain("alphanumeric"); + } + // ── Helpers ─────────────────────────────────────────────────────────── private static TestableManagement BuildJob(out Mock mockClient) @@ -312,5 +434,21 @@ private static FieldInfo FindField(Type type, string fieldName) } return null; } + + /// + /// Builds a minimal KeyVaultResource whose Id contains the vault name, + /// satisfying PerformCreateVault's success check. + /// + private static KeyVaultResource BuildFakeKeyVaultResource(string vaultName) + { + // KeyVaultResource cannot be newed directly; use a Mock so we can + // control the Id property that PerformCreateVault inspects. + var mock = new Mock(); + var resourceId = Azure.Core.ResourceIdentifier.Parse( + $"/subscriptions/00000000-0000-0000-0000-000000000000" + + $"/resourceGroups/test-rg/providers/Microsoft.KeyVault/vaults/{vaultName}"); + mock.Setup(r => r.Id).Returns(resourceId); + return mock.Object; + } } } diff --git a/AzureKeyVault/Jobs/Management.cs b/AzureKeyVault/Jobs/Management.cs index e8a69b3..984b7c0 100644 --- a/AzureKeyVault/Jobs/Management.cs +++ b/AzureKeyVault/Jobs/Management.cs @@ -49,9 +49,15 @@ public JobResult ProcessJob(ManagementJobConfiguration config) Logger.LogTrace("parsing entry parameters.. "); - tagsJSON = config.JobProperties[EntryParameters.TAGS] as string ?? string.Empty; - preserveTags = config.JobProperties[EntryParameters.PRESERVE_TAGS] as bool? ?? true; - nonExportable = config.JobProperties[EntryParameters.NON_EXPORTABLE] as bool? ?? false; + tagsJSON = config.JobProperties.ContainsKey(EntryParameters.TAGS) + ? config.JobProperties[EntryParameters.TAGS] as string ?? string.Empty + : string.Empty; + preserveTags = config.JobProperties.ContainsKey(EntryParameters.PRESERVE_TAGS) + ? config.JobProperties[EntryParameters.PRESERVE_TAGS] as bool? ?? true + : true; + nonExportable = config.JobProperties.ContainsKey(EntryParameters.NON_EXPORTABLE) + ? config.JobProperties[EntryParameters.NON_EXPORTABLE] as bool? ?? false + : false; switch (config.OperationType) { @@ -191,10 +197,26 @@ protected virtual JobResult PerformAddition(string alias, string pfxPassword, st } catch (Exception ex) { - complete.FailureMessage = $"An error occurred while adding {alias} to {ExtensionName}: " + ex.Message; + if (ex.Message.ToLowerInvariant().Contains("the request uri contains an invalid name")) + { + // The alias is not valid; return an error explaining the AKV naming rules. + // Note: this branch is tied to AKV's current "invalid name" error string; + // if AKV changes its error format we'll fall through to the generic branch. + var errMsg = $"The alias '{alias}' was not allowed by Azure Key Vault. The alias must:\n" + + $"\t- be under 127 characters in length\n" + + $"\t- contain only alphanumeric characters and dashes\n" + + $"\t- begin with a letter\n" + + $"Please update the alias to meet these requirements."; + + complete.FailureMessage = errMsg; + } + else + { + complete.FailureMessage = $"An error occurred while adding {alias} to {ExtensionName}: " + ex.Message; - if (ex.InnerException != null) - complete.FailureMessage += " - " + ex.InnerException.Message; + if (ex.InnerException != null) + complete.FailureMessage += " - " + ex.InnerException.Message; + } } } else // Non-PFX diff --git a/CHANGELOG.md b/CHANGELOG.md index ffa2a22..a5584a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +- 3.2.4 + - Bug fix: Fix for error during create job due to missing entry parameters + - Made 'alias' officially required for enrollment + - Documentation improvements and updates + - 3.2.3 - Bug fix: there was an issue where we were not passing the Key Size to Azure, and it was causing an error when the default didn't match - Now checking for empty vault name property to avoid overriding an existing value during Store Creation - [Issue 39](https://github.com/Keyfactor/azurekeyvault-orchestrator/issues/39#issuecomment-4298537246) diff --git a/README.md b/README.md index 19a3dd6..2770f6e 100644 --- a/README.md +++ b/README.md @@ -378,6 +378,15 @@ Azure and clicking "Properties" in the left menu. > :warning: The identity you are using for authentication will need to have sufficient Azure permissions to be able to > create new Keyvaults. +### Certificate Enrollment Alias Requirements + +Azure Keyvault has the following restrictions on certificate naming: +- Must be alphanumeric characters and dashes +- Must be < 127 characters +- Must start with a letter + +If, during enrollment, an alias is provided that does not meet the above requirements; the job will fail with an error message indicating the reason. + --- @@ -477,7 +486,7 @@ the Keyfactor Command Portal ##### Advanced Tab | Attribute | Value | Description | | --------- | ----- | ----- | - | Supports Custom Alias | Optional | Determines if an individual entry within a store can have a custom Alias. | + | Supports Custom Alias | Required | Determines if an individual entry within a store can have a custom Alias. | | Private Key Handling | Optional | This determines if Keyfactor can send the private key associated with a certificate to the store. Required because IIS certificates without private keys would be invalid. | | PFX Password Style | Default | 'Default' - PFX password is randomly generated, 'Custom' - PFX password may be specified when the enrollment job is created (Requires the Allow Custom Password application setting to be enabled.) | diff --git a/docsource/content.md b/docsource/content.md index d75faed..39d3668 100644 --- a/docsource/content.md +++ b/docsource/content.md @@ -336,4 +336,13 @@ Azure and clicking "Properties" in the left menu. > :warning: The identity you are using for authentication will need to have sufficient Azure permissions to be able to > create new Keyvaults. +### Certificate Enrollment Alias Requirements + +Azure Keyvault has the following restrictions on certificate naming: +- Must be alphanumeric characters and dashes +- Must be < 127 characters +- Must start with a letter + +If, during enrollment, an alias is provided that does not meet the above requirements; the job will fail with an error message indicating the reason. + --- \ No newline at end of file diff --git a/integration-manifest.json b/integration-manifest.json index 11ceec8..334124c 100644 --- a/integration-manifest.json +++ b/integration-manifest.json @@ -19,7 +19,7 @@ "BlueprintAllowed": false, "Capability": "AKV", "ClientMachineDescription": "The GUID of the tenant ID of the Azure Keyvault instance; for example, '12345678-1234-1234-1234-123456789abc'.", - "CustomAliasAllowed": "Optional", + "CustomAliasAllowed": "Required", "EntryParameters": [ { "Name": "CertificateTags", diff --git a/scripts/store_types/bash/curl_create_store_types.sh b/scripts/store_types/bash/curl_create_store_types.sh index a743ab3..40f32ee 100755 --- a/scripts/store_types/bash/curl_create_store_types.sh +++ b/scripts/store_types/bash/curl_create_store_types.sh @@ -75,7 +75,7 @@ create_store_type() { create_store_type "AKV" '{ "BlueprintAllowed": false, "Capability": "AKV", - "CustomAliasAllowed": "Optional", + "CustomAliasAllowed": "Required", "EntryParameters": [ { "Name": "CertificateTags", diff --git a/scripts/store_types/powershell/restmethod_create_store_types.ps1 b/scripts/store_types/powershell/restmethod_create_store_types.ps1 index 2eeb0b3..cfa73b7 100644 --- a/scripts/store_types/powershell/restmethod_create_store_types.ps1 +++ b/scripts/store_types/powershell/restmethod_create_store_types.ps1 @@ -68,7 +68,7 @@ New-StoreType "AKV" @' { "BlueprintAllowed": false, "Capability": "AKV", - "CustomAliasAllowed": "Optional", + "CustomAliasAllowed": "Required", "EntryParameters": [ { "Name": "CertificateTags",