From bf52770e413dc6095203cd5ec1716dc3764eee8e Mon Sep 17 00:00:00 2001
From: Tomas Nozicka <tnozicka@gmail.com>
Date: Mon, 17 Aug 2020 14:35:48 +0200
Subject: [PATCH] Add context to x509 verify failures

---
 .../pkg/authentication/request/x509/x509.go   | 29 +++++++++-
 .../authentication/request/x509/x509_test.go  | 57 +++++++++++++++++++
 2 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/request/x509/x509.go b/staging/src/k8s.io/apiserver/pkg/authentication/request/x509/x509.go
index da3ef45d26c..fc5c54ba798 100644
--- a/staging/src/k8s.io/apiserver/pkg/authentication/request/x509/x509.go
+++ b/staging/src/k8s.io/apiserver/pkg/authentication/request/x509/x509.go
@@ -19,8 +19,10 @@ package x509
 import (
 	"crypto/x509"
 	"crypto/x509/pkix"
+	"encoding/hex"
 	"fmt"
 	"net/http"
+	"strings"
 	"time"
 
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
@@ -82,6 +84,27 @@ func (f UserConversionFunc) User(chain []*x509.Certificate) (*authenticator.Resp
 	return f(chain)
 }
 
+func columnSeparatedHex(d []byte) string {
+	h := strings.ToUpper(hex.EncodeToString(d))
+	var sb strings.Builder
+	for i, r := range h {
+		sb.WriteRune(r)
+		if i%2 == 1 && i != len(h)-1 {
+			sb.WriteRune(':')
+		}
+	}
+	return sb.String()
+}
+
+func certificateIdentifier(c *x509.Certificate) string {
+	return fmt.Sprintf(
+		"SN=%d, SKID=%s, AKID=%s",
+		c.SerialNumber,
+		columnSeparatedHex(c.SubjectKeyId),
+		columnSeparatedHex(c.AuthorityKeyId),
+	)
+}
+
 // VerifyOptionFunc is function which provides a shallow copy of the VerifyOptions to the authenticator.  This allows
 // for cases where the options (particularly the CAs) can change.  If the bool is false, then the returned VerifyOptions
 // are ignored and the authenticator will express "no opinion".  This allows a clear signal for cases where a CertPool
@@ -129,7 +152,11 @@ func (a *Authenticator) AuthenticateRequest(req *http.Request) (*authenticator.R
 	clientCertificateExpirationHistogram.Observe(remaining.Seconds())
 	chains, err := req.TLS.PeerCertificates[0].Verify(optsCopy)
 	if err != nil {
-		return nil, false, err
+		return nil, false, fmt.Errorf(
+			"verifying certificate %s failed: %w",
+			certificateIdentifier(req.TLS.PeerCertificates[0]),
+			err,
+		)
 	}
 
 	var errlist []error
diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/request/x509/x509_test.go b/staging/src/k8s.io/apiserver/pkg/authentication/request/x509/x509_test.go
index e2d156152e5..2f37cc94d92 100644
--- a/staging/src/k8s.io/apiserver/pkg/authentication/request/x509/x509_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/authentication/request/x509/x509_test.go
@@ -881,6 +881,8 @@ func getCertsFromFile(t *testing.T, names ...string) []*x509.Certificate {
 }
 
 func getCert(t *testing.T, pemData string) *x509.Certificate {
+	t.Helper()
+
 	pemBlock, _ := pem.Decode([]byte(pemData))
 	cert, err := x509.ParseCertificate(pemBlock.Bytes)
 	if err != nil {
@@ -897,3 +899,58 @@ func getCerts(t *testing.T, pemData ...string) []*x509.Certificate {
 	}
 	return certs
 }
+
+func TestCertificateIdentifier(t *testing.T) {
+	tt := []struct {
+		name               string
+		cert               *x509.Certificate
+		expectedIdentifier string
+	}{
+		{
+			name:               "client cert",
+			cert:               getCert(t, clientCNCert),
+			expectedIdentifier: "SN=1, SKID=E7:FB:1F:45:F0:71:77:AF:8C:10:4A:0A:42:03:F5:1F:1F:07:CF:DF, AKID=3D:F0:F7:30:3D:3B:EB:3A:55:68:FA:F5:43:C9:C7:AC:E1:3F:10:78",
+		},
+		{
+			name: "nil serial",
+			cert: func() *x509.Certificate {
+				c := getCert(t, clientCNCert)
+				c.SerialNumber = nil
+				return c
+			}(),
+			expectedIdentifier: "SN=<nil>, SKID=E7:FB:1F:45:F0:71:77:AF:8C:10:4A:0A:42:03:F5:1F:1F:07:CF:DF, AKID=3D:F0:F7:30:3D:3B:EB:3A:55:68:FA:F5:43:C9:C7:AC:E1:3F:10:78",
+		},
+		{
+			name: "empty SKID",
+			cert: func() *x509.Certificate {
+				c := getCert(t, clientCNCert)
+				c.SubjectKeyId = nil
+				return c
+			}(),
+			expectedIdentifier: "SN=1, SKID=, AKID=3D:F0:F7:30:3D:3B:EB:3A:55:68:FA:F5:43:C9:C7:AC:E1:3F:10:78",
+		},
+		{
+			name: "empty AKID",
+			cert: func() *x509.Certificate {
+				c := getCert(t, clientCNCert)
+				c.AuthorityKeyId = nil
+				return c
+			}(),
+			expectedIdentifier: "SN=1, SKID=E7:FB:1F:45:F0:71:77:AF:8C:10:4A:0A:42:03:F5:1F:1F:07:CF:DF, AKID=",
+		},
+		{
+			name:               "self-signed",
+			cert:               getCert(t, selfSignedCert),
+			expectedIdentifier: "SN=14307769263086146430, SKID=7C:AB:02:A8:45:3F:B0:28:2F:71:91:52:A2:71:EE:D9:40:2B:43:71, AKID=7C:AB:02:A8:45:3F:B0:28:2F:71:91:52:A2:71:EE:D9:40:2B:43:71",
+		},
+	}
+
+	for _, tc := range tt {
+		t.Run(tc.name, func(t *testing.T) {
+			got := certificateIdentifier(tc.cert)
+			if got != tc.expectedIdentifier {
+				t.Errorf("expected %q, got %q", tc.expectedIdentifier, got)
+			}
+		})
+	}
+}