mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(coderd/azureidentity): set explicit roots to avoid macOS system verifier (#25136)
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. <details> <summary>Why this is the correct fix</summary> 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. </details> > Generated by Coder Agents. Reviewed manually.
This commit is contained in:
Kyle Carberry
@@ -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
|
||||
//
|
||||
|
||||
@@ -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...
|
||||
|
||||
Reference in New Issue
Block a user