Skip to content
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.

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

Invoke-AzKeyVaultKeyOperation uses ASCII encoding to get bytes for string and Default encoding for the inverse #21269

Closed
seanbamsft opened this issue Mar 21, 2023 · 4 comments · Fixed by #21753
Assignees
Labels
Azure PS Team bug This issue requires a change to an existing behavior in the product in order to be resolved. KeyVault Tracking We will track status and follow internally

Comments

@seanbamsft
Copy link
Contributor

Description

The Invoke-AzKeyVaultKeyOperation accepts a SecureString as input. For encrypt/wrap operations, the operation uses Encoding.ASCII.GetBytes() to convert the input to a byte[] for encryption. This results in data loss for any string that cannot be represented in the ASCII encoding. For compatibility, I'd recommend the UTF8 encoding instead.

Similarly, on decrypt/unwrap, PSKeyOperationResult uses System.Text.Encoding.Default which will vary based on platform. On .NET Core it'll be UTF8, but on .NET Framework it may be an ANSI code page. The same encoding needs to be used in both places to ensure that the original string is returned.

Issue script & Debug output

$result = Invoke-AzKeyVaultKeyOperation -Operation Encrypt -VaultName "myvault" -Algorithm RSA1_5 -Name "mykey" -Value (ConvertTo-SecureString -String "test`u{1f510}" -AsPlainText -Force)
$result2 = Invoke-AzKeyVaultKeyOperation -Operation Decrypt -VaultName "myvault" -Algorithm RSA1_5 -Name "mykey" -Value (ConvertTo-SecureString -String $result.Result -AsPlainText -Force)
$result2.Result

Environment data

Name                           Value
----                           -----
PSVersion                      7.3.3
PSEdition                      Core
GitCommitId                    7.3.3
OS                             Microsoft Windows 10.0.22621
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

Module versions

ModuleType Version    PreRelease Name                                ExportedCommands
---------- -------    ---------- ----                                ----------------
Script     2.12.0                Az.Accounts                         {Add-AzEnvironment, Clear-AzConfig, Clear-AzConte…
Script     4.9.2                 Az.KeyVault                         {Add-AzKeyVaultCertificate, Add-AzKeyVaultCertifi

Error output

No response

@seanbamsft seanbamsft added bug This issue requires a change to an existing behavior in the product in order to be resolved. needs-triage This is a new issue that needs to be triaged to the appropriate team. labels Mar 21, 2023
@ghost ghost removed the needs-triage This is a new issue that needs to be triaged to the appropriate team. label Mar 21, 2023
@isra-fel
Copy link
Member

Thanks for the suggestion @seanbamsft
@BethanyZhou could you evaulate the effort and impact?

@BethanyZhou
Copy link
Contributor

BethanyZhou commented Mar 22, 2023

This issue can be fixed within 3 business days. Main effort is test. User will decrypt failed if their input is ASCII encoded string which is encrypted in previous version after we adjust to UTF-encoding way. Should we fix this in this month?

@isra-fel
Copy link
Member

isra-fel commented Mar 22, 2023

Thanks @BethanyZhou . I agree with the benefits of using a fixed encoding instead of relying on the default encoding, which may be inconsistent across different systems, but I'm also worried about breaking those customers who are already depending on this behavior. So I'd prefer to do it in the upcoming major release.

cc @jlichwa

@isra-fel isra-fel added the Tracking We will track status and follow internally label Apr 3, 2023
@BethanyZhou BethanyZhou linked a pull request May 11, 2023 that will close this issue
1 task
@BethanyZhou
Copy link
Contributor

Hi @seanbamsft , we use uniform UTF8 as the encoding/decoding way between string and bytes. Please expert the changes available on "05/23/2023".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure PS Team bug This issue requires a change to an existing behavior in the product in order to be resolved. KeyVault Tracking We will track status and follow internally
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants