From: Peter Amstutz Date: Mon, 30 Nov 2020 16:03:53 +0000 (-0500) Subject: Merge branch '16774-keep-web-errors' refs #16774 X-Git-Tag: 2.2.0~205 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/c75f2e9b8a29bcdadcb092122f6d30e2930c08a3?hp=7454769dd9bbfbac3af674dfff919c6d0bbc3896 Merge branch '16774-keep-web-errors' refs #16774 Arvados-DCO-1.1-Signed-off-by: Peter Amstutz --- diff --git a/services/keep-web/handler.go b/services/keep-web/handler.go index 963948cc6b..ab1bc080bd 100644 --- a/services/keep-web/handler.go +++ b/services/keep-web/handler.go @@ -62,6 +62,9 @@ func parseCollectionIDFromDNSName(s string) string { var urlPDHDecoder = strings.NewReplacer(" ", "+", "-", "+") +var notFoundMessage = "404 Not found\r\n\r\nThe requested path was not found, or you do not have permission to access it.\r" +var unauthorizedMessage = "401 Unauthorized\r\n\r\nA valid Arvados token must be provided to access this resource.\r" + // parseCollectionIDFromURL returns a UUID or PDH if s is a UUID or a // PDH (even if it is a PDH with "+" replaced by " " or "-"); // otherwise "". @@ -279,7 +282,7 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) { } if collectionID == "" && !useSiteFS { - w.WriteHeader(http.StatusNotFound) + http.Error(w, notFoundMessage, http.StatusNotFound) return } @@ -388,14 +391,14 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) { // for additional credentials would just be // confusing), or we don't even accept // credentials at this path. - w.WriteHeader(http.StatusNotFound) + http.Error(w, notFoundMessage, http.StatusNotFound) return } for _, t := range reqTokens { if tokenResult[t] == 404 { // The client provided valid token(s), but the // collection was not found. - w.WriteHeader(http.StatusNotFound) + http.Error(w, notFoundMessage, http.StatusNotFound) return } } @@ -409,7 +412,7 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) { // data that has been deleted. Allow a referrer to // provide this context somehow? w.Header().Add("WWW-Authenticate", "Basic realm=\"collections\"") - w.WriteHeader(http.StatusUnauthorized) + http.Error(w, unauthorizedMessage, http.StatusUnauthorized) return } @@ -479,7 +482,7 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) { openPath := "/" + strings.Join(targetPath, "/") if f, err := fs.Open(openPath); os.IsNotExist(err) { // Requested non-existent path - w.WriteHeader(http.StatusNotFound) + http.Error(w, notFoundMessage, http.StatusNotFound) } else if err != nil { // Some other (unexpected) error http.Error(w, "open: "+err.Error(), http.StatusInternalServerError) @@ -533,7 +536,7 @@ func (h *handler) getClients(reqID, token string) (arv *arvadosclient.ArvadosCli func (h *handler) serveSiteFS(w http.ResponseWriter, r *http.Request, tokens []string, credentialsOK, attachment bool) { if len(tokens) == 0 { w.Header().Add("WWW-Authenticate", "Basic realm=\"collections\"") - http.Error(w, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized) + http.Error(w, unauthorizedMessage, http.StatusUnauthorized) return } if writeMethod[r.Method] { diff --git a/services/keep-web/handler_test.go b/services/keep-web/handler_test.go index f6f3de8877..8e2e05c761 100644 --- a/services/keep-web/handler_test.go +++ b/services/keep-web/handler_test.go @@ -122,7 +122,7 @@ func (s *IntegrationSuite) TestVhost404(c *check.C) { } s.testServer.Handler.ServeHTTP(resp, req) c.Check(resp.Code, check.Equals, http.StatusNotFound) - c.Check(resp.Body.String(), check.Equals, "") + c.Check(resp.Body.String(), check.Equals, notFoundMessage+"\n") } } @@ -250,7 +250,11 @@ func (s *IntegrationSuite) doVhostRequestsWithHostPath(c *check.C, authz authori // depending on the authz method. c.Check(code, check.Equals, failCode) } - c.Check(body, check.Equals, "") + if code == 404 { + c.Check(body, check.Equals, notFoundMessage+"\n") + } else { + c.Check(body, check.Equals, unauthorizedMessage+"\n") + } } } } @@ -307,7 +311,7 @@ func (s *IntegrationSuite) TestSingleOriginSecretLinkBadToken(c *check.C) { "", "", http.StatusNotFound, - "", + notFoundMessage+"\n", ) } @@ -321,7 +325,7 @@ func (s *IntegrationSuite) TestVhostRedirectQueryTokenToBogusCookie(c *check.C) "", "", http.StatusUnauthorized, - "", + unauthorizedMessage+"\n", ) } @@ -439,7 +443,7 @@ func (s *IntegrationSuite) TestVhostRedirectPOSTFormTokenToCookie404(c *check.C) "application/x-www-form-urlencoded", url.Values{"api_token": {arvadostest.SpectatorToken}}.Encode(), http.StatusNotFound, - "", + notFoundMessage+"\n", ) } @@ -463,7 +467,7 @@ func (s *IntegrationSuite) TestAnonymousTokenError(c *check.C) { "", "", http.StatusNotFound, - "", + notFoundMessage+"\n", ) } diff --git a/services/keep-web/s3.go b/services/keep-web/s3.go index 373fd9a25d..7fb90789a5 100644 --- a/services/keep-web/s3.go +++ b/services/keep-web/s3.go @@ -189,6 +189,33 @@ func (h *handler) checks3signature(r *http.Request) (string, error) { return aca.TokenV2(), nil } +func s3ErrorResponse(w http.ResponseWriter, s3code string, message string, resource string, code int) { + w.Header().Set("Content-Type", "application/xml") + w.Header().Set("X-Content-Type-Options", "nosniff") + w.WriteHeader(code) + var errstruct struct { + Code string + Message string + Resource string + RequestId string + } + errstruct.Code = s3code + errstruct.Message = message + errstruct.Resource = resource + errstruct.RequestId = "" + enc := xml.NewEncoder(w) + fmt.Fprint(w, xml.Header) + enc.EncodeElement(errstruct, xml.StartElement{Name: xml.Name{Local: "Error"}}) +} + +var NoSuchKey = "NoSuchKey" +var NoSuchBucket = "NoSuchBucket" +var InvalidArgument = "InvalidArgument" +var InternalError = "InternalError" +var UnauthorizedAccess = "UnauthorizedAccess" +var InvalidRequest = "InvalidRequest" +var SignatureDoesNotMatch = "SignatureDoesNotMatch" + // serveS3 handles r and returns true if r is a request from an S3 // client, otherwise it returns false. func (h *handler) serveS3(w http.ResponseWriter, r *http.Request) bool { @@ -196,14 +223,14 @@ func (h *handler) serveS3(w http.ResponseWriter, r *http.Request) bool { if auth := r.Header.Get("Authorization"); strings.HasPrefix(auth, "AWS ") { split := strings.SplitN(auth[4:], ":", 2) if len(split) < 2 { - http.Error(w, "malformed Authorization header", http.StatusUnauthorized) + s3ErrorResponse(w, InvalidRequest, "malformed Authorization header", r.URL.Path, http.StatusUnauthorized) return true } token = unescapeKey(split[0]) } else if strings.HasPrefix(auth, s3SignAlgorithm+" ") { t, err := h.checks3signature(r) if err != nil { - http.Error(w, "signature verification failed: "+err.Error(), http.StatusForbidden) + s3ErrorResponse(w, SignatureDoesNotMatch, "signature verification failed: "+err.Error(), r.URL.Path, http.StatusForbidden) return true } token = t @@ -213,7 +240,7 @@ func (h *handler) serveS3(w http.ResponseWriter, r *http.Request) bool { _, kc, client, release, err := h.getClients(r.Header.Get("X-Request-Id"), token) if err != nil { - http.Error(w, "Pool failed: "+h.clientPool.Err().Error(), http.StatusInternalServerError) + s3ErrorResponse(w, InternalError, "Pool failed: "+h.clientPool.Err().Error(), r.URL.Path, http.StatusInternalServerError) return true } defer release() @@ -251,9 +278,9 @@ func (h *handler) serveS3(w http.ResponseWriter, r *http.Request) bool { if err == nil && fi.IsDir() { w.WriteHeader(http.StatusOK) } else if os.IsNotExist(err) { - w.WriteHeader(http.StatusNotFound) + s3ErrorResponse(w, NoSuchBucket, "The specified bucket does not exist.", r.URL.Path, http.StatusNotFound) } else { - http.Error(w, err.Error(), http.StatusBadGateway) + s3ErrorResponse(w, InternalError, err.Error(), r.URL.Path, http.StatusBadGateway) } return true } @@ -265,7 +292,7 @@ func (h *handler) serveS3(w http.ResponseWriter, r *http.Request) bool { if os.IsNotExist(err) || (err != nil && err.Error() == "not a directory") || (fi != nil && fi.IsDir()) { - http.Error(w, "not found", http.StatusNotFound) + s3ErrorResponse(w, NoSuchKey, "The specified key does not exist.", r.URL.Path, http.StatusNotFound) return true } // shallow copy r, and change URL path @@ -275,24 +302,24 @@ func (h *handler) serveS3(w http.ResponseWriter, r *http.Request) bool { return true case r.Method == http.MethodPut: if !objectNameGiven { - http.Error(w, "missing object name in PUT request", http.StatusBadRequest) + s3ErrorResponse(w, InvalidArgument, "Missing object name in PUT request.", r.URL.Path, http.StatusBadRequest) return true } var objectIsDir bool if strings.HasSuffix(fspath, "/") { if !h.Config.cluster.Collections.S3FolderObjects { - http.Error(w, "invalid object name: trailing slash", http.StatusBadRequest) + s3ErrorResponse(w, InvalidArgument, "invalid object name: trailing slash", r.URL.Path, http.StatusBadRequest) return true } n, err := r.Body.Read(make([]byte, 1)) if err != nil && err != io.EOF { - http.Error(w, fmt.Sprintf("error reading request body: %s", err), http.StatusInternalServerError) + s3ErrorResponse(w, InternalError, fmt.Sprintf("error reading request body: %s", err), r.URL.Path, http.StatusInternalServerError) return true } else if n > 0 { - http.Error(w, "cannot create object with trailing '/' char unless content is empty", http.StatusBadRequest) + s3ErrorResponse(w, InvalidArgument, "cannot create object with trailing '/' char unless content is empty", r.URL.Path, http.StatusBadRequest) return true } else if strings.SplitN(r.Header.Get("Content-Type"), ";", 2)[0] != "application/x-directory" { - http.Error(w, "cannot create object with trailing '/' char unless Content-Type is 'application/x-directory'", http.StatusBadRequest) + s3ErrorResponse(w, InvalidArgument, "cannot create object with trailing '/' char unless Content-Type is 'application/x-directory'", r.URL.Path, http.StatusBadRequest) return true } // Given PUT "foo/bar/", we'll use "foo/bar/." @@ -304,12 +331,12 @@ func (h *handler) serveS3(w http.ResponseWriter, r *http.Request) bool { fi, err := fs.Stat(fspath) if err != nil && err.Error() == "not a directory" { // requested foo/bar, but foo is a file - http.Error(w, "object name conflicts with existing object", http.StatusBadRequest) + s3ErrorResponse(w, InvalidArgument, "object name conflicts with existing object", r.URL.Path, http.StatusBadRequest) return true } if strings.HasSuffix(r.URL.Path, "/") && err == nil && !fi.IsDir() { // requested foo/bar/, but foo/bar is a file - http.Error(w, "object name conflicts with existing object", http.StatusBadRequest) + s3ErrorResponse(w, InvalidArgument, "object name conflicts with existing object", r.URL.Path, http.StatusBadRequest) return true } // create missing parent/intermediate directories, if any @@ -318,7 +345,7 @@ func (h *handler) serveS3(w http.ResponseWriter, r *http.Request) bool { dir := fspath[:i] if strings.HasSuffix(dir, "/") { err = errors.New("invalid object name (consecutive '/' chars)") - http.Error(w, err.Error(), http.StatusBadRequest) + s3ErrorResponse(w, InvalidArgument, err.Error(), r.URL.Path, http.StatusBadRequest) return true } err = fs.Mkdir(dir, 0755) @@ -326,11 +353,11 @@ func (h *handler) serveS3(w http.ResponseWriter, r *http.Request) bool { // Cannot create a directory // here. err = fmt.Errorf("mkdir %q failed: %w", dir, err) - http.Error(w, err.Error(), http.StatusBadRequest) + s3ErrorResponse(w, InvalidArgument, err.Error(), r.URL.Path, http.StatusBadRequest) return true } else if err != nil && !os.IsExist(err) { err = fmt.Errorf("mkdir %q failed: %w", dir, err) - http.Error(w, err.Error(), http.StatusInternalServerError) + s3ErrorResponse(w, InternalError, err.Error(), r.URL.Path, http.StatusInternalServerError) return true } } @@ -342,34 +369,34 @@ func (h *handler) serveS3(w http.ResponseWriter, r *http.Request) bool { } if err != nil { err = fmt.Errorf("open %q failed: %w", r.URL.Path, err) - http.Error(w, err.Error(), http.StatusBadRequest) + s3ErrorResponse(w, InvalidArgument, err.Error(), r.URL.Path, http.StatusBadRequest) return true } defer f.Close() _, err = io.Copy(f, r.Body) if err != nil { err = fmt.Errorf("write to %q failed: %w", r.URL.Path, err) - http.Error(w, err.Error(), http.StatusBadGateway) + s3ErrorResponse(w, InternalError, err.Error(), r.URL.Path, http.StatusBadGateway) return true } err = f.Close() if err != nil { err = fmt.Errorf("write to %q failed: close: %w", r.URL.Path, err) - http.Error(w, err.Error(), http.StatusBadGateway) + s3ErrorResponse(w, InternalError, err.Error(), r.URL.Path, http.StatusBadGateway) return true } } err = fs.Sync() if err != nil { err = fmt.Errorf("sync failed: %w", err) - http.Error(w, err.Error(), http.StatusInternalServerError) + s3ErrorResponse(w, InternalError, err.Error(), r.URL.Path, http.StatusInternalServerError) return true } w.WriteHeader(http.StatusOK) return true case r.Method == http.MethodDelete: if !objectNameGiven || r.URL.Path == "/" { - http.Error(w, "missing object name in DELETE request", http.StatusBadRequest) + s3ErrorResponse(w, InvalidArgument, "missing object name in DELETE request", r.URL.Path, http.StatusBadRequest) return true } if strings.HasSuffix(fspath, "/") { @@ -379,7 +406,7 @@ func (h *handler) serveS3(w http.ResponseWriter, r *http.Request) bool { w.WriteHeader(http.StatusNoContent) return true } else if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) + s3ErrorResponse(w, InternalError, err.Error(), r.URL.Path, http.StatusInternalServerError) return true } else if !fi.IsDir() { // if "foo" exists and is a file, then @@ -403,19 +430,20 @@ func (h *handler) serveS3(w http.ResponseWriter, r *http.Request) bool { } if err != nil { err = fmt.Errorf("rm failed: %w", err) - http.Error(w, err.Error(), http.StatusBadRequest) + s3ErrorResponse(w, InvalidArgument, err.Error(), r.URL.Path, http.StatusBadRequest) return true } err = fs.Sync() if err != nil { err = fmt.Errorf("sync failed: %w", err) - http.Error(w, err.Error(), http.StatusInternalServerError) + s3ErrorResponse(w, InternalError, err.Error(), r.URL.Path, http.StatusInternalServerError) return true } w.WriteHeader(http.StatusNoContent) return true default: - http.Error(w, "method not allowed", http.StatusMethodNotAllowed) + s3ErrorResponse(w, InvalidRequest, "method not allowed", r.URL.Path, http.StatusMethodNotAllowed) + return true } } diff --git a/services/keep-web/s3_test.go b/services/keep-web/s3_test.go index bff197dede..b9a6d85ecb 100644 --- a/services/keep-web/s3_test.go +++ b/services/keep-web/s3_test.go @@ -178,7 +178,9 @@ func (s *IntegrationSuite) testS3GetObject(c *check.C, bucket *s3.Bucket, prefix // GetObject rdr, err = bucket.GetReader(prefix + "missingfile") - c.Check(err, check.ErrorMatches, `404 Not Found`) + c.Check(err.(*s3.Error).StatusCode, check.Equals, 404) + c.Check(err.(*s3.Error).Code, check.Equals, `NoSuchKey`) + c.Check(err, check.ErrorMatches, `The specified key does not exist.`) // HeadObject exists, err := bucket.Exists(prefix + "missingfile") @@ -240,7 +242,9 @@ func (s *IntegrationSuite) testS3PutObjectSuccess(c *check.C, bucket *s3.Bucket, objname := prefix + trial.path _, err := bucket.GetReader(objname) - c.Assert(err, check.ErrorMatches, `404 Not Found`) + c.Check(err.(*s3.Error).StatusCode, check.Equals, 404) + c.Check(err.(*s3.Error).Code, check.Equals, `NoSuchKey`) + c.Assert(err, check.ErrorMatches, `The specified key does not exist.`) buf := make([]byte, trial.size) rand.Read(buf) @@ -289,16 +293,22 @@ func (s *IntegrationSuite) TestS3ProjectPutObjectNotSupported(c *check.C) { c.Logf("=== %v", trial) _, err := bucket.GetReader(trial.path) - c.Assert(err, check.ErrorMatches, `404 Not Found`) + c.Check(err.(*s3.Error).StatusCode, check.Equals, 404) + c.Check(err.(*s3.Error).Code, check.Equals, `NoSuchKey`) + c.Assert(err, check.ErrorMatches, `The specified key does not exist.`) buf := make([]byte, trial.size) rand.Read(buf) err = bucket.PutReader(trial.path, bytes.NewReader(buf), int64(len(buf)), trial.contentType, s3.Private, s3.Options{}) - c.Check(err, check.ErrorMatches, `400 Bad Request`) + c.Check(err.(*s3.Error).StatusCode, check.Equals, 400) + c.Check(err.(*s3.Error).Code, check.Equals, `InvalidArgument`) + c.Check(err, check.ErrorMatches, `(mkdir "by_id/zzzzz-j7d0g-[a-z0-9]{15}/newdir2?"|open "/zzzzz-j7d0g-[a-z0-9]{15}/newfile") failed: invalid argument`) _, err = bucket.GetReader(trial.path) - c.Assert(err, check.ErrorMatches, `404 Not Found`) + c.Check(err.(*s3.Error).StatusCode, check.Equals, 404) + c.Check(err.(*s3.Error).Code, check.Equals, `NoSuchKey`) + c.Assert(err, check.ErrorMatches, `The specified key does not exist.`) } } @@ -400,13 +410,15 @@ func (s *IntegrationSuite) testS3PutObjectFailure(c *check.C, bucket *s3.Bucket, rand.Read(buf) err := bucket.PutReader(objname, bytes.NewReader(buf), int64(len(buf)), "application/octet-stream", s3.Private, s3.Options{}) - if !c.Check(err, check.ErrorMatches, `400 Bad.*`, check.Commentf("PUT %q should fail", objname)) { + if !c.Check(err, check.ErrorMatches, `(invalid object name.*|open ".*" failed.*|object name conflicts with existing object|Missing object name in PUT request.)`, check.Commentf("PUT %q should fail", objname)) { return } if objname != "" && objname != "/" { _, err = bucket.GetReader(objname) - c.Check(err, check.ErrorMatches, `404 Not Found`, check.Commentf("GET %q should return 404", objname)) + c.Check(err.(*s3.Error).StatusCode, check.Equals, 404) + c.Check(err.(*s3.Error).Code, check.Equals, `NoSuchKey`) + c.Check(err, check.ErrorMatches, `The specified key does not exist.`, check.Commentf("GET %q should return 404", objname)) } }() } diff --git a/services/keep-web/server_test.go b/services/keep-web/server_test.go index 43817b51fc..0a1c7d1b3a 100644 --- a/services/keep-web/server_test.go +++ b/services/keep-web/server_test.go @@ -43,17 +43,17 @@ func (s *IntegrationSuite) TestNoToken(c *check.C) { } { hdr, body, _ := s.runCurl(c, token, "collections.example.com", "/collections/"+arvadostest.FooCollection+"/foo") c.Check(hdr, check.Matches, `(?s)HTTP/1.1 404 Not Found\r\n.*`) - c.Check(body, check.Equals, "") + c.Check(body, check.Equals, notFoundMessage+"\n") if token != "" { hdr, body, _ = s.runCurl(c, token, "collections.example.com", "/collections/download/"+arvadostest.FooCollection+"/"+token+"/foo") c.Check(hdr, check.Matches, `(?s)HTTP/1.1 404 Not Found\r\n.*`) - c.Check(body, check.Equals, "") + c.Check(body, check.Equals, notFoundMessage+"\n") } hdr, body, _ = s.runCurl(c, token, "collections.example.com", "/bad-route") c.Check(hdr, check.Matches, `(?s)HTTP/1.1 404 Not Found\r\n.*`) - c.Check(body, check.Equals, "") + c.Check(body, check.Equals, notFoundMessage+"\n") } } @@ -86,7 +86,7 @@ func (s *IntegrationSuite) Test404(c *check.C) { hdr, body, _ := s.runCurl(c, arvadostest.ActiveToken, "collections.example.com", uri) c.Check(hdr, check.Matches, "(?s)HTTP/1.1 404 Not Found\r\n.*") if len(body) > 0 { - c.Check(body, check.Equals, "404 page not found\n") + c.Check(body, check.Equals, notFoundMessage+"\n") } } }