diff --git a/internal/federationdomain/endpoints/auth/auth_handler.go b/internal/federationdomain/endpoints/auth/auth_handler.go index f68ea7316..19cd100f6 100644 --- a/internal/federationdomain/endpoints/auth/auth_handler.go +++ b/internal/federationdomain/endpoints/auth/auth_handler.go @@ -5,6 +5,7 @@ package auth import ( + "errors" "fmt" "net/http" "net/url" @@ -96,41 +97,15 @@ func NewHandler( } func (h *authorizeHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { - if r.Method != http.MethodPost && r.Method != http.MethodGet { - // https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest - // Authorization Servers MUST support the use of the HTTP GET and POST methods defined in - // RFC 2616 [RFC2616] at the Authorization Endpoint. - responseutil.HTTPErrorf(w, http.StatusMethodNotAllowed, "%s (try GET or POST)", r.Method) - return - } - // If the client set a username or password header, they are trying to log in without using a browser. hadUsernameHeader := len(r.Header.Values(oidcapi.AuthorizeUsernameHeaderName)) > 0 hadPasswordHeader := len(r.Header.Values(oidcapi.AuthorizePasswordHeaderName)) > 0 requestedBrowserlessFlow := hadUsernameHeader || hadPasswordHeader - // Need to parse the request params, so we can get the IDP name. The style and text of the error is inspired by - // fosite's implementation of NewAuthorizeRequest(). Fosite only calls ParseMultipartForm() there. However, - // although ParseMultipartForm() calls ParseForm(), it swallows errors from ParseForm() sometimes. To avoid - // having any errors swallowed, we call both. When fosite calls ParseMultipartForm() later, it will be a noop. - if err := r.ParseForm(); err != nil { + // Need to parse the request params, so we can get the IDP name and audit log the params. + if err := parseForm(r); err != nil { oidc.WriteAuthorizeError(r, w, - h.oauthHelperWithoutStorage, - fosite.NewAuthorizeRequest(), - fosite.ErrInvalidRequest. - WithHint("Unable to parse form params, make sure to send a properly formatted query params or form request body."). - WithWrap(err).WithDebug(err.Error()), - requestedBrowserlessFlow) - return - } - if err := r.ParseMultipartForm(1 << 20); err != nil && err != http.ErrNotMultipart { - oidc.WriteAuthorizeError(r, w, - h.oauthHelperWithoutStorage, - fosite.NewAuthorizeRequest(), - fosite.ErrInvalidRequest. - WithHint("Unable to parse multipart HTTP body, make sure to send a properly formatted form request body."). - WithWrap(err).WithDebug(err.Error()), - requestedBrowserlessFlow) + h.oauthHelperWithoutStorage, fosite.NewAuthorizeRequest(), err, requestedBrowserlessFlow) return } @@ -143,6 +118,14 @@ func (h *authorizeHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { h.auditLogger.Audit(plog.AuditEventHTTPRequestParameters, r.Context(), plog.NoSessionPersisted(), plog.SanitizeParams(r.Form, paramsSafeToLog())...) + if r.Method != http.MethodPost && r.Method != http.MethodGet { + // https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest + // Authorization Servers MUST support the use of the HTTP GET and POST methods defined in + // RFC 2616 [RFC2616] at the Authorization Endpoint. + responseutil.HTTPErrorf(w, http.StatusMethodNotAllowed, "%s (try GET or POST)", r.Method) + return + } + // Note that the client might have used oidcapi.AuthorizeUpstreamIDPNameParamName and // oidcapi.AuthorizeUpstreamIDPTypeParamName query (or form) params to request a certain upstream IDP. // The Pinniped CLI has been sending these params since v0.9.0. @@ -181,6 +164,27 @@ func (h *authorizeHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { h.authorize(w, r, requestedBrowserlessFlow, idp) } +// parseForm parses the query params and/or POST body form params. It returns an error, or in the case of success it +// has the side-effect of leaving the parsed form params on the http.Request in the Form field. Request body +// parameters take precedence over URL query string values. +func parseForm(r *http.Request) error { + // The style of form parsing and the text of the error is inspired by fosite's implementation of NewAuthorizeRequest(). + // Fosite only calls ParseMultipartForm() there. However, although ParseMultipartForm() calls ParseForm(), + // it swallows errors from ParseForm() sometimes. To avoid having any errors swallowed, we call both. + // When fosite calls ParseMultipartForm() later, it will be a noop. + if err := r.ParseForm(); err != nil { + return fosite.ErrInvalidRequest. + WithHint("Unable to parse form params, make sure to send a properly formatted query params or form request body."). + WithWrap(err).WithDebug(err.Error()) + } + if err := r.ParseMultipartForm(1 << 20); err != nil && !errors.Is(err, http.ErrNotMultipart) { + return fosite.ErrInvalidRequest. + WithHint("Unable to parse multipart HTTP body, make sure to send a properly formatted form request body."). + WithWrap(err).WithDebug(err.Error()) + } + return nil +} + func (h *authorizeHandler) authorize( w http.ResponseWriter, r *http.Request, diff --git a/internal/federationdomain/endpoints/auth/auth_handler_test.go b/internal/federationdomain/endpoints/auth/auth_handler_test.go index 510b0372a..7118754a5 100644 --- a/internal/federationdomain/endpoints/auth/auth_handler_test.go +++ b/internal/federationdomain/endpoints/auth/auth_handler_test.go @@ -3882,28 +3882,70 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo name: "PUT is a bad method", idps: testidplister.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()), method: http.MethodPut, - path: "/some/path", + path: "/some/path?foo=bar&client_id=baz", wantStatus: http.StatusMethodNotAllowed, wantContentType: plainContentType, wantBodyString: "Method Not Allowed: PUT (try GET or POST)\n", + wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { + return []testutil.WantedAuditLog{ + testutil.WantAuditLog("HTTP Request Custom Headers Used", map[string]any{ + "Pinniped-Username": false, + "Pinniped-Password": false, + }), + testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ + "params": map[string]any{ + "client_id": "baz", + "foo": "redacted", + }, + }), + } + }, }, { name: "PATCH is a bad method", idps: testidplister.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()), method: http.MethodPatch, - path: "/some/path", + path: "/some/path?foo=bar&client_id=baz", wantStatus: http.StatusMethodNotAllowed, wantContentType: plainContentType, wantBodyString: "Method Not Allowed: PATCH (try GET or POST)\n", + wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { + return []testutil.WantedAuditLog{ + testutil.WantAuditLog("HTTP Request Custom Headers Used", map[string]any{ + "Pinniped-Username": false, + "Pinniped-Password": false, + }), + testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ + "params": map[string]any{ + "client_id": "baz", + "foo": "redacted", + }, + }), + } + }, }, { name: "DELETE is a bad method", idps: testidplister.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()), method: http.MethodDelete, - path: "/some/path", + path: "/some/path?foo=bar&client_id=baz", wantStatus: http.StatusMethodNotAllowed, wantContentType: plainContentType, wantBodyString: "Method Not Allowed: DELETE (try GET or POST)\n", + wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { + return []testutil.WantedAuditLog{ + testutil.WantAuditLog("HTTP Request Custom Headers Used", map[string]any{ + "Pinniped-Username": false, + "Pinniped-Password": false, + }), + testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ + "params": map[string]any{ + "client_id": "baz", + "foo": "redacted", + }, + }), + } + }, }, } diff --git a/internal/plog/audit_event.go b/internal/plog/audit_event.go index 9e491e4d9..b3d2d4492 100644 --- a/internal/plog/audit_event.go +++ b/internal/plog/audit_event.go @@ -63,7 +63,6 @@ func SanitizeParams(inputParams url.Values, allowedKeys sets.Set[string]) []any if len(multiValueParams) > 0 { return []any{"params", params, "multiValueParams", multiValueParams} - } return []any{"params", params} }