From d2fdc0d87050e6dfe969e6d0ea8c04a12c5c4cb7 Mon Sep 17 00:00:00 2001 From: Alex Aperis Date: Mon, 17 Apr 2023 21:22:13 +0200 Subject: [PATCH 01/10] update readme --- dev_utils/README.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/dev_utils/README.md b/dev_utils/README.md index 12ba859..c1ae788 100644 --- a/dev_utils/README.md +++ b/dev_utils/README.md @@ -51,10 +51,9 @@ mc admin trace -v proxydev Run the go proxy from the root directory ```bash -export GO111MODULE=on export SERVER_CONFFILE=dev_utils/config.yaml -go build main.go -./main +go build . +./S3-Upload-Proxy ``` From 5e78e111cdbcc859caccf101404d5b362e51ca49 Mon Sep 17 00:00:00 2001 From: Alex Aperis Date: Mon, 17 Apr 2023 21:40:43 +0200 Subject: [PATCH 02/10] code cleanup --- proxy.go | 1 - 1 file changed, 1 deletion(-) diff --git a/proxy.go b/proxy.go index 327e701..57e5960 100644 --- a/proxy.go +++ b/proxy.go @@ -409,7 +409,6 @@ func (p *Proxy) requestInfo(fullPath string) (string, int64, error) { return "", 0, err } - fmt.Println(strings.ReplaceAll(*result.Contents[0].ETag, "\"", "")) return fmt.Sprintf("%x", sha256.Sum256([]byte(strings.ReplaceAll(*result.Contents[0].ETag, "\"", "")))), *result.Contents[0].Size, nil From 9e4326beb957e775b0acd992014bce2ec8b56d17 Mon Sep 17 00:00:00 2001 From: Alex Aperis Date: Mon, 17 Apr 2023 21:48:42 +0200 Subject: [PATCH 03/10] disallow improper filepath names - check for improper filename characters - make filepaths os compatible - upon error return status 406 --- helper/helper.go | 27 +++++++++++++++++++++++++++ proxy.go | 18 +++++++++++++++++- 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/helper/helper.go b/helper/helper.go index 579949b..bf3cc1d 100644 --- a/helper/helper.go +++ b/helper/helper.go @@ -8,11 +8,14 @@ import ( "crypto/x509" "crypto/x509/pkix" "encoding/pem" + "fmt" "log" "math/big" "net" "os" "path/filepath" + "regexp" + "strings" "time" "github.com/golang-jwt/jwt/v4" @@ -363,3 +366,27 @@ func TLScertToFile(filename string, derBytes []byte) error { return err } + +// FormatUploadFilePath ensures that path separators are "/", and returns error if the +// filepath contains a disallowed character matched with regex +func FormatUploadFilePath(filePath string) (string, error) { + + // Check for mixed "\" and "/" in filepath. Stop and throw an error if true so that + // we do not end up with unintended folder structure when applying ToSlash later on + if strings.Contains(filePath, "\\") && strings.Contains(filePath, "/") { + return filePath, fmt.Errorf("filepath contains mixed '\\' and '/' characters") + } + + // make path separators os compatible + outPath := filepath.ToSlash(filePath) + + // [\x00-\x1F\x7F] is the control character set + re := regexp.MustCompile(`[\\:\*\?"<>\|\x00-\x1F\x7F]`) + + dissallowedChars := re.FindAllString(outPath, -1) + if dissallowedChars != nil { + return outPath, fmt.Errorf("filepath contains disallowed characters: %+v", strings.Join(dissallowedChars, ", ")) + } + + return outPath, nil +} diff --git a/proxy.go b/proxy.go index 57e5960..bd6bccb 100644 --- a/proxy.go +++ b/proxy.go @@ -14,6 +14,7 @@ import ( "strings" "time" + "github.com/NBISweden/S3-Upload-Proxy/helper" common "github.com/neicnordic/sda-common/database" "github.com/aws/aws-sdk-go/aws" @@ -87,6 +88,12 @@ func (p *Proxy) notAllowedResponse(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(403) } +func (p *Proxy) notAcceptableResponse(w http.ResponseWriter, _ *http.Request) { + log.Debug("not acceptable response") + // http.Error(w, "request not acceptable", 406) + w.WriteHeader(406) +} + func (p *Proxy) notAuthorized(w http.ResponseWriter, _ *http.Request) { log.Debug("not authorized") w.WriteHeader(401) // Actually correct! @@ -105,7 +112,16 @@ func (p *Proxy) allowedResponse(w http.ResponseWriter, r *http.Request) { p.prependBucketToHostPath(r) username := fmt.Sprintf("%v", claims["sub"]) - filepath := strings.Replace(r.URL.Path, "/"+p.s3.bucket+"/", "", 1) + rawFilepath := strings.Replace(r.URL.Path, "/"+p.s3.bucket+"/", "", 1) + + filepath, err := helper.FormatUploadFilePath(rawFilepath) + if err != nil { + log.Debugf(err.Error()) + p.notAcceptableResponse(w, r) + + return + } + // register file in database if it's the start of an upload if p.detectRequestType(r) == Put && p.fileIds[r.URL.Path] == "" { log.Debugf("registering file %v in the database", r.URL.Path) From f883dec501acbb604e225593d6c28a3608524a82 Mon Sep 17 00:00:00 2001 From: Alex Aperis Date: Mon, 17 Apr 2023 21:53:42 +0200 Subject: [PATCH 04/10] add tests for formatUploadPath func --- helper/helper_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/helper/helper_test.go b/helper/helper_test.go index 358ffcb..c77a0d5 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -2,6 +2,8 @@ package helper import ( "os" + "path/filepath" + "runtime" "testing" "github.com/stretchr/testify/assert" @@ -70,3 +72,39 @@ func TestCreateECToken(t *testing.T) { defer os.RemoveAll("dummy-folder") } + +func TestFormatUploadFilePath(t *testing.T) { + + // This is non-trivial only on windows + unixPath := "a/b/c.c4gh" + testPath := filepath.Join("a", "b", "c.c4gh") + uploadPath, err := FormatUploadFilePath(testPath) + assert.NoError(t, err) + assert.Equal(t, unixPath, uploadPath) + + testPath = "a\\b\\c.c4gh" + uploadPath, err = FormatUploadFilePath(testPath) + if runtime.GOOS == "windows" { + // this is expected to work on windows + assert.Equal(t, unixPath, uploadPath) + assert.NoError(t, err) + } else { + // this is expected to fail on linux/mac + assert.ErrorContains(t, err, "filepath contains disallowed characters: \\, \\") + + } + + // mixed "\" and "/" + weirdPath := `dq\sw:*?"<>|\t\s/df.c4gh` + _, err = FormatUploadFilePath(weirdPath) + assert.EqualError(t, err, "filepath contains mixed '\\' and '/' characters") + + // no mixed "\" and "/" but not allowed + weirdPath = `dq\sw:*?"<>|\t\sdf.c4gh` + _, err = FormatUploadFilePath(weirdPath) + if runtime.GOOS == "windows" { + assert.EqualError(t, err, "filepath contains disallowed characters: :, *, ?, \", <, >, |") + } else { + assert.EqualError(t, err, "filepath contains disallowed characters: \\, :, *, ?, \", <, >, |, \\, \\") + } +} From d4952289623efc7a64785c377485c8c3d965bd4b Mon Sep 17 00:00:00 2001 From: Alex Aperis Date: Mon, 17 Apr 2023 21:54:37 +0200 Subject: [PATCH 05/10] add test for 406 error response --- proxy_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/proxy_test.go b/proxy_test.go index 71d6433..a2dc732 100644 --- a/proxy_test.go +++ b/proxy_test.go @@ -310,6 +310,16 @@ func (suite *ProxyTests) TestServeHTTP_allowed() { proxy.ServeHTTP(w, r) assert.Equal(suite.T(), 200, w.Result().StatusCode) assert.Equal(suite.T(), true, suite.fakeServer.PingedAndRestore()) + + // Filenames with platform incompatible characters are disallowed + // not checked in TestServeHTTP_allowed() because we look for a non 403 response + r.Method = "PUT" + r.URL, _ = url.Parse("/username/fi|le") + w = httptest.NewRecorder() + proxy.ServeHTTP(w, r) + assert.Equal(suite.T(), 406, w.Result().StatusCode) + assert.Equal(suite.T(), false, suite.fakeServer.PingedAndRestore()) + } func (suite *ProxyTests) TestMessageFormatting() { From 51f816385357b65ccc2625c80b935e8cdcb6ddb8 Mon Sep 17 00:00:00 2001 From: Alex Aperis Date: Tue, 18 Apr 2023 13:22:10 +0200 Subject: [PATCH 06/10] remove leftover code --- proxy.go | 1 - 1 file changed, 1 deletion(-) diff --git a/proxy.go b/proxy.go index bd6bccb..7c7e9d3 100644 --- a/proxy.go +++ b/proxy.go @@ -90,7 +90,6 @@ func (p *Proxy) notAllowedResponse(w http.ResponseWriter, _ *http.Request) { func (p *Proxy) notAcceptableResponse(w http.ResponseWriter, _ *http.Request) { log.Debug("not acceptable response") - // http.Error(w, "request not acceptable", 406) w.WriteHeader(406) } From b4a35feddbf8b83fd966e59f56b7ea3ea8e5dc4a Mon Sep 17 00:00:00 2001 From: Alex Aperis Date: Tue, 18 Apr 2023 20:23:40 +0200 Subject: [PATCH 07/10] fix backslash to slash substitution --- helper/helper.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/helper/helper.go b/helper/helper.go index bf3cc1d..2c2ad5f 100644 --- a/helper/helper.go +++ b/helper/helper.go @@ -372,13 +372,13 @@ func TLScertToFile(filename string, derBytes []byte) error { func FormatUploadFilePath(filePath string) (string, error) { // Check for mixed "\" and "/" in filepath. Stop and throw an error if true so that - // we do not end up with unintended folder structure when applying ToSlash later on + // we do not end up with unintended folder structure when applying ReplaceAll below if strings.Contains(filePath, "\\") && strings.Contains(filePath, "/") { return filePath, fmt.Errorf("filepath contains mixed '\\' and '/' characters") } - // make path separators os compatible - outPath := filepath.ToSlash(filePath) + // make any windows path separators linux compatible + outPath := strings.ReplaceAll(filePath, "\\", "/") // [\x00-\x1F\x7F] is the control character set re := regexp.MustCompile(`[\\:\*\?"<>\|\x00-\x1F\x7F]`) From fbc0e59187d0766db8421a12be68c594290b7b6a Mon Sep 17 00:00:00 2001 From: Alex Aperis Date: Tue, 18 Apr 2023 20:26:18 +0200 Subject: [PATCH 08/10] remove windows tests --- helper/helper_test.go | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-) diff --git a/helper/helper_test.go b/helper/helper_test.go index c77a0d5..94826d7 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -2,8 +2,6 @@ package helper import ( "os" - "path/filepath" - "runtime" "testing" "github.com/stretchr/testify/assert" @@ -75,25 +73,12 @@ func TestCreateECToken(t *testing.T) { func TestFormatUploadFilePath(t *testing.T) { - // This is non-trivial only on windows unixPath := "a/b/c.c4gh" - testPath := filepath.Join("a", "b", "c.c4gh") + testPath := "a\\b\\c.c4gh" uploadPath, err := FormatUploadFilePath(testPath) assert.NoError(t, err) assert.Equal(t, unixPath, uploadPath) - testPath = "a\\b\\c.c4gh" - uploadPath, err = FormatUploadFilePath(testPath) - if runtime.GOOS == "windows" { - // this is expected to work on windows - assert.Equal(t, unixPath, uploadPath) - assert.NoError(t, err) - } else { - // this is expected to fail on linux/mac - assert.ErrorContains(t, err, "filepath contains disallowed characters: \\, \\") - - } - // mixed "\" and "/" weirdPath := `dq\sw:*?"<>|\t\s/df.c4gh` _, err = FormatUploadFilePath(weirdPath) @@ -102,9 +87,6 @@ func TestFormatUploadFilePath(t *testing.T) { // no mixed "\" and "/" but not allowed weirdPath = `dq\sw:*?"<>|\t\sdf.c4gh` _, err = FormatUploadFilePath(weirdPath) - if runtime.GOOS == "windows" { - assert.EqualError(t, err, "filepath contains disallowed characters: :, *, ?, \", <, >, |") - } else { - assert.EqualError(t, err, "filepath contains disallowed characters: \\, :, *, ?, \", <, >, |, \\, \\") - } + assert.EqualError(t, err, "filepath contains disallowed characters: :, *, ?, \", <, >, |") + } From a50700bc123013b41417e6e9d440c2a30971b2b8 Mon Sep 17 00:00:00 2001 From: Alex Aperis Date: Thu, 20 Apr 2023 09:53:12 +0200 Subject: [PATCH 09/10] readable http error handling --- proxy.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/proxy.go b/proxy.go index 7c7e9d3..2036c41 100644 --- a/proxy.go +++ b/proxy.go @@ -78,24 +78,23 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) { } func (p *Proxy) internalServerError(w http.ResponseWriter, r *http.Request) { - log.Debug("internal server error") log.Debugf("Internal server error for request (%v)", r) - w.WriteHeader(500) + w.WriteHeader(http.StatusInternalServerError) } func (p *Proxy) notAllowedResponse(w http.ResponseWriter, _ *http.Request) { log.Debug("not allowed response") - w.WriteHeader(403) + w.WriteHeader(http.StatusForbidden) } func (p *Proxy) notAcceptableResponse(w http.ResponseWriter, _ *http.Request) { log.Debug("not acceptable response") - w.WriteHeader(406) + w.WriteHeader(http.StatusNotAcceptable) } func (p *Proxy) notAuthorized(w http.ResponseWriter, _ *http.Request) { log.Debug("not authorized") - w.WriteHeader(401) // Actually correct! + w.WriteHeader(http.StatusUnauthorized) } func (p *Proxy) allowedResponse(w http.ResponseWriter, r *http.Request) { From 714639428c2094d27f3eb5f59612dd9073156b49 Mon Sep 17 00:00:00 2001 From: Alex Aperis Date: Fri, 21 Apr 2023 07:37:55 +0200 Subject: [PATCH 10/10] remove excessive debug logging --- proxy.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/proxy.go b/proxy.go index 2036c41..83f1620 100644 --- a/proxy.go +++ b/proxy.go @@ -87,11 +87,6 @@ func (p *Proxy) notAllowedResponse(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusForbidden) } -func (p *Proxy) notAcceptableResponse(w http.ResponseWriter, _ *http.Request) { - log.Debug("not acceptable response") - w.WriteHeader(http.StatusNotAcceptable) -} - func (p *Proxy) notAuthorized(w http.ResponseWriter, _ *http.Request) { log.Debug("not authorized") w.WriteHeader(http.StatusUnauthorized) @@ -115,7 +110,7 @@ func (p *Proxy) allowedResponse(w http.ResponseWriter, r *http.Request) { filepath, err := helper.FormatUploadFilePath(rawFilepath) if err != nil { log.Debugf(err.Error()) - p.notAcceptableResponse(w, r) + w.WriteHeader(http.StatusNotAcceptable) return }