diff --git a/internal/federationdomain/endpoints/auth/auth_handler.go b/internal/federationdomain/endpoints/auth/auth_handler.go index 8eb81b7df..f68ea7316 100644 --- a/internal/federationdomain/endpoints/auth/auth_handler.go +++ b/internal/federationdomain/endpoints/auth/auth_handler.go @@ -141,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())) + 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. diff --git a/internal/federationdomain/endpoints/auth/auth_handler_test.go b/internal/federationdomain/endpoints/auth/auth_handler_test.go index bcf2db9ba..510b0372a 100644 --- a/internal/federationdomain/endpoints/auth/auth_handler_test.go +++ b/internal/federationdomain/endpoints/auth/auth_handler_test.go @@ -731,7 +731,17 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo "Pinniped-Password": false, }), testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": "client_id=pinniped-cli&code_challenge=redacted&code_challenge_method=S256&nonce=redacted&pinniped_idp_name=some-oidc-idp&redirect_uri=http%3A%2F%2F127.0.0.1%2Fcallback&response_type=code&scope=openid+profile+email+username+groups&state=redacted", + "params": map[string]any{ + "client_id": "pinniped-cli", + "code_challenge": "redacted", + "code_challenge_method": "S256", + "nonce": "redacted", + "pinniped_idp_name": "some-oidc-idp", + "redirect_uri": "http://127.0.0.1/callback", + "response_type": "code", + "scope": "openid profile email username groups", + "state": "redacted", + }, }), testutil.WantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-oidc-idp", @@ -769,7 +779,17 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo "Pinniped-Password": false, }), testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": `client_id=` + dynamicClientID + `&code_challenge=redacted&code_challenge_method=S256&nonce=redacted&pinniped_idp_name=some-oidc-idp&redirect_uri=http%3A%2F%2F127.0.0.1%2Fcallback&response_type=code&scope=openid+offline_access+pinniped%3Arequest-audience+username+groups&state=redacted`, + "params": map[string]any{ + "client_id": dynamicClientID, + "code_challenge": "redacted", + "code_challenge_method": "S256", + "nonce": "redacted", + "pinniped_idp_name": "some-oidc-idp", + "redirect_uri": "http://127.0.0.1/callback", + "response_type": "code", + "scope": "openid offline_access pinniped:request-audience username groups", + "state": "redacted", + }, }), testutil.WantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-oidc-idp", @@ -806,7 +826,17 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo "Pinniped-Password": false, }), testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": "client_id=pinniped-cli&code_challenge=redacted&code_challenge_method=S256&nonce=redacted&pinniped_idp_name=some-github-idp&redirect_uri=http%3A%2F%2F127.0.0.1%2Fcallback&response_type=code&scope=openid+profile+email+username+groups&state=redacted", + "params": map[string]any{ + "client_id": "pinniped-cli", + "code_challenge": "redacted", + "code_challenge_method": "S256", + "nonce": "redacted", + "pinniped_idp_name": "some-github-idp", + "redirect_uri": "http://127.0.0.1/callback", + "response_type": "code", + "scope": "openid profile email username groups", + "state": "redacted", + }, }), testutil.WantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-github-idp", @@ -844,7 +874,17 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo "Pinniped-Password": false, }), testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": `client_id=` + dynamicClientID + `&code_challenge=redacted&code_challenge_method=S256&nonce=redacted&pinniped_idp_name=some-github-idp&redirect_uri=http%3A%2F%2F127.0.0.1%2Fcallback&response_type=code&scope=openid+offline_access+pinniped%3Arequest-audience+username+groups&state=redacted`, + "params": map[string]any{ + "client_id": dynamicClientID, + "code_challenge": "redacted", + "code_challenge_method": "S256", + "nonce": "redacted", + "pinniped_idp_name": "some-github-idp", + "redirect_uri": "http://127.0.0.1/callback", + "response_type": "code", + "scope": "openid offline_access pinniped:request-audience username groups", + "state": "redacted", + }, }), testutil.WantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-github-idp", @@ -881,7 +921,17 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo "Pinniped-Password": false, }), testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": `client_id=pinniped-cli&code_challenge=redacted&code_challenge_method=S256&nonce=redacted&pinniped_idp_name=some-ldap-idp&redirect_uri=http%3A%2F%2F127.0.0.1%2Fcallback&response_type=code&scope=openid+profile+email+username+groups&state=redacted`, + "params": map[string]any{ + "client_id": "pinniped-cli", + "code_challenge": "redacted", + "code_challenge_method": "S256", + "nonce": "redacted", + "pinniped_idp_name": "some-ldap-idp", + "redirect_uri": "http://127.0.0.1/callback", + "response_type": "code", + "scope": "openid profile email username groups", + "state": "redacted", + }, }), testutil.WantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-ldap-idp", @@ -919,7 +969,16 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo "Pinniped-Password": false, }), testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": `client_id=pinniped-cli&code_challenge=redacted&code_challenge_method=S256&nonce=redacted&redirect_uri=http%3A%2F%2F127.0.0.1%2Fcallback&response_type=code&scope=openid+profile+email+username+groups&state=redacted`, + "params": map[string]any{ + "client_id": "pinniped-cli", + "code_challenge": "redacted", + "code_challenge_method": "S256", + "nonce": "redacted", + "redirect_uri": "http://127.0.0.1/callback", + "response_type": "code", + "scope": "openid profile email username groups", + "state": "redacted", + }, }), testutil.WantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-oidc-idp", @@ -958,7 +1017,16 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo "Pinniped-Password": false, }), testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": `client_id=pinniped-cli&code_challenge=redacted&code_challenge_method=S256&nonce=redacted&redirect_uri=http%3A%2F%2F127.0.0.1%2Fcallback&response_type=code&scope=openid+profile+email+username+groups&state=redacted`, + "params": map[string]any{ + "client_id": "pinniped-cli", + "code_challenge": "redacted", + "code_challenge_method": "S256", + "nonce": "redacted", + "redirect_uri": "http://127.0.0.1/callback", + "response_type": "code", + "scope": "openid profile email username groups", + "state": "redacted", + }, }), } }, @@ -988,7 +1056,17 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo "Pinniped-Password": false, }), testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": `client_id=pinniped-cli&code_challenge=redacted&code_challenge_method=S256&nonce=redacted&pinniped_idp_name=some-oidc-idp&redirect_uri=http%3A%2F%2F127.0.0.1%2Fcallback&response_type=code&scope=openid+profile+email+username+groups&state=redacted`, + "params": map[string]any{ + "client_id": "pinniped-cli", + "code_challenge": "redacted", + "code_challenge_method": "S256", + "nonce": "redacted", + "pinniped_idp_name": "some-oidc-idp", + "redirect_uri": "http://127.0.0.1/callback", + "response_type": "code", + "scope": "openid profile email username groups", + "state": "redacted", + }, }), testutil.WantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-oidc-idp", @@ -1102,7 +1180,17 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo "Pinniped-Password": true, }), testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": `client_id=pinniped-cli&code_challenge=redacted&code_challenge_method=S256&nonce=redacted&pinniped_idp_name=some-password-granting-oidc-idp&redirect_uri=http%3A%2F%2F127.0.0.1%2Fcallback&response_type=code&scope=openid+profile+email+username+groups&state=redacted`, + "params": map[string]any{ + "client_id": "pinniped-cli", + "code_challenge": "redacted", + "code_challenge_method": "S256", + "nonce": "redacted", + "pinniped_idp_name": "some-password-granting-oidc-idp", + "redirect_uri": "http://127.0.0.1/callback", + "response_type": "code", + "scope": "openid profile email username groups", + "state": "redacted", + }, }), testutil.WantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-password-granting-oidc-idp", @@ -1177,7 +1265,17 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo "Pinniped-Password": true, }), testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": `client_id=pinniped-cli&code_challenge=redacted&code_challenge_method=S256&nonce=redacted&pinniped_idp_name=some-password-granting-oidc-idp&redirect_uri=http%3A%2F%2F127.0.0.1%2Fcallback&response_type=code&scope=openid+profile+email+username+groups&state=redacted`, + "params": map[string]any{ + "client_id": "pinniped-cli", + "code_challenge": "redacted", + "code_challenge_method": "S256", + "nonce": "redacted", + "pinniped_idp_name": "some-password-granting-oidc-idp", + "redirect_uri": "http://127.0.0.1/callback", + "response_type": "code", + "scope": "openid profile email username groups", + "state": "redacted", + }, }), testutil.WantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-password-granting-oidc-idp", @@ -1288,7 +1386,17 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo "Pinniped-Password": true, }), testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": `client_id=pinniped-cli&code_challenge=redacted&code_challenge_method=S256&nonce=redacted&pinniped_idp_name=some-ldap-idp&redirect_uri=http%3A%2F%2F127.0.0.1%2Fcallback&response_type=code&scope=openid+profile+email+username+groups&state=redacted`, + "params": map[string]any{ + "client_id": "pinniped-cli", + "code_challenge": "redacted", + "code_challenge_method": "S256", + "nonce": "redacted", + "pinniped_idp_name": "some-ldap-idp", + "redirect_uri": "http://127.0.0.1/callback", + "response_type": "code", + "scope": "openid profile email username groups", + "state": "redacted", + }, }), testutil.WantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-ldap-idp", @@ -1348,7 +1456,17 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo "Pinniped-Password": true, }), testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": `client_id=pinniped-cli&code_challenge=redacted&code_challenge_method=S256&nonce=redacted&pinniped_idp_name=some-ldap-idp&redirect_uri=http%3A%2F%2F127.0.0.1%2Fcallback&response_type=code&scope=openid+profile+email+username+groups&state=redacted`, + "params": map[string]any{ + "client_id": "pinniped-cli", + "code_challenge": "redacted", + "code_challenge_method": "S256", + "nonce": "redacted", + "pinniped_idp_name": "some-ldap-idp", + "redirect_uri": "http://127.0.0.1/callback", + "response_type": "code", + "scope": "openid profile email username groups", + "state": "redacted", + }, }), testutil.WantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-ldap-idp", @@ -1720,7 +1838,18 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo "Pinniped-Password": false, }), testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": `client_id=pinniped-cli&code_challenge=redacted&code_challenge_method=S256&nonce=redacted&pinniped_idp_name=some-oidc-idp&prompt=none&redirect_uri=http%3A%2F%2F127.0.0.1%2Fcallback&response_type=code&scope=openid+profile+email+username+groups&state=redacted`, + "params": map[string]any{ + "client_id": "pinniped-cli", + "code_challenge": "redacted", + "code_challenge_method": "S256", + "nonce": "redacted", + "pinniped_idp_name": "some-oidc-idp", + "prompt": "none", + "redirect_uri": "http://127.0.0.1/callback", + "response_type": "code", + "scope": "openid profile email username groups", + "state": "redacted", + }, }), testutil.WantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-oidc-idp", diff --git a/internal/federationdomain/endpoints/token/token_handler_test.go b/internal/federationdomain/endpoints/token/token_handler_test.go index 40ae7c6e0..4059b68bf 100644 --- a/internal/federationdomain/endpoints/token/token_handler_test.go +++ b/internal/federationdomain/endpoints/token/token_handler_test.go @@ -389,7 +389,13 @@ func TestTokenEndpointAuthcodeExchange(t *testing.T) { wantAuditLogs: func(sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": "client_id=pinniped-cli&code=redacted&code_verifier=redacted&grant_type=authorization_code&redirect_uri=http%3A%2F%2F127.0.0.1%2Fcallback", + "params": map[string]any{ + "client_id": "pinniped-cli", + "code": "redacted", + "code_verifier": "redacted", + "grant_type": "authorization_code", + "redirect_uri": "http://127.0.0.1/callback", + }, }), } }, @@ -453,7 +459,12 @@ func TestTokenEndpointAuthcodeExchange(t *testing.T) { wantAuditLogs: func(sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": "code=redacted&code_verifier=redacted&grant_type=authorization_code&redirect_uri=http%3A%2F%2F127.0.0.1%2Fcallback", + "params": map[string]any{ + "code": "redacted", + "code_verifier": "redacted", + "grant_type": "authorization_code", + "redirect_uri": "http://127.0.0.1/callback", + }, }), } }, @@ -538,7 +549,13 @@ func TestTokenEndpointAuthcodeExchange(t *testing.T) { wantAuditLogs: func(sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": "client_id=pinniped-cli&code=redacted&code_verifier=redacted&grant_type=authorization_code&redirect_uri=http%3A%2F%2F127.0.0.1%2Fcallback", + "params": map[string]any{ + "client_id": "pinniped-cli", + "code": "redacted", + "code_verifier": "redacted", + "grant_type": "authorization_code", + "redirect_uri": "http://127.0.0.1/callback", + }, }), } }, @@ -937,7 +954,12 @@ func TestTokenEndpointAuthcodeExchange(t *testing.T) { wantAuditLogs: func(sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": "client_id=pinniped-cli&code=redacted&grant_type=authorization_code&redirect_uri=http%3A%2F%2F127.0.0.1%2Fcallback", + "params": map[string]any{ + "client_id": "pinniped-cli", + "code": "redacted", + "grant_type": "authorization_code", + "redirect_uri": "http://127.0.0.1/callback", + }, }), } }, @@ -958,7 +980,13 @@ func TestTokenEndpointAuthcodeExchange(t *testing.T) { wantAuditLogs: func(sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": "client_id=pinniped-cli&code=redacted&code_verifier=redacted&grant_type=authorization_code&redirect_uri=http%3A%2F%2F127.0.0.1%2Fcallback", + "params": map[string]any{ + "client_id": "pinniped-cli", + "code": "redacted", + "code_verifier": "redacted", + "grant_type": "authorization_code", + "redirect_uri": "http://127.0.0.1/callback", + }, }), } }, @@ -1159,7 +1187,13 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn wantAuditLogs: func(sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": "client_id=pinniped-cli&code=redacted&code_verifier=redacted&grant_type=authorization_code&redirect_uri=http%3A%2F%2F127.0.0.1%2Fcallback", + "params": map[string]any{ + "client_id": "pinniped-cli", + "code": "redacted", + "code_verifier": "redacted", + "grant_type": "authorization_code", + "redirect_uri": "http://127.0.0.1/callback", + }, }), } }, @@ -1170,16 +1204,14 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn wantAuditLogs: func(sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": func() string { - params := url.Values{} - params.Set("audience", "some-workload-cluster") - params.Set("client_id", "pinniped-cli") - params.Set("grant_type", "urn:ietf:params:oauth:grant-type:token-exchange") - params.Set("requested_token_type", "urn:ietf:params:oauth:token-type:jwt") - params.Set("subject_token", "redacted") - params.Set("subject_token_type", "urn:ietf:params:oauth:token-type:access_token") - return params.Encode() - }(), + "params": map[string]any{ + "audience": "some-workload-cluster", + "client_id": "pinniped-cli", + "grant_type": "urn:ietf:params:oauth:grant-type:token-exchange", + "requested_token_type": "urn:ietf:params:oauth:token-type:jwt", + "subject_token": "redacted", + "subject_token_type": "urn:ietf:params:oauth:token-type:access_token", + }, }), } }, @@ -1359,15 +1391,13 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn wantAuditLogs: func(sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": func() string { - params := url.Values{} - params.Set("audience", "some-workload-cluster") - params.Set("grant_type", "urn:ietf:params:oauth:grant-type:token-exchange") - params.Set("requested_token_type", "urn:ietf:params:oauth:token-type:jwt") - params.Set("subject_token", "redacted") - params.Set("subject_token_type", "urn:ietf:params:oauth:token-type:access_token") - return params.Encode() - }(), + "params": map[string]any{ + "audience": "some-workload-cluster", + "grant_type": "urn:ietf:params:oauth:grant-type:token-exchange", + "requested_token_type": "urn:ietf:params:oauth:token-type:jwt", + "subject_token": "redacted", + "subject_token_type": "urn:ietf:params:oauth:token-type:access_token", + }, }), } }, @@ -1472,16 +1502,14 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn wantAuditLogs: func(sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": func() string { - params := url.Values{} - params.Set("audience", "") // make it obvious - params.Set("client_id", "pinniped-cli") - params.Set("grant_type", "urn:ietf:params:oauth:grant-type:token-exchange") - params.Set("requested_token_type", "urn:ietf:params:oauth:token-type:jwt") - params.Set("subject_token", "redacted") - params.Set("subject_token_type", "urn:ietf:params:oauth:token-type:access_token") - return params.Encode() - }(), + "params": map[string]any{ + "audience": "", // make it obvious + "client_id": "pinniped-cli", + "grant_type": "urn:ietf:params:oauth:grant-type:token-exchange", + "requested_token_type": "urn:ietf:params:oauth:token-type:jwt", + "subject_token": "redacted", + "subject_token_type": "urn:ietf:params:oauth:token-type:access_token", + }, }), } }, @@ -2304,14 +2332,12 @@ func TestRefreshGrant(t *testing.T) { func(sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": func() string { - params := url.Values{} - params.Set("client_id", "pinniped-cli") - params.Set("grant_type", "refresh_token") - params.Set("refresh_token", "redacted") - params.Set("scope", "openid") - return params.Encode() - }(), + "params": map[string]any{ + "client_id": "pinniped-cli", + "grant_type": "refresh_token", + "refresh_token": "redacted", + "scope": "openid", + }, }), testutil.WantAuditLog("Identity Refreshed From Upstream IDP", map[string]any{ "sessionID": sessionID, @@ -2499,14 +2525,12 @@ func TestRefreshGrant(t *testing.T) { wantAuditLogs: func(sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": func() string { - params := url.Values{} - params.Set("client_id", "pinniped-cli") - params.Set("grant_type", "refresh_token") - params.Set("refresh_token", "redacted") - params.Set("scope", "openid") - return params.Encode() - }(), + "params": map[string]any{ + "client_id": "pinniped-cli", + "grant_type": "refresh_token", + "refresh_token": "redacted", + "scope": "openid", + }, }), testutil.WantAuditLog("Identity Refreshed From Upstream IDP", map[string]any{ "sessionID": sessionID, @@ -2569,7 +2593,13 @@ func TestRefreshGrant(t *testing.T) { wantAuditLogs: func(sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ - "params": "client_id=pinniped-cli&code=redacted&code_verifier=redacted&grant_type=authorization_code&redirect_uri=http%3A%2F%2F127.0.0.1%2Fcallback", + "params": map[string]any{ + "client_id": "pinniped-cli", + "code": "redacted", + "code_verifier": "redacted", + "grant_type": "authorization_code", + "redirect_uri": "http://127.0.0.1/callback", + }, }), } }, diff --git a/internal/federationdomain/endpoints/tokenendpointauditor/parameter_auditor.go b/internal/federationdomain/endpoints/tokenendpointauditor/parameter_auditor.go index d55e2b35e..37f5413c3 100644 --- a/internal/federationdomain/endpoints/tokenendpointauditor/parameter_auditor.go +++ b/internal/federationdomain/endpoints/tokenendpointauditor/parameter_auditor.go @@ -53,7 +53,7 @@ func paramsSafeToLogTokenEndpoint() sets.Set[string] { func (p parameterAuditorHandler) CanHandleTokenEndpointRequest(ctx context.Context, requester fosite.AccessRequester) bool { p.auditLogger.Audit(plog.AuditEventHTTPRequestParameters, ctx, plog.NoSessionPersisted(), - "params", plog.SanitizeParams(requester.GetRequestForm(), paramsSafeToLogTokenEndpoint())) + plog.SanitizeParams(requester.GetRequestForm(), paramsSafeToLogTokenEndpoint())...) return false } diff --git a/internal/federationdomain/requestlogger/request_logger.go b/internal/federationdomain/requestlogger/request_logger.go index d82a78494..11e2400eb 100644 --- a/internal/federationdomain/requestlogger/request_logger.go +++ b/internal/federationdomain/requestlogger/request_logger.go @@ -13,7 +13,6 @@ import ( "github.com/google/uuid" "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" @@ -131,8 +130,14 @@ func (rl *requestLogger) logRequestComplete() { if err != nil { location = "unparsable location header" } else { - redactAllParams := sets.New[string]() - parsedLocation.RawQuery = plog.SanitizeParams(parsedLocation.Query(), redactAllParams) + // We don't know what this `Location` header is used for, so redact all query params + redactedParams := parsedLocation.Query() + for k, v := range redactedParams { + for i := range v { + redactedParams[k][i] = "redacted" + } + } + parsedLocation.RawQuery = redactedParams.Encode() location = parsedLocation.String() } } diff --git a/internal/federationdomain/requestlogger/request_logger_test.go b/internal/federationdomain/requestlogger/request_logger_test.go index d9345d14b..b2a22504f 100644 --- a/internal/federationdomain/requestlogger/request_logger_test.go +++ b/internal/federationdomain/requestlogger/request_logger_test.go @@ -142,13 +142,13 @@ func TestLogRequestComplete(t *testing.T) { wantAuditLogs: noAuditEventsWanted, }, { - name: "when internal paths are not Enabled, audits external path with location", + name: "when internal paths are not Enabled, audits external path with location (redacting all query params)", path: "/pretend-to-login", - location: "some-location", + location: "http://127.0.0.1?foo=bar&foo=quz&lorem=ipsum", auditCfg: supervisor.AuditSpec{ InternalPaths: "Disabled", }, - wantAuditLogs: happyAuditEventWanted("/pretend-to-login", "some-location"), + wantAuditLogs: happyAuditEventWanted("/pretend-to-login", "http://127.0.0.1?foo=redacted&foo=redacted&lorem=redacted"), }, { name: "when internal paths are not Enabled, audits external path without location", diff --git a/internal/plog/audit_event.go b/internal/plog/audit_event.go index 752a692f9..9e491e4d9 100644 --- a/internal/plog/audit_event.go +++ b/internal/plog/audit_event.go @@ -31,19 +31,39 @@ const ( // SanitizeParams can be used to redact all params not included in the allowedKeys set. // Useful when audit logging AuditEventHTTPRequestParameters events. -func SanitizeParams(params url.Values, allowedKeys sets.Set[string]) string { - if len(params) == 0 { - return "" +func SanitizeParams(inputParams url.Values, allowedKeys sets.Set[string]) []any { + params := make(map[string]string) + multiValueParams := make(url.Values) + + transform := func(key, value string) string { + if !allowedKeys.Has(key) { + return "redacted" + } + + unescape, err := url.QueryUnescape(value) + if err != nil { + // ignore these errors and just use the original query parameter + unescape = value + } + return unescape } - sanitized := url.Values{} - for key := range params { - if allowedKeys.Has(key) { - sanitized[key] = params[key] - } else { - for range params[key] { - sanitized.Add(key, "redacted") + + for key := range inputParams { + for i, p := range inputParams[key] { + transformed := transform(key, p) + if i == 0 { + params[key] = transformed + } + + if len(inputParams[key]) > 1 { + multiValueParams[key] = append(multiValueParams[key], transformed) } } } - return sanitized.Encode() + + if len(multiValueParams) > 0 { + return []any{"params", params, "multiValueParams", multiValueParams} + + } + return []any{"params", params} } diff --git a/internal/plog/audit_event_test.go b/internal/plog/audit_event_test.go index 9cb031b6a..99658f8da 100644 --- a/internal/plog/audit_event_test.go +++ b/internal/plog/audit_event_test.go @@ -16,60 +16,156 @@ func TestSanitizeParams(t *testing.T) { name string params url.Values allowedKeys sets.Set[string] - want string + want []any }{ { name: "nil values", params: nil, allowedKeys: nil, - want: "", + want: []any{ + "params", + map[string]string{}, + }, }, { name: "empty values", params: url.Values{}, allowedKeys: nil, - want: "", + want: []any{ + "params", + map[string]string{}, + }, }, { name: "all allowed values", params: url.Values{"foo": []string{"a", "b", "c"}, "bar": []string{"d", "e", "f"}}, allowedKeys: sets.New("foo", "bar"), - want: "bar=d&bar=e&bar=f&foo=a&foo=b&foo=c", + want: []any{ + "params", + map[string]string{ + "bar": "d", + "foo": "a", + }, + "multiValueParams", + url.Values{ + "bar": []string{"d", "e", "f"}, + "foo": []string{"a", "b", "c"}, + }, + }, }, { name: "all allowed values with single values", params: url.Values{"foo": []string{"a"}, "bar": []string{"d"}}, allowedKeys: sets.New("foo", "bar"), - want: "bar=d&foo=a", + want: []any{ + "params", + map[string]string{ + "foo": "a", + "bar": "d", + }, + }, }, { name: "some allowed values", params: url.Values{"foo": []string{"a", "b", "c"}, "bar": []string{"d", "e", "f"}}, allowedKeys: sets.New("foo"), - want: "bar=redacted&bar=redacted&bar=redacted&foo=a&foo=b&foo=c", + want: []any{ + "params", + map[string]string{ + "bar": "redacted", + "foo": "a", + }, + "multiValueParams", + url.Values{ + "bar": []string{"redacted", "redacted", "redacted"}, + "foo": []string{"a", "b", "c"}, + }, + }, }, { name: "some allowed values with single values", params: url.Values{"foo": []string{"a"}, "bar": []string{"d"}}, allowedKeys: sets.New("foo"), - want: "bar=redacted&foo=a", + want: []any{ + "params", + map[string]string{ + "bar": "redacted", + "foo": "a", + }, + }, }, { name: "no allowed values", params: url.Values{"foo": []string{"a", "b", "c"}, "bar": []string{"d", "e", "f"}}, allowedKeys: sets.New[string](), - want: "bar=redacted&bar=redacted&bar=redacted&foo=redacted&foo=redacted&foo=redacted", + want: []any{ + "params", + map[string]string{ + "bar": "redacted", + "foo": "redacted", + }, + "multiValueParams", + url.Values{ + "bar": {"redacted", "redacted", "redacted"}, + "foo": {"redacted", "redacted", "redacted"}, + }, + }, }, { name: "nil allowed values", params: url.Values{"foo": []string{"a", "b", "c"}, "bar": []string{"d", "e", "f"}}, allowedKeys: nil, - want: "bar=redacted&bar=redacted&bar=redacted&foo=redacted&foo=redacted&foo=redacted", + want: []any{ + "params", + map[string]string{ + "bar": "redacted", + "foo": "redacted", + }, + "multiValueParams", + url.Values{ + "bar": {"redacted", "redacted", "redacted"}, + "foo": {"redacted", "redacted", "redacted"}, + }, + }, + }, + { + name: "url decodes allowed values", + params: url.Values{ + "foo": []string{"a%3Ab", "c", "urn%3Aietf%3Aparams%3Aoauth%3Agrant-type%3Atoken-exchange"}, + "bar": []string{"d", "e", "f"}, + }, + allowedKeys: sets.New("foo"), + want: []any{ + "params", + map[string]string{ + "bar": "redacted", + "foo": "a:b", + }, + "multiValueParams", + url.Values{ + "bar": {"redacted", "redacted", "redacted"}, + "foo": {"a:b", "c", "urn:ietf:params:oauth:grant-type:token-exchange"}, + }, + }, + }, + { + name: "ignores url decode errors", + params: url.Values{ + "bad_encoding": []string{"%.."}, + }, + allowedKeys: sets.New("bad_encoding"), + want: []any{ + "params", + map[string]string{ + "bad_encoding": "%..", + }, + }, }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - require.Equal(t, tt.want, SanitizeParams(tt.params, tt.allowedKeys)) + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + // This comparison should require the exact order + require.Equal(t, test.want, SanitizeParams(test.params, test.allowedKeys)) }) } }