From 93ef9a15b3353ccd8b08f823b29294019d539f20 Mon Sep 17 00:00:00 2001 From: Tamas Zsebe Date: Wed, 4 Oct 2023 20:04:56 +0000 Subject: [PATCH 1/2] fix SPNEGO HTTP client redirect loop detection. --- v8/spnego/http.go | 19 +++++--- v8/spnego/http_test.go | 99 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 7 deletions(-) diff --git a/v8/spnego/http.go b/v8/spnego/http.go index 6bc2c0ed..9e4718ac 100644 --- a/v8/spnego/http.go +++ b/v8/spnego/http.go @@ -26,12 +26,13 @@ import ( // Client side functionality // +var errRedirectLoop = errors.New("stopped after 10 redirects") + // Client will negotiate authentication with a server using SPNEGO. type Client struct { *http.Client krb5Client *client.Client spn string - reqs []*http.Request } type redirectErr struct { @@ -80,6 +81,14 @@ func NewClient(krb5Cl *client.Client, httpCl *http.Client, spn string) *Client { // Do is the SPNEGO enabled HTTP client's equivalent of the http.Client's Do method. func (c *Client) Do(req *http.Request) (resp *http.Response, err error) { + return c.do(req, nil, nil) +} + +func (c *Client) do(req *http.Request, via []*http.Request, prevResp *http.Response) (resp *http.Response, err error) { + if len(via) >= 10 { + return resp, errRedirectLoop + } + var body bytes.Buffer if req.Body != nil { // Use a tee reader to capture any body sent in case we have to replay it again @@ -93,15 +102,11 @@ func (c *Client) Do(req *http.Request) (resp *http.Response, err error) { if e, ok := ue.Err.(redirectErr); ok { // Picked up a redirect e.reqTarget.Header.Del(HTTPHeaderAuthRequest) - c.reqs = append(c.reqs, e.reqTarget) - if len(c.reqs) >= 10 { - return resp, errors.New("stopped after 10 redirects") - } if req.Body != nil { // Refresh the body reader so the body can be sent again e.reqTarget.Body = io.NopCloser(&body) } - return c.Do(e.reqTarget) + return c.do(e.reqTarget, append(via, e.reqTarget), resp) } } return resp, err @@ -117,7 +122,7 @@ func (c *Client) Do(req *http.Request) (resp *http.Response, err error) { } io.Copy(io.Discard, resp.Body) resp.Body.Close() - return c.Do(req) + return c.do(req, via, nil) } return resp, err } diff --git a/v8/spnego/http_test.go b/v8/spnego/http_test.go index 4e8b724b..7ee66e0e 100644 --- a/v8/spnego/http_test.go +++ b/v8/spnego/http_test.go @@ -14,6 +14,7 @@ import ( "net/http/cookiejar" "net/http/httptest" "os" + "strings" "sync" "testing" @@ -28,6 +29,104 @@ import ( "github.com/stretchr/testify/assert" ) +// fakeTransport is a transport implementation allowing specification of HTTP responses. +type fakeTransport map[string]func() *http.Response + +func (ft fakeTransport) RoundTrip(r *http.Request) (*http.Response, error) { + // Note: This doesn't support requests with multiple parameters, since parameter ordering in the string is nondeterministic. + if resp, ok := ft[r.URL.String()]; ok { + return resp(), nil + } + return nil, fmt.Errorf("unexpected url: %s", r.URL.String()) +} + +func httpResponse(statusCode int, body string, extraHeaders map[string]string) func() *http.Response { + return func() *http.Response { + resp := &http.Response{ + StatusCode: statusCode, + Header: http.Header{}, + Body: io.NopCloser(strings.NewReader(body)), + } + for k, v := range extraHeaders { + resp.Header.Add(k, v) + } + return resp + } +} + +func TestClient_Do(t *testing.T) { + hc := &http.Client{ + Transport: fakeTransport{ + "http://example.com/page1": httpResponse(http.StatusOK, "page 1 ok", nil), + "http://example.com/redirect": httpResponse(http.StatusFound, "shouldn't see this", map[string]string{"Location": "http://example.com/page1"}), + "http://example.com/loop": httpResponse(http.StatusFound, "down the spiral you go", map[string]string{"Location": "http://example.com/loop"}), + }, + } + + tests := []struct { + desc string + target string + wantStatusCode int + wantBody string + wantErr error + }{ + { + desc: "single page", + target: "http://example.com/page1", + wantStatusCode: http.StatusOK, + wantBody: "page 1 ok", + }, + { + desc: "single redirect hop", + target: "http://example.com/redirect", + wantStatusCode: http.StatusOK, + wantBody: "page 1 ok", + }, + { + desc: "redirect loop", + target: "http://example.com/loop", + wantErr: errRedirectLoop, + }, + } + + for _, tc := range tests { + t.Run(tc.desc, func(t *testing.T) { + // So long as we don't return any 401s, we can safely run this without needing to fall through to Kerberos auth. + spnClient := NewClient(nil, hc, "") + + // Run the request multiple times, to ensure our loop detection is properly counting redirects on a per-request basis. + for i := 0; i < 20; i++ { + req, err := http.NewRequest("GET", tc.target, nil) + if err != nil { + t.Fatalf("Failed to create GET request for target %s: %v", tc.target, err) + } + + resp, err := spnClient.Do(req) + if err == nil { + defer resp.Body.Close() + } + if !errors.Is(err, tc.wantErr) { + t.Fatalf("Unexpected error result while attempting GET request for target %s - got: %v, want: %v", tc.target, err, tc.wantErr) + } + + if err == nil { + if resp.StatusCode != tc.wantStatusCode { + t.Errorf("Unexpected status code - got: %d, want: %d", resp.StatusCode, tc.wantStatusCode) + } + b, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("Failed to read response body for target %s: %v", tc.target, err) + } + body := string(b) + if body != tc.wantBody { + t.Errorf("Unexpected response body - got: %s, want: %s", body, tc.wantBody) + } + } + } + }) + } +} + func TestClient_SetSPNEGOHeader(t *testing.T) { test.Integration(t) b, _ := hex.DecodeString(testdata.KEYTAB_TESTUSER1_TEST_GOKRB5) From 1c0f437d35c0eeb5727726e04481e3c9be2a950f Mon Sep 17 00:00:00 2001 From: Tamas Zsebe Date: Thu, 5 Oct 2023 16:23:08 +0000 Subject: [PATCH 2/2] remove superfluous prev response. --- v8/spnego/http.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/v8/spnego/http.go b/v8/spnego/http.go index 9e4718ac..f0692eb2 100644 --- a/v8/spnego/http.go +++ b/v8/spnego/http.go @@ -81,10 +81,10 @@ func NewClient(krb5Cl *client.Client, httpCl *http.Client, spn string) *Client { // Do is the SPNEGO enabled HTTP client's equivalent of the http.Client's Do method. func (c *Client) Do(req *http.Request) (resp *http.Response, err error) { - return c.do(req, nil, nil) + return c.do(req, nil) } -func (c *Client) do(req *http.Request, via []*http.Request, prevResp *http.Response) (resp *http.Response, err error) { +func (c *Client) do(req *http.Request, via []*http.Request) (resp *http.Response, err error) { if len(via) >= 10 { return resp, errRedirectLoop } @@ -106,7 +106,7 @@ func (c *Client) do(req *http.Request, via []*http.Request, prevResp *http.Respo // Refresh the body reader so the body can be sent again e.reqTarget.Body = io.NopCloser(&body) } - return c.do(e.reqTarget, append(via, e.reqTarget), resp) + return c.do(e.reqTarget, append(via, e.reqTarget)) } } return resp, err @@ -122,7 +122,7 @@ func (c *Client) do(req *http.Request, via []*http.Request, prevResp *http.Respo } io.Copy(io.Discard, resp.Body) resp.Body.Close() - return c.do(req, via, nil) + return c.do(req, via) } return resp, err }