Pass ACS intermediate certificates to AzureSign.Core chain builder#1036
Open
Jaxelr wants to merge 2 commits into
Open
Pass ACS intermediate certificates to AzureSign.Core chain builder#1036Jaxelr wants to merge 2 commits into
Jaxelr wants to merge 2 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
ArtifactSigningServicefetches the full leaf + intermediates + root chain from Azure Code Signing but discards the intermediates, keeping only the leaf. This works for public trust since all the hierarchy certificates are automatically installed on windows, but not so for public trust test certificates. When the intermediates are not preinstalled in a Windows certificate store on the signing host,AzureSign.Core.AuthenticodeKeyVaultSigner's constructor throws:X509Chain.Build(leaf)cannot resolve the leaf's issuer because there is noExtraStoreand (no matching CA in the local stores. AIA fetch is also unreliable on ACS test PKIs where multiple intermediates share the same Subject Name but have different keys.Fix
Give the chain builder the intermediates that ACS already returned.
ICertificateProvider: addGetAdditionalCertificatesAsyncas a default interface method returning an empty collection. KeyVault, CertificateStore, and existing test stubs need no changes and keep their behavior.AzureSignToolSigner: fetch the collection and pass it toAuthenticodeKeyVaultSigner's existingadditionalCertificatesparameter, which populatesX509Chain.ChainPolicy.ExtraStorebeforeBuildis called.ArtifactSigningService: cache the non-leaf certificates from the P7B response and expose them through the new method. Leaf detection also switched from "last cert in the collection" to "the cert whose Subject is nobody else's Issuer" to be order-independent.RSAArtifactSigning: after receiving the signature from ACS, verify it against the leaf's public key viaRSA.VerifyHashand throwCryptographicExceptionon mismatch. Cheap defense-in-depth so protocol drift, key rotation races, or transport corruption fail at the source instead of producing an unverifiable signature embedded in the artifact.Tests
ArtifactSigningServiceTests(renamed fromTrustedSigningServiceTests) with two tests that build a synthetic 3-tier RSA chain, wrap it in a real PKCS#7, and verify:GetCertificateAsyncreturns the leaf.GetAdditionalCertificatesAsyncreturns the intermediate + root and does not contain the leaf.RSATrustedSigningTests.SignHash_UsesClientupdated to set up theVerifyHashmock now thatSignHashvalidates the returned signature.What this PR does not do
AzureSign.Core's chain build succeeds after this change, butSignerSignEx3(invoked later insidewintrust.dll) performs its own chain build that only consults Windows certificate stores. Signing hosts still need the intermediates and root reachable through the OS trust stores, exactly as before. This PR removes an incorrect failure inside our code; it does not remove the OS-level requirement that the chain be trusted on the signing machine.