Use Azure SDK signing and migrate acceptance tests to an emulator - #1222
Merged
usmansaleem merged 29 commits intoSep 3, 2026
Merged
Conversation
…ainers emulator Adds an AzureKeyVaultEmulator Testcontainers DSL class (ghcr.io/usmansaleem/azure-keyvault-emulator:v2.3.4) that starts a shared emulator container, generates a self-signed TLS cert, and seeds the exact BLS/SECP fixtures previously documented for manual import into real Azure. AzureKeyVaultAcceptanceTest, BlsSigningAcceptanceTest.ableToSignUsingAzure, SecpSigningAcceptanceTest.signDataWithKeyInAzure, and KeyIdentifiersAcceptanceTest.azureKeysReturnAppropriatePublicKey now run against the emulator with zero external Azure credentials. CI no longer needs any AZURE_* secrets. Threads a new AzureKeyVaultParameters.getEndpointOverride() through the config chain (CLI option, YAML metadata, DefaultAzureKeyVaultParameters, AzureConfig), mirroring the existing AWS endpoint-override pattern, so AzureKeyVault can target the emulator instead of *.vault.azure.net, use a fixed emulator bearer token, and disable Azure SDK challenge-resource verification. Fixes found only by actually running the emulator end-to-end (see individual commits/diffs for detail): - pin linux/amd64 platform for Testcontainers (fixed upstream in v2.3.4, which now ships native arm64 too, so this pin has since been removed again) - pin 127.0.0.1 instead of localhost for the container URL (Docker Desktop for Mac IPv6 quirk) - make the generated PKCS12 cert world-readable (container runs as non-root) - always set explicit enabled=true on seeded secrets (Secrets SDK NPEs on the emulator's default omission) - pin the resolved key version for the raw-REST sign call instead of the empty "latest" placeholder Also fixes the actual root-cause blocker upstream in usmansaleem/azure-keyvault-emulator: its ES256K Sign/Verify operations were re-hashing an already-computed digest before signing (via ECDsa.SignData instead of SignHash), producing signatures that don't verify against real Azure Key Vault's documented Sign semantics. Fixed and released as v2.3.4; SecpSigningAcceptanceTest.signDataWithKeyInAzure is verified working end-to-end against the real published image.
3 tasks
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
The emulator's self-signed cert wasn't trusted by the ambient JVM trust store, causing intermittent SSLHandshakeException/PKIX failures when tests ran out-of-process. Collapse the ad-hoc endpoint-override field into a single AzureOverrides record and thread explicit trust and authority configuration through the whole chain instead. - Add AzureOverrides(endpointOverride, authorityHostOverride, trustCertificateOverride) record; replace the single endpointOverride field on AzureKeyVaultParameters/AzureConfig/AzureSecretSigningMetadata/ AzureKeySigningMetadata (+ deserializers) and their PicoCli/Default implementations with one getAzureOverrides() accessor. - AzureHttpClientParameters/AzureHttpClientFactory build an explicit Netty trust manager from the override certificate instead of relying on -Djavax.net.ssl.trustStore, which a lazily-initialized JVM SSLContext could ignore. - AzureKeyVault: unconditionally build ClientSecretCredentialBuilder with authorityHost + disableInstanceDiscovery, disable challenge resource verification, wire the shared trust-aware HttpClient into both SecretClient/KeyClient and fetchKey's CryptographyClientBuilder (previously fetchKey built its own default client, bypassing trust config). Drop the emulator-specific token credential/mode. - Add hidden --Xazure-authority-host-override and --Xazure-trust-certificate-override CLI options alongside the existing endpoint override, following the project's --X experimental option convention. - Acceptance-test DSL: expose the emulator's trust cert path from AzureKeyVaultEmulator, wire it through MetadataFileHelpers YAML and CmdLineParamsConfigFileImpl/CmdLineParamsDefaultImpl, and update BlsSigningAcceptanceTest, SecpSigningAcceptanceTest, KeyIdentifiersAcceptanceTest and AzureKeyVaultAcceptanceTest to pass the override so tests no longer depend on the ambient trust store.
The in-JVM mock Microsoft Entra ID authority didn't belong inside AzureKeyVaultEmulator, and its lifecycle (a hand-rolled HttpsServer, never explicitly stopped, propped up by a daemon-thread workaround) was harder to reason about than plain JUnit5 lifecycle callbacks. - Add MockAzureAuthorityExtension: a BeforeAllCallback/AfterAllCallback extension backed by WireMock (org.wiremock:wiremock:3.13.2, pinned in gradle/versions.gradle), configured with the emulator's own PKCS12 keystore over HTTPS and a single catch-all stub returning the emulator's documented dummy JWT. Registered via @RegisterExtension in each of the four Azure test classes, so it starts once before and stops once after that class's tests - deterministic teardown, no more reliance on Gradle's test-worker force-kill or daemon threads to avoid hanging a bare JVM. - AzureKeyVaultEmulator no longer contains any HTTP-serving code; it's back to owning just the Testcontainers container, cert generation and fixture seeding, as a JVM-wide singleton. - Because each of the four test classes now gets its own mock-authority instance/port, and AzureKeyVaultAcceptanceTest's getSECPKeysFromEmulator/ getBLSSecretsFromEmulator helpers are called directly from sibling test classes, parameterize both to take the caller's own AzureOverrides instead of reaching into AzureKeyVaultAcceptanceTest's own state - removes a class-load-time NPE hazard and any dependency on which class's extension started first. - Replace the openssl ProcessBuilder cert generation in AzureKeyVaultEmulator with the existing BouncyCastle-backed SelfSignedCertificate/CertificateHelpers test fixtures already used elsewhere (e.g. HashicorpNode), adding a generate(List<String>) overload for the extra localhost.vault.azure.net SAN instead of a third cert-generation implementation. - keyManagerPassword must be set explicitly alongside keystorePassword on WireMockConfiguration - it defaults to "password" independently and otherwise fails PKCS12 key-entry decryption.
Java 25 text blocks support backslash line-continuation to suppress the inserted newline, so the three JWT segments can be laid out one per line without altering the resulting string (verified byte-for-byte identical to the prior concatenation).
…ackage The emulator's PFX carried a redundant cert-only alias (clientCert) alongside the key alias (client) that WireMock/Kestrel actually need. Empirically verified (a) the SunJSSE PKCS12KeyStore enumerates key entries before cert-only entries regardless of alias name or insertion order on this JDK, so the reused dsl/tls/support/CertificateHelpers' positional certificates().get(0)/keys().get(0) pairing wasn't actually broken in practice, but (b) it was relying on that undocumented provider-internal ordering rather than an API guarantee, and this PR was the first caller to feed that helper a multi-alias PKCS12. Drop the redundant setCertificateEntry call - TrustManagerFactory already derives a trust anchor from a key entry's own chain (verified), so nothing depended on the separate cert-only alias. Also move CertificateHelpers/SelfSignedCertificate out of the hashicorp-specific dsl.certificates package into tech.pegasys.web3signer.keystore.dsl.certificates, since they're now shared fixtures used by both Hashicorp and Azure emulator acceptance tests, not something hashicorp-specific.
certificates() and keys() held onto the java.util.Enumeration returned by KeyStore.aliases() and drove it manually via hasMoreElements()/nextElement(), which is what PMD/Error Prone's JdkObsolete check flags. Convert immediately to Collections.list(...) and iterate with a standard for-each, dropping the two @SuppressWarnings("JdkObsolete") annotations - same behavior, no obsolete collection API surface.
Production AzureKeyVault never pins a service version - it uses the SDK default (getLatest()) for both SecretClientBuilder/KeyClientBuilder, and explicitly calls KeyServiceVersion.getLatest() for the raw REST sign call, matching what a real user hits against live Azure. Pinning V7_4 only in the test-side fixture-seeding client made the tests exercise a narrower, artificially older surface than production traffic, for no benefit: empirically verified (full Azure acceptance suite rerun) that removing the pin still passes against the emulator. Confirmed separately that the azure-keyvault-emulator project itself already uses a proper _OR_LATER cascading build-constant scheme (KEYVAULT_API_7_5_OR_LATER/7_6_OR_LATER/20250701_OR_LATER) with its default KeyVaultApiVersion at 2025-07-01, not a flat 7.4 pin - nothing to change there.
Single azureKeyVaultEmulatorImage property (registry+repo+tag), same convention as besuVersion/hashicorpVaultVersion. Makes future upstream migration a one-line change instead of hunting across code and README.
There was a problem hiding this comment.
🟡 Changes recommended
The Azure parameter interface breaks existing implementations, and legacy SDK signing lacks regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
signing/src/test/java/tech/pegasys/web3signer/signing/secp256k1/azure/AzureKeyVaultSignerTest.java:59
- The SDK-signing test always selects the modern
ES256Kpath, while production still supports legacySECP256K1keys viaECDSA256. Parameterize this test (or add a second case) to select the deprecated algorithm and verify that exact value is sent toCryptographyClient; otherwise the migrated legacy-key path can regress without any unit or emulator coverage.
- Files reviewed: 47/47 changed files
- Comments generated: 1
- Review effort level: Balanced
ensureSecretsInKeyVaultAreLoadedAndReportedViaPublicKeysApi now runs with both config-file (true) and CLI-args (false) modes via MethodSource, covering the CmdLineParamsDefaultImpl path that had zero coverage after the emulator migration.
There was a problem hiding this comment.
🟡 Changes recommended
Restore the removed public AzureKeyVault factory overloads to avoid breaking embedders.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
gradle/versions.gradle:20
- The PR description still says production disables Netty
io_uringmultishot polling, but the current change instead upgrades Netty to the fixed release and removes the workaround flags. Update the description so operators and reviewers are not given contradictory mitigation details.
mavenBom 'io.netty:netty-bom:4.2.17.Final'
keystorage/src/main/java/tech/pegasys/web3signer/keystorage/azure/AzureKeyVault.java:114
- This also removes the existing public managed-identity factory signature. Preserve source and binary compatibility for callers that do not use emulator overrides by adding an overload that delegates with
AzureOverrides.NONE.
public static AzureKeyVault createUsingManagedIdentity(
final Optional<String> clientId,
final String vaultName,
final long timeout,
final AzureOverrides azureOverrides) {
- Files reviewed: 48/48 changed files
- Comments generated: 1
- Review effort level: Balanced
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
jframe
previously approved these changes
Sep 3, 2026
| .withEnv( | ||
| "ASPNETCORE_Kestrel__Certificates__Default__Path", "/app/.certs/emulator.pfx") | ||
| .withEnv("AUTH__TENANTID", TENANT_ID) | ||
| .waitingFor(Wait.forListeningPort()); |
Collaborator
There was a problem hiding this comment.
This doesn't necessarily mean that the container is accepting connections, it only means that the port is bound. Could do a Wait.forHttps instead?
Collaborator
Author
There was a problem hiding this comment.
good suggestion, applied.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
PR Description
Moves the Azure Key Vault acceptance tests off live Azure and onto a local emulator, so they run in CI with no Azure credentials.
AzureOverrides(hidden--Xazure-*CLI options) for vault endpoint, authority host, and trust certificate, so Web3Signer can connect to the emulator.CryptographyClientinstead of a hand-written REST client.AzureKeyVaultFactorycaches vault clients per credentials/vault/ overrides, reducing repeated authentication during bulk loading.4.2.17.Finalto fix io_uring read stalls on reused Linux connections.AZURE_*secrets from CI and the long vault setup steps from README.Fixed Issue(s)
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog
Testing