Merge pull request #515 from SgtCoDFish/negative-serial-special-case

RELEASE BLOCKER: Add special case handling of cert with negative serial number

go.mod

@@ -2,10 +2,6 @@ module github.com/cert-manager/trust-manager
 
 go 1.23.0
 
-// Added on bump to Go 1.23 as it seems like our current trust bundle contains certs with negative serial numbers.
-// See also https://pkg.go.dev/crypto/x509#ParseCertificate
-godebug x509negativeserial=1
-
 require (
 	github.com/go-logr/logr v1.4.2
 	github.com/onsi/ginkgo/v2 v2.22.2

make/test-unit.mk

@@ -13,9 +13,17 @@
 # limitations under the License.
 
 .PHONY: test-unit
-## Unit tests
+## Run all unit tests for trust-manager
+## @category Testing
+test-unit: test-unit-standard test-unit-negativeserial
+
+
+.PHONY: test-unit-standard
+test-unit-standard: | $(NEEDS_GOTESTSUM) $(ARTIFACTS)
+## Standard unit tests. These tests are in contrast to test-unit-negativeserial,
+## and do not set the x509negativeserial GODEBUG value.
+## We're testing against a "standard" configuration of trust-manager.
 ## @category Testing
-test-unit: | $(NEEDS_GOTESTSUM) $(ARTIFACTS)
 	$(GOTESTSUM) \
 		--junitfile=$(ARTIFACTS)/junit-go-e2e.xml \
 		-- \
@@ -24,3 +32,18 @@ test-unit: | $(NEEDS_GOTESTSUM) $(ARTIFACTS)
 		-- \
 		-ldflags $(go_manager_ldflags) \
 		-test.timeout 2m
+
+.PHONY: test-unit-negativeserial
+## Specialised unit tests which set the x509negativeserial GODEBUG value
+## so we can test our handling of a special case introduced in Go 1.23.
+## See ./pkg/compat for details
+## @category Testing
+test-unit-negativeserial: | $(NEEDS_GOTESTSUM) $(ARTIFACTS)
+	$(GOTESTSUM) \
+		--junitfile=$(ARTIFACTS)/junit-go-unit-negativeserial.xml \
+		-- \
+		-tags=testnegativeserialon \
+		./pkg/compat/... \
+		-- \
+		-ldflags $(go_manager_ldflags) \
+		-test.timeout 2m

pkg/bundle/source.go

@@ -52,7 +52,10 @@ type bundleData struct {
 // is each bundle is concatenated together with a new line character.
 func (b *bundle) buildSourceBundle(ctx context.Context, sources []trustapi.BundleSource, formats *trustapi.AdditionalFormats) (bundleData, error) {
 	var resolvedBundle bundleData
-	certPool := util.NewCertPool(util.WithFilteredExpiredCerts(b.FilterExpiredCerts))
+	certPool := util.NewCertPool(
+		util.WithFilteredExpiredCerts(b.FilterExpiredCerts),
+		util.WithLogger(b.Log.WithName("cert-pool")),
+	)
 
 	for _, source := range sources {
 		var (

pkg/compat/negative_serial_number.go

@@ -0,0 +1,139 @@
+/*
+Copyright 2024 The cert-manager Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package compat
+
+const (
+	// negativeSerialNumberCAPEM is a special-cased CA which appears in the
+	// first trust-manager public trust package,
+	// quay.io/jetstack/cert-manager-package-debian:20210119.0
+	// The cert has a negative serial number meaning that as of Go 1.23 it
+	// becomes unparseable by [x509.ParseCertificate](https://pkg.go.dev/crypto/x509#ParseCertificate)
+	// unless the x509negativeserial GODEBUG value is set to `1`.
+	// This certificate is special cased such that trust-manager won't fail to start
+	// if this cert is present in the bundle - instead, it'll be ignored.
+	// The PEM data is included for reference, and for testing
+	negativeSerialNumberCAPEM string = `-----BEGIN CERTIFICATE-----
+MIIFVjCCBD6gAwIBAgIQ7is969Qh3hSoYqwE893EATANBgkqhkiG9w0BAQUFADCB
+8zELMAkGA1UEBhMCRVMxOzA5BgNVBAoTMkFnZW5jaWEgQ2F0YWxhbmEgZGUgQ2Vy
+dGlmaWNhY2lvIChOSUYgUS0wODAxMTc2LUkpMSgwJgYDVQQLEx9TZXJ2ZWlzIFB1
+YmxpY3MgZGUgQ2VydGlmaWNhY2lvMTUwMwYDVQQLEyxWZWdldSBodHRwczovL3d3
+dy5jYXRjZXJ0Lm5ldC92ZXJhcnJlbCAoYykwMzE1MDMGA1UECxMsSmVyYXJxdWlh
+IEVudGl0YXRzIGRlIENlcnRpZmljYWNpbyBDYXRhbGFuZXMxDzANBgNVBAMTBkVD
+LUFDQzAeFw0wMzAxMDcyMzAwMDBaFw0zMTAxMDcyMjU5NTlaMIHzMQswCQYDVQQG
+EwJFUzE7MDkGA1UEChMyQWdlbmNpYSBDYXRhbGFuYSBkZSBDZXJ0aWZpY2FjaW8g
+KE5JRiBRLTA4MDExNzYtSSkxKDAmBgNVBAsTH1NlcnZlaXMgUHVibGljcyBkZSBD
+ZXJ0aWZpY2FjaW8xNTAzBgNVBAsTLFZlZ2V1IGh0dHBzOi8vd3d3LmNhdGNlcnQu
+bmV0L3ZlcmFycmVsIChjKTAzMTUwMwYDVQQLEyxKZXJhcnF1aWEgRW50aXRhdHMg
+ZGUgQ2VydGlmaWNhY2lvIENhdGFsYW5lczEPMA0GA1UEAxMGRUMtQUNDMIIBIjAN
+BgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAsyLHT+KXQpWIR4NA9h0X84NzJB5R
+85iKw5K4/0CQBXCHYMkAqbWUZRkiFRfCQ2xmRJoNBD45b6VLeqpjt4pEndljkYRm
+4CgPukLjbo73FCeTae6RDqNfDrHrZqJyTxIThmV6PttPB/SnCWDaOkKZx7J/sxaV
+HMf5NLWUhdWZXqBIoH7nF2W4onW4HvPlQn2v7fOKSGRdghST2MDk/7NQcvJ29rNd
+QlB50JQ+awwAvthrDk4q7D7SzIKiGGUzE3eeml0aE9jD2z3Il3rucO2n5nzbcc8t
+lGLfbdb1OL4/pYUKGbio2Al1QnDE6u/LDsg0qBIimAy4E5S2S+zw0JDnJwIDAQAB
+o4HjMIHgMB0GA1UdEQQWMBSBEmVjX2FjY0BjYXRjZXJ0Lm5ldDAPBgNVHRMBAf8E
+BTADAQH/MA4GA1UdDwEB/wQEAwIBBjAdBgNVHQ4EFgQUoMOLRKo3pUW/l4Ba0fF4
+opvpXY0wfwYDVR0gBHgwdjB0BgsrBgEEAfV4AQMBCjBlMCwGCCsGAQUFBwIBFiBo
+dHRwczovL3d3dy5jYXRjZXJ0Lm5ldC92ZXJhcnJlbDA1BggrBgEFBQcCAjApGidW
+ZWdldSBodHRwczovL3d3dy5jYXRjZXJ0Lm5ldC92ZXJhcnJlbCAwDQYJKoZIhvcN
+AQEFBQADggEBAKBIW4IB9k1IuDlVNZyAelOZ1Vr/sXE7zDkJlF7W2u++AVtd0x7Y
+/X1PzaBB4DSTv8vihpw3kpBWHNzrKQXlxJ7HNd+KDM3FIUPpqojlNcAZQmNaAl6k
+SBg6hW/cnbw/nZzBh7h6YQjpdwt/cKt63dmXLGQehb+8dJahw3oS7AwaboMMPOhy
+Rp/7SNVel+axofjk70YllJyJ22k4vuxcDlbHZVHlUIiIv0LVKz3l+bqeLrPK9HOS
+Agu+TGbrIP65y7WZf+a2E/rKS03Z7lNGBjvGTq2TWoF+bCpLagVFjPIhpDGQh2xl
+nJ2lYJU6Un/10asIbvPuW/mIPX64b24D5EI=
+-----END CERTIFICATE-----`
+
+	negativeSerialNumberCAFingerprint = "88497f01602f3154246ae28c4d5aef10f1d87ebb76626f4ae0b7f95ba7968799"
+
+	negativeSerialNumberErr = "x509: negative serial number"
+)
+
+/*
+
+Further details of negativeSerialNumberCA:
+
+sha256 fingerprint from OpenSSL (should correspond to negativeSerialNumberCAFingerprint)
+sha256 Fingerprint=88:49:7F:01:60:2F:31:54:24:6A:E2:8C:4D:5A:EF:10:F1:D8:7E:BB:76:62:6F:4A:E0:B7:F9:5B:A7:96:87:99
+
+OpenSSL Dump:
+Certificate:
+    Data:
+        Version: 3 (0x2)
+        Serial Number:
+             (Negative)11:d4:c2:14:2b:de:21:eb:57:9d:53:fb:0c:22:3b:ff
+        Signature Algorithm: sha1WithRSAEncryption
+        Issuer: C=ES, O=Agencia Catalana de Certificacio (NIF Q-0801176-I), OU=Serveis Publics de Certificacio, OU=Vegeu https://www.catcert.net/verarrel (c)03, OU=Jerarquia Entitats de Certificacio Catalanes, CN=EC-ACC
+        Validity
+            Not Before: Jan  7 23:00:00 2003 GMT
+            Not After : Jan  7 22:59:59 2031 GMT
+        Subject: C=ES, O=Agencia Catalana de Certificacio (NIF Q-0801176-I), OU=Serveis Publics de Certificacio, OU=Vegeu https://www.catcert.net/verarrel (c)03, OU=Jerarquia Entitats de Certificacio Catalanes, CN=EC-ACC
+        Subject Public Key Info:
+            Public Key Algorithm: rsaEncryption
+                Public-Key: (2048 bit)
+                Modulus:
+                    00:b3:22:c7:4f:e2:97:42:95:88:47:83:40:f6:1d:
+                    17:f3:83:73:24:1e:51:f3:98:8a:c3:92:b8:ff:40:
+                    90:05:70:87:60:c9:00:a9:b5:94:65:19:22:15:17:
+                    c2:43:6c:66:44:9a:0d:04:3e:39:6f:a5:4b:7a:aa:
+                    63:b7:8a:44:9d:d9:63:91:84:66:e0:28:0f:ba:42:
+                    e3:6e:8e:f7:14:27:93:69:ee:91:0e:a3:5f:0e:b1:
+                    eb:66:a2:72:4f:12:13:86:65:7a:3e:db:4f:07:f4:
+                    a7:09:60:da:3a:42:99:c7:b2:7f:b3:16:95:1c:c7:
+                    f9:34:b5:94:85:d5:99:5e:a0:48:a0:7e:e7:17:65:
+                    b8:a2:75:b8:1e:f3:e5:42:7d:af:ed:f3:8a:48:64:
+                    5d:82:14:93:d8:c0:e4:ff:b3:50:72:f2:76:f6:b3:
+                    5d:42:50:79:d0:94:3e:6b:0c:00:be:d8:6b:0e:4e:
+                    2a:ec:3e:d2:cc:82:a2:18:65:33:13:77:9e:9a:5d:
+                    1a:13:d8:c3:db:3d:c8:97:7a:ee:70:ed:a7:e6:7c:
+                    db:71:cf:2d:94:62:df:6d:d6:f5:38:be:3f:a5:85:
+                    0a:19:b8:a8:d8:09:75:42:70:c4:ea:ef:cb:0e:c8:
+                    34:a8:12:22:98:0c:b8:13:94:b6:4b:ec:f0:d0:90:
+                    e7:27
+                Exponent: 65537 (0x10001)
+        X509v3 extensions:
+            X509v3 Subject Alternative Name:
+                email:ec_acc@catcert.net
+            X509v3 Basic Constraints: critical
+                CA:TRUE
+            X509v3 Key Usage: critical
+                Certificate Sign, CRL Sign
+            X509v3 Subject Key Identifier:
+                A0:C3:8B:44:AA:37:A5:45:BF:97:80:5A:D1:F1:78:A2:9B:E9:5D:8D
+            X509v3 Certificate Policies:
+                Policy: 1.3.6.1.4.1.15096.1.3.1.10
+                  CPS: https://www.catcert.net/verarrel
+                  User Notice:
+                    Explicit Text: Vegeu https://www.catcert.net/verarrel
+    Signature Algorithm: sha1WithRSAEncryption
+    Signature Value:
+        a0:48:5b:82:01:f6:4d:48:b8:39:55:35:9c:80:7a:53:99:d5:
+        5a:ff:b1:71:3b:cc:39:09:94:5e:d6:da:ef:be:01:5b:5d:d3:
+        1e:d8:fd:7d:4f:cd:a0:41:e0:34:93:bf:cb:e2:86:9c:37:92:
+        90:56:1c:dc:eb:29:05:e5:c4:9e:c7:35:df:8a:0c:cd:c5:21:
+        43:e9:aa:88:e5:35:c0:19:42:63:5a:02:5e:a4:48:18:3a:85:
+        6f:dc:9d:bc:3f:9d:9c:c1:87:b8:7a:61:08:e9:77:0b:7f:70:
+        ab:7a:dd:d9:97:2c:64:1e:85:bf:bc:74:96:a1:c3:7a:12:ec:
+        0c:1a:6e:83:0c:3c:e8:72:46:9f:fb:48:d5:5e:97:e6:b1:a1:
+        f8:e4:ef:46:25:94:9c:89:db:69:38:be:ec:5c:0e:56:c7:65:
+        51:e5:50:88:88:bf:42:d5:2b:3d:e5:f9:ba:9e:2e:b3:ca:f4:
+        73:92:02:0b:be:4c:66:eb:20:fe:b9:cb:b5:99:7f:e6:b6:13:
+        fa:ca:4b:4d:d9:ee:53:46:06:3b:c6:4e:ad:93:5a:81:7e:6c:
+        2a:4b:6a:05:45:8c:f2:21:a4:31:90:87:6c:65:9c:9d:a5:60:
+        95:3a:52:7f:f5:d1:ab:08:6e:f3:ee:5b:f9:88:3d:7e:b8:6f:
+        6e:03:e4:42
+*/

pkg/compat/negative_serial_number_godebug_test.go

@@ -0,0 +1,65 @@
+//go:build testnegativeserialon
+
+/*
+Copyright 2024 The cert-manager Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+// 2024-12-17: The gocheckcompilerdirectives linter hasn't been updated for
+// some time, and doesn't know about the go:debug directive and so must be
+// disabled in this file.
+// The nolint is here so that we still lint the go:build at the top of the file.
+//nolint:gocheckcompilerdirectives
+//go:debug x509negativeserial=1
+
+package compat
+
+// This file is built only with the testnegativeserialon tag so that we
+// can use go:debug.
+// If we didn't have the build tag, we'd get an error since we'd have two
+// tests duplicating the x509negativeserial go:debug constraint
+
+import (
+	"crypto/x509"
+	"testing"
+)
+
+func TestNegativeSerialNumberCASanityGoDebugOn(t *testing.T) {
+	// Check that the special-cased CA doesn't produce any errors if
+	// x509negativeserial is set to `1`. This lets us be confident that
+	// ParseCertificate is only special casing the negative serial number err
+	der := negativeSerialNumberCADER(t)
+
+	// First, check that the stdlib ParseCertificate function works as expected
+	x509Cert, x509Err := x509.ParseCertificate(der)
+	if x509Err != nil {
+		// use Errorf rather than Fatalf so we can compare the errors between
+		// our implementation and x509.ParseCertificate
+		t.Errorf("expected negativeSerialNumberCA to produce no error with x509negativeserial=1 using x509.ParseCertificate but got: %s", x509Err)
+	}
+
+	// Next, check that our wrapper works as expected
+	cert, err := ParseCertificate(der)
+	if err != nil {
+		t.Errorf("expected negativeSerialNumberCA to produce no error with x509negativeserial=1 using compat.ParseCertificate but got: %s", err)
+	}
+
+	if x509Cert == nil && cert == nil {
+		return
+	}
+
+	if !x509Cert.Equal(cert) {
+		t.Errorf("expected certs from x509.ParseCertificate and compat.ParseCertificate to be equal but they differ")
+	}
+}

pkg/compat/negative_serial_number_test.go

@@ -0,0 +1,110 @@
+//go:build !testnegativeserialon
+
+/*
+Copyright 2024 The cert-manager Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+// 2024-12-17: The gocheckcompilerdirectives linter hasn't been updated for
+// some time, and doesn't know about the go:debug directive and so must be
+// disabled in this file.
+// The nolint is here so that we still lint the go:build at the top of the file.
+//nolint:gocheckcompilerdirectives
+//go:debug x509negativeserial=0
+
+package compat
+
+import (
+	"crypto/sha256"
+	"crypto/x509"
+	"encoding/hex"
+	"errors"
+	"strings"
+	"testing"
+)
+
+func TestNegativeSerialNumberCASanity(t *testing.T) {
+	// Check that parsing the special cased cert with x509negativeserial=0
+	// actually does result in an error describing the negative serial number
+	// This also checks that the our value for the err (negativeSerialNumberErr)
+	// is correct, since that error value isn't exported from the x509 package
+	der := negativeSerialNumberCADER(t)
+
+	x509Cert, x509Err := x509.ParseCertificate(der)
+	if x509Err == nil {
+		// has to be fatal since we inspect the error later
+		t.Fatalf("expected negativeSerialNumberCA to produce an error with x509negativeserial=0 and x509.ParseCertificate")
+	}
+
+	if x509Cert != nil {
+		t.Errorf("expected negativeSerialNumberCA to produce a nil cert with x509negativeserial=0 and x509.ParseCertificate")
+	}
+
+	if !strings.HasSuffix(x509Err.Error(), negativeSerialNumberErr) {
+		t.Fatalf("expected error from parsing special case cert to end with %s with x509negativeserial=0; got %s", negativeSerialNumberErr, x509Err.Error())
+	}
+}
+
+func TestParseCertificateSpecialCase(t *testing.T) {
+	// Check that our special casing logic works as expected when we use ParseCertificate
+	der := negativeSerialNumberCADER(t)
+
+	// get the error from the function we wrap, so we can compare errors later
+	_, x509Err := x509.ParseCertificate(der)
+	if x509Err == nil {
+		// has to be fatal since we use the error later
+		t.Fatalf("expected negativeSerialNumberCA to produce an error with x509negativeserial=0 and x509.ParseCertificate")
+	}
+
+	// now call _our_ compat.ParseCertificate function to test the output
+	cert, err := ParseCertificate(der)
+	if err == nil {
+		// has to be fatal since the below checks aren't valid if this doesn't error
+		t.Fatalf("expected negativeSerialNumberCA to produce an error with x509negativeserial=0 and compat.ParseCertificate")
+	}
+
+	if cert != nil {
+		t.Errorf("expected negativeSerialNumberCA to produce a nil cert with x509negativeserial=0 and compat.ParseCertificate")
+	}
+
+	if !IsSkipError(err) {
+		t.Errorf("expected IsSkipError to be true for the error from compat.ParseCertificate with negativeSerialNumberCA and x509negativeserial=0")
+	}
+
+	var compatErr Error
+	if !errors.As(err, &compatErr) {
+		t.Errorf("expected error from compat.ParseCertificate with negativeSerialNumberCA and x509negativeserial=0 to be a compat.Error")
+		return
+	}
+
+	if compatErr.Unwrap().Error() != x509Err.Error() {
+		t.Errorf("expected underlying compat error to match error from x509.ParseCertificate but got:\ncompatError: %s\nx509 Error: %s", compatErr.Unwrap().Error(), x509Err.Error())
+	}
+
+	if compatErr.Error() == x509Err.Error() {
+		t.Errorf("expected compat.Error error to be different to error from x509.ParseCertificate")
+	}
+}
+
+func TestNegativeSerialNumberCAFingerprint(t *testing.T) {
+	// ensure that the fingerprint we have matches the CA we test against
+	der := negativeSerialNumberCADER(t)
+
+	fingerprintBytes := sha256.Sum256(der)
+	fingerprint := hex.EncodeToString(fingerprintBytes[:])
+
+	if fingerprint != negativeSerialNumberCAFingerprint {
+		t.Fatalf("expected fingerprint in negativeSerialNumberCAFingerprint to be %s but got: %s", fingerprint, negativeSerialNumberCAFingerprint)
+	}
+}