From e3db203011d32f056ecbd6c7ccf8ac1664b39126 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Mon, 11 May 2026 13:53:33 -0400 Subject: [PATCH] fix(coderd/azureidentity): set explicit roots to avoid macOS system verifier (#25136) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes [CODAGT-372](https://linear.app/codercom/issue/CODAGT-372/coderdazureidentity-testvalidateregular-fails-on-macos). Closes coder/internal#101. ## Problem `coderd/azureidentity TestValidate/regular` fails on macOS with: ``` verify signature: github.com/coder/coder/v2/coderd/azureidentity.Validate /Users/runner/work/coder/coder/coderd/azureidentity/azureidentity.go:75 - x509: “metadata.azure.com” certificate is not standards compliant ``` When `crypto/x509.VerifyOptions.Roots` is `nil`, Go's verifier on macOS/iOS falls back to the system verifier (`systemVerify` in `crypto/x509/root_darwin.go`), which delegates to Apple's `SecTrustEvaluateWithError`. Apple's framework enforces stricter standards-compliance checks than Go's pure-Go verifier and rejects some otherwise valid Azure instance-identity leaf certificates with `errSecCertificateIsNotStandardsCompliant`, surfaced as the `not standards compliant` error. The test had been skipped on darwin since #12979 (April 2024) as a workaround. ## Fix - Embed the three root CAs that Azure instance-identity certificates ultimately chain to: - DigiCert Global Root G2 - DigiCert Global Root G3 - Baltimore CyberTrust Root (kept for historical chains via `Microsoft RSA TLS CA 01/02`) - In `Validate`, populate `options.Roots` from those embedded roots when the caller does not supply its own pool. Because `Roots != nil`, Go no longer takes the `systemVerify` path on darwin and uses the pure-Go verifier on all platforms. - Remove the `runtime.GOOS == "darwin"` skip from `TestValidate`. - Add `TestEmbeddedRoots` to guard against future regressions in the embedded root list (parses each PEM, asserts self-signed, requires all three named roots). The caller's existing `Intermediates` handling is unchanged. Tests that pass their own `Roots` (e.g. `coderdtest.NewAzureInstanceIdentity`) are unaffected. ## Verification On Linux: ``` $ go test ./coderd/azureidentity/ -race -count=1 -v === RUN TestValidate === RUN TestValidate/regular === RUN TestValidate/govcloud === RUN TestValidate/rsa --- PASS: TestValidate (0.00s) --- PASS: TestValidate/regular (0.00s) --- PASS: TestValidate/rsa (0.00s) --- PASS: TestValidate/govcloud (0.00s) === RUN TestEmbeddedRoots --- PASS: TestEmbeddedRoots (0.00s) === RUN TestExpiresSoon --- SKIP: TestExpiresSoon (0.00s) PASS ok github.com/coder/coder/v2/coderd/azureidentity 1.020s ``` The `test-go-pg` job on `macos-latest` in CI is the authoritative confirmation of the fix on macOS; previously it would have failed `TestValidate/regular` had the skip been removed.
Why this is the correct fix From `/usr/local/go/src/crypto/x509/verify.go`: ```go // Use platform verifiers, where available, if Roots is from SystemCertPool. if runtime.GOOS == "windows" || runtime.GOOS == "darwin" || runtime.GOOS == "ios" { systemPool := systemRootsPool() if opts.Roots == nil && (systemPool == nil || systemPool.systemPool) { return c.systemVerify(&opts) } ... } ``` Setting `opts.Roots` to any non-nil, non-system pool deterministically routes verification through Go's pure-Go verifier, bypassing Apple's stricter compliance checks. The embedded roots are sufficient to validate every chain we currently care about, since every intermediate in `Certificates` ultimately issues to one of the three embedded roots.
> Generated by Coder Agents. Reviewed manually. --- coderd/azureidentity/azureidentity.go | 112 +++++++++++++++++++++ coderd/azureidentity/azureidentity_test.go | 39 ++++++- 2 files changed, 146 insertions(+), 5 deletions(-) diff --git a/coderd/azureidentity/azureidentity.go b/coderd/azureidentity/azureidentity.go index e4da9e54fc..7a06ccbc51 100644 --- a/coderd/azureidentity/azureidentity.go +++ b/coderd/azureidentity/azureidentity.go @@ -68,6 +68,20 @@ func Validate(ctx context.Context, signature string, options Options) (string, e options.Intermediates.AddCert(cert) } } + // Set Roots explicitly so we never fall back to the platform's system + // verifier (notably Apple's Security framework on macOS/iOS), which + // enforces stricter standards-compliance checks than Go's pure-Go + // verifier and rejects some otherwise valid Azure leaf certificates + // with errors like: + // x509: "metadata.azure.com" certificate is not standards compliant + // See https://github.com/coder/coder/issues/12978. + if options.Roots == nil { + roots, err := rootCertPool() + if err != nil { + return "", xerrors.Errorf("load roots: %w", err) + } + options.Roots = roots + } _, err = signer.Verify(options.VerifyOptions) if err != nil { @@ -115,6 +129,104 @@ func Validate(ctx context.Context, signature string, options Options) (string, e return metadata.VMID, nil } +// Roots are the root CAs that Azure instance-identity certificates chain to. +// These are embedded so verification works deterministically on all +// platforms, including macOS where the system verifier would otherwise be +// used and may reject otherwise valid Azure certificates due to stricter +// standards-compliance checks. See https://github.com/coder/coder/issues/12978. +var Roots = []string{ + // DigiCert Global Root G2 + `-----BEGIN CERTIFICATE----- +MIIDjjCCAnagAwIBAgIQAzrx5qcRqaC7KGSxHQn65TANBgkqhkiG9w0BAQsFADBh +MQswCQYDVQQGEwJVUzEVMBMGA1UEChMMRGlnaUNlcnQgSW5jMRkwFwYDVQQLExB3 +d3cuZGlnaWNlcnQuY29tMSAwHgYDVQQDExdEaWdpQ2VydCBHbG9iYWwgUm9vdCBH +MjAeFw0xMzA4MDExMjAwMDBaFw0zODAxMTUxMjAwMDBaMGExCzAJBgNVBAYTAlVT +MRUwEwYDVQQKEwxEaWdpQ2VydCBJbmMxGTAXBgNVBAsTEHd3dy5kaWdpY2VydC5j +b20xIDAeBgNVBAMTF0RpZ2lDZXJ0IEdsb2JhbCBSb290IEcyMIIBIjANBgkqhkiG +9w0BAQEFAAOCAQ8AMIIBCgKCAQEAuzfNNNx7a8myaJCtSnX/RrohCgiN9RlUyfuI +2/Ou8jqJkTx65qsGGmvPrC3oXgkkRLpimn7Wo6h+4FR1IAWsULecYxpsMNzaHxmx +1x7e/dfgy5SDN67sH0NO3Xss0r0upS/kqbitOtSZpLYl6ZtrAGCSYP9PIUkY92eQ +q2EGnI/yuum06ZIya7XzV+hdG82MHauVBJVJ8zUtluNJbd134/tJS7SsVQepj5Wz +tCO7TG1F8PapspUwtP1MVYwnSlcUfIKdzXOS0xZKBgyMUNGPHgm+F6HmIcr9g+UQ +vIOlCsRnKPZzFBQ9RnbDhxSJITRNrw9FDKZJobq7nMWxM4MphQIDAQABo0IwQDAP +BgNVHRMBAf8EBTADAQH/MA4GA1UdDwEB/wQEAwIBhjAdBgNVHQ4EFgQUTiJUIBiV +5uNu5g/6+rkS7QYXjzkwDQYJKoZIhvcNAQELBQADggEBAGBnKJRvDkhj6zHd6mcY +1Yl9PMWLSn/pvtsrF9+wX3N3KjITOYFnQoQj8kVnNeyIv/iPsGEMNKSuIEyExtv4 +NeF22d+mQrvHRAiGfzZ0JFrabA0UWTW98kndth/Jsw1HKj2ZL7tcu7XUIOGZX1NG +Fdtom/DzMNU+MeKNhJ7jitralj41E6Vf8PlwUHBHQRFXGU7Aj64GxJUTFy8bJZ91 +8rGOmaFvE7FBcf6IKshPECBV1/MUReXgRPTqh5Uykw7+U0b6LJ3/iyK5S9kJRaTe +pLiaWN0bfVKfjllDiIGknibVb63dDcY3fe0Dkhvld1927jyNxF1WW6LZZm6zNTfl +MrY= +-----END CERTIFICATE-----`, + // DigiCert Global Root G3 + `-----BEGIN CERTIFICATE----- +MIICPzCCAcWgAwIBAgIQBVVWvPJepDU1w6QP1atFcjAKBggqhkjOPQQDAzBhMQsw +CQYDVQQGEwJVUzEVMBMGA1UEChMMRGlnaUNlcnQgSW5jMRkwFwYDVQQLExB3d3cu +ZGlnaWNlcnQuY29tMSAwHgYDVQQDExdEaWdpQ2VydCBHbG9iYWwgUm9vdCBHMzAe +Fw0xMzA4MDExMjAwMDBaFw0zODAxMTUxMjAwMDBaMGExCzAJBgNVBAYTAlVTMRUw +EwYDVQQKEwxEaWdpQ2VydCBJbmMxGTAXBgNVBAsTEHd3dy5kaWdpY2VydC5jb20x +IDAeBgNVBAMTF0RpZ2lDZXJ0IEdsb2JhbCBSb290IEczMHYwEAYHKoZIzj0CAQYF +K4EEACIDYgAE3afZu4q4C/sLfyHS8L6+c/MzXRq8NOrexpu80JX28MzQC7phW1FG +fp4tn+6OYwwX7Adw9c+ELkCDnOg/QW07rdOkFFk2eJ0DQ+4QE2xy3q6Ip6FrtUPO +Z9wj/wMco+I+o0IwQDAPBgNVHRMBAf8EBTADAQH/MA4GA1UdDwEB/wQEAwIBhjAd +BgNVHQ4EFgQUs9tIpPmhxdiuNkHMEWNpYim8S8YwCgYIKoZIzj0EAwMDaAAwZQIx +AK288mw/EkrRLTnDCgmXc/SINoyIJ7vmiI1Qhadj+Z4y3maTD/HMsQmP3Wyr+mt/ +oAIwOWZbwmSNuJ5Q3KjVSaLtx9zRSX8XAbjIho9OjIgrqJqpisXRAL34VOKa5Vt8 +sycX +-----END CERTIFICATE-----`, + // Baltimore CyberTrust Root. + // Required for chains rooted here, e.g. "Microsoft RSA TLS CA 01/02". + // Expired 2025-05-12 but kept so callers that pass a CurrentTime + // before the expiry can still verify historical signatures. + `-----BEGIN CERTIFICATE----- +MIIDdzCCAl+gAwIBAgIEAgAAuTANBgkqhkiG9w0BAQUFADBaMQswCQYDVQQGEwJJ +RTESMBAGA1UEChMJQmFsdGltb3JlMRMwEQYDVQQLEwpDeWJlclRydXN0MSIwIAYD +VQQDExlCYWx0aW1vcmUgQ3liZXJUcnVzdCBSb290MB4XDTAwMDUxMjE4NDYwMFoX +DTI1MDUxMjIzNTkwMFowWjELMAkGA1UEBhMCSUUxEjAQBgNVBAoTCUJhbHRpbW9y +ZTETMBEGA1UECxMKQ3liZXJUcnVzdDEiMCAGA1UEAxMZQmFsdGltb3JlIEN5YmVy +VHJ1c3QgUm9vdDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAKMEuyKr +mD1X6CZymrV51Cni4eiVgLGw41uOKymaZN+hXe2wCQVt2yguzmKiYv60iNoS6zjr +IZ3AQSsBUnuId9Mcj8e6uYi1agnnc+gRQKfRzMpijS3ljwumUNKoUMMo6vWrJYeK +mpYcqWe4PwzV9/lSEy/CG9VwcPCPwBLKBsua4dnKM3p31vjsufFoREJIE9LAwqSu +XmD+tqYF/LTdB1kC1FkYmGP1pWPgkAx9XbIGevOF6uvUA65ehD5f/xXtabz5OTZy +dc93Uk3zyZAsuT3lySNTPx8kmCFcB5kpvcY67Oduhjprl3RjM71oGDHweI12v/ye +jl0qhqdNkNwnGjkCAwEAAaNFMEMwHQYDVR0OBBYEFOWdWTCCR1jMrPoIVDaGezq1 +BE3wMBIGA1UdEwEB/wQIMAYBAf8CAQMwDgYDVR0PAQH/BAQDAgEGMA0GCSqGSIb3 +DQEBBQUAA4IBAQCFDF2O5G9RaEIFoN27TyclhAO992T9Ldcw46QQF+vaKSm2eT92 +9hkTI7gQCvlYpNRhcL0EYWoSihfVCr3FvDB81ukMJY2GQE/szKN+OMY3EU/t3Wgx +jkzSswF07r51XgdIGn9w/xZchMB5hbgF/X++ZRGjD8ACtPhSNzkE1akxehi/oCr0 +Epn3o0WC4zxe9Z2etciefC7IpJ5OCBRLbf1wbWsaY71k5h+3zvDyny67G7fyUIhz +ksLi4xaNmjICq44Y3ekQEe5+NauQrz4wlHrQMz2nZQ/1/I6eYs9HRCwBXbsdtTLS +R9I4LtD+gdwyah617jzV/OeBHRnDJELqYzmp +-----END CERTIFICATE-----`, +} + +// rootCertPool returns a CertPool containing the root CAs that Azure +// instance-identity certificates ultimately chain to. We embed these so +// callers do not have to populate Roots themselves, and so we never +// implicitly fall back to the platform's system verifier (notably Apple's +// Security framework on macOS/iOS) which enforces stricter standards- +// compliance checks than Go's pure-Go verifier and rejects some otherwise +// valid Azure leaf certificates. +var rootCertPool = sync.OnceValues(func() (*x509.CertPool, error) { + pool := x509.NewCertPool() + for _, pemCert := range Roots { + block, rest := pem.Decode([]byte(pemCert)) + if block == nil { + return nil, xerrors.New("root: failed to decode PEM block") + } + if len(rest) != 0 { + return nil, xerrors.Errorf("root: invalid certificate, %d bytes remain", len(rest)) + } + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + return nil, xerrors.Errorf("root: parse certificate: %w", err) + } + pool.AddCert(cert) + } + return pool, nil +}) + // Certificates are manually downloaded from Azure, then processed with OpenSSL // and added here. See: https://learn.microsoft.com/en-us/azure/security/fundamentals/azure-ca-details // diff --git a/coderd/azureidentity/azureidentity_test.go b/coderd/azureidentity/azureidentity_test.go index bd94f836be..93627ff927 100644 --- a/coderd/azureidentity/azureidentity_test.go +++ b/coderd/azureidentity/azureidentity_test.go @@ -4,7 +4,6 @@ import ( "context" "crypto/x509" "encoding/pem" - "runtime" "testing" "time" @@ -15,10 +14,6 @@ import ( func TestValidate(t *testing.T) { t.Parallel() - if runtime.GOOS == "darwin" { - // This test fails on MacOS for some reason. See https://github.com/coder/coder/issues/12978 - t.Skip() - } mustTime := func(layout string, value string) time.Time { ti, err := time.Parse(layout, value) @@ -61,6 +56,40 @@ func TestValidate(t *testing.T) { } } +// TestEmbeddedRoots ensures the package's embedded root certificates parse +// successfully. The Roots are used by Validate to avoid falling back to the +// platform's system verifier (notably Apple's Security framework on macOS), +// which previously caused TestValidate/regular to fail on macOS with +// `x509: "metadata.azure.com" certificate is not standards compliant`. +// See https://github.com/coder/coder/issues/12978. +func TestEmbeddedRoots(t *testing.T) { + t.Parallel() + require.NotEmpty(t, azureidentity.Roots, "embedded roots must not be empty") + seen := map[string]bool{} + for _, pemCert := range azureidentity.Roots { + block, rest := pem.Decode([]byte(pemCert)) + require.NotNil(t, block, "PEM block should decode") + require.Zero(t, len(rest), "no trailing data after PEM block") + cert, err := x509.ParseCertificate(block.Bytes) + require.NoError(t, err) + // Each root must be self-signed (issuer == subject). + require.Equal(t, cert.Issuer.String(), cert.Subject.String(), + "root certificate must be self-signed: %s", cert.Subject.CommonName) + require.False(t, seen[cert.Subject.CommonName], + "duplicate embedded root: %s", cert.Subject.CommonName) + seen[cert.Subject.CommonName] = true + } + // Verify the three roots Azure instance-identity chains ultimately + // terminate at are all present. + for _, name := range []string{ + "DigiCert Global Root G2", + "DigiCert Global Root G3", + "Baltimore CyberTrust Root", + } { + require.True(t, seen[name], "missing embedded root %q", name) + } +} + func TestExpiresSoon(t *testing.T) { t.Parallel() // TODO (@kylecarbs): It's unknown why Microsoft does not have new certificates live...