From f1dd9a611415120af5158f3feb0a0ef6a5c6d7d1 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Tue, 29 Mar 2022 10:41:40 -0400 Subject: [PATCH] Add audit annotations and log prefixes to deprecated cert warnings --- .../x509metrics/server_cert_deprecations.go | 13 ++++++++-- .../server_cert_deprecations_test.go | 24 +++++++++++++++---- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/util/x509metrics/server_cert_deprecations.go b/staging/src/k8s.io/apiserver/pkg/util/x509metrics/server_cert_deprecations.go index 6dbac3b7468..9f1e34c4d44 100644 --- a/staging/src/k8s.io/apiserver/pkg/util/x509metrics/server_cert_deprecations.go +++ b/staging/src/k8s.io/apiserver/pkg/util/x509metrics/server_cert_deprecations.go @@ -19,11 +19,13 @@ package x509metrics import ( "crypto/x509" "errors" + "fmt" "net/http" "reflect" "strings" utilnet "k8s.io/apimachinery/pkg/util/net" + "k8s.io/apiserver/pkg/audit" "k8s.io/component-base/metrics" "k8s.io/klog/v2" ) @@ -53,13 +55,18 @@ type deprecatedCertificateAttributeChecker interface { // it does not have to be reimplemented. type counterRaiser struct { counter *metrics.Counter - reason string + // programmatic id used in log and audit annotations prefixes + id string + // human readable explanation + reason string } func (c *counterRaiser) IncreaseMetricsCounter(req *http.Request) { if req != nil && req.URL != nil { if hostname := req.URL.Hostname(); len(hostname) > 0 { - klog.Infof("invalid certificate detected while connecting to %q: %s", req.URL.Hostname(), c.reason) + prefix := fmt.Sprintf("%s.invalid-cert.kubernetes.io", c.id) + klog.Infof("%s: invalid certificate detected connecting to %q: %s", prefix, hostname, c.reason) + audit.AddAuditAnnotation(req.Context(), prefix+"/"+hostname, c.reason) } } c.counter.Inc() @@ -127,6 +134,7 @@ func NewSANDeprecatedChecker(counter *metrics.Counter) *missingSANChecker { return &missingSANChecker{ counterRaiser: counterRaiser{ counter: counter, + id: "missing-san", reason: "relies on a legacy Common Name field instead of the SAN extension for subject validation", }, } @@ -174,6 +182,7 @@ func NewSHA1SignatureDeprecatedChecker(counter *metrics.Counter) *sha1SignatureC return &sha1SignatureChecker{ counterRaiser: &counterRaiser{ counter: counter, + id: "insecure-sha1", reason: "uses an insecure SHA-1 signature", }, } diff --git a/staging/src/k8s.io/apiserver/pkg/util/x509metrics/server_cert_deprecations_test.go b/staging/src/k8s.io/apiserver/pkg/util/x509metrics/server_cert_deprecations_test.go index 36403b509ae..76515c377b9 100644 --- a/staging/src/k8s.io/apiserver/pkg/util/x509metrics/server_cert_deprecations_test.go +++ b/staging/src/k8s.io/apiserver/pkg/util/x509metrics/server_cert_deprecations_test.go @@ -30,6 +30,8 @@ import ( "testing" "github.com/stretchr/testify/require" + auditapi "k8s.io/apiserver/pkg/apis/audit" + "k8s.io/apiserver/pkg/audit" "k8s.io/component-base/metrics" "k8s.io/component-base/metrics/testutil" ) @@ -243,11 +245,17 @@ func TestCheckForHostnameError(t *testing.T) { if err != nil { t.Fatalf("failed to create an http request: %v", err) } + auditCtx := &audit.AuditContext{Event: &auditapi.Event{Level: auditapi.LevelMetadata}} + req = req.WithContext(audit.WithAuditContext(req.Context(), auditCtx)) _, err = client.Transport.RoundTrip(req) if sanChecker.CheckRoundTripError(err) { sanChecker.IncreaseMetricsCounter(req) + + if len(auditCtx.Event.Annotations["missing-san.invalid-cert.kubernetes.io/"+req.URL.Hostname()]) == 0 { + t.Errorf("expected audit annotations, got %#v", auditCtx.Event.Annotations) + } } errorCounterVal := getSingleCounterValueFromRegistry(t, registry, "Test_checkForHostnameError") @@ -379,6 +387,8 @@ func TestCheckForInsecureAlgorithmError(t *testing.T) { if err != nil { t.Fatalf("failed to create an http request: %v", err) } + auditCtx := &audit.AuditContext{Event: &auditapi.Event{Level: auditapi.LevelMetadata}} + req = req.WithContext(audit.WithAuditContext(req.Context(), auditCtx)) // can't use tlsServer.Client() as it contains the server certificate // in tls.Config.Certificates. The signatures are, however, only checked @@ -402,6 +412,10 @@ func TestCheckForInsecureAlgorithmError(t *testing.T) { if sha1checker.CheckRoundTripError(err) { sha1checker.IncreaseMetricsCounter(req) + + if len(auditCtx.Event.Annotations["insecure-sha1.invalid-cert.kubernetes.io/"+req.URL.Hostname()]) == 0 { + t.Errorf("expected audit annotations, got %#v", auditCtx.Event.Annotations) + } } errorCounterVal := getSingleCounterValueFromRegistry(t, registry, "Test_checkForInsecureAlgorithmError") @@ -511,28 +525,28 @@ func Test_x509DeprecatedCertificateMetricsRTWrapper_RoundTrip(t *testing.T) { }{ { name: "no error, resp w/ cert, no counter increase", - checkers: []deprecatedCertificateAttributeChecker{&testNegativeChecker{counterRaiser{testCounter, ""}}}, + checkers: []deprecatedCertificateAttributeChecker{&testNegativeChecker{counterRaiser{testCounter, "", ""}}}, resp: httpResponseWithCert(), }, { name: "no error, resp w/o cert, no counter increase", - checkers: []deprecatedCertificateAttributeChecker{&testPositiveChecker{counterRaiser{testCounter, ""}}}, + checkers: []deprecatedCertificateAttributeChecker{&testPositiveChecker{counterRaiser{testCounter, "", ""}}}, resp: httpResponseNoCert(), }, { name: "no error, resp w/ cert, counter increase", - checkers: []deprecatedCertificateAttributeChecker{&testPositiveChecker{counterRaiser{testCounter, ""}}}, + checkers: []deprecatedCertificateAttributeChecker{&testPositiveChecker{counterRaiser{testCounter, "", ""}}}, resp: httpResponseWithCert(), counterIncrease: true, }, { name: "unrelated error, no resp, no counter increase", - checkers: []deprecatedCertificateAttributeChecker{&testNegativeChecker{counterRaiser{testCounter, ""}}}, + checkers: []deprecatedCertificateAttributeChecker{&testNegativeChecker{counterRaiser{testCounter, "", ""}}}, err: fmt.Errorf("error"), }, { name: "related error, no resp, counter increase", - checkers: []deprecatedCertificateAttributeChecker{&testPositiveChecker{counterRaiser{testCounter, ""}}}, + checkers: []deprecatedCertificateAttributeChecker{&testPositiveChecker{counterRaiser{testCounter, "", ""}}}, err: fmt.Errorf("error"), counterIncrease: true, },