diff --git a/make/test-unit.mk b/make/test-unit.mk index b17b346d..46da125d 100644 --- a/make/test-unit.mk +++ b/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 diff --git a/pkg/bundle/source.go b/pkg/bundle/source.go index 7c86af68..2a034275 100644 --- a/pkg/bundle/source.go +++ b/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 ( diff --git a/pkg/compat/negative_serial_number.go b/pkg/compat/negative_serial_number.go new file mode 100644 index 00000000..ca710a27 --- /dev/null +++ b/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 +*/ diff --git a/pkg/compat/negative_serial_number_godebug_test.go b/pkg/compat/negative_serial_number_godebug_test.go new file mode 100644 index 00000000..7be2a995 --- /dev/null +++ b/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") + } +} diff --git a/pkg/compat/negative_serial_number_test.go b/pkg/compat/negative_serial_number_test.go new file mode 100644 index 00000000..153d56d7 --- /dev/null +++ b/pkg/compat/negative_serial_number_test.go @@ -0,0 +1,109 @@ +//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") + } + + 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) + } +} diff --git a/pkg/compat/parse_cert.go b/pkg/compat/parse_cert.go new file mode 100644 index 00000000..d378e67f --- /dev/null +++ b/pkg/compat/parse_cert.go @@ -0,0 +1,90 @@ +/* +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 + +import ( + "crypto/sha256" + "crypto/x509" + "encoding/hex" + "errors" + "fmt" + "strings" +) + +// IsSkipError returns true if the error means the cert should be skipped over +// rather then being a fatal error +func IsSkipError(err error) bool { + return errors.As(err, &Error{}) +} + +// Error is returned when there's a certificate compatibility error which +// implies that a certificate should be skipped +type Error struct { + Underlying error + Message string +} + +func (e Error) Unwrap() error { + return e.Underlying +} + +func (e Error) Error() string { + return e.Message +} + +// ParseCertificate parses a single certificate from the given ASN.1 DER data +// This is a wrapper for the x509.ParseCertificate function, handling the +// special case of a cert in a public trust bundle with a negative serial number +// which produces an error by default in Go 1.23. +// If using Go 1.22 or older, or if using Go 1.23 or newer and the GODEBUG +// value `x509negativeserial` is set to `1`, that specific cert will parse +// with no error. +// Otherwise, a special Error value will be returned so that the certificate +// can be skipped using IsSkipError +func ParseCertificate(der []byte) (*x509.Certificate, error) { + cert, err := x509.ParseCertificate(der) + if err == nil { + return cert, nil + } + + // If there was an error, check if the cert is the special case + fingerprintBytes := sha256.Sum256(der) + fingerprint := hex.EncodeToString(fingerprintBytes[:]) + + if fingerprint == negativeSerialNumberCAFingerprint { + // The cert was the special case; handle it differently + return handleNegativeSerialNumberSpecialCase(cert, err) + } + + // if the error is for a cert we have NOT special cased, return the + // error as we received it (to avoid allowing negative serial numbers + // for any other CAs, such as in private PKI) + return cert, err +} + +func handleNegativeSerialNumberSpecialCase(cert *x509.Certificate, err error) (*x509.Certificate, error) { + // The cert was the special case; check if the error was due to a + // negative serial number (to account for future changes to ParseCertificate + // which could return a different error, although we do test that) + if !strings.HasSuffix(err.Error(), negativeSerialNumberErr) { + return cert, err + } + + message := fmt.Sprintf("cert in bundle with CN=EC-ACC and fingerprint '%s' has negative serial number and will be skipped", negativeSerialNumberCAFingerprint) + + return nil, Error{Underlying: err, Message: message} +} diff --git a/pkg/compat/util_test.go b/pkg/compat/util_test.go new file mode 100644 index 00000000..34b573c3 --- /dev/null +++ b/pkg/compat/util_test.go @@ -0,0 +1,36 @@ +/* +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 + +import ( + "encoding/pem" + "testing" +) + +// This function is in a separate file so it can be shared between our tests +// which set GODEBUG and have different tags + +func negativeSerialNumberCADER(t *testing.T) []byte { + var block *pem.Block + block, _ = pem.Decode([]byte(negativeSerialNumberCAPEM)) + + if block == nil { + t.Fatalf("invalid test: negativeSerialNumberCA isn't valid PEM data") + } + + return block.Bytes +} diff --git a/pkg/util/cert_pool.go b/pkg/util/cert_pool.go index 971ca5b2..8e52fdf8 100644 --- a/pkg/util/cert_pool.go +++ b/pkg/util/cert_pool.go @@ -24,6 +24,10 @@ import ( "fmt" "slices" "time" + + "github.com/go-logr/logr" + + "github.com/cert-manager/trust-manager/pkg/compat" ) // CertPool is a set of certificates. @@ -31,6 +35,8 @@ type CertPool struct { certificates map[[32]byte]*x509.Certificate filterExpired bool + + logger logr.Logger } type Option func(*CertPool) @@ -41,12 +47,20 @@ func WithFilteredExpiredCerts(filterExpired bool) Option { } } +func WithLogger(logger logr.Logger) Option { + return func(cp *CertPool) { + cp.logger = logger + } +} + // NewCertPool returns a new, empty CertPool. // It will deduplicate certificates based on their SHA256 hash. // Optionally, it can filter out expired certificates. func NewCertPool(options ...Option) *CertPool { certPool := &CertPool{ certificates: make(map[[32]byte]*x509.Certificate), + + logger: logr.Discard(), } for _, option := range options { @@ -100,8 +114,15 @@ func (cp *CertPool) AddCertsFromPEM(pemData []byte) error { return fmt.Errorf("invalid PEM block in bundle; blocks are not permitted to have PEM headers") } - certificate, err := x509.ParseCertificate(block.Bytes) + certificate, err := compat.ParseCertificate(block.Bytes) if err != nil { + if compat.IsSkipError(err) { + // there's some compatibility error; we don't want to fail + // the whole bundle for this cert, we should just skip it + cp.logger.Info("skipping a certificate in PEM bundle for compatibility reasons", "details", err) + continue + } + // the presence of an invalid cert (including things which aren't certs) // should cause the bundle to be rejected return fmt.Errorf("invalid PEM block in bundle; invalid PEM certificate: %w", err)