weiguoqiang/cert-manager
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

pki match: remove return values that are always nil

Browse Source 
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
This commit is contained in:
Tim Ramlot 2024-07-02 13:38:35 +02:00
parent ea349a0601
commit c58b08e7b7
No known key found for this signature in database
GPG Key ID: 47428728E0C2878D
5 changed files with 71 additions and 99 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
internal/controller/certificates/policies/checks.go
View File

@ -87,10 +87,7 @@ func SecretPrivateKeyMismatchesSpec(input Input) (string, string, bool) {
return InvalidKeyPair, fmt.Sprintf("Issuing certificate as Secret contains invalid private key data: %v", err), true
}
violations, err := pki.PrivateKeyMatchesSpec(pk, input.Certificate.Spec)
if err != nil {
return SecretMismatch, fmt.Sprintf("Failed to check private key is up to date: %v", err), true
}
violations := pki.PrivateKeyMatchesSpec(pk, input.Certificate.Spec)
if len(violations) > 0 {
return SecretMismatch, fmt.Sprintf("Existing private key is not up to date for spec: %v", violations), true
}
@ -240,14 +237,12 @@ func CurrentCertificateRequestMismatchesSpec(input Input) (string, string, bool)
// and is instead called by currentCertificateRequestValidForSpec if no there
// is no existing CertificateRequest resource.
func currentSecretValidForSpec(input Input) (string, string, bool) {
violations, err := pki.SecretDataAltNamesMatchSpec(input.Secret, input.Certificate.Spec)
x509Cert, err := pki.DecodeX509CertificateBytes(input.Secret.Data[corev1.TLSCertKey])
if err != nil {
// This case should never be reached as we already check the certificate data can
// be parsed in an earlier policy check, but handle it anyway.
// TODO: log a message
return "", "", false
return InvalidCertificate, fmt.Sprintf("Issuing certificate as Secret contains an invalid certificate: %v", err), true
}
// nolint: staticcheck // FuzzyX509AltNamesMatchSpec is used here for backwards compatibility
violations := pki.FuzzyX509AltNamesMatchSpec(x509Cert, input.Certificate.Spec)
if len(violations) > 0 {
return SecretMismatch, fmt.Sprintf("Issuing certificate as Existing issued Secret is not up to date for spec: %v", violations), true
}

5
pkg/controller/certificates/issuing/issuing_controller.go
View File

@ -208,10 +208,7 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error {
logf.WithResource(log, nextPrivateKeySecret).Error(err, "failed to parse next private key, waiting for keymanager controller")
return nil
}
pkViolations, err := pki.PrivateKeyMatchesSpec(pk, crt.Spec)
if err != nil {
return err
}
pkViolations := pki.PrivateKeyMatchesSpec(pk, crt.Spec)
if len(pkViolations) > 0 {
logf.WithResource(log, nextPrivateKeySecret).Info("stored next private key does not match requirements on Certificate resource, waiting for keymanager controller", "violations", pkViolations)
return nil

12
pkg/controller/certificates/keymanager/keymanager_controller.go
View File

@ -212,11 +212,7 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error {
return c.deleteSecretResources(ctx, secrets)
}
violations, err := pki.PrivateKeyMatchesSpec(pk, crt.Spec)
if err != nil {
log.Error(err, "Internal error verifying if private key matches spec - please open an issue.")
return nil
}
violations := pki.PrivateKeyMatchesSpec(pk, crt.Spec)
if len(violations) > 0 {
log.V(logf.DebugLevel).Info("Regenerating private key due to change in fields", "violations", violations)
c.recorder.Eventf(crt, corev1.EventTypeNormal, reasonDeleted, "Regenerating private key due to change in fields: %v", violations)
@ -246,11 +242,7 @@ func (c *controller) createNextPrivateKeyRotationPolicyNever(ctx context.Context
c.recorder.Eventf(crt, corev1.EventTypeWarning, reasonDecodeFailed, "Failed to decode private key stored in Secret %q - generating new key", crt.Spec.SecretName)
return c.createAndSetNextPrivateKey(ctx, crt)
}
violations, err := pki.PrivateKeyMatchesSpec(pk, crt.Spec)
if err != nil {
c.recorder.Eventf(crt, corev1.EventTypeWarning, reasonDecodeFailed, "Failed to check if private key stored in Secret %q is up to date - generating new key", crt.Spec.SecretName)
return c.createAndSetNextPrivateKey(ctx, crt)
}
violations := pki.PrivateKeyMatchesSpec(pk, crt.Spec)
if len(violations) > 0 {
c.recorder.Eventf(crt, corev1.EventTypeWarning, reasonCannotRegenerateKey, "User intervention required: existing private key in Secret %q does not match requirements on Certificate resource, mismatching fields: %v, but cert-manager cannot create new private key as the Certificate's .spec.privateKey.rotationPolicy is unset or set to Never. To allow cert-manager to create a new private key you can set .spec.privateKey.rotationPolicy to 'Always' (this will result in the private key being regenerated every time a cert is renewed) ", crt.Spec.SecretName, violations)
return nil

58
pkg/util/pki/match.go
View File

@ -22,23 +22,26 @@ import (
"crypto/ecdsa"
"crypto/ed25519"
"crypto/rsa"
"crypto/x509"
"crypto/x509/pkix"
"encoding/asn1"
"fmt"
"net"
"reflect"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/sets"
cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
"github.com/cert-manager/cert-manager/pkg/util"
)
// PrivateKeyMatchesSpec returns an error if the private key bit size
// doesn't match the provided spec. RSA, Ed25519 and ECDSA are supported.
// If any error is returned, a list of violations will also be returned.
func PrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec) ([]string, error) {
// PrivateKeyMatchesSpec returns a list of violations for the provided private
// key against the provided CertificateSpec. It will return an empty list/ nil
// if there are no violations found. RSA, Ed25519 and ECDSA private keys are
// supported.
// The function panics if the CertificateSpec contains an unknown key algorithm,
// since this should have been caught by the CertificateSpec validation already.
func PrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec) []string {
spec = *spec.DeepCopy()
if spec.PrivateKey == nil {
spec.PrivateKey = &cmapi.CertificatePrivateKey{}
@ -51,14 +54,16 @@ func PrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec) ([]
case cmapi.ECDSAKeyAlgorithm:
return ecdsaPrivateKeyMatchesSpec(pk, spec)
default:
return nil, fmt.Errorf("unrecognised key algorithm type %q", spec.PrivateKey.Algorithm)
// This should never happen as the CertificateSpec validation should
// catch this before it reaches this point.
panic(fmt.Sprintf("[PROGRAMMING ERROR] unrecognised key algorithm type %q", spec.PrivateKey.Algorithm))
}
}
func rsaPrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec) ([]string, error) {
func rsaPrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec) []string {
rsaPk, ok := pk.(*rsa.PrivateKey)
if !ok {
return []string{"spec.privateKey.algorithm"}, nil
return []string{"spec.privateKey.algorithm"}
}
var violations []string
// TODO: we should not use implicit defaulting here, and instead rely on
@ -73,13 +78,13 @@ func rsaPrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec)
if rsaPk.N.BitLen() != keySize {
violations = append(violations, "spec.privateKey.size")
}
return violations, nil
return violations
}
func ecdsaPrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec) ([]string, error) {
func ecdsaPrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec) []string {
ecdsaPk, ok := pk.(*ecdsa.PrivateKey)
if !ok {
return []string{"spec.privateKey.algorithm"}, nil
return []string{"spec.privateKey.algorithm"}
}
var violations []string
// TODO: we should not use implicit defaulting here, and instead rely on
@ -94,16 +99,16 @@ func ecdsaPrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec
if expectedKeySize != ecdsaPk.Curve.Params().BitSize {
violations = append(violations, "spec.privateKey.size")
}
return violations, nil
return violations
}
func ed25519PrivateKeyMatchesSpec(pk crypto.PrivateKey) ([]string, error) {
func ed25519PrivateKeyMatchesSpec(pk crypto.PrivateKey) []string {
_, ok := pk.(ed25519.PrivateKey)
if !ok {
return []string{"spec.privateKey.algorithm"}, nil
return []string{"spec.privateKey.algorithm"}
}
return nil, nil
return nil
}
func ipSlicesMatch(parsedIPs []net.IP, stringIPs []string) bool {
@ -273,17 +278,16 @@ func matchOtherNames(extension []pkix.Extension, specOtherNames []cmapi.OtherNam
return true, nil
}
// SecretDataAltNamesMatchSpec will compare a Secret resource containing certificate
// data to a CertificateSpec and return a list of 'violations' for any fields that
// do not match their counterparts.
// FuzzyX509AltNamesMatchSpec will compare a X509 Certificate to a CertificateSpec
// and return a list of 'violations' for any fields that do not match their counterparts.
//
// This is a purposely less comprehensive check than RequestMatchesSpec as some
// issuers override/force certain fields.
func SecretDataAltNamesMatchSpec(secret *corev1.Secret, spec cmapi.CertificateSpec) ([]string, error) {
x509cert, err := DecodeX509CertificateBytes(secret.Data[corev1.TLSCertKey])
if err != nil {
return nil, err
}
//
// Deprecated: This function is very fuzzy and makes too many assumptions about
// how the issuer maps a CSR to a certificate. We only keep it for backward compatibility
// reasons, but use other comparison functions when possible.
func FuzzyX509AltNamesMatchSpec(x509cert *x509.Certificate, spec cmapi.CertificateSpec) []string {
var violations []string
// Perform a 'loose' check on the x509 certificate to determine if the
@ -291,11 +295,11 @@ func SecretDataAltNamesMatchSpec(secret *corev1.Secret, spec cmapi.CertificateSp
// This check allows names to move between the DNSNames and CommonName
// field freely in order to account for CAs behaviour of promoting DNSNames
// to be CommonNames or vice-versa.
expectedDNSNames := sets.New[string](spec.DNSNames...)
expectedDNSNames := sets.New(spec.DNSNames...)
if spec.CommonName != "" {
expectedDNSNames.Insert(spec.CommonName)
}
allDNSNames := sets.New[string](x509cert.DNSNames...)
allDNSNames := sets.New(x509cert.DNSNames...)
if x509cert.Subject.CommonName != "" {
allDNSNames.Insert(x509cert.Subject.CommonName)
}
@ -322,7 +326,7 @@ func SecretDataAltNamesMatchSpec(secret *corev1.Secret, spec cmapi.CertificateSp
violations = append(violations, "spec.emailAddresses")
}
return violations, nil
return violations
}
func extractSANExtension(extensions []pkix.Extension) (pkix.Extension, error) {

80
pkg/util/pki/match_test.go
View File

@ -18,12 +18,12 @@ package pki_test
import (
"crypto"
"crypto/rand"
"crypto/x509"
"encoding/asn1"
"reflect"
"testing"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
@ -61,7 +61,6 @@ func TestPrivateKeyMatchesSpec(t *testing.T) {
expectedAlgo cmapi.PrivateKeyAlgorithm
expectedSize int
violations []string
err string
}{
"should match if keySize and algorithm are correct (RSA)": {
key: mustGenerateRSA(t, 2048),
@ -98,7 +97,7 @@ func TestPrivateKeyMatchesSpec(t *testing.T) {
}
for name, test := range tests {
t.Run(name, func(t *testing.T) {
violations, err := pki.PrivateKeyMatchesSpec(
violations := pki.PrivateKeyMatchesSpec(
test.key,
cmapi.CertificateSpec{
PrivateKey: &cmapi.CertificatePrivateKey{
@ -107,16 +106,6 @@ func TestPrivateKeyMatchesSpec(t *testing.T) {
},
},
)
switch {
case err != nil:
if test.err != err.Error() {
t.Errorf("error text did not match, got=%s, exp=%s", err.Error(), test.err)
}
default:
if test.err != "" {
t.Errorf("got no error but expected: %s", test.err)
}
}
if !reflect.DeepEqual(violations, test.violations) {
t.Errorf("violations did not match, got=%s, exp=%s", violations, test.violations)
}
@ -302,11 +291,10 @@ func TestRequestMatchesSpecSubject(t *testing.T) {
}
}
func TestSecretDataAltNamesMatchSpec(t *testing.T) {
func TestFuzzyX509AltNamesMatchSpec(t *testing.T) {
tests := map[string]struct {
data []byte
x509 *x509.Certificate
spec cmapi.CertificateSpec
err string
violations []string
}{
"should match if common name and dns names exactly equal": {
@ -314,7 +302,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) {
CommonName: "cn",
DNSNames: []string{"at", "least", "one"},
},
data: selfSignCertificate(t, cmapi.CertificateSpec{
x509: selfSignCertificate(t, cmapi.CertificateSpec{
CommonName: "cn",
DNSNames: []string{"at", "least", "one"},
}),
@ -324,7 +312,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) {
CommonName: "cn",
DNSNames: []string{"at", "least", "one"},
},
data: selfSignCertificate(t, cmapi.CertificateSpec{
x509: selfSignCertificate(t, cmapi.CertificateSpec{
DNSNames: []string{"cn", "at", "least", "one"},
}),
},
@ -333,7 +321,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) {
CommonName: "cn",
DNSNames: []string{"at", "least", "one"},
},
data: selfSignCertificate(t, cmapi.CertificateSpec{
x509: selfSignCertificate(t, cmapi.CertificateSpec{
DNSNames: []string{"at", "least", "one", "cn"},
}),
},
@ -341,7 +329,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) {
spec: cmapi.CertificateSpec{
DNSNames: []string{"at", "least", "one"},
},
data: selfSignCertificate(t, cmapi.CertificateSpec{
x509: selfSignCertificate(t, cmapi.CertificateSpec{
CommonName: "at",
DNSNames: []string{"least", "one"},
}),
@ -351,7 +339,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) {
CommonName: "cn",
DNSNames: []string{"at", "least", "one"},
},
data: selfSignCertificate(t, cmapi.CertificateSpec{
x509: selfSignCertificate(t, cmapi.CertificateSpec{
DNSNames: []string{"at", "least", "one"},
}),
violations: []string{"spec.commonName"},
@ -361,7 +349,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) {
CommonName: "cn",
DNSNames: []string{"at", "least", "one", "other"},
},
data: selfSignCertificate(t, cmapi.CertificateSpec{
x509: selfSignCertificate(t, cmapi.CertificateSpec{
DNSNames: []string{"at", "least", "one"},
}),
violations: []string{"spec.commonName", "spec.dnsNames"},
@ -370,7 +358,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) {
spec: cmapi.CertificateSpec{
DNSNames: []string{"at", "least", "one"},
},
data: selfSignCertificate(t, cmapi.CertificateSpec{
x509: selfSignCertificate(t, cmapi.CertificateSpec{
CommonName: "cn",
DNSNames: []string{"at", "least", "one", "other"},
}),
@ -381,7 +369,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) {
CommonName: "cn",
DNSNames: []string{"at", "least", "one"},
},
data: selfSignCertificate(t, cmapi.CertificateSpec{
x509: selfSignCertificate(t, cmapi.CertificateSpec{
CommonName: "cn",
DNSNames: []string{"at", "least", "one", "other"},
}),
@ -391,7 +379,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) {
spec: cmapi.CertificateSpec{
DNSNames: []string{"at", "least", "one"},
},
data: selfSignCertificate(t, cmapi.CertificateSpec{
x509: selfSignCertificate(t, cmapi.CertificateSpec{
CommonName: "at",
DNSNames: []string{"at", "least", "one"},
}),
@ -401,7 +389,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) {
CommonName: "cn",
DNSNames: []string{"at", "least", "one"},
},
data: selfSignCertificate(t, cmapi.CertificateSpec{
x509: selfSignCertificate(t, cmapi.CertificateSpec{
CommonName: "at",
DNSNames: []string{"at", "least", "one", "cn"},
}),
@ -410,7 +398,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) {
spec: cmapi.CertificateSpec{
IPAddresses: []string{"127.0.0.1"},
},
data: selfSignCertificate(t, cmapi.CertificateSpec{
x509: selfSignCertificate(t, cmapi.CertificateSpec{
IPAddresses: []string{"127.0.0.1"},
}),
},
@ -418,7 +406,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) {
spec: cmapi.CertificateSpec{
IPAddresses: []string{"127.0.0.1"},
},
data: selfSignCertificate(t, cmapi.CertificateSpec{
x509: selfSignCertificate(t, cmapi.CertificateSpec{
IPAddresses: []string{"127.0.2.1"},
}),
violations: []string{"spec.ipAddresses"},
@ -427,7 +415,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) {
spec: cmapi.CertificateSpec{
IPAddresses: []string{"127.0.0.1"},
},
data: selfSignCertificate(t, cmapi.CertificateSpec{
x509: selfSignCertificate(t, cmapi.CertificateSpec{
CommonName: "127.0.0.1",
IPAddresses: []string{"127.0.0.1"},
}),
@ -436,17 +424,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) {
}
for name, test := range tests {
t.Run(name, func(t *testing.T) {
violations, err := pki.SecretDataAltNamesMatchSpec(&corev1.Secret{Data: map[string][]byte{corev1.TLSCertKey: test.data}}, test.spec)
switch {
case err != nil:
if test.err != err.Error() {
t.Errorf("error text did not match, got=%s, exp=%s", err.Error(), test.err)
}
default:
if test.err != "" {
t.Errorf("got no error but expected: %s", test.err)
}
}
violations := pki.FuzzyX509AltNamesMatchSpec(test.x509, test.spec)
if !reflect.DeepEqual(violations, test.violations) {
t.Errorf("violations did not match, got=%s, exp=%s", violations, test.violations)
}
@ -454,23 +432,29 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) {
}
}
func selfSignCertificate(t *testing.T, spec cmapi.CertificateSpec) []byte {
pk, err := pki.GenerateRSAPrivateKey(2048)
if err != nil {
t.Fatal(err)
}
func selfSignCertificate(t *testing.T, spec cmapi.CertificateSpec) *x509.Certificate {
template, err := pki.CertificateTemplateFromCertificate(&cmapi.Certificate{Spec: spec})
if err != nil {
t.Fatal(err)
}
pemData, _, err := pki.SignCertificate(template, template, pk.Public(), pk)
pk, err := pki.GenerateRSAPrivateKey(2048)
if err != nil {
t.Fatal(err)
}
return pemData
// Marshal and unmarshal to ensure all fields are set correctly
certDER, err := x509.CreateCertificate(rand.Reader, template, template, pk.Public(), pk)
if err != nil {
t.Fatal(err)
}
cert, err := x509.ParseCertificate(certDER)
if err != nil {
t.Fatal(err)
}
return cert
}
func mustBuildCertificateRequest(t *testing.T, crt *cmapi.Certificate) *cmapi.CertificateRequest {