Add assembly signing for Microsoft.Data.SqlClient.Extensions.Abstractions - #4568
Add assembly signing for Microsoft.Data.SqlClient.Extensions.Abstractions#4568paulmedynski wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the SqlClient build/test pipeline and the Extensions.Abstractions projects to support strong-name signing for internal (ADO.NET) package-mode builds, ensuring InternalsVisibleTo (IVT) continues to function when assemblies are signed.
Changes:
- Updated Abstractions test TFMs to build
net462only on Windows, matching the Abstractions build availability. - Added signed-assembly IVT handling in
Abstractions.csproj(public-key-qualified IVT when signing in Package mode). - Plumbed internal-build signing support through CI templates (download signing keys, pass signing properties, and propagate
isInternalBuild).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient.Extensions/Abstractions/test/Abstractions.Test.csproj | Makes net462 conditional on Windows and adds test-assembly signing when a test key is provided. |
| src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Abstractions.csproj | Adds IVT configuration for signed Package-mode builds using a public-key-qualified entry. |
| eng/pipelines/stages/build-abstractions-package-ci-stage.yml | Adds isInternalBuild parameter plumbing and passes computed package version into test jobs. |
| eng/pipelines/jobs/test-abstractions-package-ci-job.yml | Adds internal-build signing support and package-mode dependency download for Logging artifacts. |
| eng/pipelines/jobs/pack-abstractions-package-ci-job.yml | Adds internal-build signing support when packing Abstractions in Package mode. |
| eng/pipelines/dotnet-sqlclient-ci-core.yml | Threads isInternalBuild into the Abstractions build stage. |
| build.proj | Enables Abstractions tests to run in Package mode and supports signing arguments for internal builds. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| packagesToPack: $(project) | ||
| configurationToPack: ${{ parameters.buildConfiguration }} | ||
| packDirectory: $(dotnetPackagesDir) | ||
| verbosityToPack: ${{ parameters.dotnetVerbosity }} | ||
| buildProperties: SqlClientPackageVersion=${{ parameters.packageVersion }};ReferenceType=Package;BuildNumber=$(Build.BuildNumber);SigningKeyPath=$(driverKeyFile.secureFilePath) |
329c000 to
73cd48a
Compare
| <!-- | ||
| The Abstractions project is not built for net462 unless it is built for Windows. Thus, we | ||
| will not be able to fulfill the Abstractions for net462 reference unless we are building on | ||
| Windows. | ||
| --> |
| # The version to apply to the SqlClient family packages. This is used when | ||
| # referenceType is 'Package'. | ||
| - name: packageVersion | ||
| type: string | ||
|
|
| <!-- TestAbstractions: Runs Microsoft.Data.SqlClient.Extensions.Abstractions.Tests --> | ||
| <Target Name="TestAbstractions"> | ||
| <PropertyGroup> | ||
| <!-- | ||
| Note: This test exclusively uses project references, so neither ReferenceType nor any | ||
| package version arguments are specified in this command. | ||
| --> | ||
| <LogFilePrefix>AbstractionsTests-$(OS)</LogFilePrefix> | ||
| <LogFilePrefix Condition="'$(TestFramework)' != ''">$(LogFilePrefix)-$(TestFramework)</LogFilePrefix> | ||
|
|
73cd48a to
9936011
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The Abstractions pack/test pipeline jobs reference a signing-key download template that does not exist in the repo, which will break internal Package-mode CI runs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
| - ${{ if and(eq(parameters.isInternalBuild, true), ne(parameters.referenceType, 'Project')) }}: | ||
| - template: /eng/pipelines/common/steps/download-assembly-signing-key.yml@self |
| - template: /eng/pipelines/common/steps/download-assembly-signing-key.yml@self | ||
| - template: /eng/pipelines/common/steps/download-assembly-signing-key.yml@self | ||
| parameters: | ||
| isTest: true | ||
|
|
…ions Sign the Abstractions package and its test assembly on internal Package-mode builds, and make the Abstractions CI jobs actually honour Package mode. - Thread isInternalBuild through the Abstractions stage into the pack and test jobs, downloading the driver and test signing keys as needed. - Add a signed InternalsVisibleTo grant for the test assembly, and sign the test assembly when a test key is supplied. - Pass packageVersion, loggingArtifactsName and referenceType into the test jobs, and download the Logging artifacts, so Package mode is exercised. - Correct the pack job to use the canonical SqlClientPackageVersion and BuildNumber properties instead of undeclared per-package parameters. - Thread signing and reference-type arguments into the build.proj TestAbstractions target. - Build the Abstractions test project for net462 only on Windows hosts.
9936011 to
8fd209e
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The Abstractions CI jobs reference a non-existent signing-key download template and also need quoting fixes for secure-file paths to avoid pipeline/build breaks.
Review details
Suppressed comments (4)
eng/pipelines/jobs/test-abstractions-package-ci-job.yml:167
- The referenced template
/eng/pipelines/common/steps/download-assembly-signing-key.ymldoes not exist in the repo, so internal Package-mode builds will fail at template expansion time. Use the existing driver/test signing key download step templates instead.
- template: /eng/pipelines/common/steps/download-assembly-signing-key.yml@self
- template: /eng/pipelines/common/steps/download-assembly-signing-key.yml@self
parameters:
isTest: true
eng/pipelines/jobs/pack-abstractions-package-ci-job.yml:140
- The referenced template
/eng/pipelines/common/steps/download-assembly-signing-key.ymldoes not exist in the repo, so internal Package-mode packing will fail at template expansion time. Switch to the existing driver signing key download template.
- ${{ if and(eq(parameters.isInternalBuild, true), ne(parameters.referenceType, 'Project')) }}:
- template: /eng/pipelines/common/steps/download-assembly-signing-key.yml@self
eng/pipelines/jobs/pack-abstractions-package-ci-job.yml:152
SigningKeyPathshould quote$(driverKeyFile.secureFilePath)inbuildPropertiesto avoid breakingdotnet packwhen the secure file path contains spaces (consistent with other pack jobs in this repo).
buildProperties: SqlClientPackageVersion=${{ parameters.packageVersion }};ReferenceType=Package;BuildNumber=$(Build.BuildNumber);SigningKeyPath=$(driverKeyFile.secureFilePath)
eng/pipelines/jobs/test-abstractions-package-ci-job.yml:130
$(driverKeyFile.secureFilePath)/$(testKeyFile.secureFilePath)should be quoted when passed as MSBuild property values; otherwise, any spaces in the agent temp path will break argument parsing.
value: >-
-p:SigningKeyPath=$(driverKeyFile.secureFilePath)
-p:TestSigningKeyPath=$(testKeyFile.secureFilePath)
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
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. - Quote SigningKeyPath and TestSigningKeyPath to tolerate whitespace in the secure file paths. - Fold the driver key download into the existing signing conditional in the pack job, removing the duplicated conditional. - Use positive referenceType comparisons (eq 'Package'). - Drop the referenceType and isInternalBuild parameter defaults; both are already passed explicitly by every caller. - Move the BuildNumber/FileVersion note directly above buildProperties in every pack branch. - Restore "strong-name signing" terminology and name the driver or test key explicitly.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive and the only identified issue is a minor inaccurate comment in the test csproj that can be corrected without affecting behavior.
Review details
Suppressed comments (1)
src/Microsoft.Data.SqlClient.Extensions/Abstractions/test/Abstractions.Test.csproj:11
- The comment explaining the OS-conditional
net462target is inaccurate:Microsoft.Data.SqlClient.Extensions.Abstractionsisnetstandard2.0, so the limiting factor here is that .NET Framework (net462) can only be built/run on Windows. Reword this to avoid confusion for future maintainers.
<!--
The Abstractions project is not built for net462 unless it is built for Windows. Thus, we
will not be able to fulfill the Abstractions for net462 reference unless we are building on
Windows.
-->
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
What's This All About?
This is PR 3 of 6 in the assembly signing stack that builds on #4567.
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 the Abstractions project.
Microsoft.Data.SqlClient.Extensions.Abstractionswas not receivingSigningKeyPathwhen it was packed by the internal, Package-reference CI flow, so that package could be produced without the strong-name signing applied to the other internal package artifacts.Abstractions is the first project in this stack that has a test project, so it is also the first place where the test side of the story matters: once the product assembly is strong named, an unsigned test assembly can no longer be granted access via
InternalsVisibleTo. This PR therefore signs the test assembly with the test key and adds a public-key-qualified IVT grant to match.Along the way, the Abstractions CI jobs were not actually honouring Package mode, so this PR fixes that too - otherwise the signed Package-mode path would never have been exercised.
This PR:
isInternalBuildandreferenceTypethrough the CI core, the Abstractions stage, the pack job, and the three test jobs;SigningKeyPathwhen packingMicrosoft.Data.SqlClient.Extensions.Abstractions, and bothSigningKeyPathandTestSigningKeyPathwhen building the test project, for internal Package-reference builds only;InternalsVisibleTogrant for the test assembly, used only when the build is both signed and in Package mode. The existing unqualified grant still covers every unsigned configuration, and signed + Project mode is deliberately omitted so that production-signed assemblies never grant internal access to locally built test assemblies;TestSigningKeyPathis supplied, matching the pattern already used by the SqlClient test projects;packageVersion,loggingArtifactsNameandreferenceTypeand downloading the Logging package artifacts;SqlClientPackageVersionandBuildNumberproperties rather than undeclared per-package parameters;build.projTestAbstractionstarget, which previously assumed Project references only; andnet462target to Windows hosts, since the Abstractions project itself only targetsnet462on Windows and the reference would otherwise be unsatisfiable.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, quoted secure-file paths, positivereferenceTypecomparisons, no parameter defaults, the duplicated signing conditional collapsed, and consistent "strong-name signing" terminology.Issues
Works towards addressing #4193.
Testing
This is a pipeline and project-file change, so no unit or integration tests were added.
The change does add meaningful new test coverage, though: the Abstractions test suite now runs in Package mode against the Logging package artifacts, and on internal CI it runs with both the product and test assemblies strong named. That is the first time our inter-assembly IVT is exercised the way a real signed app would see it. A broken public-key-qualified IVT grant now shows up as a compile error in the test job rather than silently passing.
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
Build Abstractions Packagestage succeeds in all four runs, that theDownload Driver Signing KeyandDownload Test Signing Keysteps appear only in MDS Main CI-Package, and that only that quadrant'sAbstractions.Artifactspackage reportsSignedwith the23ec7fc2d6eaa4a5public key token.