Add configuration to audit internal endpoints and backfill unit tests

internal/config/supervisor/types.go

@@ -7,7 +7,7 @@ import (
 	"go.pinniped.dev/internal/plog"
 )
 
-// Config contains knobs to setup an instance of the Pinniped Supervisor.
+// Config contains knobs to set up an instance of the Pinniped Supervisor.
 type Config struct {
 	APIGroupSuffix          *string           `json:"apiGroupSuffix,omitempty"`
 	Labels                  map[string]string `json:"labels"`
@@ -16,6 +16,15 @@ type Config struct {
 	Endpoints               *Endpoints        `json:"endpoints"`
 	AggregatedAPIServerPort *int64            `json:"aggregatedAPIServerPort"`
 	TLS                     TLSSpec           `json:"tls"`
+	Audit                   AuditSpec         `json:"audit"`
+}
+
+type AuditInternalPaths string
+
+const AuditInternalPathsEnabled = "Enabled"
+
+type AuditSpec struct {
+	InternalPaths AuditInternalPaths `json:"internalPaths"`
 }
 
 type TLSSpec struct {

internal/federationdomain/endpoints/auth/auth_handler.go

@@ -36,20 +36,21 @@ const (
 	promptParamNone = "none"
 )
 
-//nolint:gochecknoglobals // please treat this as a readonly const, do not mutate
-var paramsSafeToLog = sets.New[string](
-	// Standard params from https://openid.net/specs/openid-connect-core-1_0.html, some of which are ignored.
-	// Redacting state and nonce params, in case they contain any info that the client considers sensitive.
-	"scope", "response_type", "client_id", "redirect_uri", "response_mode", "display", "prompt",
-	"max_age", "ui_locales", "id_token_hint", "login_hint", "acr_values", "claims_locales", "claims",
-	"request", "request_uri", "registration",
-	// PKCE params from https://datatracker.ietf.org/doc/html/rfc7636. Let code_challenge be redacted.
-	"code_challenge_method",
-	// Custom Pinniped authorization params.
-	oidcapi.AuthorizeUpstreamIDPNameParamName, oidcapi.AuthorizeUpstreamIDPTypeParamName,
-	// Google-specific param that some client libraries will send anyway. Ignored by Pinniped but safe to log.
-	"access_type",
-)
+func paramsSafeToLog() sets.Set[string] {
+	return sets.New[string](
+		// Standard params from https://openid.net/specs/openid-connect-core-1_0.html, some of which are ignored.
+		// Redacting state and nonce params, in case they contain any info that the client considers sensitive.
+		"scope", "response_type", "client_id", "redirect_uri", "response_mode", "display", "prompt",
+		"max_age", "ui_locales", "id_token_hint", "login_hint", "acr_values", "claims_locales", "claims",
+		"request", "request_uri", "registration",
+		// PKCE params from https://datatracker.ietf.org/doc/html/rfc7636. Let code_challenge be redacted.
+		"code_challenge_method",
+		// Custom Pinniped authorization params.
+		oidcapi.AuthorizeUpstreamIDPNameParamName, oidcapi.AuthorizeUpstreamIDPTypeParamName,
+		// Google-specific param that some client libraries will send anyway. Ignored by Pinniped but safe to log.
+		"access_type",
+	)
+}
 
 type authorizeHandler struct {
 	downstreamIssuerURL       string
@@ -140,7 +141,7 @@ func (h *authorizeHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		oidcapi.AuthorizePasswordHeaderName, hadPasswordHeader)
 
 	h.auditLogger.Audit(plog.AuditEventHTTPRequestParameters, r.Context(), plog.NoSessionPersisted(),
-		"params", plog.SanitizeParams(r.Form, paramsSafeToLog))
+		"params", plog.SanitizeParams(r.Form, paramsSafeToLog()))
 
 	// Note that the client might have used oidcapi.AuthorizeUpstreamIDPNameParamName and
 	// oidcapi.AuthorizeUpstreamIDPTypeParamName query (or form) params to request a certain upstream IDP.

internal/federationdomain/endpoints/token/token_handler.go

@@ -30,16 +30,17 @@ import (
 	"go.pinniped.dev/internal/psession"
 )
 
-//nolint:gochecknoglobals // please treat this as a readonly const, do not mutate
-var paramsSafeToLog = sets.New[string](
-	// Standard params from https://openid.net/specs/openid-connect-core-1_0.html for authcode and refresh grants.
-	// Redacting code, client_secret, refresh_token, and PKCE code_verifier params.
-	"grant_type", "client_id", "redirect_uri", "scope",
-	// Token exchange params from https://datatracker.ietf.org/doc/html/rfc8693.
-	// Redact subject_token and actor_token.
-	// We don't allow all of these, but they should be safe to log.
-	"audience", "resource", "scope", "requested_token_type", "actor_token_type", "subject_token_type",
-)
+func paramsSafeToLog() sets.Set[string] {
+	return sets.New(
+		// Standard params from https://openid.net/specs/openid-connect-core-1_0.html for authcode and refresh grants.
+		// Redacting code, client_secret, refresh_token, and PKCE code_verifier params.
+		"grant_type", "client_id", "redirect_uri", "scope",
+		// Token exchange params from https://datatracker.ietf.org/doc/html/rfc8693.
+		// Redact subject_token and actor_token.
+		// We don't allow all of these, but they should be safe to log.
+		"audience", "resource", "scope", "requested_token_type", "actor_token_type", "subject_token_type",
+	)
+}
 
 func NewHandler(
 	idpLister federationdomainproviders.FederationDomainIdentityProvidersListerI,
@@ -59,7 +60,7 @@ func NewHandler(
 
 		// Note that r.PostForm and accessRequest were populated by NewAccessRequest().
 		auditLogger.Audit(plog.AuditEventHTTPRequestParameters, r.Context(), accessRequest,
-			"params", plog.SanitizeParams(r.PostForm, paramsSafeToLog))
+			"params", plog.SanitizeParams(r.PostForm, paramsSafeToLog()))
 
 		// Check if we are performing a refresh grant.
 		if accessRequest.GetGrantTypes().ExactOne(oidcapi.GrantTypeRefreshToken) {

internal/federationdomain/endpointsmanager/manager.go

@@ -12,6 +12,7 @@ import (
 	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
 
 	"go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/typed/config/v1alpha1"
+	"go.pinniped.dev/internal/config/supervisor"
 	"go.pinniped.dev/internal/federationdomain/csrftoken"
 	"go.pinniped.dev/internal/federationdomain/dynamiccodec"
 	"go.pinniped.dev/internal/federationdomain/endpoints/auth"
@@ -63,6 +64,7 @@ func NewManager(
 	secretsClient corev1client.SecretInterface,
 	oidcClientsClient v1alpha1.OIDCClientInterface,
 	auditLogger plog.AuditLogger,
+	auditCfg supervisor.AuditSpec,
 ) *Manager {
 	m := &Manager{
 		providerHandlers:    make(map[string]http.Handler),
@@ -74,7 +76,7 @@ func NewManager(
 		auditLogger:         auditLogger,
 	}
 	// nextHandler is the next handler in the chain, called when this manager didn't know how to handle a request
-	m.buildHandlerChain(nextHandler)
+	m.buildHandlerChain(nextHandler, auditCfg)
 	return m
 }
 
@@ -191,11 +193,11 @@ func (m *Manager) SetFederationDomains(federationDomains ...*federationdomainpro
 	}
 }
 
-func (m *Manager) buildHandlerChain(nextHandler http.Handler) {
+func (m *Manager) buildHandlerChain(nextHandler http.Handler, auditCfg supervisor.AuditSpec) {
 	// build the basic handler for FederationDomain endpoints
 	handler := m.buildManagerHandler(nextHandler)
 	// log all requests, including audit ID
-	handler = requestlogger.WithHTTPRequestAuditLogging(handler, m.auditLogger)
+	handler = requestlogger.WithHTTPRequestAuditLogging(handler, m.auditLogger, auditCfg)
 	// add random audit ID to request context and response headers
 	handler = requestlogger.WithAuditID(handler, func() string {
 		return uuid.New().String()

internal/federationdomain/requestlogger/request_logger.go

@@ -8,14 +8,17 @@ import (
 	"net"
 	"net/http"
 	"net/url"
+	"slices"
 	"time"
 
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 	apisaudit "k8s.io/apiserver/pkg/apis/audit"
 	"k8s.io/apiserver/pkg/audit"
 	"k8s.io/apiserver/pkg/endpoints/responsewriter"
+	"k8s.io/utils/clock"
 
+	"go.pinniped.dev/internal/config/supervisor"
 	"go.pinniped.dev/internal/httputil/requestutil"
 	"go.pinniped.dev/internal/plog"
 )
@@ -41,9 +44,9 @@ func WithAuditID(handler http.Handler, newAuditIDFunc func() string) http.Handle
 	})
 }
 
-func WithHTTPRequestAuditLogging(handler http.Handler, auditLogger plog.AuditLogger) http.Handler {
+func WithHTTPRequestAuditLogging(handler http.Handler, auditLogger plog.AuditLogger, auditCfg supervisor.AuditSpec) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
-		rl := newRequestLogger(req, w, auditLogger, time.Now())
+		rl := newRequestLogger(req, w, auditLogger, time.Now(), auditCfg)
 
 		rl.LogRequestReceived()
 		defer rl.LogRequestComplete()
@@ -55,6 +58,7 @@ func WithHTTPRequestAuditLogging(handler http.Handler, auditLogger plog.AuditLog
 
 type requestLogger struct {
 	startTime time.Time
+	clock     clock.Clock // clock is used to calculate the response latency, and useful for unit tests.
 
 	hijacked       bool
 	statusRecorded bool
@@ -65,23 +69,37 @@ type requestLogger struct {
 	w         http.ResponseWriter
 
 	auditLogger plog.AuditLogger
+	auditCfg    supervisor.AuditSpec
 }
 
-func newRequestLogger(req *http.Request, w http.ResponseWriter, auditLogger plog.AuditLogger, startTime time.Time) *requestLogger {
+func newRequestLogger(req *http.Request, w http.ResponseWriter, auditLogger plog.AuditLogger, startTime time.Time, auditCfg supervisor.AuditSpec) *requestLogger {
 	return &requestLogger{
 		req:         req,
 		w:           w,
 		startTime:   startTime,
+		clock:       clock.RealClock{},
 		userAgent:   req.UserAgent(), // cache this from the req to avoid any possibility of concurrent read/write problems with headers map
 		auditLogger: auditLogger,
+		auditCfg:    auditCfg,
+	}
+}
+
+func internalPaths() []string {
+	return []string{
+		"/healthz",
 	}
 }
 
 func (rl *requestLogger) LogRequestReceived() {
 	r := rl.req
+
+	if rl.auditCfg.InternalPaths != supervisor.AuditInternalPathsEnabled && slices.Contains(internalPaths(), r.URL.Path) {
+		return
+	}
+
 	rl.auditLogger.Audit(plog.AuditEventHTTPRequestReceived,
 		r.Context(),
-		nil, // no session available yet in this context
+		plog.NoSessionPersisted(),
 		"proto", r.Proto,
 		"method", r.Method,
 		"host", r.Host,
@@ -94,7 +112,12 @@ func (rl *requestLogger) LogRequestReceived() {
 
 func (rl *requestLogger) LogRequestComplete() {
 	r := rl.req
-	location := rl.w.Header().Get("Location")
+
+	if rl.auditCfg.InternalPaths != supervisor.AuditInternalPathsEnabled && slices.Contains(internalPaths(), r.URL.Path) {
+		return
+	}
+
+	location := rl.Header().Get("Location")
 	if location == "" {
 		location = "no location header"
 	} else {
@@ -110,9 +133,9 @@ func (rl *requestLogger) LogRequestComplete() {
 
 	rl.auditLogger.Audit(plog.AuditEventHTTPRequestCompleted,
 		r.Context(),
-		nil,                // no session available yet in this context
+		plog.NoSessionPersisted(),
 		"path", r.URL.Path, // include the path again to make it easy to "grep -v healthz" to watch all other audit events
-		"latency", time.Since(rl.startTime),
+		"latency", rl.clock.Since(rl.startTime),
 		"responseStatus", rl.status,
 		"location", location,
 	)