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>

1  2 
services/keep-web/s3.go
services/keep-web/s3_test.go
services/keep-web/server_test.go

diff --combined services/keep-web/s3.go
index 373fd9a25d56608d1c418da45221836b1f5cdb16,37fee37604ccd0280b93d142587852382d4e6fd1..7fb90789a55b164bd2465bf8d2454cf2d30e4f52
@@@ -16,7 -16,6 +16,7 @@@ import 
        "net/url"
        "os"
        "path/filepath"
 +      "regexp"
        "sort"
        "strconv"
        "strings"
@@@ -112,21 -111,6 +112,21 @@@ func s3signature(secretKey, scope, sign
        return hashdigest(hmac.New(sha256.New, key), stringToSign), nil
  }
  
 +var v2tokenUnderscore = regexp.MustCompile(`^v2_[a-z0-9]{5}-gj3su-[a-z0-9]{15}_`)
 +
 +func unescapeKey(key string) string {
 +      if v2tokenUnderscore.MatchString(key) {
 +              // Entire Arvados token, with "/" replaced by "_" to
 +              // avoid colliding with the Authorization header
 +              // format.
 +              return strings.Replace(key, "_", "/", -1)
 +      } else if s, err := url.PathUnescape(key); err == nil {
 +              return s
 +      } else {
 +              return key
 +      }
 +}
 +
  // checks3signature verifies the given S3 V4 signature and returns the
  // Arvados token that corresponds to the given accessKey. An error is
  // returned if accessKey is not a valid token UUID or the signature
@@@ -168,7 -152,7 +168,7 @@@ func (h *handler) checks3signature(r *h
        } else {
                // Access key and secret key are both an entire
                // Arvados token or OIDC access token.
 -              ctx := arvados.ContextWithAuthorization(r.Context(), "Bearer "+key)
 +              ctx := arvados.ContextWithAuthorization(r.Context(), "Bearer "+unescapeKey(key))
                err = client.RequestAndDecodeContext(ctx, &aca, "GET", "arvados/v1/api_client_authorizations/current", nil, nil)
                secret = key
        }
        } else if expect != signature {
                return "", fmt.Errorf("signature does not match (scope %q signedHeaders %q stringToSign %q)", scope, signedHeaders, stringToSign)
        }
 -      return secret, nil
 +      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 {
        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 = split[0]
 +              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
  
        _, 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()
        fs := client.SiteFileSystem(kc)
        fs.ForwardSlashNameSubstitution(h.Config.cluster.Collections.ForwardSlashNameSubstitution)
  
 -      objectNameGiven := strings.Count(strings.TrimSuffix(r.URL.Path, "/"), "/") > 1
 +      var objectNameGiven bool
 +      fspath := "/by_id"
 +      if id := parseCollectionIDFromDNSName(r.Host); id != "" {
 +              fspath += "/" + id
 +              objectNameGiven = strings.Count(strings.TrimSuffix(r.URL.Path, "/"), "/") > 0
 +      } else {
 +              objectNameGiven = strings.Count(strings.TrimSuffix(r.URL.Path, "/"), "/") > 1
 +      }
 +      fspath += r.URL.Path
  
        switch {
        case r.Method == http.MethodGet && !objectNameGiven:
                }
                return true
        case r.Method == http.MethodGet || r.Method == http.MethodHead:
 -              fspath := "/by_id" + r.URL.Path
                fi, err := fs.Stat(fspath)
                if r.Method == "HEAD" && !objectNameGiven {
                        // HeadBucket
                        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
                }
                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
                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
                }
 -              fspath := "by_id" + r.URL.Path
                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/."
                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
                                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)
                                        // 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
                                }
                        }
                        }
                        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
                }
 -              fspath := "by_id" + r.URL.Path
                if strings.HasSuffix(fspath, "/") {
                        fspath = strings.TrimSuffix(fspath, "/")
                        fi, err := fs.Stat(fspath)
                                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
                }
                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,fb759e901cac096a3d0a61ab753af19545706729..b9a6d85ecb1a6bacc75b2a9b94b3ebc54c851f7e
@@@ -10,7 -10,6 +10,7 @@@ import 
        "fmt"
        "io/ioutil"
        "net/http"
 +      "net/url"
        "os"
        "os/exec"
        "strings"
@@@ -119,15 -118,11 +119,15 @@@ func (s *IntegrationSuite) TestS3Signat
                secretkey string
        }{
                {true, aws.V2Signature, arvadostest.ActiveToken, "none"},
 +              {true, aws.V2Signature, url.QueryEscape(arvadostest.ActiveTokenV2), "none"},
 +              {true, aws.V2Signature, strings.Replace(arvadostest.ActiveTokenV2, "/", "_", -1), "none"},
                {false, aws.V2Signature, "none", "none"},
                {false, aws.V2Signature, "none", arvadostest.ActiveToken},
  
                {true, aws.V4Signature, arvadostest.ActiveTokenUUID, arvadostest.ActiveToken},
                {true, aws.V4Signature, arvadostest.ActiveToken, arvadostest.ActiveToken},
 +              {true, aws.V4Signature, url.QueryEscape(arvadostest.ActiveTokenV2), url.QueryEscape(arvadostest.ActiveTokenV2)},
 +              {true, aws.V4Signature, strings.Replace(arvadostest.ActiveTokenV2, "/", "_", -1), strings.Replace(arvadostest.ActiveTokenV2, "/", "_", -1)},
                {false, aws.V4Signature, arvadostest.ActiveToken, ""},
                {false, aws.V4Signature, arvadostest.ActiveToken, "none"},
                {false, aws.V4Signature, "none", arvadostest.ActiveToken},
@@@ -178,7 -173,9 +178,9 @@@ func (s *IntegrationSuite) testS3GetObj
  
        // 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 -237,9 +242,9 @@@ func (s *IntegrationSuite) testS3PutObj
                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 -288,22 +293,22 @@@ func (s *IntegrationSuite) TestS3Projec
                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 -405,15 +410,15 @@@ func (s *IntegrationSuite) testS3PutObj
                        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))
                        }
                }()
        }
@@@ -705,12 -712,3 +717,12 @@@ func (s *IntegrationSuite) TestS3cmd(c 
        c.Check(err, check.IsNil)
        c.Check(string(buf), check.Matches, `.* 3 +s3://`+arvadostest.FooCollection+`/foo\n`)
  }
 +
 +func (s *IntegrationSuite) TestS3BucketInHost(c *check.C) {
 +      stage := s.s3setup(c)
 +      defer stage.teardown(c)
 +
 +      hdr, body, _ := s.runCurl(c, "AWS "+arvadostest.ActiveTokenV2+":none", stage.coll.UUID+".collections.example.com", "/sailboat.txt")
 +      c.Check(hdr, check.Matches, `(?s)HTTP/1.1 200 OK\r\n.*`)
 +      c.Check(body, check.Equals, "⛵\n")
 +}
index 43817b51fcc78adaefe6525798b8ef400dbe2512,b15c33ea74b4f1593f621614bd159694f9571957..0a1c7d1b3a89a8338428cf25597ee27050633ccd
@@@ -43,17 -43,17 +43,17 @@@ func (s *IntegrationSuite) TestNoToken(
        } {
                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 +86,7 @@@ func (s *IntegrationSuite) Test404(c *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")
                }
        }
  }
@@@ -257,16 -257,12 +257,16 @@@ func (s *IntegrationSuite) Test200(c *c
  }
  
  // Return header block and body.
 -func (s *IntegrationSuite) runCurl(c *check.C, token, host, uri string, args ...string) (hdr, bodyPart string, bodySize int64) {
 +func (s *IntegrationSuite) runCurl(c *check.C, auth, host, uri string, args ...string) (hdr, bodyPart string, bodySize int64) {
        curlArgs := []string{"--silent", "--show-error", "--include"}
        testHost, testPort, _ := net.SplitHostPort(s.testServer.Addr)
        curlArgs = append(curlArgs, "--resolve", host+":"+testPort+":"+testHost)
 -      if token != "" {
 -              curlArgs = append(curlArgs, "-H", "Authorization: OAuth2 "+token)
 +      if strings.Contains(auth, " ") {
 +              // caller supplied entire Authorization header value
 +              curlArgs = append(curlArgs, "-H", "Authorization: "+auth)
 +      } else if auth != "" {
 +              // caller supplied Arvados token
 +              curlArgs = append(curlArgs, "-H", "Authorization: Bearer "+auth)
        }
        curlArgs = append(curlArgs, args...)
        curlArgs = append(curlArgs, "http://"+host+":"+testPort+uri)