Skip to content

Conversation

simonzhang0428
Copy link

@simonzhang0428 simonzhang0428 commented Oct 10, 2025

Description

SQL already made Management Service change to support versionless key as parameter, including the scenarios like add versionless key into server, set it as server Encryption Protector, create/update database when supply it as keys or encryption protector.
We want to update corresponding PowerShell also support these new functionalities.

Mandatory Checklist

  • SHOULD update ChangeLog.md file(s) appropriately
    • Update src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.
      • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header in the past tense.
    • Should not change ChangeLog.md if no new release is required, such as fixing test case only.
  • SHOULD regenerate markdown help files if there is cmdlet API change. Instruction
  • SHOULD have proper test coverage for changes in pull request.
  • SHOULD NOT adjust version of module manually in pull request

(cherry picked from commit e35f2a44f89d17a5dadc1433e92ef6ad15e42ac3)
@Copilot Copilot AI review requested due to automatic review settings October 10, 2025 05:23
Copy link

Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for versionless Azure Key Vault keys across SQL server key management operations. The change allows users to specify key IDs without version identifiers in addition to the existing versioned key support.

Key changes include:

  • Updated key validation regex to accept both versioned and versionless key formats
  • Modified server key name generation logic to handle optional version segments
  • Enhanced documentation and help text to clarify versionless key support

Reviewed Changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/Sql/Sql/Common/TdeKeyHelper.cs Core logic changes to support versionless key parsing and validation
src/Sql/Sql/Properties/Resources.resx Updated error message to include versionless key format examples
src/Sql/Sql/help/Set-AzSqlServerTransparentDataEncryptionProtector.md Added documentation for versionless key support in TDE protector
src/Sql/Sql/help/Get-AzSqlServerKeyVaultKey.md Updated help text to mention versionless key ID support
src/Sql/Sql/help/Add-AzSqlServerKeyVaultKey.md Added versionless key documentation to server key addition
Files not reviewed (1)
  • src/Sql/Sql/Properties/Resources.Designer.cs: Language not supported

// Validate that the url is a keyvault url and has a key and version
Regex r = new Regex(@"https://(.)+\.(managedhsm.azure.net|managedhsm-preview.azure.net|vault.azure.net|vault-int.azure-int.net|vault.azure.cn|managedhsm.azure.cn|vault.usgovcloudapi.net|managedhsm.usgovcloudapi.net|vault.microsoftazure.de|managedhsm.microsoftazure.de|vault.cloudapi.eaglex.ic.gov|vault.cloudapi.microsoft.scloud)(:443)?\/keys/[^\/]+\/[0-9a-zA-Z]+$", RegexOptions.IgnoreCase);
// Validate that the url is a keyvault url and has a key with an optional version
Regex r = new Regex(@"^https://(.)+\.(managedhsm\.azure\.net|managedhsm-preview\.azure\.net|vault\.azure\.net|vault-int\.azure-int\.net|vault\.azure\.cn|managedhsm\.azure\.cn|vault\.usgovcloudapi\.net|managedhsm\.usgovcloudapi\.net|vault\.microsoftazure\.de|managedhsm\.microsoftazure\.de|vault\.cloudapi\.eaglex\.ic\.gov|vault\.cloudapi\.microsoft\.scloud|mdep\.azure\.net)(:443)?/keys/[^/]+(/([0-9a-zA-Z]+))?/?$", RegexOptions.IgnoreCase);
Copy link

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

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

This regex pattern is extremely long and difficult to read/maintain. Consider extracting the domain patterns into a separate constant or using a more readable approach with string concatenation or interpolation.

Suggested change
Regex r = new Regex(@"^https://(.)+\.(managedhsm\.azure\.net|managedhsm-preview\.azure\.net|vault\.azure\.net|vault-int\.azure-int\.net|vault\.azure\.cn|managedhsm\.azure\.cn|vault\.usgovcloudapi\.net|managedhsm\.usgovcloudapi\.net|vault\.microsoftazure\.de|managedhsm\.microsoftazure\.de|vault\.cloudapi\.eaglex\.ic\.gov|vault\.cloudapi\.microsoft\.scloud|mdep\.azure\.net)(:443)?/keys/[^/]+(/([0-9a-zA-Z]+))?/?$", RegexOptions.IgnoreCase);
const string AllowedKeyVaultDomainsPattern =
@"managedhsm\.azure\.net|managedhsm-preview\.azure\.net|vault\.azure\.net|vault-int\.azure-int\.net|vault\.azure\.cn|managedhsm\.azure\.cn|vault\.usgovcloudapi\.net|managedhsm\.usgovcloudapi\.net|vault\.microsoftazure\.de|managedhsm\.microsoftazure\.de|vault\.cloudapi\.eaglex\.ic\.gov|vault\.cloudapi\.microsoft\.scloud|mdep\.azure\.net";
string pattern = $@"^https://(.)+\.({AllowedKeyVaultDomainsPattern})(:443)?/keys/[^/]+(/([0-9a-zA-Z]+))?/?$";
Regex r = new Regex(pattern, RegexOptions.IgnoreCase);

Copilot uses AI. Check for mistakes.

string vault = uri.Host.Split('.').First();
string key = uri.Segments[2].TrimEnd('/');
string version = uri.Segments.Last();
string[] pathSegments = uri.AbsolutePath.Trim('/').Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries);
Copy link

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

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

Potential IndexOutOfRangeException if pathSegments has fewer than 2 elements. The code assumes pathSegments[1] exists without validation, but if the URI path structure is unexpected, this will throw an exception.

Suggested change
string[] pathSegments = uri.AbsolutePath.Trim('/').Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries);
string[] pathSegments = uri.AbsolutePath.Trim('/').Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries);
if (pathSegments.Length < 2)
{
throw new ArgumentException(message:String.Format(Properties.Resources.InvalidKeyId, keyId), paramName:"KeyId");
}

Copilot uses AI. Check for mistakes.

@isra-fel
Copy link
Member

/azp run

Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants