[KeyVault] Migrate BouncyCastle Library #28924
Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
There was a problem hiding this comment.
Pull request overview
This PR attempts to migrate the KeyVault module from using the Portable.BouncyCastle library to built-in .NET cryptography APIs for PEM key handling in security domain operations. The change addresses a CodeQL security finding about using approved cryptographic libraries by replacing third-party crypto code with .NET's native implementation.
Key changes:
- Replaced BouncyCastle's
PemReaderwith .NET'sRSA.ImportFromPemandImportFromEncryptedPemmethods - Removed the Portable.BouncyCastle NuGet package dependency
- Removed BouncyCastle assembly from module manifest
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/KeyVault/KeyVault/SecurityDomain/Models/CertKey.cs | Replaced BouncyCastle PEM parsing with .NET 5+ ImportFromPem methods; removed helper methods for RSA parameter conversion |
| src/KeyVault/KeyVault/Track2Models/Track2KeyVaultDataServiceClient.cs | Removed unused BouncyCastle X509 import |
| src/KeyVault/KeyVault/KeyVault.csproj | Removed Portable.BouncyCastle package reference |
| src/KeyVault/KeyVault/Az.KeyVault.psd1 | Removed BouncyCastle assembly from RequiredAssemblies list |
| src/KeyVault/KeyVault/ChangeLog.md | Added changelog entry describing the migration |
| rsa.ImportFromPem(pem); | ||
| } | ||
| else | ||
| { | ||
| rsa.ImportFromEncryptedPem(pem, password); |
There was a problem hiding this comment.
The methods ImportFromPem and ImportFromEncryptedPem require .NET 5.0 or later, but this project targets netstandard2.0 (as defined in Az.props). This will cause runtime errors when the code is executed on .NET Framework or .NET Standard 2.0 runtimes.
Since the BouncyCastle library was previously used to support these older runtimes, removing it without ensuring the replacement methods are available will break backwards compatibility. You need to either:
- Keep using a PEM parsing library compatible with netstandard2.0, or
- Add conditional compilation to use different implementations based on the target framework, or
- Upgrade the project's target framework to net5.0 or later (which may have broader implications for Azure PowerShell)
| * Updated security domain PEM key handling to use built-in .NET cryptography instead of Portable.BouncyCastle | ||
| - Maintains support for standard PKCS#1 and PKCS#8 keys while resolving the CodeQL cs/use-approved-crypto-library finding |
There was a problem hiding this comment.
The changelog entry uses technical acronyms "PEM", "PKCS#1", and "PKCS#8" without explanation. According to the ChangeLog.md guidelines for Azure PowerShell, less-obvious acronyms should be explained on first use in a release section, as the primary audience is Azure PowerShell users, not developers.
Consider updating to:
* Updated security domain PEM (Privacy Enhanced Mail) key handling to use built-in .NET cryptography instead of Portable.BouncyCastle
- Maintains support for standard PKCS#1 and PKCS#8 key formats while resolving the CodeQL cs/use-approved-crypto-library finding
Description
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.mdand reviewed the following information:ChangeLog.mdfile(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.## Upcoming Releaseheader in the past tense.ChangeLog.mdif no new release is required, such as fixing test case only.