Merge branch '16774-keep-web-errors' refs #16774
authorPeter Amstutz <peter.amstutz@curii.com>
Mon, 30 Nov 2020 16:03:53 +0000 (11:03 -0500)
committerPeter Amstutz <peter.amstutz@curii.com>
Mon, 30 Nov 2020 16:03:53 +0000 (11:03 -0500)
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz@curii.com>

services/keep-web/handler.go
services/keep-web/handler_test.go
services/keep-web/s3.go
services/keep-web/s3_test.go
services/keep-web/server_test.go

index 963948cc6bf53cbc86a2568bbc0286310dda5e4b..ab1bc080bd5928d5d03fe3d6764beb6bae167dce 100644 (file)
@@ -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] {
index f6f3de8877fa14abca9fc479216f13d4cb85a308..8e2e05c76184823d0a550bce92b93473879ec254 100644 (file)
@@ -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",
        )
 }
 
index 373fd9a25d56608d1c418da45221836b1f5cdb16..7fb90789a55b164bd2465bf8d2454cf2d30e4f52 100644 (file)
@@ -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
        }
 }
index bff197dede52e90c117e2d1e830841f4cea01d65..b9a6d85ecb1a6bacc75b2a9b94b3ebc54c851f7e 100644 (file)
@@ -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))
                        }
                }()
        }
index 43817b51fcc78adaefe6525798b8ef400dbe2512..0a1c7d1b3a89a8338428cf25597ee27050633ccd 100644 (file)
@@ -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")
                }
        }
 }