From 49fec5458eb4816b72943e30f196d2f43fce02cd Mon Sep 17 00:00:00 2001 From: Terry Hardie Date: Fri, 8 Sep 2023 12:45:06 -0700 Subject: [PATCH 1/4] Added POST support to request_tracker In the case where you've used samlsp to wrap an endpoint that may get a POST request, the previous implemetation just did a HTTP Redirect after the SAML assertion was done. --- samlsp/middleware.go | 49 ++++++++++++++++++++++++++------ samlsp/middleware_test.go | 1 + samlsp/request_tracker.go | 9 ++++-- samlsp/request_tracker_cookie.go | 5 ++++ 4 files changed, 53 insertions(+), 11 deletions(-) diff --git a/samlsp/middleware.go b/samlsp/middleware.go index f5eabb16..202cb6e6 100644 --- a/samlsp/middleware.go +++ b/samlsp/middleware.go @@ -3,6 +3,8 @@ package samlsp import ( "bytes" "encoding/xml" + "errors" + "fmt" "net/http" "github.com/crewjam/saml" @@ -193,24 +195,31 @@ func (m *Middleware) HandleStartAuthFlow(w http.ResponseWriter, r *http.Request) // CreateSessionFromAssertion is invoked by ServeHTTP when we have a new, valid SAML assertion. func (m *Middleware) CreateSessionFromAssertion(w http.ResponseWriter, r *http.Request, assertion *saml.Assertion, redirectURI string) { + var err error + + trackedRequest := &TrackedRequest{ + Method: "GET", + URI: redirectURI, + } + if trackedRequestIndex := r.Form.Get("RelayState"); trackedRequestIndex != "" { - trackedRequest, err := m.RequestTracker.GetTrackedRequest(r, trackedRequestIndex) + trackedRequest, err = m.RequestTracker.GetTrackedRequest(r, trackedRequestIndex) if err != nil { - if err == http.ErrNoCookie && m.ServiceProvider.AllowIDPInitiated { - if uri := r.Form.Get("RelayState"); uri != "" { - redirectURI = uri + if errors.Is(err, http.ErrNoCookie) && m.ServiceProvider.AllowIDPInitiated { + // We don't need to re-read RelayState from the form and check it for nil. The test above did that + trackedRequest = &TrackedRequest{ + Method: "GET", + URI: trackedRequestIndex, } } else { m.OnError(w, r, err) return } } else { - if err := m.RequestTracker.StopTrackingRequest(w, r, trackedRequestIndex); err != nil { + if err = m.RequestTracker.StopTrackingRequest(w, r, trackedRequestIndex); err != nil { m.OnError(w, r, err) return } - - redirectURI = trackedRequest.URI } } @@ -218,8 +227,32 @@ func (m *Middleware) CreateSessionFromAssertion(w http.ResponseWriter, r *http.R m.OnError(w, r, err) return } + m.HandleRedirectAfterAssertion(w, r, trackedRequest) +} - http.Redirect(w, r, redirectURI, http.StatusFound) +// HandleRedirectAfterAssertion is called after we've handled receiving a SAML assertion and created a session with the +// browser. Most normal cases are just a redirect, but if the original request was a POST, it's a little more tricky. +func (m *Middleware) HandleRedirectAfterAssertion(w http.ResponseWriter, r *http.Request, trackedRequest *TrackedRequest) { + switch trackedRequest.Method { + case "POST": + text := fmt.Sprintf(``+ + `
`, trackedRequest.URI) + for key, values := range trackedRequest.PostData { + for _, value := range values { + text = fmt.Sprintf(`%s`, text, key, value) + } + } + text += `` + + `
` + + `` + + `` + + `` + + w.Write([]byte(text)) + return + default: + http.Redirect(w, r, trackedRequest.URI, http.StatusFound) + } } // RequireAttribute returns a middleware function that requires that the diff --git a/samlsp/middleware_test.go b/samlsp/middleware_test.go index 801aad08..b6a193ed 100644 --- a/samlsp/middleware_test.go +++ b/samlsp/middleware_test.go @@ -115,6 +115,7 @@ func (test *MiddlewareTest) makeTrackedRequest(id string) string { Index: "KCosLjAyNDY4Ojw-QEJERkhKTE5QUlRWWFpcXmBiZGZoamxucHJ0dnh6", SAMLRequestID: id, URI: "/frob", + Method: "GET", }) if err != nil { panic(err) diff --git a/samlsp/request_tracker.go b/samlsp/request_tracker.go index f5477c8d..b3cfc5a2 100644 --- a/samlsp/request_tracker.go +++ b/samlsp/request_tracker.go @@ -2,6 +2,7 @@ package samlsp import ( "net/http" + "net/url" ) // RequestTracker tracks pending authentication requests. @@ -31,9 +32,11 @@ type RequestTracker interface { // TrackedRequest holds the data we store for each pending request. type TrackedRequest struct { - Index string `json:"-"` - SAMLRequestID string `json:"id"` - URI string `json:"uri"` + Index string `json:"-"` + SAMLRequestID string `json:"id"` + URI string `json:"uri"` + Method string `json:"method"` + PostData url.Values `json:"post_data"` } // TrackedRequestCodec handles encoding and decoding of a TrackedRequest. diff --git a/samlsp/request_tracker_cookie.go b/samlsp/request_tracker_cookie.go index d9189f63..8fc4d87f 100644 --- a/samlsp/request_tracker_cookie.go +++ b/samlsp/request_tracker_cookie.go @@ -26,10 +26,15 @@ type CookieRequestTracker struct { // TrackRequest starts tracking the SAML request with the given ID. It returns an // `index` that should be used as the RelayState in the SAMl request flow. func (t CookieRequestTracker) TrackRequest(w http.ResponseWriter, r *http.Request, samlRequestID string) (string, error) { + if r.Method == "POST" { + r.ParseForm() + } trackedRequest := TrackedRequest{ Index: base64.RawURLEncoding.EncodeToString(randomBytes(42)), SAMLRequestID: samlRequestID, URI: r.URL.String(), + Method: r.Method, + PostData: r.PostForm, } if t.RelayStateFunc != nil { From daf70b85415eb623223fcae5f12b62007508bcfd Mon Sep 17 00:00:00 2001 From: Terry Hardie Date: Fri, 8 Sep 2023 15:22:25 -0700 Subject: [PATCH 2/4] Added a TODO for the other HTTP methods --- samlsp/middleware.go | 1 + 1 file changed, 1 insertion(+) diff --git a/samlsp/middleware.go b/samlsp/middleware.go index 202cb6e6..06ea0cf7 100644 --- a/samlsp/middleware.go +++ b/samlsp/middleware.go @@ -250,6 +250,7 @@ func (m *Middleware) HandleRedirectAfterAssertion(w http.ResponseWriter, r *http w.Write([]byte(text)) return + // TODO: Handle HEAD, DELETE, etc. default: http.Redirect(w, r, trackedRequest.URI, http.StatusFound) } From f060980c0b981a0157c1cb1d3bba75edccce37cb Mon Sep 17 00:00:00 2001 From: Terry Hardie Date: Fri, 8 Sep 2023 17:01:46 -0700 Subject: [PATCH 3/4] Missed checking error return on a Write and a ParseForm --- samlsp/middleware.go | 5 ++++- samlsp/request_tracker_cookie.go | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/samlsp/middleware.go b/samlsp/middleware.go index 06ea0cf7..e057a031 100644 --- a/samlsp/middleware.go +++ b/samlsp/middleware.go @@ -248,7 +248,10 @@ func (m *Middleware) HandleRedirectAfterAssertion(w http.ResponseWriter, r *http `` + `` - w.Write([]byte(text)) + if _, err := w.Write([]byte(text)); err != nil { + m.OnError(w, r, err) + return + } return // TODO: Handle HEAD, DELETE, etc. default: diff --git a/samlsp/request_tracker_cookie.go b/samlsp/request_tracker_cookie.go index 8fc4d87f..13bb6fa0 100644 --- a/samlsp/request_tracker_cookie.go +++ b/samlsp/request_tracker_cookie.go @@ -27,7 +27,9 @@ type CookieRequestTracker struct { // `index` that should be used as the RelayState in the SAMl request flow. func (t CookieRequestTracker) TrackRequest(w http.ResponseWriter, r *http.Request, samlRequestID string) (string, error) { if r.Method == "POST" { - r.ParseForm() + if err := r.ParseForm(); err != nil { + return "", err + } } trackedRequest := TrackedRequest{ Index: base64.RawURLEncoding.EncodeToString(randomBytes(42)), From e0c2156fbf22d9da0ebe092fd4798fe6fee471e8 Mon Sep 17 00:00:00 2001 From: Terry Hardie Date: Mon, 5 Feb 2024 14:56:17 -0800 Subject: [PATCH 4/4] Added ability to set ACSEndpoint.Location --- identity_provider.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/identity_provider.go b/identity_provider.go index b03dc89b..655df321 100644 --- a/identity_provider.go +++ b/identity_provider.go @@ -915,7 +915,11 @@ func (req *IdpAuthnRequest) PostBinding() (IdpAuthnRequestForm, error) { req.ACSEndpoint.Binding) } - form.URL = req.ACSEndpoint.Location + if req.ACSEndpoint.ResponseLocation != nil { + form.URL = *req.ACSEndpoint.ResponseLocation + } else { + form.URL = req.ACSEndpoint.Location + } form.SAMLResponse = base64.StdEncoding.EncodeToString(responseBuf) form.RelayState = req.RelayState