From e7c3a477fc4f75321671a6f601cc07a9180e4646 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Fri, 16 Jun 2023 02:05:59 -0400 Subject: [PATCH] 20647: Fix CORS preflight request handling. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- lib/controller/localdb/container_gateway.go | 11 +++ .../localdb/container_gateway_test.go | 67 ++++++++++++++----- services/keep-web/handler.go | 26 ++++--- 3 files changed, 77 insertions(+), 27 deletions(-) diff --git a/lib/controller/localdb/container_gateway.go b/lib/controller/localdb/container_gateway.go index d17f2e10df..9526c01f8e 100644 --- a/lib/controller/localdb/container_gateway.go +++ b/lib/controller/localdb/container_gateway.go @@ -31,6 +31,7 @@ import ( "git.arvados.org/arvados.git/sdk/go/auth" "git.arvados.org/arvados.git/sdk/go/ctxlog" "git.arvados.org/arvados.git/sdk/go/httpserver" + keepweb "git.arvados.org/arvados.git/services/keep-web" "github.com/hashicorp/yamux" "golang.org/x/net/webdav" ) @@ -78,6 +79,16 @@ var ( // ...or the request may be handled locally using an empty-collection // stub. func (conn *Conn) ContainerRequestLog(ctx context.Context, opts arvados.ContainerLogOptions) (http.Handler, error) { + if opts.Method == "OPTIONS" && opts.Header.Get("Access-Control-Request-Method") != "" { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !keepweb.ServeCORSPreflight(w, opts.Header) { + // Inconceivable. We already checked + // for the only condition where + // ServeCORSPreflight returns false. + httpserver.Error(w, "unhandled CORS preflight request", http.StatusInternalServerError) + } + }), nil + } cr, err := conn.railsProxy.ContainerRequestGet(ctx, arvados.GetOptions{UUID: opts.UUID, Select: []string{"uuid", "container_uuid", "log_uuid"}}) if err != nil { if se := httpserver.HTTPStatusError(nil); errors.As(err, &se) && se.HTTPStatus() == http.StatusUnauthorized { diff --git a/lib/controller/localdb/container_gateway_test.go b/lib/controller/localdb/container_gateway_test.go index dc8a8cea0d..f7310e8de1 100644 --- a/lib/controller/localdb/container_gateway_test.go +++ b/lib/controller/localdb/container_gateway_test.go @@ -6,6 +6,7 @@ package localdb import ( "bytes" + "context" "crypto/hmac" "crypto/sha256" "fmt" @@ -28,6 +29,7 @@ import ( "git.arvados.org/arvados.git/sdk/go/arvados" "git.arvados.org/arvados.git/sdk/go/arvadosclient" "git.arvados.org/arvados.git/sdk/go/arvadostest" + "git.arvados.org/arvados.git/sdk/go/auth" "git.arvados.org/arvados.git/sdk/go/ctxlog" "git.arvados.org/arvados.git/sdk/go/httpserver" "git.arvados.org/arvados.git/sdk/go/keepclient" @@ -298,9 +300,16 @@ func (s *ContainerGatewaySuite) TestContainerRequestLogViaTunnel(c *check.C) { defer delete(s.cluster.Services.Controller.InternalURLs, *forceInternalURLForTest) } + r, err := http.NewRequestWithContext(s.userctx, "GET", "https://controller.example/arvados/v1/container_requests/"+s.reqUUID+"/log/"+s.ctrUUID+"/stderr.txt", nil) + c.Assert(err, check.IsNil) + r.Header.Set("Authorization", "Bearer "+arvadostest.ActiveTokenV2) handler, err := s.localdb.ContainerRequestLog(s.userctx, arvados.ContainerLogOptions{ - UUID: s.reqUUID, - WebDAVOptions: arvados.WebDAVOptions{Path: "/" + s.ctrUUID + "/stderr.txt"}, + UUID: s.reqUUID, + WebDAVOptions: arvados.WebDAVOptions{ + Method: "GET", + Header: r.Header, + Path: "/" + s.ctrUUID + "/stderr.txt", + }, }) if broken { c.Check(err, check.ErrorMatches, `.*tunnel endpoint is invalid.*`) @@ -308,9 +317,6 @@ func (s *ContainerGatewaySuite) TestContainerRequestLogViaTunnel(c *check.C) { } c.Check(err, check.IsNil) c.Assert(handler, check.NotNil) - r, err := http.NewRequestWithContext(s.userctx, "GET", "https://controller.example/arvados/v1/container_requests/"+s.reqUUID+"/log/"+s.ctrUUID+"/stderr.txt", nil) - c.Assert(err, check.IsNil) - r.Header.Set("Authorization", "Bearer "+arvadostest.ActiveTokenV2) rec := httptest.NewRecorder() handler.ServeHTTP(rec, r) resp := rec.Result() @@ -334,12 +340,13 @@ func (s *ContainerGatewaySuite) TestContainerRequestLogViaKeepWeb(c *check.C) { func (s *ContainerGatewaySuite) testContainerRequestLog(c *check.C) { for _, trial := range []struct { - method string - path string - header http.Header - expectStatus int - expectBodyRe string - expectHeader http.Header + method string + path string + header http.Header + unauthenticated bool + expectStatus int + expectBodyRe string + expectHeader http.Header }{ { method: "GET", @@ -373,6 +380,22 @@ func (s *ContainerGatewaySuite) testContainerRequestLog(c *check.C) { "Allow": {"OPTIONS, LOCK, GET, HEAD, POST, DELETE, PROPPATCH, COPY, MOVE, UNLOCK, PROPFIND, PUT"}, }, }, + { + method: "OPTIONS", + path: s.ctrUUID + "/stderr.txt", + unauthenticated: true, + header: http.Header{ + "Access-Control-Request-Method": {"POST"}, + }, + expectStatus: http.StatusOK, + expectBodyRe: "", + expectHeader: http.Header{ + "Access-Control-Allow-Headers": {"Authorization, Content-Type, Range, Depth, Destination, If, Lock-Token, Overwrite, Timeout, Cache-Control"}, + "Access-Control-Allow-Methods": {"COPY, DELETE, GET, LOCK, MKCOL, MOVE, OPTIONS, POST, PROPFIND, PROPPATCH, PUT, RMCOL, UNLOCK"}, + "Access-Control-Allow-Origin": {"*"}, + "Access-Control-Max-Age": {"86400"}, + }, + }, { method: "PROPFIND", path: s.ctrUUID + "/", @@ -411,17 +434,25 @@ func (s *ContainerGatewaySuite) testContainerRequestLog(c *check.C) { }, } { c.Logf("trial %#v", trial) - handler, err := s.localdb.ContainerRequestLog(s.userctx, arvados.ContainerLogOptions{ - UUID: s.reqUUID, - WebDAVOptions: arvados.WebDAVOptions{Path: "/" + trial.path}, - }) - c.Assert(err, check.IsNil) - c.Assert(handler, check.NotNil) - r, err := http.NewRequestWithContext(s.userctx, trial.method, "https://controller.example/arvados/v1/container_requests/"+s.reqUUID+"/log/"+trial.path, nil) + ctx := s.userctx + if trial.unauthenticated { + ctx = auth.NewContext(context.Background(), auth.CredentialsFromRequest(&http.Request{URL: &url.URL{}, Header: http.Header{}})) + } + r, err := http.NewRequestWithContext(ctx, trial.method, "https://controller.example/arvados/v1/container_requests/"+s.reqUUID+"/log/"+trial.path, nil) c.Assert(err, check.IsNil) for k := range trial.header { r.Header.Set(k, trial.header.Get(k)) } + handler, err := s.localdb.ContainerRequestLog(ctx, arvados.ContainerLogOptions{ + UUID: s.reqUUID, + WebDAVOptions: arvados.WebDAVOptions{ + Method: trial.method, + Header: r.Header, + Path: "/" + trial.path, + }, + }) + c.Assert(err, check.IsNil) + c.Assert(handler, check.NotNil) rec := httptest.NewRecorder() handler.ServeHTTP(rec, r) resp := rec.Result() diff --git a/services/keep-web/handler.go b/services/keep-web/handler.go index 27981c487d..3cdaf5d2b5 100644 --- a/services/keep-web/handler.go +++ b/services/keep-web/handler.go @@ -182,15 +182,7 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) { w := httpserver.WrapResponseWriter(wOrig) - if method := r.Header.Get("Access-Control-Request-Method"); method != "" && r.Method == "OPTIONS" { - if !browserMethod[method] && !webdavMethod[method] { - w.WriteHeader(http.StatusMethodNotAllowed) - return - } - w.Header().Set("Access-Control-Allow-Headers", corsAllowHeadersHeader) - w.Header().Set("Access-Control-Allow-Methods", "COPY, DELETE, GET, LOCK, MKCOL, MOVE, OPTIONS, POST, PROPFIND, PROPPATCH, PUT, RMCOL, UNLOCK") - w.Header().Set("Access-Control-Allow-Origin", "*") - w.Header().Set("Access-Control-Max-Age", "86400") + if r.Method == "OPTIONS" && ServeCORSPreflight(w, r.Header) { return } @@ -949,3 +941,19 @@ func (h *handler) determineCollection(fs arvados.CustomFileSystem, path string) } return nil, "" } + +func ServeCORSPreflight(w http.ResponseWriter, header http.Header) bool { + method := header.Get("Access-Control-Request-Method") + if method == "" { + return false + } + if !browserMethod[method] && !webdavMethod[method] { + w.WriteHeader(http.StatusMethodNotAllowed) + return true + } + w.Header().Set("Access-Control-Allow-Headers", corsAllowHeadersHeader) + w.Header().Set("Access-Control-Allow-Methods", "COPY, DELETE, GET, LOCK, MKCOL, MOVE, OPTIONS, POST, PROPFIND, PROPPATCH, PUT, RMCOL, UNLOCK") + w.Header().Set("Access-Control-Allow-Origin", "*") + w.Header().Set("Access-Control-Max-Age", "86400") + return true +} -- 2.30.2