From 54adcb1fc7544102dc7837775141c8643c3fc7ce Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Mon, 9 Aug 2021 15:35:13 +0200 Subject: [PATCH 1/3] Verify uploaded files Restic uses the sha256 hash to calculate filenames based on the file content. Check on the rest-server side that the uploaded file is intact and reject it otherwise. --- changelog/unreleased/issue-122 | 8 ++++ handlers_test.go | 76 +++++++++++++++++++++------------- repo/repo.go | 12 +++++- 3 files changed, 66 insertions(+), 30 deletions(-) create mode 100644 changelog/unreleased/issue-122 diff --git a/changelog/unreleased/issue-122 b/changelog/unreleased/issue-122 new file mode 100644 index 0000000..c98294e --- /dev/null +++ b/changelog/unreleased/issue-122 @@ -0,0 +1,8 @@ +Enhancement: Verify uploaded files + +rest-server now verifies that the hash of content of uploaded files matches +their filename. This ensures that transmission errors are detected and forces +restic to retry the upload. + +https://github.com/restic/rest-server/issues/122 +https://github.com/restic/rest-server/pull/130 diff --git a/handlers_test.go b/handlers_test.go index 46d4fed..b9edc2b 100644 --- a/handlers_test.go +++ b/handlers_test.go @@ -3,6 +3,7 @@ package restserver import ( "bytes" "crypto/rand" + "crypto/sha256" "encoding/hex" "fmt" "io" @@ -103,47 +104,58 @@ type TestRequest struct { // createOverwriteDeleteSeq returns a sequence which will create a new file at // path, and then try to overwrite and delete it. -func createOverwriteDeleteSeq(t testing.TB, path string) []TestRequest { +func createOverwriteDeleteSeq(t testing.TB, path string, data string) []TestRequest { // add a file, try to overwrite and delete it req := []TestRequest{ { req: newRequest(t, "GET", path, nil), want: []wantFunc{wantCode(http.StatusNotFound)}, }, - { - req: newRequest(t, "POST", path, strings.NewReader("foobar test config")), + } + + if !strings.HasSuffix(path, "/config") { + req = append(req, TestRequest{ + // broken upload must fail + req: newRequest(t, "POST", path, strings.NewReader(data+"broken")), + want: []wantFunc{wantCode(http.StatusBadRequest)}, + }) + } + + req = append(req, + TestRequest{ + req: newRequest(t, "POST", path, strings.NewReader(data)), want: []wantFunc{wantCode(http.StatusOK)}, }, - { + TestRequest{ req: newRequest(t, "GET", path, nil), want: []wantFunc{ wantCode(http.StatusOK), - wantBody("foobar test config"), + wantBody(data), }, }, - { - req: newRequest(t, "POST", path, strings.NewReader("other config")), + TestRequest{ + req: newRequest(t, "POST", path, strings.NewReader(data+"other stuff")), want: []wantFunc{wantCode(http.StatusForbidden)}, }, - { + TestRequest{ req: newRequest(t, "GET", path, nil), want: []wantFunc{ wantCode(http.StatusOK), - wantBody("foobar test config"), + wantBody(data), }, }, - { + TestRequest{ req: newRequest(t, "DELETE", path, nil), want: []wantFunc{wantCode(http.StatusForbidden)}, }, - { + TestRequest{ req: newRequest(t, "GET", path, nil), want: []wantFunc{ wantCode(http.StatusOK), - wantBody("foobar test config"), + wantBody(data), }, }, - } + ) return req } @@ -154,53 +166,59 @@ func TestResticHandler(t *testing.T) { if err != nil { t.Fatal(err) } - randomID := hex.EncodeToString(buf) + data := "random data file " + hex.EncodeToString(buf) + dataHash := sha256.Sum256([]byte(data)) + fileID := hex.EncodeToString(dataHash[:]) var tests = []struct { seq []TestRequest }{ - {createOverwriteDeleteSeq(t, "/config")}, - {createOverwriteDeleteSeq(t, "/data/"+randomID)}, + {createOverwriteDeleteSeq(t, "/config", data)}, + {createOverwriteDeleteSeq(t, "/data/"+fileID, data)}, { // ensure we can add and remove lock files []TestRequest{ { - req: newRequest(t, "GET", "/locks/"+randomID, nil), + req: newRequest(t, "GET", "/locks/"+fileID, nil), want: []wantFunc{wantCode(http.StatusNotFound)}, }, { - req: newRequest(t, "POST", "/locks/"+randomID, strings.NewReader("lock file")), + req: newRequest(t, "POST", "/locks/"+fileID, strings.NewReader(data+"broken")), + want: []wantFunc{wantCode(http.StatusBadRequest)}, + }, + { + req: newRequest(t, "POST", "/locks/"+fileID, strings.NewReader(data)), want: []wantFunc{wantCode(http.StatusOK)}, }, { - req: newRequest(t, "GET", "/locks/"+randomID, nil), + req: newRequest(t, "GET", "/locks/"+fileID, nil), want: []wantFunc{ wantCode(http.StatusOK), - wantBody("lock file"), + wantBody(data), }, }, { - req: newRequest(t, "POST", "/locks/"+randomID, strings.NewReader("other lock file")), + req: newRequest(t, "POST", "/locks/"+fileID, strings.NewReader(data+"other data")), want: []wantFunc{wantCode(http.StatusForbidden)}, }, { - req: newRequest(t, "DELETE", "/locks/"+randomID, nil), + req: newRequest(t, "DELETE", "/locks/"+fileID, nil), want: []wantFunc{wantCode(http.StatusOK)}, }, { - req: newRequest(t, "GET", "/locks/"+randomID, nil), + req: newRequest(t, "GET", "/locks/"+fileID, nil), want: []wantFunc{wantCode(http.StatusNotFound)}, }, }, }, // Test subrepos - {createOverwriteDeleteSeq(t, "/parent1/sub1/config")}, - {createOverwriteDeleteSeq(t, "/parent1/sub1/data/"+randomID)}, - {createOverwriteDeleteSeq(t, "/parent1/config")}, - {createOverwriteDeleteSeq(t, "/parent1/data/"+randomID)}, - {createOverwriteDeleteSeq(t, "/parent2/config")}, - {createOverwriteDeleteSeq(t, "/parent2/data/"+randomID)}, + {createOverwriteDeleteSeq(t, "/parent1/sub1/config", "foobar")}, + {createOverwriteDeleteSeq(t, "/parent1/sub1/data/"+fileID, data)}, + {createOverwriteDeleteSeq(t, "/parent1/config", "foobar")}, + {createOverwriteDeleteSeq(t, "/parent1/data/"+fileID, data)}, + {createOverwriteDeleteSeq(t, "/parent2/config", "foobar")}, + {createOverwriteDeleteSeq(t, "/parent2/data/"+fileID, data)}, } // setup rclone with a local backend in a temporary directory diff --git a/repo/repo.go b/repo/repo.go index adc5294..ee24ac6 100644 --- a/repo/repo.go +++ b/repo/repo.go @@ -1,6 +1,8 @@ package repo import ( + "crypto/sha256" + "encoding/hex" "encoding/json" "fmt" "io" @@ -569,7 +571,15 @@ func (h *Handler) saveBlob(w http.ResponseWriter, r *http.Request) { return } - written, err := io.Copy(outFile, r.Body) + // calculate hash for current request + hasher := sha256.New() + written, err := io.Copy(outFile, io.TeeReader(r.Body, hasher)) + + // reject if file content doesn't match file name + if err == nil && hex.EncodeToString(hasher.Sum(nil)) != objectID { + err = fmt.Errorf("file content does not match hash") + } + if err != nil { _ = tf.Close() _ = os.Remove(path) From 16889717c6ace14ae3b6d5534120b6da86260367 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Mon, 9 Aug 2021 15:40:50 +0200 Subject: [PATCH 2/3] Add option to disable integrity check on upload --- cmd/rest-server/main.go | 2 ++ handlers.go | 10 ++++++---- repo/repo.go | 28 ++++++++++++++++++---------- 3 files changed, 26 insertions(+), 14 deletions(-) diff --git a/cmd/rest-server/main.go b/cmd/rest-server/main.go index 686acd8..3a26df5 100644 --- a/cmd/rest-server/main.go +++ b/cmd/rest-server/main.go @@ -45,6 +45,8 @@ func init() { flags.StringVar(&server.TLSCert, "tls-cert", server.TLSCert, "TLS certificate path") flags.StringVar(&server.TLSKey, "tls-key", server.TLSKey, "TLS key path") flags.BoolVar(&server.NoAuth, "no-auth", server.NoAuth, "disable .htpasswd authentication") + flags.BoolVar(&server.NoVerifyUpload, "no-verify-upload", server.NoVerifyUpload, + "do not verify the integrity of uploaded data. DO NOT enable unless the rest-server runs on a very low-power device") flags.BoolVar(&server.AppendOnly, "append-only", server.AppendOnly, "enable append only mode") flags.BoolVar(&server.PrivateRepos, "private-repos", server.PrivateRepos, "users can only access their private repo") flags.BoolVar(&server.Prometheus, "prometheus", server.Prometheus, "enable Prometheus metrics") diff --git a/handlers.go b/handlers.go index 5e1b9fc..9df6adf 100644 --- a/handlers.go +++ b/handlers.go @@ -29,6 +29,7 @@ type Server struct { Debug bool MaxRepoSize int64 PanicOnError bool + NoVerifyUpload bool htpasswdFile *HtpasswdFile quotaManager *quota.Manager @@ -84,10 +85,11 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { // Pass the request to the repo.Handler opt := repo.Options{ - AppendOnly: s.AppendOnly, - Debug: s.Debug, - QuotaManager: s.quotaManager, // may be nil - PanicOnError: s.PanicOnError, + AppendOnly: s.AppendOnly, + Debug: s.Debug, + QuotaManager: s.quotaManager, // may be nil + PanicOnError: s.PanicOnError, + NoVerifyUpload: s.NoVerifyUpload, } if s.Prometheus { opt.BlobMetricFunc = makeBlobMetricFunc(username, folderPath) diff --git a/repo/repo.go b/repo/repo.go index ee24ac6..70e7693 100644 --- a/repo/repo.go +++ b/repo/repo.go @@ -21,10 +21,11 @@ import ( // Options are options for the Handler accepted by New type Options struct { - AppendOnly bool // if set, delete actions are not allowed - Debug bool - DirMode os.FileMode - FileMode os.FileMode + AppendOnly bool // if set, delete actions are not allowed + Debug bool + DirMode os.FileMode + FileMode os.FileMode + NoVerifyUpload bool // If set, we will panic when an internal server error happens. This // makes it easier to debug such errors. @@ -571,13 +572,20 @@ func (h *Handler) saveBlob(w http.ResponseWriter, r *http.Request) { return } - // calculate hash for current request - hasher := sha256.New() - written, err := io.Copy(outFile, io.TeeReader(r.Body, hasher)) + var written int64 - // reject if file content doesn't match file name - if err == nil && hex.EncodeToString(hasher.Sum(nil)) != objectID { - err = fmt.Errorf("file content does not match hash") + if h.opt.NoVerifyUpload { + // just write the file without checking the contents + written, err = io.Copy(outFile, r.Body) + } else { + // calculate hash for current request + hasher := sha256.New() + written, err = io.Copy(outFile, io.TeeReader(r.Body, hasher)) + + // reject if file content doesn't match file name + if err == nil && hex.EncodeToString(hasher.Sum(nil)) != objectID { + err = fmt.Errorf("file content does not match hash") + } } if err != nil { From 4c8a076976f142b3e808b8e9b681bb2b3f0b1a2b Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Mon, 9 Aug 2021 16:27:11 +0200 Subject: [PATCH 3/3] Reword changelog --- changelog/unreleased/issue-122 | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/changelog/unreleased/issue-122 b/changelog/unreleased/issue-122 index c98294e..7176c88 100644 --- a/changelog/unreleased/issue-122 +++ b/changelog/unreleased/issue-122 @@ -1,8 +1,9 @@ Enhancement: Verify uploaded files -rest-server now verifies that the hash of content of uploaded files matches -their filename. This ensures that transmission errors are detected and forces -restic to retry the upload. +The rest-server now by default verifies that the hash of content of uploaded +files matches their filename. This ensures that transmission errors are +detected and forces restic to retry the upload. On low-power devices it can +make sense to disable this check by passing the `--no-verify-upload` flag. https://github.com/restic/rest-server/issues/122 https://github.com/restic/rest-server/pull/130