Add assembly signing for Microsoft.Data.SqlClient and the AKV Provider - #4569
Add assembly signing for Microsoft.Data.SqlClient and the AKV Provider#4569paulmedynski wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR wires up internal CI support for strong-name signing of Microsoft.Data.SqlClient and the AKV provider by threading an isInternalBuild flag through pipeline templates, downloading signing keys from ADO secure files, and passing signing-related MSBuild properties into build.proj-driven build/test steps.
Changes:
- Plumbs a new
isInternalBuildparameter through core/stage templates so internal builds can enable extra steps (assembly signing). - Adds secure-file download + parameter plumbing to pass signing key paths into build/test template invocations (including separate test signing key support).
- Updates
build.projtest targetdotnet testinvocations to include signing-related MSBuild arguments (with review feedback about unintended signing of test assemblies).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| eng/pipelines/stages/build-sqlclient-package-ci-stage.yml | Adds isInternalBuild parameter and forwards it to the SqlClient package build job. |
| eng/pipelines/dotnet-sqlclient-ci-core.yml | Threads isInternalBuild into the SqlClient package stage and test stage template calls. |
| eng/pipelines/common/templates/steps/run-all-tests-step.yml | Adds signingKeyPath / testSigningKeyPath parameters and passes them through to build.proj test targets. |
| eng/pipelines/common/templates/steps/ci-project-build-step.yml | Adds signingKeyPath parameter and forwards it to build.proj for MDS/AKV builds. |
| eng/pipelines/common/templates/stages/ci-run-tests-stage.yml | Adds isInternalBuild parameter and forwards it into the test job template. |
| eng/pipelines/common/templates/jobs/ci-run-tests-job.yml | Downloads driver/test signing keys (internal + Package mode) and forwards their paths to the test step template. |
| eng/pipelines/common/templates/jobs/ci-build-nugets-job.yml | Downloads the driver signing key (internal + Package mode) and forwards it to the build step template for package builds. |
| build.proj | Threads signing-related properties into dotnet test commands; also uses SigningKeyPath to include/exclude category=signed tests. |
Suppressed comments (2)
build.proj:694
- Forwarding
SigningKeyPathinto the functional/manual test project build will strong-name sign those test assemblies with the driver key viasrc/Directory.Build.props. This is likely unnecessary (and potentially undesirable) if only UnitTests needs signing (withTestSigningKeyPath) to satisfyInternalsVisibleTofor signed packages.
<!-- Build arguments -->
-p:Configuration=$(Configuration)
$(SigningKeyPathArgument)
<!-- Test arguments -->
build.proj:728
- For UnitTests,
TestSigningKeyPathis the mechanism intended to sign the test assembly for IVT when the driver is signed in Package mode. ForwardingSigningKeyPathhere additionally signs the test assembly with the driver key (unless overridden) and enablesSTRONG_NAME_SIGNINGfor the test build, which seems outside the stated goal of signing tests only with the test key.
<!-- Build arguments -->
-p:Configuration=$(Configuration)
$(SigningKeyPathArgument)
$(TestSigningKeyPathArgument)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| <!-- Build arguments --> | ||
| -p:Configuration=$(Configuration) | ||
| $(SigningKeyPathArgument) | ||
|
|
||
| <!-- Test arguments --> |
ca6b9db to
bd2f4a9
Compare
Sign the SqlClient and AKV Provider assemblies, and the SqlClient test assemblies, on internal Package-mode CI builds. SqlClient references Abstractions, Logging and SqlServer.Server, so those packages must already be signed for this to build; a strong-named assembly referencing a weak-named one fails with CS8002. - Thread isInternalBuild from the CI core into the SqlClient package stage and the test stage, and download the driver and test signing keys in the build and test jobs. - Pass signingKeyPath to the project build step and both signing key paths to the test steps. - Thread SigningKeyPath into the build.proj TestSqlClientFunctional and TestSqlClientManual targets, and both key arguments into TestSqlClientUnit.
Pre-emptively align this PR with the review feedback already addressed lower in the stack, so the same comments aren't raised again. - Reference the split download-driver-signing-key-step.yml and download-test-signing-key-step.yml templates instead of the removed download-assembly-signing-key.yml. - Use positive referenceType comparisons (eq 'Package'). - Drop the isInternalBuild parameter defaults; every caller already passes it explicitly. - Use "strong-name signing" terminology and name the driver or test key in the signingKeyPath/testSigningKeyPath parameter docs, correcting the stale note about test-filter categories.
bd2f4a9 to
681bb35
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The changes affect CI orchestration and internal-only secure-file signing flows, which are high-impact and best validated via full pipeline execution and human review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
What's This All About?
This is PR 4 of 6 in the assembly signing stack that builds on #4568.
The first 5 PRs in this stack are all about adding assembly signing and public-key-protected InternalsVisibleTo support to the legacy CI pipeline. This is infrastructure work necessary to support the Native AOT fix in the final PR.
We have never been including assembly signing in our CI, which IMO was a blind spot. Now, when CI runs in our internal ADO.Net project, all assemblies will be signed, we will be running tests against signed assemblies, and fully testing our nascent inter-assembly IVT just as it would be in a real app. Public project CI and all of our PR pipelines will continue to use unsigned assemblies, and testing that requires inter-assembly IVT will only be done in Project mode (PR, legacy CI Project-mode pipeline) or via internal CI with signed assemblies.
The 6th and final PR in the stack addresses the Native AOT issue #4193 by eliminating inter-assembly reflection and using signed IVT with proper package dependencies.
Description
This PR sets up assembly signing in the legacy CI pipeline for
Microsoft.Data.SqlClientitself and for the Always Encrypted Azure Key Vault provider. These are the last two shipping packages in the legacy CI flow that were not receivingSigningKeyPath, so internal Package-mode builds could produce a mix of signed and unsigned artifacts.Unlike the earlier PRs in this stack, MDS already carries a public-key-qualified
InternalsVisibleTogrant forUnitTestsinMicrosoft.Data.SqlClient.csproj. What was missing was the pipeline plumbing to actually supply the keys, so that grant was never exercised. This PR supplies them.This PR:
isInternalBuildthrough the CI core into the SqlClient package stage, the MDS/AKV build job, and the test stage and test jobs;signingKeyPathparameter toci-project-build-step.ymland passes it through to both the MDS and AKV Provider builds;signingKeyPathandtestSigningKeyPathparameters torun-all-tests-step.ymland passes them to everybuild.projtest invocation - unit, functional and manual, including the flaky variants, the x86 variants, and the Linux/macOS paths - so tests always build and run against a consistently signed set of assemblies; andSigningKeyPathinto theTestSqlClientUnit,TestSqlClientFunctionalandTestSqlClientManualtargets inbuild.proj, and groups the arguments in those targets into build / test / reference-type sections to match the rest of the file.TestSqlClientUnitalready passedTestSigningKeyPath; without the matchingSigningKeyPaththe driver would be rebuilt unsigned underneath a signed test assembly, and the public-key-qualified IVT grant would not match.Supplying
SigningKeyPathactivates the existing signing behavior insrc/Directory.Build.props; this PR does not change product source, public APIs, package contents beyond assembly signing, or compatibility behavior.The second commit pre-applies the review feedback already addressed on #4566 and #4567 so the same comments aren't raised again here: the split
download-driver-signing-key-step.yml/download-test-signing-key-step.ymltemplates, positivereferenceTypecomparisons, no parameter defaults, and consistent "strong-name signing" terminology in the parameter documentation.Issues
Works towards addressing #4193.
Testing
This is a pipeline and
build.projchange, so no unit or integration tests were added.The coverage gain is that the full MDS test suite - unit, functional and manual - now runs against strong-named driver and test assemblies on internal CI. That is the configuration in which the
UnitTestspublic-key-qualified IVT grant actually has to match; previously it was only ever compiled in the unsigned configuration.The GitHub PR validation pipelines exercise the public Package-reference and Project-reference paths. The internal signing branch requires the ADO.Net secure files and is exercised only by an internal Package-reference pipeline run.
CI pipeline runs
The GitHub PR pipelines never set
isInternalBuild, so they always take the unsigned path. As with #4566 and #4567, the four CI runs below are what actually exercise this change, covering all four quadrants of the signing matrix. Run links and thetools/PackageValidatorartifact verification table will be added here once the runs complete.The expectation is that the MDS and AKV package stages and the test stages succeed in all four runs, that the
Download Driver Signing KeyandDownload Test Signing Keysteps appear only in MDS Main CI-Package, and that only that quadrant'sMDS.Artifactsand AKV packages reportSignedwith the23ec7fc2d6eaa4a5public key token.