[SQL] Add support for versionless TDE keys#32764
Conversation
|
Validation for Azure CLI Full Test Starting...
Thanks for your contribution! |
|
Validation for Breaking Change Starting...
Thanks for your contribution! |
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
microsoft-github-policy-service
bot
requested review from
calvinhzy,
evelyn-ys and
yonzhan
microsoft-github-policy-service
bot
added
Auto-Assign
There was a problem hiding this comment.
Pull request overview
This pull request adds support for versionless Transparent Data Encryption (TDE) Azure Key Vault (AKV) keys for SQL server and database-level Customer Managed Keys (CMK). This enhancement allows users to specify AKV keys without a version identifier, enabling automatic key rotation.
Changes:
- Updates azure-mgmt-sql dependency from 4.0.0b22 to 4.0.0b23 across all platform-specific requirements files
- Modifies
_get_server_key_name_from_urifunction to parse and handle versionless key URIs - Adds comprehensive test coverage for versionless key operations (create, show, set, delete)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| requirements.py3.windows.txt | Updates azure-mgmt-sql dependency to 4.0.0b23 for Windows |
| requirements.py3.Linux.txt | Updates azure-mgmt-sql dependency to 4.0.0b23 for Linux |
| requirements.py3.Darwin.txt | Updates azure-mgmt-sql dependency to 4.0.0b23 for macOS |
| custom.py | Modifies URI parsing logic and regex to support versionless AKV keys, adds mdep.azure.net domain |
| test_sql_commands.py | Adds comprehensive test coverage for versionless key CRUD operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Segments = ["/", "keys/", "someKey/", "01234567890123456789012345678901"] | ||
| # Therefore, a versionless key uri will have a segment array of length 3 and a versioned key uri will have a segment array of length 4. | ||
| # | ||
| isVersionlessKeyId = uri.Segments.Length == 3 |
There was a problem hiding this comment.
This line contains invalid Python syntax. In Python, strings don't have a Segments attribute or a Length property. This appears to be C# code accidentally included in the Python file. To determine if the URI is versionless, you should check if the URI ends with '/' or if splitting by '/' yields the key name as the last element (without a version). For example: version = uri.rstrip('/').split('/')[-1] and then check if version == key or if the URI ends with the key name.
| vault = uri.split('.')[0].split('/')[-1] | ||
| key = uri.split('/')[-2] | ||
| version = uri.split('/')[-1] |
There was a problem hiding this comment.
When the URI ends with a trailing slash (e.g., "https://vault.azure.net/keys/keyname/"), line 4822 will set version to an empty string, and line 4821 will incorrectly extract the key. For a versionless URI ending without trailing slash (e.g., "https://vault.azure.net/keys/keyname"), version will equal key, which will also cause issues. The logic needs to check whether the URI is versionless before extracting the version variable.
| import re | ||
|
|
||
| match = re.match(r'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]+$', uri) | ||
| match = re.match(r'^https://(?!.*\.\.)[a-zA-Z0-9][a-zA-Z0-9.-]+[a-zA-Z0-9]\.(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]+|\/|)$', uri) |
There was a problem hiding this comment.
The regex pattern allows URIs ending with '/' (via the \/| option), but this creates ambiguity in parsing. When the URI is "https://vault.azure.net/keys/keyname/", the pattern matches but uri.split('/')[-2] will get "keyname" and uri.split('/')[-1] will get an empty string. However, the comment in the error message says versionless keys should be "https://YourVaultName.vault.azure.net/keys/YourKeyName" (without trailing slash). Consider removing the \/| option from the regex to enforce the format without trailing slash, which will make parsing more straightforward.
| # The AKV segments is an array of the uri components. | ||
| # For AKV uri "https://someVault.vault.azure.net/keys/someKey/01234567890123456789012345678901", the segment contents are as follows: | ||
| # Segments = ["/", "keys/", "someKey/", "01234567890123456789012345678901"] | ||
| # Therefore, a versionless key uri will have a segment array of length 3 and a versioned key uri will have a segment array of length 4. | ||
| # | ||
| isVersionlessKeyId = uri.Segments.Length == 3 |
There was a problem hiding this comment.
The comment describes "Segments" array and references C# URI properties (uri.Segments.Length), but this is Python code. The comment should be updated to accurately describe Python string manipulation. For example: "For AKV uri 'https://someVault.vault.azure.net/keys/someKey/01234567890123456789012345678901', splitting by '/' yields ['https:', '', 'someVault.vault.azure.net', 'keys', 'someKey', '01234567890123456789012345678901']. A versionless key uri will have fewer segments after splitting."
| # The AKV segments is an array of the uri components. | |
| # For AKV uri "https://someVault.vault.azure.net/keys/someKey/01234567890123456789012345678901", the segment contents are as follows: | |
| # Segments = ["/", "keys/", "someKey/", "01234567890123456789012345678901"] | |
| # Therefore, a versionless key uri will have a segment array of length 3 and a versioned key uri will have a segment array of length 4. | |
| # | |
| isVersionlessKeyId = uri.Segments.Length == 3 | |
| # For an AKV or Managed HSM key id, we can split the URI on '/' to inspect its components. | |
| # For example, for AKV uri | |
| # "https://someVault.vault.azure.net/keys/someKey/01234567890123456789012345678901" | |
| # splitting by '/' yields: | |
| # ["https:", "", "someVault.vault.azure.net", "keys", "someKey", "01234567890123456789012345678901"] | |
| # A versionless key uri such as "https://someVault.vault.azure.net/keys/someKey" | |
| # becomes: | |
| # ["https:", "", "someVault.vault.azure.net", "keys", "someKey"] | |
| # Therefore, after stripping any trailing '/', a versionless key id has 5 segments and | |
| # a versioned key id has 6 segments. | |
| # | |
| uri_segments = uri.rstrip('/').split('/') | |
| isVersionlessKeyId = len(uri_segments) == 5 |
| isVersionlessKeyId = uri.Segments.Length == 3 | ||
|
|
||
| if isVersionlessKeyId: |
There was a problem hiding this comment.
Variable name isVersionlessKeyId does not follow Python naming conventions. Python uses snake_case for variable names, not camelCase. This should be is_versionless_key_id.
| isVersionlessKeyId = uri.Segments.Length == 3 | |
| if isVersionlessKeyId: | |
| is_versionless_key_id = uri.Segments.Length == 3 | |
| if is_versionless_key_id: |
| # create a versionless key | ||
| versionless_key_name = self.create_random_name(resource_prefix + 'vless', 32) | ||
| versionless_key_resp = self.cmd('keyvault key create -n {} -p software --vault-name {}' | ||
| .format(versionless_key_name, key_vault)).get_output_in_json() | ||
| versioned_kid = versionless_key_resp['key']['kid'] | ||
|
|
||
| # extract versionless key identifier (remove the version part) | ||
| # kid format: https://{vault}.vault.azure.net/keys/{keyname}/{version} | ||
| versionless_kid = '/'.join(versioned_kid.split('/')[:-1]) |
There was a problem hiding this comment.
The test only covers versionless key URIs without a trailing slash (line 4639 creates URIs like "https://vault.azure.net/keys/keyname"). However, the regex pattern in custom.py (line 4812) also accepts URIs with trailing slashes or even just ending with "/keys/keyname/". Consider adding test cases for these edge cases to ensure the parsing logic handles all accepted formats correctly, or update the regex to be more restrictive if these formats should not be supported.
|
Please fix CI issues |
4 participants
Related command
Description
This PR adds the support for versionless TDE AKV keys for server and database level CMK
Testing Guide
History Notes
This checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.