Merge pull request #109107 from liggitt/deprecated-cert-audit

Add audit annotations and log prefixes to deprecated cert warnings
This commit is contained in:
Kubernetes Prow Robot 2022-03-29 20:37:09 -07:00 committed by GitHub
commit 7fba4f75c5
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 30 additions and 7 deletions

View File

@ -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",
},
}

View File

@ -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,
},