mirrors/pinniped
1
0
Fork 0
mirror of https://github.com/vmware-tanzu/pinniped.git synced 2026-09-20 06:54:20 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Add configuration to audit internal endpoints and backfill unit tests

Browse Source
This commit is contained in:
Joshua Casey
2024-11-27 13:53:01 -06:00
parent cf4b29de4b
commit 369316556a
10 changed files with 392 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/config/supervisor/types.go
+10 -1
View File
@@ -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
+16 -15
View File
@@ -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
+12 -11
View File
@@ -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
+5 -3
View File
@@ -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
+30 -7
View File
@@ -8,6 +8,7 @@ import (
"net"
"net/http"
"net/url"
"slices"
"time"
"k8s.io/apimachinery/pkg/types"
@@ -15,7 +16,9 @@ import (
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,
)
internal/federationdomain/requestlogger/request_logger_test.go
+225
View File
@@ -0,0 +1,225 @@
package requestlogger
import (
"crypto/tls"
"net/http"
"net/url"
"testing"
"time"
"go.uber.org/mock/gomock"
clocktesting "k8s.io/utils/clock/testing"
"go.pinniped.dev/internal/config/supervisor"
"go.pinniped.dev/internal/mocks/mockresponsewriter"
"go.pinniped.dev/internal/plog"
"go.pinniped.dev/internal/testutil"
)
func TestLogRequestReceived(t *testing.T) {
var noAuditEventsWanted []testutil.WantedAuditLog
happyAuditEventWanted := func(path string) []testutil.WantedAuditLog {
return []testutil.WantedAuditLog{
testutil.WantAuditLog("HTTP Request Received",
map[string]any{
"proto": "some-proto",
"method": "some-method",
"host": "some-host",
"serverName": "some-sni-server-name",
"path": path,
"userAgent": "some-user-agent",
"remoteAddr": "some-remote-addr",
},
),
}
}
tests := []struct {
name string
path string
auditCfg supervisor.AuditSpec
wantAuditLogs []testutil.WantedAuditLog
}{
{
name: "when internal paths are not Enabled, ignores internal paths",
path: "/healthz",
auditCfg: supervisor.AuditSpec{
InternalPaths: "Disabled",
},
wantAuditLogs: noAuditEventsWanted,
},
{
name: "when internal paths are not Enabled, audits external path",
path: "/pretend-to-login",
auditCfg: supervisor.AuditSpec{
InternalPaths: "Disabled",
},
wantAuditLogs: happyAuditEventWanted("/pretend-to-login"),
},
{
name: "when internal paths are Enabled, audits internal paths",
path: "/healthz",
auditCfg: supervisor.AuditSpec{
InternalPaths: "Enabled",
},
wantAuditLogs: happyAuditEventWanted("/healthz"),
},
{
name: "when internal paths are Enabled, audits external paths",
path: "/pretend-to-login",
auditCfg: supervisor.AuditSpec{
InternalPaths: "Enabled",
},
wantAuditLogs: happyAuditEventWanted("/pretend-to-login"),
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
t.Parallel()
logger, log := plog.TestLogger(t)
subject := requestLogger{
auditLogger: logger,
req: &http.Request{
Method: "some-method",
Proto: "some-proto",
Host: "some-host",
URL: &url.URL{
Path: test.path,
},
RemoteAddr: "some-remote-addr",
TLS: &tls.ConnectionState{
ServerName: "some-sni-server-name",
},
},
userAgent: "some-user-agent",
auditCfg: test.auditCfg,
}
subject.LogRequestReceived()
testutil.CompareAuditLogs(t, test.wantAuditLogs, log.String())
})
}
}
func TestLogRequestComplete(t *testing.T) {
wantLatency := time.Minute + 2*time.Second + 345*time.Millisecond
var noAuditEventsWanted []testutil.WantedAuditLog
happyAuditEventWanted := func(path, location string) []testutil.WantedAuditLog {
return []testutil.WantedAuditLog{
testutil.WantAuditLog("HTTP Request Completed",
map[string]any{
"path": path,
"latency": "1m2.345s",
"responseStatus": 777.0, // JSON serializes this as a float
"location": location,
},
),
}
}
tests := []struct {
name string
path string
location string
auditCfg supervisor.AuditSpec
wantAuditLogs []testutil.WantedAuditLog
}{
{
name: "when internal paths are not Enabled, ignores internal paths",
path: "/healthz",
auditCfg: supervisor.AuditSpec{
InternalPaths: "Disabled",
},
wantAuditLogs: noAuditEventsWanted,
},
{
name: "when internal paths are not Enabled, audits external path with location",
path: "/pretend-to-login",
location: "some-location",
auditCfg: supervisor.AuditSpec{
InternalPaths: "Disabled",
},
wantAuditLogs: happyAuditEventWanted("/pretend-to-login", "some-location"),
},
{
name: "when internal paths are not Enabled, audits external path without location",
path: "/pretend-to-login",
location: "", // make it obvious
auditCfg: supervisor.AuditSpec{
InternalPaths: "Disabled",
},
wantAuditLogs: happyAuditEventWanted("/pretend-to-login", "no location header"),
},
{
name: "when internal paths are not Enabled, audits external path with invalid location",
path: "/pretend-to-login",
location: "http://e x a m p l e.com",
auditCfg: supervisor.AuditSpec{
InternalPaths: "Disabled",
},
wantAuditLogs: happyAuditEventWanted("/pretend-to-login", "unparsable location header"),
},
{
name: "when internal paths are Enabled, audits internal paths",
path: "/healthz",
location: "some-location",
auditCfg: supervisor.AuditSpec{
InternalPaths: "Enabled",
},
wantAuditLogs: happyAuditEventWanted("/healthz", "some-location"),
},
{
name: "when internal paths are Enabled, audits external paths",
path: "/pretend-to-login",
location: "some-location",
auditCfg: supervisor.AuditSpec{
InternalPaths: "Enabled",
},
wantAuditLogs: happyAuditEventWanted("/pretend-to-login", "some-location"),
},
}
nowDoesntMatter := time.Date(1122, time.September, 33, 4, 55, 56, 778899, time.Local)
startTime := nowDoesntMatter.Add(-wantLatency)
frozenClock := clocktesting.NewFakeClock(nowDoesntMatter)
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
t.Parallel()
ctrl := gomock.NewController(t)
mockResponseWriter := mockresponsewriter.NewMockResponseWriter(ctrl)
if len(test.wantAuditLogs) > 0 {
mockResponseWriter.EXPECT().Header().Return(http.Header{
"Location": []string{test.location},
})
}
logger, log := plog.TestLogger(t)
subject := requestLogger{
auditLogger: logger,
startTime: startTime,
clock: frozenClock,
req: &http.Request{
URL: &url.URL{
Path: test.path,
},
},
status: 777,
w: mockResponseWriter,
auditCfg: test.auditCfg,
}
subject.LogRequestComplete()
testutil.CompareAuditLogs(t, test.wantAuditLogs, log.String())
})
}
}
internal/mocks/mockresponsewriter/generate.go
+6
View File
@@ -0,0 +1,6 @@
// Copyright 2024 the Pinniped contributors. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
package mockresponsewriter
//go:generate go run -v go.uber.org/mock/mockgen -destination=mockresponsewriter.go -package=mockresponsewriter -copyright_file=../../../hack/header.txt net/http ResponseWriter
internal/mocks/mockresponsewriter/mockresponsewriter.go
+86
View File
@@ -0,0 +1,86 @@
// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
//
// Code generated by MockGen. DO NOT EDIT.
// Source: net/http (interfaces: ResponseWriter)
//
// Generated by this command:
//
// mockgen -destination=mockresponsewriter.go -package=mockresponsewriter -copyright_file=../../../hack/header.txt net/http ResponseWriter
//
// Package mockresponsewriter is a generated GoMock package.
package mockresponsewriter
import (
http "net/http"
reflect "reflect"
gomock "go.uber.org/mock/gomock"
)
// MockResponseWriter is a mock of ResponseWriter interface.
type MockResponseWriter struct {
ctrl *gomock.Controller
recorder *MockResponseWriterMockRecorder
isgomock struct{}
}
// MockResponseWriterMockRecorder is the mock recorder for MockResponseWriter.
type MockResponseWriterMockRecorder struct {
mock *MockResponseWriter
}
// NewMockResponseWriter creates a new mock instance.
func NewMockResponseWriter(ctrl *gomock.Controller) *MockResponseWriter {
mock := &MockResponseWriter{ctrl: ctrl}
mock.recorder = &MockResponseWriterMockRecorder{mock}
return mock
}
// EXPECT returns an object that allows the caller to indicate expected use.
func (m *MockResponseWriter) EXPECT() *MockResponseWriterMockRecorder {
return m.recorder
}
// Header mocks base method.
func (m *MockResponseWriter) Header() http.Header {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "Header")
ret0, _ := ret[0].(http.Header)
return ret0
}
// Header indicates an expected call of Header.
func (mr *MockResponseWriterMockRecorder) Header() *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Header", reflect.TypeOf((*MockResponseWriter)(nil).Header))
}
// Write mocks base method.
func (m *MockResponseWriter) Write(arg0 []byte) (int, error) {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "Write", arg0)
ret0, _ := ret[0].(int)
ret1, _ := ret[1].(error)
return ret0, ret1
}
// Write indicates an expected call of Write.
func (mr *MockResponseWriterMockRecorder) Write(arg0 any) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Write", reflect.TypeOf((*MockResponseWriter)(nil).Write), arg0)
}
// WriteHeader mocks base method.
func (m *MockResponseWriter) WriteHeader(statusCode int) {
m.ctrl.T.Helper()
m.ctrl.Call(m, "WriteHeader", statusCode)
}
// WriteHeader indicates an expected call of WriteHeader.
func (mr *MockResponseWriterMockRecorder) WriteHeader(statusCode any) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WriteHeader", reflect.TypeOf((*MockResponseWriter)(nil).WriteHeader), statusCode)
}
internal/supervisor/server/server.go
+1
View File
@@ -485,6 +485,7 @@ func runSupervisor(ctx context.Context, podInfo *downward.PodInfo, cfg *supervis
clientWithoutLeaderElection.Kubernetes.CoreV1().Secrets(serverInstallationNamespace), // writes to kube storage are allowed for non-leaders
client.PinnipedSupervisor.ConfigV1alpha1().OIDCClients(serverInstallationNamespace),
plog.New(),
cfg.Audit,
)
// Get the "real" name of the client secret supervisor API group (i.e., the API group name with the
site/content/docs/reference/audit-logging.md
+1 -1
View File
@@ -116,7 +116,7 @@ TODO: Document this configuration, probably something like so:
```yaml
audit:
show_personally_identifiable_information: enabled
audit_internal_endpoints: enabled
internal_endpoints: enabled
```
#</TODO>