mirror of
https://github.com/coder/coder.git
synced 2026-06-05 05:58:20 +00:00
fix: verify PKCS7 signature on Azure instance identity tokens (backport 2.24) (#25309)
Backport of #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> | 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> <!-- If you have used AI to produce some or all of this PR, please ensure you have read our [AI Contribution guidelines](https://coder.com/docs/about/contributing/AI_CONTRIBUTING) before submitting. --> Co-authored-by: Jakub Domeracki <jakub@coder.com>
This commit is contained in:
@@ -32,12 +32,12 @@ import (
|
||||
"unicode"
|
||||
|
||||
"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/moby/moby/pkg/namesgenerator"
|
||||
"github.com/prometheus/client_golang/prometheus"
|
||||
"github.com/smallstep/pkcs7"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
"golang.org/x/xerrors"
|
||||
@@ -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/cryptokeys"
|
||||
"github.com/coder/coder/v2/coderd/database"
|
||||
"github.com/coder/coder/v2/coderd/database/db2sdk"
|
||||
@@ -107,7 +108,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
|
||||
@@ -1438,27 +1439,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)
|
||||
require.NoError(t, err)
|
||||
|
||||
certificate, err := x509.ParseCertificate(rawCertificate)
|
||||
}
|
||||
signDER, err := x509.CreateCertificate(rand.Reader, signTmpl, interCert, &signKey.PublicKey, interKey)
|
||||
require.NoError(t, err)
|
||||
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)
|
||||
@@ -1471,12 +1508,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.
|
||||
|
||||
Reference in New Issue
Block a user