From 44e218194b3c36d8928bb1d080a5e2a0b5e9b2e2 Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Thu, 31 Oct 2024 17:00:52 -0500 Subject: [PATCH] Add 'AuthorizeID From Parameters' audit logs to the /callback and /login endpoints Co-authored-by: Ryan Richard --- internal/crypto/ptls/log_profiles_test.go | 5 +- .../downstreamsession/downstream_session.go | 12 +- .../endpoints/auth/auth_handler.go | 19 +- .../endpoints/auth/auth_handler_test.go | 229 ++++++++++++------ .../endpoints/callback/callback_handler.go | 24 +- .../callback/callback_handler_test.go | 10 +- .../endpoints/login/get_login_handler.go | 5 +- .../endpoints/login/get_login_handler_test.go | 7 +- .../endpoints/login/login_handler.go | 7 +- .../endpoints/login/login_handler_test.go | 22 +- .../endpoints/login/post_login_handler.go | 5 +- .../endpoints/loginurl/login_url.go | 5 +- .../endpointsmanager/manager.go | 1 + internal/federationdomain/oidc/oidc.go | 5 +- .../requestlogger/request_logger.go | 17 ++ .../resolvedprovider/resolved_provider.go | 3 +- .../resolved_github_provider.go | 2 +- .../resolvedoidc/resolved_oidc_provider.go | 2 +- .../federationdomain/stateparam/encoded.go | 19 ++ .../stateparam/encoded_test.go | 22 ++ internal/plog/audit_event.go | 1 + internal/plog/plog.go | 12 + internal/testutil/log_lines.go | 24 +- .../expected_upstream_state_param.go | 5 +- 24 files changed, 321 insertions(+), 142 deletions(-) create mode 100644 internal/federationdomain/stateparam/encoded.go create mode 100644 internal/federationdomain/stateparam/encoded_test.go diff --git a/internal/crypto/ptls/log_profiles_test.go b/internal/crypto/ptls/log_profiles_test.go index 9f5c39153..497dac13b 100644 --- a/internal/crypto/ptls/log_profiles_test.go +++ b/internal/crypto/ptls/log_profiles_test.go @@ -1,11 +1,12 @@ // Copyright 2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -package ptls +package ptls_test import ( "testing" + "go.pinniped.dev/internal/crypto/ptls" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/testutil" ) @@ -13,7 +14,7 @@ import ( func TestLogAllProfiles(t *testing.T) { logger, log := plog.TestLogger(t) - LogAllProfiles(logger) + ptls.LogAllProfiles(logger) expectedLines := []string{ `{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","caller":"ptls/log_profiles.go:$ptls.logProfile","message":"tls configuration","profile name":"Default","MinVersion":"TLS 1.2","MaxVersion":"NONE","CipherSuites":["TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256","TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256","TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384","TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384","TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256","TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256"],"NextProtos":["h2","http/1.1"]}`, diff --git a/internal/federationdomain/downstreamsession/downstream_session.go b/internal/federationdomain/downstreamsession/downstream_session.go index e22c9b3bf..7dd1d377e 100644 --- a/internal/federationdomain/downstreamsession/downstream_session.go +++ b/internal/federationdomain/downstreamsession/downstream_session.go @@ -49,18 +49,18 @@ func NewPinnipedSession( ) (*psession.PinnipedSession, error) { now := time.Now().UTC() - // Do not associate this audit event with a session ID. - // The session has not yet "started" and may not be persisted to permanent storage. - auditLogger.Audit(plog.AuditEventIdentityFromUpstreamIDP, ctx, nil, + auditLogger.Audit(plog.AuditEventIdentityFromUpstreamIDP, ctx, plog.NoSessionPersisted(), + "upstreamIDPDisplayName", c.IdentityProvider.GetDisplayName(), + "upstreamIDPType", c.IdentityProvider.GetSessionProviderType(), + "upstreamIDPResourceName", c.IdentityProvider.GetProvider().GetResourceName(), + "upstreamIDPResourceUID", c.IdentityProvider.GetProvider().GetResourceUID(), "upstreamUsername", c.UpstreamIdentity.UpstreamUsername, "upstreamGroups", c.UpstreamIdentity.UpstreamGroups) downstreamUsername, downstreamGroups, err := applyIdentityTransformations(ctx, c.IdentityProvider.GetTransforms(), c.UpstreamIdentity.UpstreamUsername, c.UpstreamIdentity.UpstreamGroups) if err != nil { - // Do not associate this audit event with a session ID. - // This session is being rejected and will never be persisted to permanent storage. - auditLogger.Audit(plog.AuditEventAuthenticationRejectedByTransforms, ctx, nil, + auditLogger.Audit(plog.AuditEventAuthenticationRejectedByTransforms, ctx, plog.NoSessionPersisted(), "reason", err) return nil, err } diff --git a/internal/federationdomain/endpoints/auth/auth_handler.go b/internal/federationdomain/endpoints/auth/auth_handler.go index 64a746d16..78cac0624 100644 --- a/internal/federationdomain/endpoints/auth/auth_handler.go +++ b/internal/federationdomain/endpoints/auth/auth_handler.go @@ -5,8 +5,6 @@ package auth import ( - "crypto/sha256" - "encoding/hex" "fmt" "net/http" "net/url" @@ -24,6 +22,7 @@ import ( "go.pinniped.dev/internal/federationdomain/formposthtml" "go.pinniped.dev/internal/federationdomain/oidc" "go.pinniped.dev/internal/federationdomain/resolvedprovider" + "go.pinniped.dev/internal/federationdomain/stateparam" "go.pinniped.dev/internal/httputil/responseutil" "go.pinniped.dev/internal/httputil/securityheader" "go.pinniped.dev/internal/plog" @@ -136,11 +135,11 @@ func (h *authorizeHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // Log if these headers were present, but don't log the actual values. The password is obviously sensitive, // and sometimes users use their password as their username by mistake. - h.auditLogger.Audit(plog.AuditEventHTTPRequestCustomHeadersUsed, r.Context(), nil, + h.auditLogger.Audit(plog.AuditEventHTTPRequestCustomHeadersUsed, r.Context(), plog.NoSessionPersisted(), oidcapi.AuthorizeUsernameHeaderName, hadUsernameHeader, oidcapi.AuthorizePasswordHeaderName, hadPasswordHeader) - h.auditLogger.Audit(plog.AuditEventHTTPRequestParameters, r.Context(), nil, + h.auditLogger.Audit(plog.AuditEventHTTPRequestParameters, r.Context(), plog.NoSessionPersisted(), "params", plog.SanitizeParams(r.Form, paramsSafeToLog)) // Note that the client might have used oidcapi.AuthorizeUpstreamIDPNameParamName and @@ -172,7 +171,7 @@ func (h *authorizeHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - h.auditLogger.Audit(plog.AuditEventUsingUpstreamIDP, r.Context(), nil, + h.auditLogger.Audit(plog.AuditEventUsingUpstreamIDP, r.Context(), plog.NoSessionPersisted(), "displayName", idp.GetDisplayName(), "resourceName", idp.GetProvider().GetResourceName(), "resourceUID", idp.GetProvider().GetResourceUID(), @@ -216,7 +215,7 @@ func (h *authorizeHandler) authorize( authorizeID, err = h.authorizeWithBrowser(r, w, oauthHelper, authorizeRequester, idp) if err == nil { - h.auditLogger.Audit(plog.AuditEventUpstreamAuthorizeRedirect, r.Context(), nil, + h.auditLogger.Audit(plog.AuditEventUpstreamAuthorizeRedirect, r.Context(), plog.NoSessionPersisted(), "authorizeID", authorizeID) } } @@ -295,9 +294,7 @@ func (h *authorizeHandler) authorizeWithBrowser( http.StatusSeeOther, // match fosite and https://tools.ietf.org/id/draft-ietf-oauth-security-topics-18.html#section-4.11 ) - upstreamStateHash := sha256.Sum256([]byte(authRequestState.EncodedStateParam)) - authorizeID := hex.EncodeToString(upstreamStateHash[:]) - return authorizeID, nil + return authRequestState.EncodedStateParam.AuthorizeID(), nil } func shouldShowIDPChooser( @@ -473,7 +470,7 @@ func upstreamStateParam( csrfValue csrftoken.CSRFToken, pkceValue pkce.Code, encoder oidc.Encoder, -) (string, error) { +) (stateparam.Encoded, error) { stateParamData := oidc.UpstreamStateParamData{ // The auth params might have included oidcapi.AuthorizeUpstreamIDPNameParamName and // oidcapi.AuthorizeUpstreamIDPTypeParamName, but those can be ignored by other handlers @@ -492,7 +489,7 @@ func upstreamStateParam( if err != nil { return "", fmt.Errorf("error encoding upstream state param: %w", err) } - return encodedStateParamValue, nil + return stateparam.Encoded(encodedStateParamValue), nil } func removeCustomIDPParams(params url.Values) url.Values { diff --git a/internal/federationdomain/endpoints/auth/auth_handler_test.go b/internal/federationdomain/endpoints/auth/auth_handler_test.go index b9bf6f21f..e5db3cdc3 100644 --- a/internal/federationdomain/endpoints/auth/auth_handler_test.go +++ b/internal/federationdomain/endpoints/auth/auth_handler_test.go @@ -6,8 +6,6 @@ package auth import ( "bytes" "context" - "crypto/sha256" - "encoding/hex" "errors" "fmt" "html" @@ -38,6 +36,7 @@ import ( "go.pinniped.dev/internal/federationdomain/oidc" "go.pinniped.dev/internal/federationdomain/oidcclientvalidator" "go.pinniped.dev/internal/federationdomain/requestlogger" + "go.pinniped.dev/internal/federationdomain/stateparam" "go.pinniped.dev/internal/federationdomain/storage" "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/plog" @@ -661,19 +660,8 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo prefixUsernameAndGroupsPipeline := transformtestutil.NewPrefixingPipeline(t, transformationUsernamePrefix, transformationGroupsPrefix) rejectAuthPipeline := transformtestutil.NewRejectAllAuthPipeline(t) - generateAuthorizeId := func(encodedStateParam string) string { - upstreamStateHash := sha256.Sum256([]byte(encodedStateParam)) - return hex.EncodeToString(upstreamStateHash[:]) - } - - buildWantedAuditLog := func(message string, params map[string]any) testutil.WantedAuditLog { - wantedAuditLog := testutil.WantedAuditLog{ - Message: message, - Params: params, - } - wantedAuditLog.Params["auditID"] = "some-audit-id" - wantedAuditLog.Params["timestamp"] = "2099-08-08T13:57:36.123456Z" - return wantedAuditLog + wantAuditLog := func(message string, params map[string]any) testutil.WantedAuditLog { + return testutil.WantAuditLog(message, params, "some-audit-id") } type testCase struct { @@ -703,7 +691,7 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo wantBodyStringWithLocationInHref bool wantLocationHeader string wantUpstreamStateParamInLocationHeader bool - wantAuditLogs func(encodedStateParam, sessionID string) []testutil.WantedAuditLog + wantAuditLogs func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog // Assertions for when an authcode should be returned, i.e. the request was authenticated by an // upstream LDAP provider or an upstream OIDC password grant flow. @@ -740,23 +728,23 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo wantLocationHeader: expectedRedirectLocationForUpstreamOIDC(expectedUpstreamStateParam(nil, "", oidcUpstreamName, "oidc"), nil), wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, - wantAuditLogs: func(encodedStateParam, sessionID string) []testutil.WantedAuditLog { + wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ - buildWantedAuditLog("HTTP Request Custom Headers Used", map[string]any{ + wantAuditLog("HTTP Request Custom Headers Used", map[string]any{ "Pinniped-Username": false, "Pinniped-Password": false, }), - buildWantedAuditLog("HTTP Request Parameters", map[string]any{ + 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", }), - buildWantedAuditLog("Using Upstream IDP", map[string]any{ + wantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-oidc-idp", "resourceName": "some-oidc-idp", "resourceUID": "oidc-resource-uid", "type": "oidc", }), - buildWantedAuditLog("Upstream Authorize Redirect", map[string]any{ - "authorizeID": generateAuthorizeId(encodedStateParam), + wantAuditLog("Upstream Authorize Redirect", map[string]any{ + "authorizeID": encodedStateParam.AuthorizeID(), }), } }, @@ -778,23 +766,23 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo wantLocationHeader: expectedRedirectLocationForUpstreamOIDC(expectedUpstreamStateParam(map[string]string{"client_id": dynamicClientID, "scope": testutil.AllDynamicClientScopesSpaceSep}, "", oidcUpstreamName, "oidc"), nil), wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, - wantAuditLogs: func(encodedStateParam, sessionID string) []testutil.WantedAuditLog { + wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ - buildWantedAuditLog("HTTP Request Custom Headers Used", map[string]any{ + wantAuditLog("HTTP Request Custom Headers Used", map[string]any{ "Pinniped-Username": false, "Pinniped-Password": false, }), - buildWantedAuditLog("HTTP Request Parameters", map[string]any{ + 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`, }), - buildWantedAuditLog("Using Upstream IDP", map[string]any{ + wantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-oidc-idp", "resourceName": "some-oidc-idp", "resourceUID": "oidc-resource-uid", "type": "oidc", }), - buildWantedAuditLog("Upstream Authorize Redirect", map[string]any{ - "authorizeID": generateAuthorizeId(encodedStateParam), + wantAuditLog("Upstream Authorize Redirect", map[string]any{ + "authorizeID": encodedStateParam.AuthorizeID(), }), } }, @@ -815,23 +803,23 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo wantLocationHeader: expectedRedirectLocationForUpstreamGithub(expectedUpstreamStateParam(nil, "", githubUpstreamName, "github")), wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, - wantAuditLogs: func(encodedStateParam, sessionID string) []testutil.WantedAuditLog { + wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ - buildWantedAuditLog("HTTP Request Custom Headers Used", map[string]any{ + wantAuditLog("HTTP Request Custom Headers Used", map[string]any{ "Pinniped-Username": false, "Pinniped-Password": false, }), - buildWantedAuditLog("HTTP Request Parameters", map[string]any{ + 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", }), - buildWantedAuditLog("Using Upstream IDP", map[string]any{ + wantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-github-idp", "resourceName": "some-github-idp", "resourceUID": "github-resource-uid", "type": "github", }), - buildWantedAuditLog("Upstream Authorize Redirect", map[string]any{ - "authorizeID": generateAuthorizeId(encodedStateParam), + wantAuditLog("Upstream Authorize Redirect", map[string]any{ + "authorizeID": encodedStateParam.AuthorizeID(), }), } }, @@ -853,23 +841,23 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo wantLocationHeader: expectedRedirectLocationForUpstreamGithub(expectedUpstreamStateParam(map[string]string{"client_id": dynamicClientID, "scope": testutil.AllDynamicClientScopesSpaceSep}, "", githubUpstreamName, "github")), wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, - wantAuditLogs: func(encodedStateParam, sessionID string) []testutil.WantedAuditLog { + wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ - buildWantedAuditLog("HTTP Request Custom Headers Used", map[string]any{ + wantAuditLog("HTTP Request Custom Headers Used", map[string]any{ "Pinniped-Username": false, "Pinniped-Password": false, }), - buildWantedAuditLog("HTTP Request Parameters", map[string]any{ + 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`, }), - buildWantedAuditLog("Using Upstream IDP", map[string]any{ + wantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-github-idp", "resourceName": "some-github-idp", "resourceUID": "github-resource-uid", "type": "github", }), - buildWantedAuditLog("Upstream Authorize Redirect", map[string]any{ - "authorizeID": generateAuthorizeId(encodedStateParam), + wantAuditLog("Upstream Authorize Redirect", map[string]any{ + "authorizeID": encodedStateParam.AuthorizeID(), }), } }, @@ -890,6 +878,26 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo wantLocationHeader: urlWithQuery(downstreamIssuer+"/login", map[string]string{"state": expectedUpstreamStateParam(nil, "", ldapUpstreamName, "ldap")}), wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, + wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { + return []testutil.WantedAuditLog{ + wantAuditLog("HTTP Request Custom Headers Used", map[string]any{ + "Pinniped-Username": false, + "Pinniped-Password": false, + }), + 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`, + }), + wantAuditLog("Using Upstream IDP", map[string]any{ + "displayName": "some-ldap-idp", + "resourceName": "some-ldap-idp", + "resourceUID": "ldap-resource-uid", + "type": "ldap", + }), + wantAuditLog("Upstream Authorize Redirect", map[string]any{ + "authorizeID": encodedStateParam.AuthorizeID(), + }), + } + }, }, { name: "OIDC upstream browser flow happy path using GET without a CSRF cookie using backwards compatibility mode to have a default IDP (display name does not need to be sent as query param)", @@ -908,6 +916,26 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo wantLocationHeader: expectedRedirectLocationForUpstreamOIDC(expectedUpstreamStateParam(nil, "", oidcUpstreamName, "oidc"), nil), wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, + wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { + return []testutil.WantedAuditLog{ + wantAuditLog("HTTP Request Custom Headers Used", map[string]any{ + "Pinniped-Username": false, + "Pinniped-Password": false, + }), + 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`, + }), + wantAuditLog("Using Upstream IDP", map[string]any{ + "displayName": "some-oidc-idp", + "resourceName": "some-oidc-idp", + "resourceUID": "oidc-resource-uid", + "type": "oidc", + }), + wantAuditLog("Upstream Authorize Redirect", map[string]any{ + "authorizeID": encodedStateParam.AuthorizeID(), + }), + } + }, }, { name: "with multiple IDPs available, request does not choose which IDP to use", @@ -927,6 +955,17 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo wantLocationHeader: urlWithQuery(downstreamIssuer+"/choose_identity_provider", happyGetRequestQueryMap), wantUpstreamStateParamInLocationHeader: false, // it should copy the params of the original request, not add a new state param wantBodyStringWithLocationInHref: true, + wantAuditLogs: func(_ stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { + return []testutil.WantedAuditLog{ + wantAuditLog("HTTP Request Custom Headers Used", map[string]any{ + "Pinniped-Username": false, + "Pinniped-Password": false, + }), + 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`, + }), + } + }, }, { name: "with multiple IDPs available, request chooses to use OIDC browser flow", @@ -946,6 +985,26 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo wantLocationHeader: expectedRedirectLocationForUpstreamOIDC(expectedUpstreamStateParam(nil, "", oidcUpstreamName, "oidc"), nil), wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, + wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { + return []testutil.WantedAuditLog{ + wantAuditLog("HTTP Request Custom Headers Used", map[string]any{ + "Pinniped-Username": false, + "Pinniped-Password": false, + }), + 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`, + }), + wantAuditLog("Using Upstream IDP", map[string]any{ + "displayName": "some-oidc-idp", + "resourceName": "some-oidc-idp", + "resourceUID": "oidc-resource-uid", + "type": "oidc", + }), + wantAuditLog("Upstream Authorize Redirect", map[string]any{ + "authorizeID": encodedStateParam.AuthorizeID(), + }), + } + }, }, { name: "with multiple IDPs available, request chooses to use LDAP browser flow", @@ -1040,26 +1099,30 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: expectedHappyOIDCPasswordGrantCustomSession, - wantAuditLogs: func(encodedStateParam, sessionID string) []testutil.WantedAuditLog { + wantAuditLogs: func(_ stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ - buildWantedAuditLog("HTTP Request Custom Headers Used", map[string]any{ + wantAuditLog("HTTP Request Custom Headers Used", map[string]any{ "Pinniped-Username": true, "Pinniped-Password": true, }), - buildWantedAuditLog("HTTP Request Parameters", map[string]any{ + 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`, }), - buildWantedAuditLog("Using Upstream IDP", map[string]any{ + wantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-password-granting-oidc-idp", "resourceName": "some-password-granting-oidc-idp", "resourceUID": "some-password-granting-resource-uid", "type": "oidc", }), - buildWantedAuditLog("Identity From Upstream IDP", map[string]any{ - "upstreamUsername": "test-oidc-pinniped-username", - "upstreamGroups": []any{"test-pinniped-group-0", "test-pinniped-group-1"}, + wantAuditLog("Identity From Upstream IDP", map[string]any{ + "upstreamIDPDisplayName": "some-password-granting-oidc-idp", + "upstreamIDPResourceName": "some-password-granting-oidc-idp", + "upstreamIDPResourceUID": "some-password-granting-resource-uid", + "upstreamIDPType": "oidc", + "upstreamUsername": "test-oidc-pinniped-username", + "upstreamGroups": []any{"test-pinniped-group-0", "test-pinniped-group-1"}, }), - buildWantedAuditLog("Session Started", map[string]any{ + wantAuditLog("Session Started", map[string]any{ "sessionID": sessionID, "username": "test-oidc-pinniped-username", "groups": []any{"test-pinniped-group-0", "test-pinniped-group-1"}, @@ -1111,26 +1174,30 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo wantContentType: jsonContentType, wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithConfiguredPolicyRejectionHintErrorQuery), wantBodyString: "", - wantAuditLogs: func(encodedStateParam, sessionID string) []testutil.WantedAuditLog { + wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ - buildWantedAuditLog("HTTP Request Custom Headers Used", map[string]any{ + wantAuditLog("HTTP Request Custom Headers Used", map[string]any{ "Pinniped-Username": true, "Pinniped-Password": true, }), - buildWantedAuditLog("HTTP Request Parameters", map[string]any{ + 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`, }), - buildWantedAuditLog("Using Upstream IDP", map[string]any{ + wantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-password-granting-oidc-idp", "resourceName": "some-password-granting-oidc-idp", "resourceUID": "some-password-granting-resource-uid", "type": "oidc", }), - buildWantedAuditLog("Identity From Upstream IDP", map[string]any{ - "upstreamUsername": "test-oidc-pinniped-username", - "upstreamGroups": []any{"test-pinniped-group-0", "test-pinniped-group-1"}, + wantAuditLog("Identity From Upstream IDP", map[string]any{ + "upstreamIDPDisplayName": "some-password-granting-oidc-idp", + "upstreamIDPResourceName": "some-password-granting-oidc-idp", + "upstreamIDPResourceUID": "some-password-granting-resource-uid", + "upstreamIDPType": "oidc", + "upstreamUsername": "test-oidc-pinniped-username", + "upstreamGroups": []any{"test-pinniped-group-0", "test-pinniped-group-1"}, }), - buildWantedAuditLog("Authentication Rejected By Transforms", map[string]any{ + wantAuditLog("Authentication Rejected By Transforms", map[string]any{ "reason": "configured identity policy rejected this authentication: authentication was rejected by a configured policy", }), } @@ -1218,26 +1285,30 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: expectedHappyLDAPUpstreamCustomSession, - wantAuditLogs: func(encodedStateParam, sessionID string) []testutil.WantedAuditLog { + wantAuditLogs: func(_ stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ - buildWantedAuditLog("HTTP Request Custom Headers Used", map[string]any{ + wantAuditLog("HTTP Request Custom Headers Used", map[string]any{ "Pinniped-Username": true, "Pinniped-Password": true, }), - buildWantedAuditLog("HTTP Request Parameters", map[string]any{ + 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`, }), - buildWantedAuditLog("Using Upstream IDP", map[string]any{ + wantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-ldap-idp", "resourceName": "some-ldap-idp", "resourceUID": "ldap-resource-uid", "type": "ldap", }), - buildWantedAuditLog("Identity From Upstream IDP", map[string]any{ - "upstreamUsername": "some-ldap-username-from-authenticator", - "upstreamGroups": []any{"group1", "group2", "group3"}, + wantAuditLog("Identity From Upstream IDP", map[string]any{ + "upstreamIDPDisplayName": "some-ldap-idp", + "upstreamIDPResourceName": "some-ldap-idp", + "upstreamIDPResourceUID": "ldap-resource-uid", + "upstreamIDPType": "ldap", + "upstreamUsername": "some-ldap-username-from-authenticator", + "upstreamGroups": []any{"group1", "group2", "group3"}, }), - buildWantedAuditLog("Session Started", map[string]any{ + wantAuditLog("Session Started", map[string]any{ "sessionID": sessionID, "username": "some-ldap-username-from-authenticator", "groups": []any{"group1", "group2", "group3"}, @@ -1274,26 +1345,30 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo happyLDAPUsernameFromAuthenticator, happyLDAPGroups, ), - wantAuditLogs: func(encodedStateParam, sessionID string) []testutil.WantedAuditLog { + wantAuditLogs: func(_ stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ - buildWantedAuditLog("HTTP Request Custom Headers Used", map[string]any{ + wantAuditLog("HTTP Request Custom Headers Used", map[string]any{ "Pinniped-Username": true, "Pinniped-Password": true, }), - buildWantedAuditLog("HTTP Request Parameters", map[string]any{ + 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`, }), - buildWantedAuditLog("Using Upstream IDP", map[string]any{ + wantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-ldap-idp", "resourceName": "some-ldap-idp", "resourceUID": "ldap-resource-uid", "type": "ldap", }), - buildWantedAuditLog("Identity From Upstream IDP", map[string]any{ - "upstreamUsername": "some-ldap-username-from-authenticator", - "upstreamGroups": []any{"group1", "group2", "group3"}, + wantAuditLog("Identity From Upstream IDP", map[string]any{ + "upstreamIDPDisplayName": "some-ldap-idp", + "upstreamIDPResourceName": "some-ldap-idp", + "upstreamIDPResourceUID": "ldap-resource-uid", + "upstreamIDPType": "ldap", + "upstreamUsername": "some-ldap-username-from-authenticator", + "upstreamGroups": []any{"group1", "group2", "group3"}, }), - buildWantedAuditLog("Session Started", map[string]any{ + wantAuditLog("Session Started", map[string]any{ "sessionID": sessionID, "username": "username_prefix:some-ldap-username-from-authenticator", "groups": []any{"groups_prefix:group1", "groups_prefix:group2", "groups_prefix:group3"}, @@ -1642,16 +1717,16 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo wantContentType: jsonContentType, wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeLoginRequiredErrorQuery), wantBodyString: "", - wantAuditLogs: func(encodedStateParam, sessionID string) []testutil.WantedAuditLog { + wantAuditLogs: func(_ stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ - buildWantedAuditLog("HTTP Request Custom Headers Used", map[string]any{ + wantAuditLog("HTTP Request Custom Headers Used", map[string]any{ "Pinniped-Username": false, "Pinniped-Password": false, }), - buildWantedAuditLog("HTTP Request Parameters", map[string]any{ + 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`, }), - buildWantedAuditLog("Using Upstream IDP", map[string]any{ + wantAuditLog("Using Upstream IDP", map[string]any{ "displayName": "some-oidc-idp", "resourceName": "some-oidc-idp", "resourceUID": "oidc-resource-uid", @@ -3806,7 +3881,7 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo } if test.wantAuditLogs != nil { - wantAuditLogs := test.wantAuditLogs(actualQueryStateParam, sessionID) + wantAuditLogs := test.wantAuditLogs(stateparam.Encoded(actualQueryStateParam), sessionID) testutil.CompareAuditLogs(t, wantAuditLogs, auditLog.String()) } diff --git a/internal/federationdomain/endpoints/callback/callback_handler.go b/internal/federationdomain/endpoints/callback/callback_handler.go index 5c8d32165..60853f6b1 100644 --- a/internal/federationdomain/endpoints/callback/callback_handler.go +++ b/internal/federationdomain/endpoints/callback/callback_handler.go @@ -14,6 +14,7 @@ import ( "go.pinniped.dev/internal/federationdomain/federationdomainproviders" "go.pinniped.dev/internal/federationdomain/formposthtml" "go.pinniped.dev/internal/federationdomain/oidc" + "go.pinniped.dev/internal/federationdomain/stateparam" "go.pinniped.dev/internal/httputil/httperr" "go.pinniped.dev/internal/httputil/securityheader" "go.pinniped.dev/internal/plog" @@ -27,18 +28,21 @@ func NewHandler( auditLogger plog.AuditLogger, ) http.Handler { handler := httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { - state, err := validateRequest(r, stateDecoder, cookieDecoder) + encodedState, decodedState, err := validateRequest(r, stateDecoder, cookieDecoder) if err != nil { return err } - idp, err := upstreamIDPs.FindUpstreamIDPByDisplayName(state.UpstreamName) + auditLogger.Audit(plog.AuditEventAuthorizeIDFromParameters, r.Context(), plog.NoSessionPersisted(), + "authorizeID", encodedState.AuthorizeID()) + + idp, err := upstreamIDPs.FindUpstreamIDPByDisplayName(decodedState.UpstreamName) if err != nil || idp == nil { plog.Warning("upstream provider not found") return httperr.New(http.StatusUnprocessableEntity, "upstream provider not found") } - downstreamAuthParams, err := url.ParseQuery(state.AuthParams) + downstreamAuthParams, err := url.ParseQuery(decodedState.AuthParams) if err != nil { plog.Error("error reading state downstream auth params", err) return httperr.New(http.StatusBadRequest, "error reading state downstream auth params") @@ -61,7 +65,7 @@ func NewHandler( // an error if the client requested a scope that they are not allowed to request, so we don't need to worry about that here. downstreamsession.AutoApproveScopes(authorizeRequester) - identity, loginExtras, err := idp.LoginFromCallback(r.Context(), authcode(r), state.PKCECode, state.Nonce, redirectURI) + identity, loginExtras, err := idp.LoginFromCallback(r.Context(), authcode(r), decodedState.PKCECode, decodedState.Nonce, redirectURI) if err != nil { plog.WarningErr("unable to complete login from callback", err, "identityProviderDisplayName", idp.GetDisplayName(), @@ -107,21 +111,21 @@ func authcode(r *http.Request) string { return r.FormValue("code") } -func validateRequest(r *http.Request, stateDecoder, cookieDecoder oidc.Decoder) (*oidc.UpstreamStateParamData, error) { +func validateRequest(r *http.Request, stateDecoder, cookieDecoder oidc.Decoder) (stateparam.Encoded, *oidc.UpstreamStateParamData, error) { if r.Method != http.MethodGet { - return nil, httperr.Newf(http.StatusMethodNotAllowed, "%s (try GET)", r.Method) + return "", nil, httperr.Newf(http.StatusMethodNotAllowed, "%s (try GET)", r.Method) } - _, decodedState, err := oidc.ReadStateParamAndValidateCSRFCookie(r, cookieDecoder, stateDecoder) + encodedState, decodedState, err := oidc.ReadStateParamAndValidateCSRFCookie(r, cookieDecoder, stateDecoder) if err != nil { plog.InfoErr("state or CSRF error", err) - return nil, err + return "", nil, err } if authcode(r) == "" { plog.Info("code param not found") - return nil, httperr.New(http.StatusBadRequest, "code param not found") + return "", nil, httperr.New(http.StatusBadRequest, "code param not found") } - return decodedState, nil + return encodedState, decodedState, nil } diff --git a/internal/federationdomain/endpoints/callback/callback_handler_test.go b/internal/federationdomain/endpoints/callback/callback_handler_test.go index 366929aaf..1e4189aa2 100644 --- a/internal/federationdomain/endpoints/callback/callback_handler_test.go +++ b/internal/federationdomain/endpoints/callback/callback_handler_test.go @@ -25,6 +25,7 @@ import ( "go.pinniped.dev/internal/federationdomain/endpoints/jwks" "go.pinniped.dev/internal/federationdomain/oidc" "go.pinniped.dev/internal/federationdomain/oidcclientvalidator" + "go.pinniped.dev/internal/federationdomain/stateparam" "go.pinniped.dev/internal/federationdomain/storage" "go.pinniped.dev/internal/federationdomain/upstreamprovider" "go.pinniped.dev/internal/plog" @@ -1870,12 +1871,13 @@ type expectedGitHubAuthcodeExchange struct { } type requestPath struct { - code, state *string + code *string + state *stateparam.Encoded } func newRequestPath() *requestPath { c := happyUpstreamAuthcode - s := "4321" + s := stateparam.Encoded("4321") return &requestPath{ code: &c, state: &s, @@ -1892,7 +1894,7 @@ func (r *requestPath) WithoutCode() *requestPath { return r } -func (r *requestPath) WithState(state string) *requestPath { +func (r *requestPath) WithState(state stateparam.Encoded) *requestPath { r.state = &state return r } @@ -1909,7 +1911,7 @@ func (r *requestPath) String() string { params.Add("code", *r.code) } if r.state != nil { - params.Add("state", *r.state) + params.Add("state", r.state.String()) } return path + params.Encode() } diff --git a/internal/federationdomain/endpoints/login/get_login_handler.go b/internal/federationdomain/endpoints/login/get_login_handler.go index 8b5beb2ff..772ea224c 100644 --- a/internal/federationdomain/endpoints/login/get_login_handler.go +++ b/internal/federationdomain/endpoints/login/get_login_handler.go @@ -9,6 +9,7 @@ import ( "go.pinniped.dev/internal/federationdomain/endpoints/login/loginhtml" "go.pinniped.dev/internal/federationdomain/endpoints/loginurl" "go.pinniped.dev/internal/federationdomain/oidc" + "go.pinniped.dev/internal/federationdomain/stateparam" ) const ( @@ -17,12 +18,12 @@ const ( ) func NewGetHandler(loginPath string) HandlerFunc { - return func(w http.ResponseWriter, r *http.Request, encodedState string, decodedState *oidc.UpstreamStateParamData) error { + return func(w http.ResponseWriter, r *http.Request, encodedState stateparam.Encoded, decodedState *oidc.UpstreamStateParamData) error { alertMessage, hasAlert := getAlert(r) pageInputs := &loginhtml.PageData{ PostPath: loginPath, - State: encodedState, + State: encodedState.String(), IDPName: decodedState.UpstreamName, HasAlertError: hasAlert, AlertMessage: alertMessage, diff --git a/internal/federationdomain/endpoints/login/get_login_handler_test.go b/internal/federationdomain/endpoints/login/get_login_handler_test.go index 74405d497..7ff8a7714 100644 --- a/internal/federationdomain/endpoints/login/get_login_handler_test.go +++ b/internal/federationdomain/endpoints/login/get_login_handler_test.go @@ -1,4 +1,4 @@ -// Copyright 2022-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2022-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package login @@ -13,6 +13,7 @@ import ( "go.pinniped.dev/internal/federationdomain/endpoints/login/loginhtml" "go.pinniped.dev/internal/federationdomain/idplister" "go.pinniped.dev/internal/federationdomain/oidc" + "go.pinniped.dev/internal/federationdomain/stateparam" "go.pinniped.dev/internal/testutil" ) @@ -27,7 +28,7 @@ func TestGetLogin(t *testing.T) { tests := []struct { name string decodedState *oidc.UpstreamStateParamData - encodedState string + encodedState stateparam.Encoded errParam string idps idplister.UpstreamIdentityProvidersLister wantStatus int @@ -98,7 +99,7 @@ func TestGetLogin(t *testing.T) { t.Parallel() handler := NewGetHandler(testPath) - target := testPath + "?state=" + tt.encodedState + target := testPath + "?state=" + tt.encodedState.String() if tt.errParam != "" { target += "&err=" + tt.errParam } diff --git a/internal/federationdomain/endpoints/login/login_handler.go b/internal/federationdomain/endpoints/login/login_handler.go index 2b2bf4491..e37c6d40b 100644 --- a/internal/federationdomain/endpoints/login/login_handler.go +++ b/internal/federationdomain/endpoints/login/login_handler.go @@ -10,6 +10,7 @@ import ( "go.pinniped.dev/internal/federationdomain/endpoints/login/loginhtml" "go.pinniped.dev/internal/federationdomain/formposthtml" "go.pinniped.dev/internal/federationdomain/oidc" + "go.pinniped.dev/internal/federationdomain/stateparam" "go.pinniped.dev/internal/httputil/httperr" "go.pinniped.dev/internal/httputil/securityheader" "go.pinniped.dev/internal/plog" @@ -19,7 +20,7 @@ import ( type HandlerFunc func( w http.ResponseWriter, r *http.Request, - encodedState string, + encodedState stateparam.Encoded, decodedState *oidc.UpstreamStateParamData, ) error @@ -38,6 +39,7 @@ func NewHandler( cookieDecoder oidc.Decoder, getHandler HandlerFunc, // use NewGetHandler() for production postHandler HandlerFunc, // use NewPostHandler() for production + auditLogger plog.AuditLogger, ) http.Handler { loginHandler := httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { var handler HandlerFunc @@ -56,6 +58,9 @@ func NewHandler( return err } + auditLogger.Audit(plog.AuditEventAuthorizeIDFromParameters, r.Context(), plog.NoSessionPersisted(), + "authorizeID", encodedState.AuthorizeID()) + switch decodedState.UpstreamType { case string(idpdiscoveryv1alpha1.IDPTypeLDAP), string(idpdiscoveryv1alpha1.IDPTypeActiveDirectory): // these are the types supported by this endpoint, so no error here diff --git a/internal/federationdomain/endpoints/login/login_handler_test.go b/internal/federationdomain/endpoints/login/login_handler_test.go index 854484b35..3e761d442 100644 --- a/internal/federationdomain/endpoints/login/login_handler_test.go +++ b/internal/federationdomain/endpoints/login/login_handler_test.go @@ -1,4 +1,4 @@ -// Copyright 2022-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2022-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package login @@ -14,7 +14,9 @@ import ( "github.com/stretchr/testify/require" "go.pinniped.dev/internal/federationdomain/oidc" + "go.pinniped.dev/internal/federationdomain/stateparam" "go.pinniped.dev/internal/httputil/httperr" + "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/testutil" "go.pinniped.dev/internal/testutil/oidctestutil" ) @@ -118,7 +120,7 @@ func TestLoginEndpoint(t *testing.T) { wantStatus int wantContentType string wantBody string - wantEncodedState string + wantEncodedState stateparam.Encoded wantDecodedState *oidc.UpstreamStateParamData }{ { @@ -381,12 +383,12 @@ func TestLoginEndpoint(t *testing.T) { testGetHandler := func( w http.ResponseWriter, r *http.Request, - encodedState string, + encodedState stateparam.Encoded, decodedState *oidc.UpstreamStateParamData, ) error { require.Equal(t, req, r) require.Equal(t, rsp, w) - require.Equal(t, tt.wantEncodedState, encodedState) + require.Equal(t, stateparam.Encoded(tt.wantEncodedState), encodedState) require.Equal(t, tt.wantDecodedState, decodedState) if tt.getHandlerErr == nil { _, err := w.Write([]byte(happyGetResult)) @@ -398,12 +400,12 @@ func TestLoginEndpoint(t *testing.T) { testPostHandler := func( w http.ResponseWriter, r *http.Request, - encodedState string, + encodedState stateparam.Encoded, decodedState *oidc.UpstreamStateParamData, ) error { require.Equal(t, req, r) require.Equal(t, rsp, w) - require.Equal(t, tt.wantEncodedState, encodedState) + require.Equal(t, stateparam.Encoded(tt.wantEncodedState), encodedState) require.Equal(t, tt.wantDecodedState, decodedState) if tt.postHandlerErr == nil { _, err := w.Write([]byte(happyPostResult)) @@ -412,7 +414,7 @@ func TestLoginEndpoint(t *testing.T) { return tt.postHandlerErr } - subject := NewHandler(happyStateCodec, happyCookieCodec, testGetHandler, testPostHandler) + subject := NewHandler(happyStateCodec, happyCookieCodec, testGetHandler, testPostHandler, plog.New()) subject.ServeHTTP(rsp, req) @@ -430,14 +432,14 @@ func TestLoginEndpoint(t *testing.T) { } type requestPath struct { - state *string + state *stateparam.Encoded } func newRequestPath() *requestPath { return &requestPath{} } -func (r *requestPath) WithState(state string) *requestPath { +func (r *requestPath) WithState(state stateparam.Encoded) *requestPath { r.state = &state return r } @@ -451,7 +453,7 @@ func (r *requestPath) String() string { path := "/login?" params := url.Values{} if r.state != nil { - params.Add("state", *r.state) + params.Add("state", r.state.String()) } return path + params.Encode() } diff --git a/internal/federationdomain/endpoints/login/post_login_handler.go b/internal/federationdomain/endpoints/login/post_login_handler.go index 084a530ae..176782f00 100644 --- a/internal/federationdomain/endpoints/login/post_login_handler.go +++ b/internal/federationdomain/endpoints/login/post_login_handler.go @@ -15,6 +15,7 @@ import ( "go.pinniped.dev/internal/federationdomain/federationdomainproviders" "go.pinniped.dev/internal/federationdomain/oidc" "go.pinniped.dev/internal/federationdomain/resolvedprovider/resolvedldap" + "go.pinniped.dev/internal/federationdomain/stateparam" "go.pinniped.dev/internal/httputil/httperr" "go.pinniped.dev/internal/plog" ) @@ -25,7 +26,7 @@ func NewPostHandler( oauthHelper fosite.OAuth2Provider, auditLogger plog.AuditLogger, ) HandlerFunc { - return func(w http.ResponseWriter, r *http.Request, encodedState string, decodedState *oidc.UpstreamStateParamData) error { + return func(w http.ResponseWriter, r *http.Request, encodedState stateparam.Encoded, decodedState *oidc.UpstreamStateParamData) error { // Note that the login handler prevents this handler from being called with OIDC upstreams. idp, err := upstreamIDPs.FindUpstreamIDPByDisplayName(decodedState.UpstreamName) if err != nil { @@ -114,7 +115,7 @@ func redirectToLoginPage( r *http.Request, w http.ResponseWriter, downstreamIssuer string, - encodedStateParamValue string, + encodedStateParamValue stateparam.Encoded, errToDisplay loginurl.ErrorParamValue, ) error { loginURL, err := loginurl.URL(downstreamIssuer, encodedStateParamValue, errToDisplay) diff --git a/internal/federationdomain/endpoints/loginurl/login_url.go b/internal/federationdomain/endpoints/loginurl/login_url.go index f7d1b7911..c64205eee 100644 --- a/internal/federationdomain/endpoints/loginurl/login_url.go +++ b/internal/federationdomain/endpoints/loginurl/login_url.go @@ -7,6 +7,7 @@ import ( "net/url" "go.pinniped.dev/internal/federationdomain/oidc" + "go.pinniped.dev/internal/federationdomain/stateparam" ) const ( @@ -27,7 +28,7 @@ type ErrorParamValue string // provider.FederationDomainIssuer when the issuer string comes from that type. func URL( downstreamIssuer string, - encodedStateParamValue string, + encodedStateParamValue stateparam.Encoded, errToDisplay ErrorParamValue, ) (string, error) { loginURL, err := url.Parse(downstreamIssuer + oidc.PinnipedLoginPath) @@ -36,7 +37,7 @@ func URL( } q := loginURL.Query() - q.Set(StateParamName, encodedStateParamValue) + q.Set(StateParamName, encodedStateParamValue.String()) if errToDisplay != ShowNoError { q.Set(ErrParamName, string(errToDisplay)) } diff --git a/internal/federationdomain/endpointsmanager/manager.go b/internal/federationdomain/endpointsmanager/manager.go index 0512d1b3e..aa5d2602d 100644 --- a/internal/federationdomain/endpointsmanager/manager.go +++ b/internal/federationdomain/endpointsmanager/manager.go @@ -184,6 +184,7 @@ func (m *Manager) SetFederationDomains(federationDomains ...*federationdomainpro csrfCookieEncoder, login.NewGetHandler(incomingFederationDomain.IssuerPath()+oidc.PinnipedLoginPath), login.NewPostHandler(issuerURL, idpLister, oauthHelperWithKubeStorage, m.auditLogger), + m.auditLogger, ) plog.Debug("oidc provider manager added or updated issuer", "issuer", issuerURL) diff --git a/internal/federationdomain/oidc/oidc.go b/internal/federationdomain/oidc/oidc.go index 88354159f..db187acfc 100644 --- a/internal/federationdomain/oidc/oidc.go +++ b/internal/federationdomain/oidc/oidc.go @@ -25,6 +25,7 @@ import ( "go.pinniped.dev/internal/federationdomain/endpoints/tokenexchange" "go.pinniped.dev/internal/federationdomain/formposthtml" "go.pinniped.dev/internal/federationdomain/idtokenlifespan" + "go.pinniped.dev/internal/federationdomain/stateparam" "go.pinniped.dev/internal/federationdomain/strategy" "go.pinniped.dev/internal/federationdomain/timeouts" "go.pinniped.dev/internal/httputil/httperr" @@ -326,7 +327,7 @@ func ScopeWasRequested(authorizeRequester fosite.AuthorizeRequester, scopeName s return false } -func ReadStateParamAndValidateCSRFCookie(r *http.Request, cookieDecoder Decoder, stateDecoder Decoder) (string, *UpstreamStateParamData, error) { +func ReadStateParamAndValidateCSRFCookie(r *http.Request, cookieDecoder Decoder, stateDecoder Decoder) (stateparam.Encoded, *UpstreamStateParamData, error) { csrfValue, err := readCSRFCookie(r, cookieDecoder) if err != nil { return "", nil, err @@ -342,7 +343,7 @@ func ReadStateParamAndValidateCSRFCookie(r *http.Request, cookieDecoder Decoder, return "", nil, err } - return encodedState, decodedState, nil + return stateparam.Encoded(encodedState), decodedState, nil } func readCSRFCookie(r *http.Request, cookieDecoder Decoder) (csrftoken.CSRFToken, error) { diff --git a/internal/federationdomain/requestlogger/request_logger.go b/internal/federationdomain/requestlogger/request_logger.go index 7657cebbd..cc4609209 100644 --- a/internal/federationdomain/requestlogger/request_logger.go +++ b/internal/federationdomain/requestlogger/request_logger.go @@ -7,9 +7,11 @@ import ( "bufio" "net" "net/http" + "net/url" "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" @@ -92,12 +94,27 @@ func (rl *requestLogger) LogRequestReceived() { func (rl *requestLogger) LogRequestComplete() { r := rl.req + location := rl.w.Header().Get("Location") + if location == "" { + location = "no location header" + } else { + parsedLocation, err := url.Parse(location) + if err != nil { + location = "unparsable location header" + } else { + redactAllParams := sets.New[string]() + parsedLocation.RawQuery = plog.SanitizeParams(parsedLocation.Query(), redactAllParams) + location = parsedLocation.String() + } + } + rl.auditLogger.Audit(plog.AuditEventHTTPRequestCompleted, r.Context(), nil, // no session available yet in this context "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), "responseStatus", rl.status, + "location", location, ) } diff --git a/internal/federationdomain/resolvedprovider/resolved_provider.go b/internal/federationdomain/resolvedprovider/resolved_provider.go index 226564245..664e94da8 100644 --- a/internal/federationdomain/resolvedprovider/resolved_provider.go +++ b/internal/federationdomain/resolvedprovider/resolved_provider.go @@ -10,6 +10,7 @@ import ( "github.com/ory/fosite" "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1" + "go.pinniped.dev/internal/federationdomain/stateparam" "go.pinniped.dev/internal/federationdomain/upstreamprovider" "go.pinniped.dev/internal/idtransform" "go.pinniped.dev/internal/psession" @@ -86,7 +87,7 @@ type RefreshedIdentity struct { // upstream authorization request does not allow PKCE, then implementations of // FederationDomainResolvedIdentityProvider.UpstreamAuthorizeRedirectURL may choose to ignore that struct field. type UpstreamAuthorizeRequestState struct { - EncodedStateParam string + EncodedStateParam stateparam.Encoded PKCE pkce.Code Nonce nonce.Nonce } diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go index 9bec7de01..79ccc08f8 100644 --- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go +++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go @@ -81,7 +81,7 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamAuthorizeRedire RedirectURL: fmt.Sprintf("%s/callback", downstreamIssuerURL), Scopes: p.Provider.GetScopes(), } - redirectURL := upstreamOAuthConfig.AuthCodeURL(state.EncodedStateParam) + redirectURL := upstreamOAuthConfig.AuthCodeURL(state.EncodedStateParam.String()) return redirectURL, nil } diff --git a/internal/federationdomain/resolvedprovider/resolvedoidc/resolved_oidc_provider.go b/internal/federationdomain/resolvedprovider/resolvedoidc/resolved_oidc_provider.go index a2e3644dd..be6ad5836 100644 --- a/internal/federationdomain/resolvedprovider/resolvedoidc/resolved_oidc_provider.go +++ b/internal/federationdomain/resolvedprovider/resolvedoidc/resolved_oidc_provider.go @@ -118,7 +118,7 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) UpstreamAuthorizeRedirect } redirectURL := upstreamOAuthConfig.AuthCodeURL( - state.EncodedStateParam, + state.EncodedStateParam.String(), authCodeOptions..., ) diff --git a/internal/federationdomain/stateparam/encoded.go b/internal/federationdomain/stateparam/encoded.go new file mode 100644 index 000000000..38b65bdf6 --- /dev/null +++ b/internal/federationdomain/stateparam/encoded.go @@ -0,0 +1,19 @@ +// Copyright 2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package stateparam + +import ( + "crypto/sha256" + "fmt" +) + +type Encoded string + +func (e Encoded) String() string { + return string(e) +} + +func (e Encoded) AuthorizeID() string { + return fmt.Sprintf("%x", sha256.Sum256([]byte(e))) +} diff --git a/internal/federationdomain/stateparam/encoded_test.go b/internal/federationdomain/stateparam/encoded_test.go new file mode 100644 index 000000000..cf00ad2df --- /dev/null +++ b/internal/federationdomain/stateparam/encoded_test.go @@ -0,0 +1,22 @@ +// Copyright 2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package stateparam + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestAuthorizeID(t *testing.T) { + // $ echo -n "foo" | shasum -a 256 + // 2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7ae + require.Equal(t, "2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7ae", + Encoded("foo").AuthorizeID()) + + // $ echo -n "" | shasum -a 256 + // e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 + require.Equal(t, "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", + Encoded("").AuthorizeID()) +} diff --git a/internal/plog/audit_event.go b/internal/plog/audit_event.go index bc0774521..752a692f9 100644 --- a/internal/plog/audit_event.go +++ b/internal/plog/audit_event.go @@ -17,6 +17,7 @@ const ( AuditEventHTTPRequestParameters AuditEventMessage = "HTTP Request Parameters" AuditEventHTTPRequestCustomHeadersUsed AuditEventMessage = "HTTP Request Custom Headers Used" AuditEventUsingUpstreamIDP AuditEventMessage = "Using Upstream IDP" + AuditEventAuthorizeIDFromParameters AuditEventMessage = "AuthorizeID From Parameters" AuditEventIdentityFromUpstreamIDP AuditEventMessage = "Identity From Upstream IDP" AuditEventIdentityRefreshedFromUpstreamIDP AuditEventMessage = "Identity Refreshed From Upstream IDP" AuditEventSessionStarted AuditEventMessage = "Session Started" diff --git a/internal/plog/plog.go b/internal/plog/plog.go index 894b0acc7..6253e8027 100644 --- a/internal/plog/plog.go +++ b/internal/plog/plog.go @@ -42,6 +42,18 @@ type SessionIDGetter interface { GetID() string } +// NoSessionPersisted means do not associate this audit event with a session ID. +// The session has not yet "started" and may or may not ever be persisted to permanent storage. +func NoSessionPersisted() SessionIDGetter { + return nil +} + +// NoHTTPRequestAvailable means there is no request context for this audit event. +// Use this when an audit event is emitted from a controller or some other place that does not have a request context. +func NoHTTPRequestAvailable() context.Context { + return nil +} + // AuditLogger is only the audit logging part of Logger. There is no global function for Audit because // that would make unit testing of audit logs harder. type AuditLogger interface { diff --git a/internal/testutil/log_lines.go b/internal/testutil/log_lines.go index 7e2c1bd40..7f1cd7600 100644 --- a/internal/testutil/log_lines.go +++ b/internal/testutil/log_lines.go @@ -27,9 +27,16 @@ type WantedAuditLog struct { Params map[string]any } -//"message":"HTTP Request Custom Headers Used", -//"auditID":"some-audit-id", -//"Pinniped-Username":false,"Pinniped-Password":false}`, +func WantAuditLog(message string, params map[string]any, auditID string) WantedAuditLog { + result := WantedAuditLog{ + Message: message, + Params: params, + } + if auditID != "" { + result.Params["auditID"] = auditID + } + return result +} func CompareAuditLogs(t *testing.T, wantAuditLogs []WantedAuditLog, actualAuditLogsOneLiner string) { t.Helper() @@ -42,6 +49,7 @@ func CompareAuditLogs(t *testing.T, wantAuditLogs []WantedAuditLog, actualAuditL wantJsonAuditLog["message"] = wantAuditLog.Message wantMessages = append(wantMessages, wantAuditLog.Message) wantJsonAuditLog["auditEvent"] = true + wantJsonAuditLog["timestamp"] = "2099-08-08T13:57:36.123456Z" for k, v := range wantAuditLog.Params { wantJsonAuditLog[k] = v } @@ -58,7 +66,10 @@ func CompareAuditLogs(t *testing.T, wantAuditLogs []WantedAuditLog, actualAuditL err := json.Unmarshal([]byte(actualAuditLog), &actualJsonAuditLog) require.NoError(t, err) - // we don't care to test the caller + // we don't care to test exact equality on the caller - just make sure it is a non-empty string + caller, ok := actualJsonAuditLog["caller"] + require.True(t, ok) + require.NotEmpty(t, caller, "caller for message %q must not be empty", actualJsonAuditLog["message"]) delete(actualJsonAuditLog, "caller") actualJsonAuditLogs = append(actualJsonAuditLogs, actualJsonAuditLog) @@ -67,6 +78,9 @@ func CompareAuditLogs(t *testing.T, wantAuditLogs []WantedAuditLog, actualAuditL actualMessages = append(actualMessages, actualMessage) } + // TODO: remove this + t.Logf("LAST AUDIT EVENT: %s", actualAuditLogs[len(actualAuditLogs)-1]) + // We should check array indices first so that we don't exceed any boundaries. // But we also want to be sure to indicate to the caller what went wrong, so compare the messages. require.Equal(t, wantMessages, actualMessages) @@ -75,6 +89,6 @@ func CompareAuditLogs(t *testing.T, wantAuditLogs []WantedAuditLog, actualAuditL for i := range len(wantJsonAuditLogs) { // compare each item individually so we know which message it is require.Equal(t, wantJsonAuditLogs[i], actualJsonAuditLogs[i], - "audit log for message %q does not match", wantJsonAuditLogs[i]["message"]) + "audit event for message %q does not match", wantJsonAuditLogs[i]["message"]) } } diff --git a/internal/testutil/oidctestutil/expected_upstream_state_param.go b/internal/testutil/oidctestutil/expected_upstream_state_param.go index 72ff6398d..1518f466f 100644 --- a/internal/testutil/oidctestutil/expected_upstream_state_param.go +++ b/internal/testutil/oidctestutil/expected_upstream_state_param.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/require" idpdiscoveryv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1" + "go.pinniped.dev/internal/federationdomain/stateparam" ) // ExpectedUpstreamStateParamFormat is a separate type from the production code to ensure that the state @@ -28,10 +29,10 @@ type ExpectedUpstreamStateParamFormat struct { type UpstreamStateParamBuilder ExpectedUpstreamStateParamFormat -func (b *UpstreamStateParamBuilder) Build(t *testing.T, stateEncoder *securecookie.SecureCookie) string { +func (b *UpstreamStateParamBuilder) Build(t *testing.T, stateEncoder *securecookie.SecureCookie) stateparam.Encoded { state, err := stateEncoder.Encode("s", b) require.NoError(t, err) - return state + return stateparam.Encoded(state) } func (b *UpstreamStateParamBuilder) WithAuthorizeRequestParams(params string) *UpstreamStateParamBuilder {