fix: verify PKCS7 signature on Azure instance identity tokens (#25286)

Migrates Azure instance identity verification from
`go.mozilla.org/pkcs7` and `github.com/fullsailor/pkcs7` to
`github.com/smallstep/pkcs7`, using `VerifyWithChainAtTime` to validate
both the PKCS7 signature and the certificate chain in one call. The
previous code only verified the signer certificate against a set of
intermediates/roots but did not verify that the PKCS7 signature itself
covered the content, meaning tampered payloads could be accepted.

The `Options` struct is restructured to accept `Roots`, `Intermediates`,
and `CurrentTime` as explicit fields instead of embedding
`x509.VerifyOptions`. The test helper `NewAzureInstanceIdentity` now
builds a realistic 3-level certificate chain (Root CA -> Intermediate CA
-> Signing Cert) matching real Azure trust hierarchy. New tests
(`TestValidate_TamperedContent`,
`TestValidate_UntrustedCertWithValidSignature`) confirm tampered and
untrusted envelopes are rejected.

Addresses GHSA-6x44-w3xg-hqqf.

> [!NOTE]
> This PR was authored by Coder Agents.

<details>
<summary>Implementation Plan</summary>

### Files Changed

| File | Summary |
|------|---------|
| `coderd/azureidentity/azureidentity.go` | Replace `signer.Verify()`
with `VerifyWithChainAtTime`; restructure `Options` struct; add
`ParseCertificates()` helper |
| `coderd/azureidentity/azureidentity_test.go` | Add `testCertChain`
builder, tampered-content and untrusted-cert tests; update existing
tests for new `Options` API |
| `coderd/coderd.go` | Change `AzureCertificates` field from
`x509.VerifyOptions` to `azureidentity.Options` |
| `coderd/workspaceresourceauth.go` | Pass `api.AzureCertificates`
directly instead of wrapping |
| `coderd/coderdtest/coderdtest.go` | Migrate to `smallstep/pkcs7`;
build 3-level cert chain in test helper |
| `go.mod` / `go.sum` | Add `github.com/smallstep/pkcs7`; remove
`fullsailor/pkcs7` and `go.mozilla.org/pkcs7` |

</details>
This commit is contained in:
Jakub Domeracki
2026-05-13 14:14:07 +00:00
committed by GitHub
parent b52c0bdb56
commit 1a1f06aa79
7 changed files with 324 additions and 75 deletions
+74 -35
View File
@@ -6,7 +6,6 @@ import (
"encoding/base64"
"encoding/json"
"encoding/pem"
"errors"
"io"
"net"
"net/http"
@@ -15,7 +14,7 @@ import (
"sync"
"time"
"go.mozilla.org/pkcs7"
"github.com/smallstep/pkcs7"
"golang.org/x/xerrors"
)
@@ -184,12 +183,31 @@ type metadata struct {
}
type Options struct {
x509.VerifyOptions
// Roots is the trusted root certificate pool. If nil,
// the embedded root certificate pool is used.
Roots *x509.CertPool
// Intermediates are additional intermediate certificates to
// inject into the PKCS7 object for chain verification. Azure
// PKCS7 envelopes typically only contain the signing cert, so
// intermediates must be supplied externally. When nil, the
// hardcoded Azure intermediate certificates are used.
Intermediates []*x509.Certificate
// CurrentTime, if non-zero, overrides the verification
// timestamp for certificate chain validation.
CurrentTime time.Time
// Offline disables fetching of issuing certificates when
// chain verification fails.
Offline bool
}
// Validate ensures the signature was signed by an Azure certificate.
// It returns the associated VM ID if successful.
//
// Verification has two parts, both handled by VerifyWithChainAtTime:
// 1. PKCS7 signature check: proves the content was signed by the
// private key corresponding to the certificate in the envelope.
// 2. Certificate chain check: proves the signing certificate
// chains to a trusted root through known intermediates.
func Validate(ctx context.Context, signature string, options Options) (string, error) {
data, err := base64.StdEncoding.DecodeString(signature)
if err != nil {
@@ -208,44 +226,47 @@ func Validate(ctx context.Context, signature string, options Options) (string, e
if !allowedSigners.MatchString(signer.Subject.CommonName) {
return "", xerrors.Errorf("unmatched common name of signer: %q", signer.Subject.CommonName)
}
if options.Intermediates == nil {
options.Intermediates = x509.NewCertPool()
for _, cert := range Certificates {
block, rest := pem.Decode([]byte(cert))
if len(rest) != 0 {
return "", xerrors.Errorf("invalid certificate. %d bytes remain", len(rest))
}
cert, err := x509.ParseCertificate(block.Bytes)
if err != nil {
return "", xerrors.Errorf("parse certificate: %w", err)
}
options.Intermediates.AddCert(cert)
// Azure PKCS7 envelopes typically contain only the signing
// certificate. Inject intermediate certificates so the
// library can build a chain from signer to trusted root.
intermediates := options.Intermediates
if intermediates == nil {
intermediates, err = ParseCertificates()
if err != nil {
return "", xerrors.Errorf("parse hardcoded certificates: %w", err)
}
}
// 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()
pkcs7Data.Certificates = append(pkcs7Data.Certificates, intermediates...)
// Resolve root trust store. VerifyWithChainAtTime skips
// chain verification when the trust store is nil, so we
// must always provide one.
roots := options.Roots
if roots == nil {
roots, err = rootCertPool()
if err != nil {
return "", xerrors.Errorf("load roots: %w", err)
}
options.Roots = roots
}
_, err = signer.Verify(options.VerifyOptions)
currentTime := options.CurrentTime
if currentTime.IsZero() {
currentTime = time.Now()
}
// VerifyWithChainAtTime validates both the PKCS7 signature
// (proving the content was signed by the certificate's
// private key) and the certificate chain (proving the signer
// chains to a trusted root).
err = pkcs7Data.VerifyWithChainAtTime(roots, currentTime)
if err != nil {
if !errors.As(err, &x509.UnknownAuthorityError{}) {
return "", xerrors.Errorf("verify signature: %w", err)
}
if options.Offline {
return "", xerrors.Errorf("certificate from %v is not cached: %w", signer.IssuingCertificateURL, err)
return "", xerrors.Errorf("verify pkcs7: %w", err)
}
// The chain verification may fail when the signing
// certificate was issued by an intermediate not yet in
// our hardcoded list. Fetch the issuing certificates
// and retry.
ctx, cancelFunc := context.WithTimeout(ctx, 5*time.Second)
defer cancelFunc()
for _, certURL := range signer.IssuingCertificateURL {
@@ -261,17 +282,17 @@ func Validate(ctx context.Context, signature string, options Options) (string, e
return "", xerrors.New("certificate fetch unsuccessful")
}
limited := io.LimitReader(res.Body, maxCertResponseBytes+1)
data, err := io.ReadAll(limited)
certData, err := io.ReadAll(limited)
_ = res.Body.Close()
if err != nil {
return "", xerrors.New("read certificate response body")
}
if int64(len(data)) > maxCertResponseBytes {
if int64(len(certData)) > maxCertResponseBytes {
return "", xerrors.New(
"certificate response exceeds maximum size",
)
}
cert, err := x509.ParseCertificate(data)
cert, err := x509.ParseCertificate(certData)
if err != nil {
// Do not wrap the parse error; it may contain
// fragments of the HTTP response body, which
@@ -280,9 +301,9 @@ func Validate(ctx context.Context, signature string, options Options) (string, e
"fetched data is not a valid certificate",
)
}
options.Intermediates.AddCert(cert)
pkcs7Data.Certificates = append(pkcs7Data.Certificates, cert)
}
_, err = signer.Verify(options.VerifyOptions)
err = pkcs7Data.VerifyWithChainAtTime(roots, currentTime)
if err != nil {
return "", xerrors.New("signature verification failed after fetching issuing certificates")
}
@@ -296,6 +317,24 @@ func Validate(ctx context.Context, signature string, options Options) (string, e
return metadata.VMID, nil
}
// ParseCertificates parses the hardcoded Azure intermediate
// certificates and returns them as x509.Certificate values.
func ParseCertificates() ([]*x509.Certificate, error) {
var certs []*x509.Certificate
for _, certPEM := range Certificates {
block, rest := pem.Decode([]byte(certPEM))
if len(rest) != 0 {
return nil, xerrors.Errorf("invalid certificate. %d bytes remain", len(rest))
}
cert, err := x509.ParseCertificate(block.Bytes)
if err != nil {
return nil, xerrors.Errorf("parse certificate: %w", err)
}
certs = append(certs, cert)
}
return certs, 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
+182 -9
View File
@@ -1,12 +1,20 @@
package azureidentity_test
import (
"bytes"
"context"
"crypto/rand"
"crypto/rsa"
"crypto/x509"
"crypto/x509/pkix"
"encoding/base64"
"encoding/pem"
"math/big"
"runtime"
"testing"
"time"
"github.com/smallstep/pkcs7"
"github.com/stretchr/testify/require"
"github.com/coder/coder/v2/coderd/azureidentity"
@@ -45,10 +53,8 @@ func TestValidate(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
vm, err := azureidentity.Validate(context.Background(), tc.payload, azureidentity.Options{
VerifyOptions: x509.VerifyOptions{
CurrentTime: tc.date,
},
Offline: true,
CurrentTime: tc.date,
Offline: true,
})
require.NoError(t, err)
require.Equal(t, tc.vmID, vm)
@@ -98,12 +104,10 @@ func TestExpiresSoon(t *testing.T) {
t.Skip()
const threshold = 1
for _, c := range azureidentity.Certificates {
block, rest := pem.Decode([]byte(c))
require.Zero(t, len(rest))
cert, err := x509.ParseCertificate(block.Bytes)
require.NoError(t, err)
certs, err := azureidentity.ParseCertificates()
require.NoError(t, err)
for _, cert := range certs {
expiresSoon := cert.NotAfter.Before(time.Now().AddDate(0, threshold, 0))
if expiresSoon {
t.Errorf("certificate expires within %d months %s: %s", threshold, cert.NotAfter, cert.Subject.CommonName)
@@ -150,3 +154,172 @@ func TestIsAllowedCertificateURL(t *testing.T) {
})
}
}
// testCertChain holds a three-level certificate hierarchy (Root CA,
// Intermediate CA, Signing/leaf) together with their private keys.
type testCertChain struct {
RootCert *x509.Certificate
RootKey *rsa.PrivateKey
IntermediateCert *x509.Certificate
IntermediateKey *rsa.PrivateKey
SigningCert *x509.Certificate
SigningKey *rsa.PrivateKey
}
// newTestCertChain creates a fresh three-level certificate chain for
// testing. All certificates are valid at time.Now().
func newTestCertChain(t *testing.T) testCertChain {
t.Helper()
// Smaller key sizes are fine for tests; keeps them fast.
const keyBits = 2048
// ---- Root CA ----
rootKey, err := rsa.GenerateKey(rand.Reader, keyBits)
require.NoError(t, err)
rootTmpl := &x509.Certificate{
SerialNumber: big.NewInt(1),
Subject: pkix.Name{CommonName: "Test Root CA"},
NotBefore: time.Now().Add(-time.Hour),
NotAfter: time.Now().Add(24 * time.Hour),
KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageCRLSign,
BasicConstraintsValid: true,
IsCA: true,
}
rootDER, err := x509.CreateCertificate(rand.Reader, rootTmpl, rootTmpl, &rootKey.PublicKey, rootKey)
require.NoError(t, err)
rootCert, err := x509.ParseCertificate(rootDER)
require.NoError(t, err)
// ---- Intermediate CA ----
intermediateKey, err := rsa.GenerateKey(rand.Reader, keyBits)
require.NoError(t, err)
intermediateTmpl := &x509.Certificate{
SerialNumber: big.NewInt(2),
Subject: pkix.Name{CommonName: "Test Intermediate CA"},
NotBefore: time.Now().Add(-time.Hour),
NotAfter: time.Now().Add(24 * time.Hour),
KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageCRLSign,
BasicConstraintsValid: true,
IsCA: true,
}
intermediateDER, err := x509.CreateCertificate(rand.Reader, intermediateTmpl, rootCert, &intermediateKey.PublicKey, rootKey)
require.NoError(t, err)
intermediateCert, err := x509.ParseCertificate(intermediateDER)
require.NoError(t, err)
// ---- Signing (leaf) certificate ----
signingKey, err := rsa.GenerateKey(rand.Reader, keyBits)
require.NoError(t, err)
signingTmpl := &x509.Certificate{
SerialNumber: big.NewInt(3),
Subject: pkix.Name{CommonName: "metadata.azure.com"},
NotBefore: time.Now().Add(-time.Hour),
NotAfter: time.Now().Add(24 * time.Hour),
KeyUsage: x509.KeyUsageDigitalSignature,
}
signingDER, err := x509.CreateCertificate(rand.Reader, signingTmpl, intermediateCert, &signingKey.PublicKey, intermediateKey)
require.NoError(t, err)
signingCert, err := x509.ParseCertificate(signingDER)
require.NoError(t, err)
return testCertChain{
RootCert: rootCert,
RootKey: rootKey,
IntermediateCert: intermediateCert,
IntermediateKey: intermediateKey,
SigningCert: signingCert,
SigningKey: signingKey,
}
}
// createSignedPKCS7 produces a base64-encoded PKCS7 SignedData
// envelope over content, signed by the chain's leaf certificate.
func (tc *testCertChain) createSignedPKCS7(t *testing.T, content []byte) string {
t.Helper()
sd, err := pkcs7.NewSignedData(content)
require.NoError(t, err)
err = sd.AddSignerChain(tc.SigningCert, tc.SigningKey, []*x509.Certificate{tc.IntermediateCert}, pkcs7.SignerInfoConfig{})
require.NoError(t, err)
der, err := sd.Finish()
require.NoError(t, err)
return base64.StdEncoding.EncodeToString(der)
}
// validationOptions returns azureidentity.Options that trust only this
// chain's Root CA.
func (tc *testCertChain) validationOptions() azureidentity.Options {
roots := x509.NewCertPool()
roots.AddCert(tc.RootCert)
return azureidentity.Options{
Roots: roots,
Intermediates: []*x509.Certificate{tc.IntermediateCert},
Offline: true,
}
}
func TestValidate_TamperedContent(t *testing.T) {
t.Parallel()
if runtime.GOOS == "darwin" {
t.Skip("pkcs7 signing uses SHA1 which may be restricted on macOS")
}
chain := newTestCertChain(t)
// Build a valid PKCS7 envelope.
original := []byte(`{"vmId":"tamper-test-vm"}`)
signed := chain.createSignedPKCS7(t, original)
// Decode, tamper with the content, re-encode.
raw, err := base64.StdEncoding.DecodeString(signed)
require.NoError(t, err)
tampered := bytes.Replace(raw, []byte("tamper-test-vm"), []byte("tampered!!!!!!"), 1)
require.NotEqual(t, raw, tampered, "payload should have changed")
tamperedB64 := base64.StdEncoding.EncodeToString(tampered)
opts := chain.validationOptions()
_, err = azureidentity.Validate(context.Background(), tamperedB64, opts)
require.Error(t, err, "tampered content must not pass validation")
}
func TestValidate_UntrustedCertWithValidSignature(t *testing.T) {
t.Parallel()
if runtime.GOOS == "darwin" {
t.Skip("pkcs7 signing uses SHA1 which may be restricted on macOS")
}
chain := newTestCertChain(t)
content := []byte(`{"vmId":"untrusted-test-vm"}`)
signed := chain.createSignedPKCS7(t, content)
// Build options that trust a DIFFERENT root, so the chain
// should not verify.
otherRoot, err := rsa.GenerateKey(rand.Reader, 2048)
require.NoError(t, err)
otherRootTmpl := &x509.Certificate{
SerialNumber: big.NewInt(99),
Subject: pkix.Name{CommonName: "Other Root CA"},
NotBefore: time.Now().Add(-time.Hour),
NotAfter: time.Now().Add(24 * time.Hour),
KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageCRLSign,
BasicConstraintsValid: true,
IsCA: true,
}
otherRootDER, err := x509.CreateCertificate(rand.Reader, otherRootTmpl, otherRootTmpl, &otherRoot.PublicKey, otherRoot)
require.NoError(t, err)
otherRootCert, err := x509.ParseCertificate(otherRootDER)
require.NoError(t, err)
untrustedRoots := x509.NewCertPool()
untrustedRoots.AddCert(otherRootCert)
opts := azureidentity.Options{
Roots: untrustedRoots,
Intermediates: []*x509.Certificate{chain.IntermediateCert},
Offline: true,
}
_, err = azureidentity.Validate(context.Background(), signed, opts)
require.Error(t, err, "signature from untrusted CA must not pass validation")
}
+2 -2
View File
@@ -3,7 +3,6 @@ package coderd
import (
"context"
"crypto/tls"
"crypto/x509"
"database/sql"
_ "embed"
"errors"
@@ -52,6 +51,7 @@ import (
"github.com/coder/coder/v2/coderd/appearance"
"github.com/coder/coder/v2/coderd/audit"
"github.com/coder/coder/v2/coderd/awsidentity"
"github.com/coder/coder/v2/coderd/azureidentity"
"github.com/coder/coder/v2/coderd/boundaryusage"
"github.com/coder/coder/v2/coderd/connectionlog"
"github.com/coder/coder/v2/coderd/cryptokeys"
@@ -173,7 +173,7 @@ type Options struct {
ChatdInstructionLookupTimeout time.Duration
AWSCertificates awsidentity.Certificates
Authorizer rbac.Authorizer
AzureCertificates x509.VerifyOptions
AzureCertificates azureidentity.Options
GoogleTokenValidator *idtoken.Validator
GithubOAuth2Config *GithubOAuth2Config
OIDCConfig *OIDCConfig
+57 -20
View File
@@ -32,11 +32,11 @@ import (
"time"
"cloud.google.com/go/compute/metadata"
"github.com/fullsailor/pkcs7"
"github.com/go-chi/chi/v5"
"github.com/golang-jwt/jwt/v4"
"github.com/google/uuid"
"github.com/prometheus/client_golang/prometheus"
"github.com/smallstep/pkcs7"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"golang.org/x/text/cases"
@@ -59,6 +59,7 @@ import (
"github.com/coder/coder/v2/coderd/audit"
"github.com/coder/coder/v2/coderd/autobuild"
"github.com/coder/coder/v2/coderd/awsidentity"
"github.com/coder/coder/v2/coderd/azureidentity"
"github.com/coder/coder/v2/coderd/connectionlog"
"github.com/coder/coder/v2/coderd/cryptokeys"
"github.com/coder/coder/v2/coderd/database"
@@ -119,7 +120,7 @@ type Options struct {
AppHostname string
AWSCertificates awsidentity.Certificates
Authorizer rbac.Authorizer
AzureCertificates x509.VerifyOptions
AzureCertificates azureidentity.Options
GithubOAuth2Config *coderd.GithubOAuth2Config
RealIPConfig *httpmw.RealIPConfig
OIDCConfig *coderd.OIDCConfig
@@ -1634,27 +1635,63 @@ func NewAWSInstanceIdentity(t testing.TB, instanceID string) (awsidentity.Certif
}
}
// NewAzureInstanceIdentity returns a metadata client and ID token validator for faking
// instance authentication for Azure.
func NewAzureInstanceIdentity(t testing.TB, instanceID string) (x509.VerifyOptions, *http.Client) {
privateKey, err := rsa.GenerateKey(rand.Reader, 2048)
// NewAzureInstanceIdentity returns a metadata client and ID token
// validator for faking instance authentication for Azure. It builds
// a realistic 3-level certificate chain (Root CA -> Intermediate ->
// Signing Cert) to match the real Azure trust hierarchy.
func NewAzureInstanceIdentity(t testing.TB, instanceID string) (azureidentity.Options, *http.Client) {
// Root CA (self-signed, trusted).
rootKey, err := rsa.GenerateKey(rand.Reader, 2048)
require.NoError(t, err)
rootTmpl := &x509.Certificate{
SerialNumber: big.NewInt(1),
Subject: pkix.Name{CommonName: "Test Root CA"},
NotBefore: time.Now().Add(-time.Hour),
NotAfter: time.Now().AddDate(10, 0, 0),
IsCA: true,
BasicConstraintsValid: true,
KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageCRLSign,
}
rootDER, err := x509.CreateCertificate(rand.Reader, rootTmpl, rootTmpl, &rootKey.PublicKey, rootKey)
require.NoError(t, err)
rootCert, err := x509.ParseCertificate(rootDER)
require.NoError(t, err)
rawCertificate, err := x509.CreateCertificate(rand.Reader, &x509.Certificate{
SerialNumber: big.NewInt(2022),
// Intermediate CA (signed by root).
interKey, err := rsa.GenerateKey(rand.Reader, 2048)
require.NoError(t, err)
interTmpl := &x509.Certificate{
SerialNumber: big.NewInt(2),
Subject: pkix.Name{CommonName: "Test Intermediate CA"},
NotBefore: time.Now().Add(-time.Hour),
NotAfter: time.Now().AddDate(5, 0, 0),
IsCA: true,
BasicConstraintsValid: true,
KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageCRLSign,
}
interDER, err := x509.CreateCertificate(rand.Reader, interTmpl, rootCert, &interKey.PublicKey, rootKey)
require.NoError(t, err)
interCert, err := x509.ParseCertificate(interDER)
require.NoError(t, err)
// Signing cert (leaf, signed by intermediate).
signKey, err := rsa.GenerateKey(rand.Reader, 2048)
require.NoError(t, err)
signTmpl := &x509.Certificate{
SerialNumber: big.NewInt(3),
Subject: pkix.Name{CommonName: "metadata.azure.com"},
NotBefore: time.Now().Add(-time.Hour),
NotAfter: time.Now().AddDate(1, 0, 0),
Subject: pkix.Name{
CommonName: "metadata.azure.com",
},
}, &x509.Certificate{}, &privateKey.PublicKey, privateKey)
}
signDER, err := x509.CreateCertificate(rand.Reader, signTmpl, interCert, &signKey.PublicKey, interKey)
require.NoError(t, err)
certificate, err := x509.ParseCertificate(rawCertificate)
signCert, err := x509.ParseCertificate(signDER)
require.NoError(t, err)
// Build PKCS7 signed data with only the signing cert.
signed, err := pkcs7.NewSignedData([]byte(`{"vmId":"` + instanceID + `"}`))
require.NoError(t, err)
err = signed.AddSigner(certificate, privateKey, pkcs7.SignerInfoConfig{})
err = signed.AddSigner(signCert, signKey, pkcs7.SignerInfoConfig{})
require.NoError(t, err)
signatureRaw, err := signed.Finish()
require.NoError(t, err)
@@ -1667,12 +1704,12 @@ func NewAzureInstanceIdentity(t testing.TB, instanceID string) (x509.VerifyOptio
})
require.NoError(t, err)
certPool := x509.NewCertPool()
certPool.AddCert(certificate)
roots := x509.NewCertPool()
roots.AddCert(rootCert)
return x509.VerifyOptions{
Intermediates: certPool,
Roots: certPool,
return azureidentity.Options{
Roots: roots,
Intermediates: []*x509.Certificate{interCert},
}, &http.Client{
Transport: roundTripper(func(r *http.Request) (*http.Response, error) {
// Only handle metadata server requests.
+1 -3
View File
@@ -35,9 +35,7 @@ func (api *API) postWorkspaceAuthAzureInstanceIdentity(rw http.ResponseWriter, r
if !httpapi.Read(ctx, rw, r, &req) {
return
}
instanceID, err := azureidentity.Validate(r.Context(), req.Signature, azureidentity.Options{
VerifyOptions: api.AzureCertificates,
})
instanceID, err := azureidentity.Validate(r.Context(), req.Signature, api.AzureCertificates)
if err != nil {
// Log the full error for operators but return only a
// generic message to the caller. Errors from the