fix(metadata): consistent revocation policy and client timeouts (#740)

Revocation of an intermediate was gated on the certificate carrying an issuing certificate URL, while a leaf in the same position was rejected outright. Both now share one check: a certificate is rejected when it is known to be revoked.

Fetch and the cached provider created clients with no timeout, so an unresponsive service stalled the caller indefinitely; both now use DefaultMDSTimeout. The decoder held a client that was never read, which is removed, and Decode no longer claims to close the reader it is given.

The metadata service rate limits with a short plain text body, which was handed to the JWT parser and surfaced as an unintelligible base64 failure. The network tests now check the response status before decoding, and the conformance test no longer leaks a response body per endpoint.

Author: james-d-elliott

metadata/const.go

@@ -1,5 +1,7 @@
 package metadata
 
+import "time"
+
 const (
 	// ProductionMDSRoot is the root certificate for the MDS.
 	//
@@ -25,6 +27,10 @@ const (
 	HeaderX509Certificate = "x5c"
 )
 
+// DefaultMDSTimeout is the timeout applied to the [http.Client] values this package creates for itself. It is
+// deliberately generous as the production blob is several megabytes. Provide your own client to use a different value.
+const DefaultMDSTimeout = time.Second * 30
+
 var (
 	errIntermediateCertRevoked = &Error{
 		Type:    "intermediate_revoked",

metadata/decode.go

@@ -6,7 +6,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"net/http"
 	"strings"
 	"time"
 
@@ -19,7 +18,6 @@ import (
 // NewDecoder returns a new metadata decoder.
 func NewDecoder(opts ...DecoderOption) (decoder *Decoder, err error) {
 	decoder = &Decoder{
-		client: &http.Client{},
 		parser: jwt.NewParser(),
 		hook:   mapstructure.ComposeDecodeHookFunc(),
 	}
@@ -39,7 +37,6 @@ func NewDecoder(opts ...DecoderOption) (decoder *Decoder, err error) {
 
 // Decoder handles decoding and specialized parsing of the metadata blob.
 type Decoder struct {
-	client                   *http.Client
 	parser                   *jwt.Parser
 	hook                     mapstructure.DecodeHookFunc
 	root                     string
@@ -82,7 +79,8 @@ func (d *Decoder) Parse(payload *PayloadJSON) (metadata *Metadata, err error) {
 	return metadata, nil
 }
 
-// Decode the blob from an [io.Reader]. This function will close the [io.ReadCloser] after completing.
+// Decode the blob from an [io.Reader]. The reader is read in full but is not closed; closing it remains the
+// responsibility of the caller.
 func (d *Decoder) Decode(r io.Reader) (payload *PayloadJSON, err error) {
 	bytes, err := io.ReadAll(r)
 	if err != nil {
@@ -238,14 +236,8 @@ func validateChain(root string, chain []any) (bool, error) {
 			return false, err
 		}
 
-		if revoked, ok := revoke.VerifyCertificate(intcert); !ok {
-			issuer := intcert.IssuingCertificateURL
-
-			if issuer != nil {
-				return false, errCRLUnavailable
-			}
-		} else if revoked {
-			return false, errIntermediateCertRevoked
+		if err = validateChainCheckRevocation(intcert, errIntermediateCertRevoked); err != nil {
+			return false, err
 		}
 
 		ints.AddCert(intcert)
@@ -256,10 +248,8 @@ func validateChain(root string, chain []any) (bool, error) {
 		return false, err
 	}
 
-	if revoked, ok := revoke.VerifyCertificate(leafcert); !ok {
-		return false, errCRLUnavailable
-	} else if revoked {
-		return false, errLeafCertRevoked
+	if err = validateChainCheckRevocation(leafcert, errLeafCertRevoked); err != nil {
+		return false, err
 	}
 
 	opts := x509.VerifyOptions{
@@ -273,6 +263,14 @@ func validateChain(root string, chain []any) (bool, error) {
 	return err == nil, err
 }
 
+func validateChainCheckRevocation(cert *x509.Certificate, revokedErr error) error {
+	if revoked, ok := revoke.VerifyCertificate(cert); ok && revoked {
+		return revokedErr
+	}
+
+	return nil
+}
+
 func mdsParseX509Certificate(value string) (certificate *x509.Certificate, err error) {
 	var n int
 

metadata/decode_test.go

@@ -7,7 +7,10 @@ import (
 	"crypto/x509"
 	"crypto/x509/pkix"
 	"encoding/base64"
+	"io"
 	"math/big"
+	"net/http"
+	"net/http/httptest"
 	"strings"
 	"testing"
 	"time"
@@ -126,7 +129,7 @@ func TestValidateChainDepth(t *testing.T) {
 	}
 }
 
-func newTestCertificate(t *testing.T, serial int64, name string, parent *x509.Certificate, parentKey *ecdsa.PrivateKey, ca bool) (cert *x509.Certificate, key *ecdsa.PrivateKey, encoded string) {
+func newTestCertificate(t *testing.T, serial int64, name string, parent *x509.Certificate, parentKey *ecdsa.PrivateKey, ca bool, mutators ...func(template *x509.Certificate)) (cert *x509.Certificate, key *ecdsa.PrivateKey, encoded string) {
 	t.Helper()
 
 	key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
@@ -148,6 +151,10 @@ func newTestCertificate(t *testing.T, serial int64, name string, parent *x509.Ce
 		template.ExtKeyUsage = []x509.ExtKeyUsage{x509.ExtKeyUsageAny}
 	}
 
+	for _, mutator := range mutators {
+		mutator(template)
+	}
+
 	signer, signerKey := template, key
 
 	if parent != nil {
@@ -162,3 +169,132 @@ func newTestCertificate(t *testing.T, serial int64, name string, parent *x509.Ce
 
 	return cert, key, base64.StdEncoding.EncodeToString(der)
 }
+
+// unreachableCRL points the certificate at a distribution point which cannot be reached, so that its revocation status
+// cannot be determined.
+func unreachableCRL(template *x509.Certificate) {
+	template.CRLDistributionPoints = []string{"http://127.0.0.1:1/crl"}
+}
+
+// newRevocationListServer serves a CRL signed by the given issuer which lists the given serials as revoked.
+func newRevocationListServer(t *testing.T, issuer *x509.Certificate, issuerKey *ecdsa.PrivateKey, revoked ...int64) *httptest.Server {
+	t.Helper()
+
+	entries := make([]x509.RevocationListEntry, len(revoked))
+
+	for i, serial := range revoked {
+		entries[i] = x509.RevocationListEntry{
+			SerialNumber:   big.NewInt(serial),
+			RevocationTime: time.Now().Add(-time.Hour),
+		}
+	}
+
+	der, err := x509.CreateRevocationList(rand.Reader, &x509.RevocationList{
+		Number:                    big.NewInt(1),
+		ThisUpdate:                time.Now().Add(-time.Hour),
+		NextUpdate:                time.Now().Add(time.Hour),
+		RevokedCertificateEntries: entries,
+	}, issuer, issuerKey)
+	require.NoError(t, err)
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/pkix-crl")
+
+		_, _ = w.Write(der)
+	}))
+
+	t.Cleanup(server.Close)
+
+	return server
+}
+
+func TestValidateChainRevocation(t *testing.T) {
+	root, rootKey, rootEncoded := newTestCertificate(t, 20, "root", nil, nil, true)
+	inter, interKey, interEncoded := newTestCertificate(t, 21, "intermediate", root, rootKey, true)
+
+	// A certificate whose status cannot be determined is treated as not revoked, as the distribution point is not
+	// reliably reachable at the moment a blob is decoded.
+	_, _, leafUnknown := newTestCertificate(t, 22, "leaf unknown", inter, interKey, false, unreachableCRL)
+
+	leafCRL := newRevocationListServer(t, inter, interKey, 23)
+
+	_, _, leafRevoked := newTestCertificate(t, 23, "leaf revoked", inter, interKey, false, func(template *x509.Certificate) {
+		template.CRLDistributionPoints = []string{leafCRL.URL}
+	})
+
+	interCRL := newRevocationListServer(t, root, rootKey, 25)
+
+	interRevoked, interRevokedKey, interRevokedEncoded := newTestCertificate(t, 25, "intermediate revoked", root, rootKey, true, func(template *x509.Certificate) {
+		template.CRLDistributionPoints = []string{interCRL.URL}
+	})
+
+	_, _, leafOfRevoked := newTestCertificate(t, 26, "leaf of revoked", interRevoked, interRevokedKey, false)
+	_, _, leafOK := newTestCertificate(t, 24, "leaf", inter, interKey, false)
+
+	testCases := []struct {
+		name  string
+		chain []any
+		valid bool
+		err   error
+	}{
+		{
+			name:  "ShouldPermitLeafWithUnknownStatus",
+			chain: []any{leafUnknown, interEncoded},
+			valid: true,
+		},
+		{
+			name:  "ShouldPermitDeterminableChain",
+			chain: []any{leafOK, interEncoded},
+			valid: true,
+		},
+		{
+			name:  "ShouldRejectRevokedLeaf",
+			chain: []any{leafRevoked, interEncoded},
+			valid: false,
+			err:   errLeafCertRevoked,
+		},
+		{
+			name:  "ShouldRejectRevokedIntermediate",
+			chain: []any{leafOfRevoked, interRevokedEncoded},
+			valid: false,
+			err:   errIntermediateCertRevoked,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			valid, err := validateChain(rootEncoded, tc.chain)
+
+			assert.Equal(t, tc.valid, valid)
+
+			if tc.err != nil {
+				assert.Equal(t, tc.err, err)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
+
+type recordingReadCloser struct {
+	io.Reader
+	closed bool
+}
+
+func (r *recordingReadCloser) Close() error {
+	r.closed = true
+
+	return nil
+}
+
+func TestDecodeDoesNotCloseTheReader(t *testing.T) {
+	decoder, err := NewDecoder()
+	require.NoError(t, err)
+
+	rc := &recordingReadCloser{Reader: strings.NewReader("not a jwt")}
+
+	_, err = decoder.Decode(rc)
+	require.Error(t, err)
+
+	assert.False(t, rc.closed, "Decode must leave closing the reader to the caller")
+}

metadata/metadata.go

@@ -21,7 +21,7 @@ func Fetch() (metadata *Metadata, err error) {
 		resp    *http.Response
 	)
 
-	client := &http.Client{}
+	client := &http.Client{Timeout: DefaultMDSTimeout}
 
 	if resp, err = client.Get(ProductionMDSURL); err != nil {
 		return nil, err

metadata/metadata_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"io"
 	"net/http"
 	"testing"
@@ -20,11 +21,17 @@ func TestProductionMetadataTOCParsing(t *testing.T) {
 	decoder, err := NewDecoder(WithIgnoreEntryParsingErrors())
 	require.NoError(t, err)
 
-	client := &http.Client{}
+	client := &http.Client{Timeout: DefaultMDSTimeout}
 
 	res, err := client.Get(ProductionMDSURL)
 	require.NoError(t, err)
 
+	defer func() {
+		_ = res.Body.Close()
+	}()
+
+	require.Equalf(t, http.StatusOK, res.StatusCode, "unexpected status code %d fetching the metadata blob", res.StatusCode)
+
 	payload, err := decoder.Decode(res.Body)
 	require.NoError(t, err)
 
@@ -89,7 +96,13 @@ func TestConformanceMetadataTOCParsing(t *testing.T) {
 		res, err = client.Get(endpoint)
 		require.NoError(t, err)
 
-		if blob, err = decoder.Decode(res.Body); err != nil {
+		require.Equalf(t, http.StatusOK, res.StatusCode, "unexpected status code %d fetching conformance blob '%s'", res.StatusCode, endpoint)
+
+		blob, err = decoder.Decode(res.Body)
+
+		_ = res.Body.Close()
+
+		if err != nil {
 			if errors.As(err, &me) {
 				t.Log(me.Details)
 			}
@@ -341,6 +354,10 @@ func getEndpoints(c *http.Client) ([]string, error) {
 
 	defer req.Body.Close()
 
+	if req.StatusCode != http.StatusOK {
+		return nil, fmt.Errorf("error occurred requesting the conformance endpoints: unexpected status code %d", req.StatusCode)
+	}
+
 	body, _ := io.ReadAll(req.Body)
 
 	var resp MDSGetEndpointsResponse
@@ -374,6 +391,10 @@ func getTestMetadata(s string, c *http.Client) (StatementJSON, error) {
 
 	defer req.Body.Close()
 
+	if req.StatusCode != http.StatusOK {
+		return statement, fmt.Errorf("error occurred requesting the conformance test metadata for '%s': unexpected status code %d", s, req.StatusCode)
+	}
+
 	body, err := io.ReadAll(req.Body)
 	if err != nil {
 		return statement, err

metadata/providers/cached/provider.go

@@ -156,7 +156,7 @@ func (p *Provider) outdated(mds *metadata.Metadata) bool {
 
 func (p *Provider) get() (data []byte, err error) {
 	if p.client == nil {
-		p.client = &http.Client{}
+		p.client = &http.Client{Timeout: metadata.DefaultMDSTimeout}
 	}
 
 	var res *http.Response