Merge 3.2.2 to main#78
Conversation
…should not be exportable from KeyVault Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io>
Added entry parameter to indicate the private key should not be exportable from KeyVault Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io> --------- Co-authored-by: Joe VanWanzeele <76071503+joevanwanzeeleKF@users.noreply.github.com> Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io> * cleaned up docs, split RBAC permissions into seperate file for brevity * Update generated docs * Updated changelog, nuget package references * Explicit update of Newtonsoft.Json.Bson from 1.0.2 (used by Microsoft.AspNet.WebApi.Client) to 1.0.3 to address vulnerability --------- Co-authored-by: Morgan Gangwere <470584+indrora@users.noreply.github.com> Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io>
* Merge 3.2.0 to main (#74) * chore: create 3.2 branch * feat: release 3.2, Added entry parameter to indicate the private key should not be exportable from KeyVault Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io> --------- Co-authored-by: Joe VanWanzeele <76071503+joevanwanzeeleKF@users.noreply.github.com> Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io> * cleaned up docs, split RBAC permissions into seperate file for brevity * Update generated docs * Updated changelog, nuget package references * Explicit update of Newtonsoft.Json.Bson from 1.0.2 (used by Microsoft.AspNet.WebApi.Client) to 1.0.3 to address vulnerability * now returning the serialized certificate tags, as well as the exportable flag. Updated README image * Update generated docs --------- Co-authored-by: Morgan Gangwere <470584+indrora@users.noreply.github.com> Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io>
There was a problem hiding this comment.
Pull request overview
Automated merge of release-3.2.2 into main, updating the Azure Key Vault Universal Orchestrator extension with new/updated entry-parameter behavior and refreshed documentation/assets.
Changes:
- Added a new
NonExportableentry parameter and wired it through Management/Add and Azure Key Vault import policy. - Updated inventory output to return entry parameters (tags + non-exportable flag) with inventory items.
- Refreshed documentation (including splitting RBAC guidance into
rbac.md) and added store-type creation scripts.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
AzureKeyVault/Jobs/Management.cs |
Adds NonExportable handling; changes entry-parameter defaults and passes flag into import. |
AzureKeyVault/AzureClient.cs |
Adds non-exportable import policy and returns entry parameters during inventory. |
AzureKeyVault/Constants.cs |
Adds NonExportable entry-parameter constant. |
AzureKeyVault/AzureKeyVault.csproj |
Updates dependency versions; adds a new package reference. |
integration-manifest.json |
Adds NonExportable entry parameter to the store type definition. |
scripts/store_types/** |
Adds generated scripts for creating the AKV store type via REST, curl, and kfutil. |
README.md |
Major doc edits including discovery steps, RBAC pointer, and updated installation matrix. |
docsource/content.md |
Mirrors README doc updates for generated documentation. |
rbac.md |
New RBAC guidance doc extracted from README. |
CHANGELOG.md |
Adds 3.2.2 entry and notes new behaviors. |
docsource/images/AKV-entry-parameters-store-type-dialog.png |
Updated screenshot asset. |
AzureKeyVault.sln |
Adds rbac.md to Solution Items. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| tagsJSON = config.JobProperties[EntryParameters.TAGS] as string ?? string.Empty; | ||
| preserveTags = config.JobProperties[EntryParameters.PRESERVE_TAGS] as bool? ?? false; | ||
| preserveTags = config.JobProperties[EntryParameters.PRESERVE_TAGS] as bool? ?? true; |
There was a problem hiding this comment.
PreserveExistingTags is documented/declared with a default of False (see integration-manifest and store-type scripts), but the runtime default here is true when the entry parameter is missing/null. This changes behavior (tags will be preserved even if the user didn't opt in). Consider defaulting to false and only enabling preservation when the entry parameter is explicitly set to true.
| preserveTags = config.JobProperties[EntryParameters.PRESERVE_TAGS] as bool? ?? true; | |
| preserveTags = config.JobProperties[EntryParameters.PRESERVE_TAGS] as bool? ?? false; |
| using Keyfactor.Orchestrators.Common.Enums; | ||
| using Keyfactor.Orchestrators.Extensions; | ||
| using Microsoft.Extensions.Logging; | ||
| using Microsoft.VisualBasic; |
There was a problem hiding this comment.
Unused using Microsoft.VisualBasic; appears to have been added but nothing in this file references it. Please remove the unused import to avoid unnecessary dependencies and compiler warnings.
| using Microsoft.VisualBasic; |
| if (cert.Value.Policy != null) // set nonexportable entry parameter to value | ||
| { | ||
| var exportable = cert.Value.Policy?.Exportable; | ||
| itemEntryParams.Add(EntryParameters.NON_EXPORTABLE, !exportable); |
There was a problem hiding this comment.
cert.Value.Policy?.Exportable is nullable; using !exportable can yield null, which means NonExportable may be returned as a null value even though the store type defines it as a Bool. Consider normalizing this to a concrete boolean (or omitting the parameter when the exportable state is unknown) so inventory consistently returns the expected type.
| itemEntryParams.Add(EntryParameters.NON_EXPORTABLE, !exportable); | |
| if (exportable.HasValue) | |
| { | |
| itemEntryParams.Add(EntryParameters.NON_EXPORTABLE, !exportable.Value); | |
| } |
| <PackageReference Include="System.Drawing.Common" Version="9.0.2" /> | ||
| <PackageReference Include="Microsoft.Identity.Client" Version="4.82.1" /> | ||
| <PackageReference Include="Microsoft.Identity.Client.Extensions.Msal" Version="4.82.1" /> | ||
| <PackageReference Include="Newtonsoft.Json.Bson" Version="1.0.3" /> |
There was a problem hiding this comment.
Newtonsoft.Json.Bson is referenced in the project file but does not appear to be used anywhere in the codebase. Removing unused package references helps reduce build time, artifact size, and dependency surface area.
| <PackageReference Include="Newtonsoft.Json.Bson" Version="1.0.3" /> |
| elif [ -n "${KEYFACTOR_AUTH_CLIENT_ID}" ] && [ -n "${KEYFACTOR_AUTH_CLIENT_SECRET}" ] && [ -n "${KEYFACTOR_AUTH_TOKEN_URL}" ]; then | ||
| echo "Fetching OAuth token..." | ||
| BEARER_TOKEN=$(curl -s -X POST "${KEYFACTOR_AUTH_TOKEN_URL}" \ | ||
| -H "Content-Type: application/x-www-form-urlencoded" \ | ||
| --data-urlencode "grant_type=client_credentials" \ | ||
| --data-urlencode "client_id=${KEYFACTOR_AUTH_CLIENT_ID}" \ | ||
| --data-urlencode "client_secret=${KEYFACTOR_AUTH_CLIENT_SECRET}" | jq -r '.access_token') | ||
| if [ -z "${BEARER_TOKEN}" ] || [ "${BEARER_TOKEN}" = "null" ]; then | ||
| echo "ERROR: Failed to fetch OAuth token from ${KEYFACTOR_AUTH_TOKEN_URL}" | ||
| exit 1 |
There was a problem hiding this comment.
The OAuth token flow pipes the token response through jq, but the script never checks that jq is installed. Add a dependency check (similar to the kfutil scripts) or avoid jq so the script fails with a clear error instead of command not found.
| "/subscriptions/{subscriptionId1}", // allow to be applied to a specific subscription | ||
| "/subscriptions/{subscriptionId2}", // and another.. etc. | ||
| "/subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}", // allow to be scoped to a specific resource group | ||
| "/subscriptions/{subscriptionId2}/resourcegroups/{resourceGroupName2}", // and another.. | ||
| "/providers/Microsoft.Management/managementGroups/{groupId1}" // allow to be applied for all subscriptions under management group | ||
| ], | ||
| "permissions": [ | ||
| { | ||
| "actions": [ | ||
| "Microsoft.KeyVault/vaults/*", | ||
| "Microsoft.Authorization/*/read", | ||
| "Microsoft.KeyVault/register/action", | ||
| "Microsoft.KeyVault/checkNameAvailability/read", | ||
| "Microsoft.KeyVault/vaults/accessPolicies/*", | ||
| "Microsoft.Resources/deployments/*", | ||
| "Microsoft.Resources/subscriptions/resourceGroups/read", | ||
| "Microsoft.Management/managementGroups/read", | ||
| "Microsoft.Resources/subscriptions/read", | ||
| "Microsoft.Authorization/roleAssignments/*", | ||
| "Microsoft.KeyVault/operations/read" | ||
| "Microsoft.KeyVault/locations/*/read", | ||
| "Microsoft.KeyVault/vaults/*/read", |
There was a problem hiding this comment.
The JSON in this example role definition is not valid as written (missing a comma after "Microsoft.KeyVault/operations/read" and has a trailing comma after the last entry). Since this is meant to be copy/pasted, please fix the example so it parses as valid JSON.
| "/subscriptions/{subscriptionId1}", // allow to be applied to a specific subscription | |
| "/subscriptions/{subscriptionId2}", // and another.. etc. | |
| "/subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}", // allow to be scoped to a specific resource group | |
| "/subscriptions/{subscriptionId2}/resourcegroups/{resourceGroupName2}", // and another.. | |
| "/providers/Microsoft.Management/managementGroups/{groupId1}" // allow to be applied for all subscriptions under management group | |
| ], | |
| "permissions": [ | |
| { | |
| "actions": [ | |
| "Microsoft.KeyVault/vaults/*", | |
| "Microsoft.Authorization/*/read", | |
| "Microsoft.KeyVault/register/action", | |
| "Microsoft.KeyVault/checkNameAvailability/read", | |
| "Microsoft.KeyVault/vaults/accessPolicies/*", | |
| "Microsoft.Resources/deployments/*", | |
| "Microsoft.Resources/subscriptions/resourceGroups/read", | |
| "Microsoft.Management/managementGroups/read", | |
| "Microsoft.Resources/subscriptions/read", | |
| "Microsoft.Authorization/roleAssignments/*", | |
| "Microsoft.KeyVault/operations/read" | |
| "Microsoft.KeyVault/locations/*/read", | |
| "Microsoft.KeyVault/vaults/*/read", | |
| "/subscriptions/{subscriptionId1}", | |
| "/subscriptions/{subscriptionId2}", | |
| "/subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}", | |
| "/subscriptions/{subscriptionId2}/resourcegroups/{resourceGroupName2}", | |
| "/providers/Microsoft.Management/managementGroups/{groupId1}" | |
| ], | |
| "permissions": [ | |
| { | |
| "actions": [ | |
| "Microsoft.KeyVault/vaults/*", | |
| "Microsoft.Authorization/*/read", | |
| "Microsoft.KeyVault/register/action", | |
| "Microsoft.KeyVault/checkNameAvailability/read", | |
| "Microsoft.KeyVault/vaults/accessPolicies/*", | |
| "Microsoft.Resources/deployments/*", | |
| "Microsoft.Resources/subscriptions/resourceGroups/read", | |
| "Microsoft.Management/managementGroups/read", | |
| "Microsoft.Resources/subscriptions/read", | |
| "Microsoft.Authorization/roleAssignments/*", | |
| "Microsoft.KeyVault/operations/read", | |
| "Microsoft.KeyVault/locations/*/read", | |
| "Microsoft.KeyVault/vaults/*/read" |
| - 3.2.0 | ||
| - Added an optional entry parameter to indicate whether the private key of the cert should be not exportable when stored in KeyVault | ||
| - Now specifying the pkcs12 format when wirting certs to Azure KeyVault. This should prevent the error when a PEM cert was added outside of Command and then we attempt to update without specifying the format (Azure assumes PEM and throws an error if not). | ||
|
|
There was a problem hiding this comment.
Spelling: wirting should be writing. Also consider removing the stray tab-only line above to keep the changelog clean and consistent.
| It allows the assignment of granular level, inheretable access control on both the contents of the KeyVaults, as well as higher-level | ||
| management operations. For more information and a comparison of the two access control strategies, refer | ||
| to [this article](learn.microsoft.com/en-us/azure/key-vault/general/rbac-access-policy). |
There was a problem hiding this comment.
The RBAC article link is missing a URL scheme (https://), so it will be treated as a relative link on GitHub. Also, inheretable is misspelled (should be inheritable).
| It allows the assignment of granular level, inheretable access control on both the contents of the KeyVaults, as well as higher-level | |
| management operations. For more information and a comparison of the two access control strategies, refer | |
| to [this article](learn.microsoft.com/en-us/azure/key-vault/general/rbac-access-policy). | |
| It allows the assignment of granular level, inheritable access control on both the contents of the KeyVaults, as well as higher-level | |
| management operations. For more information and a comparison of the two access control strategies, refer | |
| to [this article](https://learn.microsoft.com/en-us/azure/key-vault/general/rbac-access-policy). |
| It allows the assignment of granular level, inheretable access control on both the contents of the KeyVaults, as well as higher-level | ||
| management operations. For more information and a comparison of the two access control strategies, refer | ||
| to [this article](learn.microsoft.com/en-us/azure/key-vault/general/rbac-access-policy). |
There was a problem hiding this comment.
Spelling: inheretable should be inheritable. Also the RBAC article link is missing https://, which will break the link on GitHub.
| It allows the assignment of granular level, inheretable access control on both the contents of the KeyVaults, as well as higher-level | |
| management operations. For more information and a comparison of the two access control strategies, refer | |
| to [this article](learn.microsoft.com/en-us/azure/key-vault/general/rbac-access-policy). | |
| It allows the assignment of granular level, inheritable access control on both the contents of the KeyVaults, as well as higher-level | |
| management operations. For more information and a comparison of the two access control strategies, refer | |
| to [this article](https://learn.microsoft.com/en-us/azure/key-vault/general/rbac-access-policy). |
| > :warning: You still may decide to split the capabilities into seperate roles in order to apply each of them to the | ||
| > lowest level scope |
There was a problem hiding this comment.
Spelling: seperate should be separate.
Merge release-3.2 to main - Automated PR