From a0c3593e8f5d6e7157dccb30924e422dd0e69fd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Thu, 14 Jan 2021 15:39:52 +0000 Subject: [PATCH 01/12] initial checksum support for ocis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- internal/http/services/owncloud/ocdav/get.go | 9 ++-- internal/http/services/owncloud/ocdav/head.go | 2 + .../http/services/owncloud/ocdav/propfind.go | 11 ++-- internal/http/services/owncloud/ocdav/put.go | 3 ++ internal/http/services/owncloud/ocdav/tus.go | 4 ++ pkg/storage/fs/ocis/node.go | 39 ++++++++++---- pkg/storage/fs/ocis/ocis.go | 6 +-- pkg/storage/fs/ocis/upload.go | 53 ++++++++++++++++++- 8 files changed, 104 insertions(+), 23 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/get.go b/internal/http/services/owncloud/ocdav/get.go index 5e892e8337..15972022c5 100644 --- a/internal/http/services/owncloud/ocdav/get.go +++ b/internal/http/services/owncloud/ocdav/get.go @@ -19,6 +19,7 @@ package ocdav import ( + "fmt" "io" "net/http" "path" @@ -144,11 +145,9 @@ func (s *svc) handleGet(w http.ResponseWriter, r *http.Request, ns string) { } else { w.Header().Set("Content-Length", strconv.FormatUint(info.Size, 10)) } - /* - if md.Checksum != "" { - w.Header().Set("OC-Checksum", md.Checksum) - } - */ + if info.Checksum != nil { + w.Header().Set("OC-Checksum", fmt.Sprintf("%s:%s", info.Checksum.Type, info.Checksum.Sum)) + } var c int64 if c, err = io.Copy(w, httpRes.Body); err != nil { sublog.Error().Err(err).Msg("error finishing copying data to response") diff --git a/internal/http/services/owncloud/ocdav/head.go b/internal/http/services/owncloud/ocdav/head.go index 13d4f1fb02..57b2ab34b5 100644 --- a/internal/http/services/owncloud/ocdav/head.go +++ b/internal/http/services/owncloud/ocdav/head.go @@ -19,6 +19,7 @@ package ocdav import ( + "fmt" "net/http" "path" "strconv" @@ -68,6 +69,7 @@ func (s *svc) handleHead(w http.ResponseWriter, r *http.Request, ns string) { w.Header().Set("ETag", info.Etag) w.Header().Set("OC-FileId", wrapResourceID(info.Id)) w.Header().Set("OC-ETag", info.Etag) + w.Header().Set("OC-Checksum", fmt.Sprintf("%s:%s", info.Checksum.Type, info.Checksum.Sum)) t := utils.TSToTime(info.Mtime).UTC() lastModifiedString := t.Format(time.RFC1123Z) w.Header().Set("Last-Modified", lastModifiedString) diff --git a/internal/http/services/owncloud/ocdav/propfind.go b/internal/http/services/owncloud/ocdav/propfind.go index c56bb11bfb..14ce2c0a60 100644 --- a/internal/http/services/owncloud/ocdav/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind.go @@ -38,6 +38,7 @@ import ( rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" link "github.com/cs3org/go-cs3apis/cs3/sharing/link/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/cs3org/reva/internal/grpc/services/storageprovider" "github.com/cs3org/reva/internal/http/services/owncloud/ocs/conversions" "github.com/cs3org/reva/pkg/appctx" ctxuser "github.com/cs3org/reva/pkg/user" @@ -207,7 +208,7 @@ func requiresExplicitFetching(n *xml.Name) bool { return false case _nsOwncloud: switch n.Local { - case "favorite", "share-types": + case "favorite", "share-types", "checksums": return true default: return false @@ -389,8 +390,8 @@ func (s *svc) mdToPropResponse(ctx context.Context, pf *propfindXML, md *provide // // SHA1:9bd253a09d58be107bcb4169ebf338c8df34d086 MD5:d90bcc6bf847403d22a4abba64e79994 ADLER32:fca23ff5 // - // yep, correct, space delimited key value pairs inside an oc:checksum tag inside an oc:checksums tag - value := fmt.Sprintf("%s:%s", md.Checksum.Type, md.Checksum.Sum) + // yep, correct, space delimited key value pairs inside an oc:checksum tag inside an oc:checksums tag 🤦 ... legacy reasons ... 😭 + value := fmt.Sprintf("%s:%s", strings.ToUpper(string(storageprovider.GRPC2PKGXS(md.Checksum.Type))), md.Checksum.Sum) response.Propstat[0].Prop = append(response.Propstat[0].Prop, s.newProp("oc:checksums", value)) } @@ -533,8 +534,8 @@ func (s *svc) mdToPropResponse(ctx context.Context, pf *propfindXML, md *provide // // SHA1:9bd253a09d58be107bcb4169ebf338c8df34d086 MD5:d90bcc6bf847403d22a4abba64e79994 ADLER32:fca23ff5 // - // yep, correct, space delimited key value pairs inside an oc:checksum tag inside an oc:checksums tag - value := fmt.Sprintf("%s:%s", md.Checksum.Type, md.Checksum.Sum) + // yep, correct, space delimited key value pairs inside an oc:checksum tag inside an oc:checksums tag 🤦 ... legacy reasons ... 😭 + value := fmt.Sprintf("%s:%s", strings.ToUpper(string(storageprovider.GRPC2PKGXS(md.Checksum.Type))), md.Checksum.Sum) propstatOK.Prop = append(propstatOK.Prop, s.newProp("oc:checksums", value)) } else { propstatNotFound.Prop = append(propstatNotFound.Prop, s.newProp("oc:checksums", "")) diff --git a/internal/http/services/owncloud/ocdav/put.go b/internal/http/services/owncloud/ocdav/put.go index 77ac6e8859..ae67fa2e6f 100644 --- a/internal/http/services/owncloud/ocdav/put.go +++ b/internal/http/services/owncloud/ocdav/put.go @@ -203,6 +203,9 @@ func (s *svc) handlePutHelper(w http.ResponseWriter, r *http.Request, content io // TODO: find a way to check if the storage really accepted the value w.Header().Set("X-OC-Mtime", "accepted") } + // r.Header.Get("OC-Checksum") + // TODO must be SHA1, ADLER32 or MD5 ... in capital letters???? + // curl -X PUT https://demo.owncloud.com/remote.php/webdav/testcs.bin -u demo:demo -d '123' -v -H 'OC-Checksum: SHA1:40bd001563085fc35165329ea1ff5c5ecbdbbeef' uReq := &provider.InitiateFileUploadRequest{ Ref: ref, diff --git a/internal/http/services/owncloud/ocdav/tus.go b/internal/http/services/owncloud/ocdav/tus.go index d173cb752a..2e25085866 100644 --- a/internal/http/services/owncloud/ocdav/tus.go +++ b/internal/http/services/owncloud/ocdav/tus.go @@ -56,6 +56,10 @@ func (s *svc) handleTusPost(w http.ResponseWriter, r *http.Request, ns string) { w.WriteHeader(http.StatusPreconditionFailed) return } + // r.Header.Get("OC-Checksum") + // TODO must be SHA1, ADLER32 or MD5 ... in capital letters???? + // curl -X PUT https://demo.owncloud.com/remote.php/webdav/testcs.bin -u demo:demo -d '123' -v -H 'OC-Checksum: SHA1:40bd001563085fc35165329ea1ff5c5ecbdbbeef' + //TODO check Expect: 100-continue // read filename from metadata diff --git a/pkg/storage/fs/ocis/node.go b/pkg/storage/fs/ocis/node.go index c2ab5537e7..6527ecbbeb 100644 --- a/pkg/storage/fs/ocis/node.go +++ b/pkg/storage/fs/ocis/node.go @@ -21,6 +21,7 @@ package ocis import ( "context" "crypto/md5" + "encoding/hex" "fmt" "io" "os" @@ -31,6 +32,7 @@ import ( userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" types "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" + "github.com/cs3org/reva/internal/grpc/services/storageprovider" "github.com/cs3org/reva/pkg/appctx" "github.com/cs3org/reva/pkg/errtypes" "github.com/cs3org/reva/pkg/mime" @@ -45,7 +47,8 @@ const ( _shareTypesKey = "http://owncloud.org/ns/share-types" _userShareType = "0" - _favoriteKey = "http://owncloud.org/ns/favorite" + _favoriteKey = "http://owncloud.org/ns/favorite" + _checksumsKey = "http://owncloud.org/ns/checksums" ) // Node represents a node in the tree and provides methods to get a Parent or Child instance @@ -342,7 +345,7 @@ func calculateEtag(nodeID string, tmTime time.Time) (string, error) { } else { return "", err } - return fmt.Sprintf(`"%x"`, h.Sum(nil)), nil + return `"` + hex.EncodeToString(h.Sum(nil)) + `"`, nil } // SetMtime sets the mtime and atime of a node @@ -481,7 +484,7 @@ func (n *Node) AsResourceInfo(ctx context.Context, rp *provider.ResourcePermissi // use temporary etag if it is set if b, err := xattr.Get(nodePath, tmpEtagAttr); err == nil { - ri.Etag = fmt.Sprintf(`"%x"`, string(b)) + ri.Etag = fmt.Sprintf(`"%x"`, string(b)) // TODO why do we convert string(b)? is the temporary etag stored as string? -> should we use bytes? use hex.EncodeToString? } else if ri.Etag, err = calculateEtag(n.ID, tmTime); err != nil { sublog.Debug().Err(err).Msg("could not calculate etag") } @@ -535,24 +538,37 @@ func (n *Node) AsResourceInfo(ctx context.Context, rp *provider.ResourcePermissi } } + // checksums + if _, ok := mdKeysMap[_checksumsKey]; returnAllKeys || ok { + // TODO which checksum was requested? sha1 adler32 or md5? for now hardcode sha1? + if v, err := xattr.Get(nodePath, checksumPrefix+"sha1"); err == nil { + ri.Checksum = &provider.ResourceChecksum{ + Type: storageprovider.PKG2GRPCXS("sha1"), + Sum: hex.EncodeToString(v), + } + } else { + sublog.Error().Err(err).Str("cstype", "sha1").Msg("could not get checksum value") + } + } + // only read the requested metadata attributes - list, err := xattr.List(nodePath) + attrs, err := xattr.List(nodePath) if err != nil { sublog.Error().Err(err).Msg("error getting list of extended attributes") } else { - for _, entry := range list { + for i := range attrs { // filter out non-custom properties - if !strings.HasPrefix(entry, metadataPrefix) { + if !strings.HasPrefix(attrs[i], metadataPrefix) { continue } // only read when key was requested - k := entry[len(metadataPrefix):] + k := attrs[i][len(metadataPrefix):] if _, ok := mdKeysMap[k]; returnAllKeys || ok { - if val, err := xattr.Get(nodePath, entry); err == nil { + if val, err := xattr.Get(nodePath, attrs[i]); err == nil { metadata[k] = string(val) } else { sublog.Error().Err(err). - Str("entry", entry). + Str("entry", attrs[i]). Msg("error retrieving xattr metadata") } } @@ -592,6 +608,11 @@ func (n *Node) SetTMTime(t time.Time) (err error) { return xattr.Set(n.lu.toInternalPath(n.ID), treeMTimeAttr, []byte(t.UTC().Format(time.RFC3339Nano))) } +// SetChecksum writes the checksum with the given checksum type to the extended attributes +func (n *Node) SetChecksum(csType string, bytes []byte) (err error) { + return xattr.Set(n.lu.toInternalPath(n.ID), checksumPrefix+csType, bytes) +} + // UnsetTempEtag removes the temporary etag attribute func (n *Node) UnsetTempEtag() (err error) { if err = xattr.Remove(n.lu.toInternalPath(n.ID), tmpEtagAttr); err != nil { diff --git a/pkg/storage/fs/ocis/ocis.go b/pkg/storage/fs/ocis/ocis.go index 0d17aebf66..9ad76e76e4 100644 --- a/pkg/storage/fs/ocis/ocis.go +++ b/pkg/storage/fs/ocis/ocis.go @@ -67,9 +67,9 @@ const ( favPrefix string = ocisPrefix + "fav." // a temporary etag for a folder that is removed when the mtime propagation happens - tmpEtagAttr string = ocisPrefix + "tmp.etag" - referenceAttr string = ocisPrefix + "cs3.ref" // arbitrary metadata - //checksumPrefix string = ocisPrefix + "cs." // TODO add checksum support + tmpEtagAttr string = ocisPrefix + "tmp.etag" + referenceAttr string = ocisPrefix + "cs3.ref" // arbitrary metadata + checksumPrefix string = ocisPrefix + "cs." // checksum support trashOriginAttr string = ocisPrefix + "trash.origin" // trash origin // we use a single attribute to enable or disable propagation of both: synctime and treesize diff --git a/pkg/storage/fs/ocis/upload.go b/pkg/storage/fs/ocis/upload.go index d79093ab0a..1875c3e8fb 100644 --- a/pkg/storage/fs/ocis/upload.go +++ b/pkg/storage/fs/ocis/upload.go @@ -20,7 +20,10 @@ package ocis import ( "context" + "crypto/md5" + "crypto/sha1" "encoding/json" + "hash/adler32" "io" "io/ioutil" "os" @@ -61,7 +64,7 @@ func (fs *ocisfs) Upload(ctx context.Context, ref *provider.Reference, r io.Read uploadInfo := upload.(*fileUpload) p := uploadInfo.info.Storage["NodeName"] - ok, err := chunking.IsChunked(p) + ok, err := chunking.IsChunked(p) // check chunking v1 if err != nil { return errors.Wrap(err, "ocisfs: error checking path") } @@ -130,6 +133,7 @@ func (fs *ocisfs) InitiateUpload(ctx context.Context, ref *provider.Reference, u info.SizeIsDeferred = true } } + // checksum? log.Debug().Interface("info", info).Interface("node", n).Interface("metadata", metadata).Msg("ocisfs: resolved filename") @@ -428,6 +432,53 @@ func (upload *fileUpload) FinishUpload(ctx context.Context) (err error) { return } + // TODO the checksum could be calculated in a goroutine. Leaving in sync until we have a way to implement async upload handling + { + f, err := os.Open(targetPath) + if err != nil { + log.Err(err).Interface("info", upload.info). + Str("binPath", upload.binPath). + Str("targetPath", targetPath). + Msg("ocisfs: could not open file for checksumming") + } + defer f.Close() + + sha1h := sha1.New() + r1 := io.TeeReader(f, sha1h) + md5h := md5.New() + r2 := io.TeeReader(r1, md5h) + adler32h := adler32.New() + + if _, err := io.Copy(adler32h, r2); err != nil { + log.Err(err).Interface("info", upload.info). + Str("binPath", upload.binPath). + Str("targetPath", targetPath). + Msg("ocisfs: could not open file for checksumming") + } + + if err := n.SetChecksum("sha1", sha1h.Sum(nil)); err != nil { + log.Err(err).Interface("info", upload.info). + Str("binPath", upload.binPath). + Str("targetPath", targetPath). + Str("csType", "sha1"). + Msg("ocisfs: could not write checksum") + } + if err := n.SetChecksum("md5", md5h.Sum(nil)); err != nil { + log.Err(err).Interface("info", upload.info). + Str("binPath", upload.binPath). + Str("targetPath", targetPath). + Str("csType", "md5"). + Msg("ocisfs: could not write checksum") + } + if err := n.SetChecksum("adler32", adler32h.Sum(nil)); err != nil { + log.Err(err).Interface("info", upload.info). + Str("binPath", upload.binPath). + Str("targetPath", targetPath). + Str("csType", "adler32"). + Msg("ocisfs: could not write checksum") + } + } + // who will become the owner? the owner of the parent actually ... not the currently logged in user err = n.writeMetadata(&userpb.UserId{ Idp: upload.info.Storage["OwnerIdp"], From 3946b20cf4484f01a3756b1319de5631ccbedcc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Fri, 15 Jan 2021 13:44:25 +0000 Subject: [PATCH 02/12] check checksum header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../storageprovider/storageprovider.go | 16 ++ internal/http/services/owncloud/ocdav/put.go | 48 ++++- pkg/errtypes/errtypes.go | 19 ++ pkg/rhttp/datatx/manager/simple/simple.go | 26 +-- pkg/storage/fs/ocis/upload.go | 167 ++++++++++++------ 5 files changed, 205 insertions(+), 71 deletions(-) diff --git a/internal/grpc/services/storageprovider/storageprovider.go b/internal/grpc/services/storageprovider/storageprovider.go index fc2eadbc85..6c94cfc417 100644 --- a/internal/grpc/services/storageprovider/storageprovider.go +++ b/internal/grpc/services/storageprovider/storageprovider.go @@ -315,6 +315,11 @@ func (s *service) InitiateFileUpload(ctx context.Context, req *provider.Initiate }, nil } } + // TUS forward Upload-Checksum header as checksum, uses '[type] [hash]' format + if req.Opaque.Map["Upload-Checksum"] != nil { + metadata["checksum"] = string(req.Opaque.Map["Upload-Checksum"].Value) + } + // ownCloud mtime to set for the uploaded file if req.Opaque.Map["X-OC-Mtime"] != nil { metadata["mtime"] = string(req.Opaque.Map["X-OC-Mtime"].Value) } @@ -325,6 +330,17 @@ func (s *service) InitiateFileUpload(ctx context.Context, req *provider.Initiate switch err.(type) { case errtypes.IsNotFound: st = status.NewNotFound(ctx, "path not found when initiating upload") + case errtypes.IsBadRequest, errtypes.IsChecksumMismatch: + st = status.NewInvalidArg(ctx, err.Error()) + // TODO TUS uses a custom ChecksumMismatch 460 http status which is in an unnasigned range in + // https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml + // maybe 409 conflict is good enough + // someone is proposing `419 Checksum Error`, see https://stackoverflow.com/a/35665694 + // - it is also unassigned + // - ends in 9 as the 409 conflict + // - is near the 4xx errors about conditions: 415 Unsupported Media Type, 416 Range Not Satisfiable or 417 Expectation Failed + // owncloud only expects a 400 Bad request so InvalidArg is good enough for now + // seealso errtypes.StatusChecksumMismatch case errtypes.PermissionDenied: st = status.NewPermissionDenied(ctx, err, "permission denied") default: diff --git a/internal/http/services/owncloud/ocdav/put.go b/internal/http/services/owncloud/ocdav/put.go index ae67fa2e6f..5f87323ed3 100644 --- a/internal/http/services/owncloud/ocdav/put.go +++ b/internal/http/services/owncloud/ocdav/put.go @@ -23,6 +23,7 @@ import ( "net/http" "path" "strconv" + "strings" "time" rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" @@ -30,6 +31,7 @@ import ( typespb "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" "github.com/cs3org/reva/internal/http/services/datagateway" "github.com/cs3org/reva/pkg/appctx" + "github.com/cs3org/reva/pkg/errtypes" "github.com/cs3org/reva/pkg/rhttp" "github.com/cs3org/reva/pkg/storage/utils/chunking" "github.com/cs3org/reva/pkg/utils" @@ -203,10 +205,37 @@ func (s *svc) handlePutHelper(w http.ResponseWriter, r *http.Request, content io // TODO: find a way to check if the storage really accepted the value w.Header().Set("X-OC-Mtime", "accepted") } - // r.Header.Get("OC-Checksum") - // TODO must be SHA1, ADLER32 or MD5 ... in capital letters???? + // curl -X PUT https://demo.owncloud.com/remote.php/webdav/testcs.bin -u demo:demo -d '123' -v -H 'OC-Checksum: SHA1:40bd001563085fc35165329ea1ff5c5ecbdbbeef' + var cparts []string + // TUS Upload-Checksum header takes precedence + if checksum := r.Header.Get("Upload-Checksum"); checksum != "" { + cparts = strings.SplitN(checksum, " ", 2) + if len(cparts) != 2 { + sublog.Debug().Str("upload-checksum", checksum).Msg("invalid Upload-Checksum format, expected '[algorithm] [checksum]'") + w.WriteHeader(http.StatusBadRequest) + return + } + // Then try owncloud header + } else if checksum := r.Header.Get("OC-Checksum"); checksum != "" { + cparts = strings.SplitN(checksum, ":", 2) + if len(cparts) != 2 { + sublog.Debug().Str("oc-checksum", checksum).Msg("invalid OC-Checksum format, expected '[algorithm]:[checksum]'") + w.WriteHeader(http.StatusBadRequest) + return + } + } + // we do not check the algorithm here, because it might depend on the storage + if len(cparts) == 2 { + // Translate into TUS style Upload-Checksum header + opaqueMap["Upload-Checksum"] = &typespb.OpaqueEntry{ + Decoder: "plain", + // algorithm is always lowercase, checksum is separated by space + Value: []byte(strings.ToLower(cparts[0]) + " " + cparts[1]), + } + } + uReq := &provider.InitiateFileUploadRequest{ Ref: ref, Opaque: &typespb.Opaque{Map: opaqueMap}, @@ -252,8 +281,21 @@ func (s *svc) handlePutHelper(w http.ResponseWriter, r *http.Request, content io w.WriteHeader(http.StatusPartialContent) return } + if httpRes.StatusCode == errtypes.StatusChecksumMismatch { + w.WriteHeader(http.StatusBadRequest) + xml := ` + + Sabre\DAV\Exception\BadRequest + The computed checksum does not match the one received from the client. +` + _, err := w.Write([]byte(xml)) + if err != nil { + sublog.Err(err).Msg("error writing response") + } + return + } sublog.Error().Err(err).Msg("PUT request to data server failed") - w.WriteHeader(http.StatusInternalServerError) + w.WriteHeader(httpRes.StatusCode) return } } diff --git a/pkg/errtypes/errtypes.go b/pkg/errtypes/errtypes.go index aa50e7c7d6..68c4d68414 100644 --- a/pkg/errtypes/errtypes.go +++ b/pkg/errtypes/errtypes.go @@ -94,6 +94,19 @@ func (e BadRequest) Error() string { return "error: bad request: " + string(e) } // IsBadRequest implements the IsBadRequest interface. func (e BadRequest) IsBadRequest() {} +// ChecksumMismatch is the error to use when the sent hash does not match the calculated hash. +type ChecksumMismatch string + +func (e ChecksumMismatch) Error() string { return "error: checksum mismatch: " + string(e) } + +// IsChecksumMismatch implements the IsChecksumMismatch interface. +func (e ChecksumMismatch) IsChecksumMismatch() {} + +// StatusChecksumMismatch 419 is an inofficial http status code in an unassigned range that is used for checksum mismatches +// TUS uses unassigned 460 Checksum-Mismatch +// see https://stackoverflow.com/a/35665694 and the storageprovider +const StatusChecksumMismatch = 419 + // IsNotFound is the interface to implement // to specify that an a resource is not found. type IsNotFound interface { @@ -147,3 +160,9 @@ type IsPartialContent interface { type IsBadRequest interface { IsBadRequest() } + +// IsChecksumMismatch is the interface to implement +// to specify that a checksum does not match. +type IsChecksumMismatch interface { + IsChecksumMismatch() +} diff --git a/pkg/rhttp/datatx/manager/simple/simple.go b/pkg/rhttp/datatx/manager/simple/simple.go index 7de66a7ee9..8f3cddd392 100644 --- a/pkg/rhttp/datatx/manager/simple/simple.go +++ b/pkg/rhttp/datatx/manager/simple/simple.go @@ -76,18 +76,24 @@ func (m *manager) Handler(fs storage.FS) (http.Handler, error) { ref := &provider.Reference{Spec: &provider.Reference_Path{Path: fn}} err := fs.Upload(ctx, ref, r.Body) - if err != nil { - if _, ok := err.(errtypes.IsPartialContent); ok { - w.WriteHeader(http.StatusPartialContent) - return - } - sublog.Error().Err(err).Msg("error uploading file") + switch v := err.(type) { + case nil: + w.WriteHeader(http.StatusOK) + case errtypes.PartialContent: + w.WriteHeader(http.StatusPartialContent) + case errtypes.ChecksumMismatch: + w.WriteHeader(errtypes.StatusChecksumMismatch) + case errtypes.NotFound: + w.WriteHeader(http.StatusNotFound) + case errtypes.PermissionDenied: + w.WriteHeader(http.StatusForbidden) + case errtypes.InvalidCredentials: + w.WriteHeader(http.StatusUnauthorized) + default: + sublog.Error().Err(v).Msg("error uploading file") w.WriteHeader(http.StatusInternalServerError) - return } - - w.WriteHeader(http.StatusOK) - + return default: w.WriteHeader(http.StatusNotImplemented) } diff --git a/pkg/storage/fs/ocis/upload.go b/pkg/storage/fs/ocis/upload.go index 1875c3e8fb..c8c727a79e 100644 --- a/pkg/storage/fs/ocis/upload.go +++ b/pkg/storage/fs/ocis/upload.go @@ -22,12 +22,15 @@ import ( "context" "crypto/md5" "crypto/sha1" + "encoding/hex" "encoding/json" + "fmt" "hash/adler32" "io" "io/ioutil" "os" "path/filepath" + "strings" "time" userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" @@ -44,6 +47,8 @@ import ( var defaultFilePerm = os.FileMode(0664) +// TODO Upload (and InitiateUpload) needs a way to receive the expected checksum. +// Maybe in metadata as 'checksum' => 'sha1 aeosvp45w5xaeoe' = lowercase, space separated? func (fs *ocisfs) Upload(ctx context.Context, ref *provider.Reference, r io.ReadCloser) (err error) { upload, err := fs.GetUpload(ctx, ref.GetPath()) if err != nil { @@ -99,6 +104,7 @@ func (fs *ocisfs) Upload(ctx context.Context, ref *provider.Reference, r io.Read // InitiateUpload returns upload ids corresponding to different protocols it supports // TODO read optional content for small files in this request +// TODO InitiateUpload (and Upload) needs a way to receive the expected checksum. Maybe in metadata as 'checksum' => 'sha1 aeosvp45w5xaeoe' = lowercase, space separated? func (fs *ocisfs) InitiateUpload(ctx context.Context, ref *provider.Reference, uploadLength int64, metadata map[string]string) (map[string]string, error) { log := appctx.GetLogger(ctx) @@ -132,8 +138,19 @@ func (fs *ocisfs) InitiateUpload(ctx context.Context, ref *provider.Reference, u if _, ok := metadata["sizedeferred"]; ok { info.SizeIsDeferred = true } + if metadata["checksum"] != "" { + parts := strings.SplitN(metadata["checksum"], " ", 2) + if len(parts) != 2 { + return nil, errtypes.BadRequest("invalid checksum format. must be '[algorithm] [checksum]'") + } + switch parts[0] { + case "sha1", "md5", "adler32": + info.MetaData["checksum"] = metadata["checksum"] + default: + return nil, errtypes.BadRequest("unsupported checksum algorithm: " + parts[0]) + } + } } - // checksum? log.Debug().Interface("info", info).Interface("node", n).Interface("metadata", metadata).Msg("ocisfs: resolved filename") @@ -357,6 +374,10 @@ func (upload *fileUpload) WriteChunk(ctx context.Context, offset int64, src io.R } defer file.Close() + // calculate cheksum here? needed for the TUS checksum extension. https://tus.io/protocols/resumable-upload.html#checksum + // TODO but how do we get the `Upload-Checksum`? WriteChunk() only has a context, offset and the reader ... + // It is sent with the PATCH request, well or in the POST when the creation-with-upload extension is used + // but the tus handler uses a context.Background() so we cannot really check the header and put it in the context ... n, err := io.Copy(file, src) // If the HTTP PATCH request gets interrupted in the middle (e.g. because @@ -391,7 +412,6 @@ func (upload *fileUpload) writeInfo() error { // FinishUpload finishes an upload and moves the file to the internal destination func (upload *fileUpload) FinishUpload(ctx context.Context) (err error) { - log := appctx.GetLogger(upload.ctx) n := &Node{ lu: upload.fs.lu, @@ -405,6 +425,58 @@ func (upload *fileUpload) FinishUpload(ctx context.Context) (err error) { } targetPath := upload.fs.lu.toInternalPath(n.ID) + sublog := appctx.GetLogger(upload.ctx).With().Interface("info", upload.info).Str("binPath", upload.binPath).Str("targetPath", targetPath).Logger() + + // calculate the checksum of the written bytes + // TODO only calculate in sync if checksum was sent? + sha1h := sha1.New() + md5h := md5.New() + adler32h := adler32.New() + + f, err := os.Open(upload.binPath) + if err != nil { + sublog.Err(err).Msg("ocisfs: could not open file for checksumming") + // we can continue if no oc checksum header is set + } + defer f.Close() + + r1 := io.TeeReader(f, sha1h) + r2 := io.TeeReader(r1, md5h) + + if _, err := io.Copy(adler32h, r2); err != nil { + sublog.Err(err).Msg("ocisfs: could not copy bytes for checksumming") + } + + // compare if they match the sent checksum + // TODO the tus checksum extension would do this on every chunk, but I currently don't see an easy way to pass in the requested checksum. for now we do it in FinishUpload which is also called for chunked uploads + if upload.info.MetaData["checksum"] != "" { + parts := strings.SplitN(upload.info.MetaData["checksum"], " ", 2) + if len(parts) != 2 { + return errtypes.BadRequest("invalid checksum format. must be '[algorithm] [checksum]'") + } + switch parts[0] { + case "sha1": + if parts[1] != hex.EncodeToString(sha1h.Sum(nil)) { + upload.discardChunk() + return errtypes.ChecksumMismatch(fmt.Sprintf("invalid checksum: expected %s got %x", upload.info.MetaData["checksum"], sha1h.Sum(nil))) + } + case "md5": + if parts[1] != hex.EncodeToString(md5h.Sum(nil)) { + upload.discardChunk() + return errtypes.ChecksumMismatch(fmt.Sprintf("invalid checksum: expected %s got %x", upload.info.MetaData["checksum"], md5h.Sum(nil))) + } + case "adler32": + if parts[1] != hex.EncodeToString(adler32h.Sum(nil)) { + upload.discardChunk() + return errtypes.ChecksumMismatch(fmt.Sprintf("invalid checksum: expected %s got %x", upload.info.MetaData["checksum"], adler32h.Sum(nil))) + } + default: + return errtypes.BadRequest("unsupported checksum algorithm: " + parts[0]) + } + } + + // defer writing the checksums until the node is in place + // if target exists create new version var fi os.FileInfo if fi, err = os.Stat(targetPath); err == nil { @@ -412,9 +484,8 @@ func (upload *fileUpload) FinishUpload(ctx context.Context) (err error) { versionsPath := upload.fs.lu.toInternalPath(n.ID + ".REV." + fi.ModTime().UTC().Format(time.RFC3339Nano)) if err = os.Rename(targetPath, versionsPath); err != nil { - log.Err(err).Interface("info", upload.info). - Str("binPath", upload.binPath). - Str("targetPath", targetPath). + sublog.Err(err). + Str("versionsPath", versionsPath). Msg("ocisfs: could not create version") return } @@ -424,59 +495,32 @@ func (upload *fileUpload) FinishUpload(ctx context.Context) (err error) { // TODO put uploads on the same underlying storage as the destination dir? // TODO trigger a workflow as the final rename might eg involve antivirus scanning if err = os.Rename(upload.binPath, targetPath); err != nil { - log := appctx.GetLogger(upload.ctx) - log.Err(err).Interface("info", upload.info). - Str("binPath", upload.binPath). - Str("targetPath", targetPath). + sublog.Err(err). Msg("ocisfs: could not rename") return } - // TODO the checksum could be calculated in a goroutine. Leaving in sync until we have a way to implement async upload handling - { - f, err := os.Open(targetPath) - if err != nil { - log.Err(err).Interface("info", upload.info). - Str("binPath", upload.binPath). - Str("targetPath", targetPath). - Msg("ocisfs: could not open file for checksumming") - } - defer f.Close() - - sha1h := sha1.New() - r1 := io.TeeReader(f, sha1h) - md5h := md5.New() - r2 := io.TeeReader(r1, md5h) - adler32h := adler32.New() - - if _, err := io.Copy(adler32h, r2); err != nil { - log.Err(err).Interface("info", upload.info). - Str("binPath", upload.binPath). - Str("targetPath", targetPath). - Msg("ocisfs: could not open file for checksumming") - } - - if err := n.SetChecksum("sha1", sha1h.Sum(nil)); err != nil { - log.Err(err).Interface("info", upload.info). - Str("binPath", upload.binPath). - Str("targetPath", targetPath). - Str("csType", "sha1"). - Msg("ocisfs: could not write checksum") - } - if err := n.SetChecksum("md5", md5h.Sum(nil)); err != nil { - log.Err(err).Interface("info", upload.info). - Str("binPath", upload.binPath). - Str("targetPath", targetPath). - Str("csType", "md5"). - Msg("ocisfs: could not write checksum") - } - if err := n.SetChecksum("adler32", adler32h.Sum(nil)); err != nil { - log.Err(err).Interface("info", upload.info). - Str("binPath", upload.binPath). - Str("targetPath", targetPath). - Str("csType", "adler32"). - Msg("ocisfs: could not write checksum") - } + // now write the checksums + if err := n.SetChecksum("sha1", sha1h.Sum(nil)); err != nil { + sublog.Err(err). + Str("csType", "sha1"). + Bytes("hash", sha1h.Sum(nil)). + Msg("ocisfs: could not write checksum") + // this is not critical, the bytes are there so we will continue + } + if err := n.SetChecksum("md5", md5h.Sum(nil)); err != nil { + sublog.Err(err). + Str("csType", "md5"). + Bytes("hash", md5h.Sum(nil)). + Msg("ocisfs: could not write checksum") + // this is not critical, the bytes are there so we will continue + } + if err := n.SetChecksum("adler32", adler32h.Sum(nil)); err != nil { + sublog.Err(err). + Str("csType", "adler32"). + Bytes("hash", adler32h.Sum(nil)). + Msg("ocisfs: could not write checksum") + // this is not critical, the bytes are there so we will continue } // who will become the owner? the owner of the parent actually ... not the currently logged in user @@ -493,10 +537,8 @@ func (upload *fileUpload) FinishUpload(ctx context.Context) (err error) { var link string link, err = os.Readlink(childNameLink) if err == nil && link != "../"+n.ID { - log.Err(err). - Interface("info", upload.info). + sublog.Err(err). Interface("node", n). - Str("targetPath", targetPath). Str("childNameLink", childNameLink). Str("link", link). Msg("ocisfs: child name link has wrong target id, repairing") @@ -514,7 +556,7 @@ func (upload *fileUpload) FinishUpload(ctx context.Context) (err error) { // only delete the upload if it was successfully written to the storage if err = os.Remove(upload.infoPath); err != nil { if !os.IsNotExist(err) { - log.Err(err).Interface("info", upload.info).Msg("ocisfs: could not delete upload info") + sublog.Err(err).Msg("ocisfs: could not delete upload info") return } } @@ -532,6 +574,15 @@ func (upload *fileUpload) FinishUpload(ctx context.Context) (err error) { return upload.fs.tp.Propagate(upload.ctx, n) } +func (upload *fileUpload) discardChunk() { + if err := os.Remove(upload.binPath); err != nil { + if !os.IsNotExist(err) { + appctx.GetLogger(upload.ctx).Err(err).Interface("info", upload.info).Str("binPath", upload.binPath).Interface("info", upload.info).Msg("ocisfs: could not discard chunk") + return + } + } +} + // To implement the termination extension as specified in https://tus.io/protocols/resumable-upload.html#termination // - the storage needs to implement AsTerminatableUpload // - the upload needs to implement Terminate From a21b60cf3b8346786e4ef244cd3e0161ec9593d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Fri, 15 Jan 2021 16:05:56 +0000 Subject: [PATCH 03/12] return multiple checksums MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../http/services/owncloud/ocdav/propfind.go | 62 ++++++++++++++----- pkg/errtypes/errtypes.go | 9 ++- pkg/storage/fs/ocis/node.go | 26 ++++++++ 3 files changed, 79 insertions(+), 18 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/propfind.go b/internal/http/services/owncloud/ocdav/propfind.go index 14ce2c0a60..d49fafae9a 100644 --- a/internal/http/services/owncloud/ocdav/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind.go @@ -385,14 +385,30 @@ func (s *svc) mdToPropResponse(ctx context.Context, pf *propfindXML, md *provide lastModifiedString := t.Format(time.RFC1123Z) response.Propstat[0].Prop = append(response.Propstat[0].Prop, s.newProp("d:getlastmodified", lastModifiedString)) + var checksums strings.Builder if md.Checksum != nil { - // TODO(jfd): the actual value is an abomination like this: - // - // SHA1:9bd253a09d58be107bcb4169ebf338c8df34d086 MD5:d90bcc6bf847403d22a4abba64e79994 ADLER32:fca23ff5 - // - // yep, correct, space delimited key value pairs inside an oc:checksum tag inside an oc:checksums tag 🤦 ... legacy reasons ... 😭 - value := fmt.Sprintf("%s:%s", strings.ToUpper(string(storageprovider.GRPC2PKGXS(md.Checksum.Type))), md.Checksum.Sum) - response.Propstat[0].Prop = append(response.Propstat[0].Prop, s.newProp("oc:checksums", value)) + checksums.WriteString("") + checksums.WriteString(strings.ToUpper(string(storageprovider.GRPC2PKGXS(md.Checksum.Type)))) + checksums.WriteString(":") + checksums.WriteString(md.Checksum.Sum) + checksums.WriteString("") + } + if md.Opaque != nil { + if e, ok := md.Opaque.Map["md5"]; ok { + checksums.WriteString("") + checksums.WriteString("MD5:") + checksums.WriteString(string(e.Value)) + checksums.WriteString("") + } + if e, ok := md.Opaque.Map["adler32"]; ok { + checksums.WriteString("") + checksums.WriteString("ADLER32:") + checksums.WriteString(string(e.Value)) + checksums.WriteString("") + } + } + if checksums.Len() > 0 { + response.Propstat[0].Prop = append(response.Propstat[0].Prop, s.newProp("oc:checksums", checksums.String())) } // favorites from arbitrary metadata @@ -528,15 +544,31 @@ func (s *svc) mdToPropResponse(ctx context.Context, pf *propfindXML, md *provide } else { propstatOK.Prop = append(propstatOK.Prop, s.newProp("oc:favorite", "0")) } - case "checksums": // desktop + case "checksums": // desktop ... not really ... the desktop sends the OC-Checksum header + var checksums strings.Builder if md.Checksum != nil { - // TODO(jfd): the actual value is an abomination like this: - // - // SHA1:9bd253a09d58be107bcb4169ebf338c8df34d086 MD5:d90bcc6bf847403d22a4abba64e79994 ADLER32:fca23ff5 - // - // yep, correct, space delimited key value pairs inside an oc:checksum tag inside an oc:checksums tag 🤦 ... legacy reasons ... 😭 - value := fmt.Sprintf("%s:%s", strings.ToUpper(string(storageprovider.GRPC2PKGXS(md.Checksum.Type))), md.Checksum.Sum) - propstatOK.Prop = append(propstatOK.Prop, s.newProp("oc:checksums", value)) + checksums.WriteString("") + checksums.WriteString(strings.ToUpper(string(storageprovider.GRPC2PKGXS(md.Checksum.Type)))) + checksums.WriteString(":") + checksums.WriteString(md.Checksum.Sum) + checksums.WriteString("") + } + if md.Opaque != nil { + if e, ok := md.Opaque.Map["md5"]; ok { + checksums.WriteString("") + checksums.WriteString("MD5:") + checksums.WriteString(string(e.Value)) + checksums.WriteString("") + } + if e, ok := md.Opaque.Map["adler32"]; ok { + checksums.WriteString("") + checksums.WriteString("ADLER32:") + checksums.WriteString(string(e.Value)) + checksums.WriteString("") + } + } + if checksums.Len() > 0 { + propstatOK.Prop = append(propstatOK.Prop, s.newProp("oc:checksums", checksums.String())) } else { propstatNotFound.Prop = append(propstatNotFound.Prop, s.newProp("oc:checksums", "")) } diff --git a/pkg/errtypes/errtypes.go b/pkg/errtypes/errtypes.go index 68c4d68414..d85b6f9055 100644 --- a/pkg/errtypes/errtypes.go +++ b/pkg/errtypes/errtypes.go @@ -102,9 +102,12 @@ func (e ChecksumMismatch) Error() string { return "error: checksum mismatch: " + // IsChecksumMismatch implements the IsChecksumMismatch interface. func (e ChecksumMismatch) IsChecksumMismatch() {} -// StatusChecksumMismatch 419 is an inofficial http status code in an unassigned range that is used for checksum mismatches -// TUS uses unassigned 460 Checksum-Mismatch -// see https://stackoverflow.com/a/35665694 and the storageprovider +// StatusChecksumMismatch 419 is an unofficial http status code in an unassigned range that is used for checksum mismatches +// Proposed by https://stackoverflow.com/a/35665694 +// Official HTTP status code registry: https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml +// Note: TUS uses unassigned 460 Checksum-Mismatch +// RFC proposal for checksum digest uses a `Want-Digest` header: https://tools.ietf.org/html/rfc3230 +// oc clienst issue: https://github.com/owncloud/core/issues/22711 const StatusChecksumMismatch = 419 // IsNotFound is the interface to implement diff --git a/pkg/storage/fs/ocis/node.go b/pkg/storage/fs/ocis/node.go index 6527ecbbeb..30bde6822d 100644 --- a/pkg/storage/fs/ocis/node.go +++ b/pkg/storage/fs/ocis/node.go @@ -549,6 +549,32 @@ func (n *Node) AsResourceInfo(ctx context.Context, rp *provider.ResourcePermissi } else { sublog.Error().Err(err).Str("cstype", "sha1").Msg("could not get checksum value") } + if v, err := xattr.Get(nodePath, checksumPrefix+"md5"); err == nil { + if ri.Opaque == nil { + ri.Opaque = &types.Opaque{ + Map: map[string]*types.OpaqueEntry{}, + } + } + ri.Opaque.Map["md5"] = &types.OpaqueEntry{ + Decoder: "plain", + Value: []byte(hex.EncodeToString(v)), + } + } else { + sublog.Error().Err(err).Str("cstype", "md5").Msg("could not get checksum value") + } + if v, err := xattr.Get(nodePath, checksumPrefix+"adler32"); err == nil { + if ri.Opaque == nil { + ri.Opaque = &types.Opaque{ + Map: map[string]*types.OpaqueEntry{}, + } + } + ri.Opaque.Map["adler32"] = &types.OpaqueEntry{ + Decoder: "plain", + Value: []byte(hex.EncodeToString(v)), + } + } else { + sublog.Error().Err(err).Str("cstype", "adler32").Msg("could not get checksum value") + } } // only read the requested metadata attributes From 192b69901f513d7d893e6c57164d37e22771008c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Mon, 18 Jan 2021 11:39:08 +0000 Subject: [PATCH 04/12] stay bug compatible with oc10 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../http/services/owncloud/ocdav/propfind.go | 43 ++++++++++++------- 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/propfind.go b/internal/http/services/owncloud/ocdav/propfind.go index d49fafae9a..c7c073c324 100644 --- a/internal/http/services/owncloud/ocdav/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind.go @@ -385,29 +385,34 @@ func (s *svc) mdToPropResponse(ctx context.Context, pf *propfindXML, md *provide lastModifiedString := t.Format(time.RFC1123Z) response.Propstat[0].Prop = append(response.Propstat[0].Prop, s.newProp("d:getlastmodified", lastModifiedString)) + // stay bug compatible with oc10, see https://github.com/owncloud/core/pull/38304#issuecomment-762185241 var checksums strings.Builder if md.Checksum != nil { checksums.WriteString("") checksums.WriteString(strings.ToUpper(string(storageprovider.GRPC2PKGXS(md.Checksum.Type)))) checksums.WriteString(":") checksums.WriteString(md.Checksum.Sum) - checksums.WriteString("") } if md.Opaque != nil { if e, ok := md.Opaque.Map["md5"]; ok { - checksums.WriteString("") - checksums.WriteString("MD5:") + if checksums.Len() == 0 { + checksums.WriteString("MD5:") + } else { + checksums.WriteString(" MD5:") + } checksums.WriteString(string(e.Value)) - checksums.WriteString("") } if e, ok := md.Opaque.Map["adler32"]; ok { - checksums.WriteString("") - checksums.WriteString("ADLER32:") + if checksums.Len() == 0 { + checksums.WriteString("ADLER32:") + } else { + checksums.WriteString(" ADLER32:") + } checksums.WriteString(string(e.Value)) - checksums.WriteString("") } } - if checksums.Len() > 0 { + if checksums.Len() > 13 { + checksums.WriteString("") response.Propstat[0].Prop = append(response.Propstat[0].Prop, s.newProp("oc:checksums", checksums.String())) } @@ -545,29 +550,35 @@ func (s *svc) mdToPropResponse(ctx context.Context, pf *propfindXML, md *provide propstatOK.Prop = append(propstatOK.Prop, s.newProp("oc:favorite", "0")) } case "checksums": // desktop ... not really ... the desktop sends the OC-Checksum header + + // stay bug compatible with oc10, see https://github.com/owncloud/core/pull/38304#issuecomment-762185241 var checksums strings.Builder if md.Checksum != nil { checksums.WriteString("") checksums.WriteString(strings.ToUpper(string(storageprovider.GRPC2PKGXS(md.Checksum.Type)))) checksums.WriteString(":") checksums.WriteString(md.Checksum.Sum) - checksums.WriteString("") } if md.Opaque != nil { if e, ok := md.Opaque.Map["md5"]; ok { - checksums.WriteString("") - checksums.WriteString("MD5:") + if checksums.Len() == 0 { + checksums.WriteString("MD5:") + } else { + checksums.WriteString(" MD5:") + } checksums.WriteString(string(e.Value)) - checksums.WriteString("") } if e, ok := md.Opaque.Map["adler32"]; ok { - checksums.WriteString("") - checksums.WriteString("ADLER32:") + if checksums.Len() == 0 { + checksums.WriteString("ADLER32:") + } else { + checksums.WriteString(" ADLER32:") + } checksums.WriteString(string(e.Value)) - checksums.WriteString("") } } - if checksums.Len() > 0 { + if checksums.Len() > 13 { + checksums.WriteString("") propstatOK.Prop = append(propstatOK.Prop, s.newProp("oc:checksums", checksums.String())) } else { propstatNotFound.Prop = append(propstatNotFound.Prop, s.newProp("oc:checksums", "")) From 461aa55d56d4b8e816d48fe6fa7aba98c3dd9f5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Mon, 18 Jan 2021 12:01:11 +0000 Subject: [PATCH 05/12] render correct algorithm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- internal/http/services/owncloud/ocdav/get.go | 4 +++- internal/http/services/owncloud/ocdav/head.go | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/get.go b/internal/http/services/owncloud/ocdav/get.go index 15972022c5..d6ab7d6c89 100644 --- a/internal/http/services/owncloud/ocdav/get.go +++ b/internal/http/services/owncloud/ocdav/get.go @@ -24,8 +24,10 @@ import ( "net/http" "path" "strconv" + "strings" "time" + "github.com/cs3org/reva/internal/grpc/services/storageprovider" "github.com/cs3org/reva/internal/http/services/datagateway" "go.opencensus.io/trace" @@ -146,7 +148,7 @@ func (s *svc) handleGet(w http.ResponseWriter, r *http.Request, ns string) { w.Header().Set("Content-Length", strconv.FormatUint(info.Size, 10)) } if info.Checksum != nil { - w.Header().Set("OC-Checksum", fmt.Sprintf("%s:%s", info.Checksum.Type, info.Checksum.Sum)) + w.Header().Set("OC-Checksum", fmt.Sprintf("%s:%s", strings.ToUpper(string(storageprovider.GRPC2PKGXS(info.Checksum.Type))), info.Checksum.Sum)) } var c int64 if c, err = io.Copy(w, httpRes.Body); err != nil { diff --git a/internal/http/services/owncloud/ocdav/head.go b/internal/http/services/owncloud/ocdav/head.go index 57b2ab34b5..8784629685 100644 --- a/internal/http/services/owncloud/ocdav/head.go +++ b/internal/http/services/owncloud/ocdav/head.go @@ -23,10 +23,12 @@ import ( "net/http" "path" "strconv" + "strings" "time" rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/cs3org/reva/internal/grpc/services/storageprovider" "github.com/cs3org/reva/pkg/appctx" "github.com/cs3org/reva/pkg/utils" "go.opencensus.io/trace" @@ -69,7 +71,7 @@ func (s *svc) handleHead(w http.ResponseWriter, r *http.Request, ns string) { w.Header().Set("ETag", info.Etag) w.Header().Set("OC-FileId", wrapResourceID(info.Id)) w.Header().Set("OC-ETag", info.Etag) - w.Header().Set("OC-Checksum", fmt.Sprintf("%s:%s", info.Checksum.Type, info.Checksum.Sum)) + w.Header().Set("OC-Checksum", fmt.Sprintf("%s:%s", strings.ToUpper(string(storageprovider.GRPC2PKGXS(info.Checksum.Type))), info.Checksum.Sum)) t := utils.TSToTime(info.Mtime).UTC() lastModifiedString := t.Format(time.RFC1123Z) w.Header().Set("Last-Modified", lastModifiedString) From f5c8267afce809e9409bdf7d3b7678904b50a3dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Mon, 18 Jan 2021 12:24:10 +0000 Subject: [PATCH 06/12] update expected tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../expected-failures-on-OCIS-storage.md | 22 -------- .../apiOcisSpecific/apiMain-checksums.feature | 50 +------------------ 2 files changed, 1 insertion(+), 71 deletions(-) diff --git a/tests/acceptance/expected-failures-on-OCIS-storage.md b/tests/acceptance/expected-failures-on-OCIS-storage.md index 5f3e8cf1f3..0362729fe6 100644 --- a/tests/acceptance/expected-failures-on-OCIS-storage.md +++ b/tests/acceptance/expected-failures-on-OCIS-storage.md @@ -4,19 +4,8 @@ - [apiWebdavProperties1/setFileProperties.feature:33](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties1/setFileProperties.feature#L33) ### [Checksum feature](https://github.com/owncloud/ocis-reva/issues/196) -- [apiMain/checksums.feature:24](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L24) -- [apiMain/checksums.feature:25](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L25) -- [apiMain/checksums.feature:35](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L35) -- [apiMain/checksums.feature:36](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L36) -- [apiMain/checksums.feature:46](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L46) -- [apiMain/checksums.feature:47](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L47) -- [apiMain/checksums.feature:50](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L50) - [apiMain/checksums.feature:58](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L58) - [apiMain/checksums.feature:67](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L67) -- [apiMain/checksums.feature:99](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L99) -- [apiMain/checksums.feature:100](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L100) -- [apiMain/checksums.feature:103](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L103) -- [apiMain/checksums.feature:110](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L110) - [apiMain/checksums.feature:119](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L119) - [apiMain/checksums.feature:129](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L129) - [apiMain/checksums.feature:138](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L138) @@ -24,17 +13,7 @@ - [apiMain/checksums.feature:158](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L158) - [apiMain/checksums.feature:174](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L174) - [apiMain/checksums.feature:192](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L192) -- [apiMain/checksums.feature:217](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L217) -- [apiMain/checksums.feature:218](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L218) -- [apiMain/checksums.feature:239](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L239) -- [apiMain/checksums.feature:240](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L240) - [apiMain/checksums.feature:258](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L258) -- [apiMain/checksums.feature:279](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L279) -- [apiMain/checksums.feature:280](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L280) -- [apiMain/checksums.feature:295](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L295) -- [apiMain/checksums.feature:296](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L296) -- [apiMain/checksums.feature:308](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L308) -- [apiMain/checksums.feature:309](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L309) - [apiMain/checksums.feature:312](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L312) - [apiMain/checksums.feature:324](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L324) @@ -900,7 +879,6 @@ special character username not valid - [apiVersions/fileVersions.feature:88](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L88) - [apiVersions/fileVersions.feature:89](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L89) - [apiVersions/fileVersions.feature:93](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L93) -- [apiVersions/fileVersions.feature:104](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L104) - [apiVersions/fileVersions.feature:288](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L288) - [apiVersions/fileVersions.feature:362](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L362) - [apiVersions/fileVersions.feature:373](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L373) diff --git a/tests/acceptance/features/apiOcisSpecific/apiMain-checksums.feature b/tests/acceptance/features/apiOcisSpecific/apiMain-checksums.feature index 2576d2a1d7..080aa7b57d 100644 --- a/tests/acceptance/features/apiOcisSpecific/apiMain-checksums.feature +++ b/tests/acceptance/features/apiOcisSpecific/apiMain-checksums.feature @@ -4,40 +4,6 @@ Feature: checksums Background: Given user "Alice" has been created with default attributes and without skeleton files - @issue-ocis-reva-98 - # after fixing all issues delete this Scenario and use the one from oC10 core - Scenario Outline: Uploading a file with checksum should return the checksum in the download header - Given using DAV path - And user "Alice" has uploaded file "filesForUpload/textfile.txt" to "/myChecksumFile.txt" with checksum "MD5:d70b40f177b14b470d1756a3c12b963a" - When user "Alice" downloads file "/myChecksumFile.txt" using the WebDAV API - Then the following headers should not be set - | header | - | OC-Checksum | - Examples: - | dav_version | - | old | - | new | - - @issue-ocis-reva-98 - # after fixing all issues delete this Scenario and use the one from oC10 core - Scenario: Copying file with checksum should return the checksum in the download header using new DAV path - Given using new DAV path - And user "Alice" has uploaded file "filesForUpload/textfile.txt" to "/myChecksumFile.txt" with checksum "MD5:d70b40f177b14b470d1756a3c12b963a" - When user "Alice" copies file "/myChecksumFile.txt" to "/myChecksumFileCopy.txt" using the WebDAV API - And user "Alice" downloads file "/myChecksumFileCopy.txt" using the WebDAV API - Then the following headers should not be set - | header | - | OC-Checksum | - - @issue-ocis-reva-99 - # after fixing all issues delete this Scenario and use the one from oC10 core - Scenario: Upload a file where checksum does not match (old DAV path) - Given using old DAV path - When user "Alice" uploads file with checksum "SHA1:f005ba11" and content "Some Text" to "/chksumtst.txt" using the WebDAV API - Then the HTTP status code should be "201" - And user "Alice" should see the following elements - | /chksumtst.txt | - @issue-ocis-reva-99 @skipOnOcis-OCIS-Storage # after fixing all issues delete this Scenario and use the one from oC10 core Scenario: Upload a file where checksum does not match (new DAV path) @@ -45,18 +11,4 @@ Feature: checksums When user "Alice" uploads file with checksum "SHA1:f005ba11" and content "Some Text" to "/chksumtst.txt" using the WebDAV API Then the HTTP status code should be "201" And user "Alice" should see the following elements - | /chksumtst.txt | - - @issue-ocis-reva-99 - # after fixing all issues delete this Scenario and use the one from oC10 core - Scenario Outline: Uploaded file should have the same checksum when downloaded - Given using DAV path - And user "Alice" has uploaded file with checksum "SHA1:ce5582148c6f0c1282335b87df5ed4be4b781399" and content "Some Text" to "/chksumtst.txt" - When user "Alice" downloads file "/chksumtst.txt" using the WebDAV API - Then the following headers should not be set - | header | - | OC-Checksum | - Examples: - | dav_version | - | old | - | new | + | /chksumtst.txt | \ No newline at end of file From 966a2c5645c4691b7fb0bf8164e755b8503bdbc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Mon, 18 Jan 2021 13:11:32 +0000 Subject: [PATCH 07/12] add changelog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- changelog/unreleased/checksums.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changelog/unreleased/checksums.md diff --git a/changelog/unreleased/checksums.md b/changelog/unreleased/checksums.md new file mode 100644 index 0000000000..aa1cf28ab9 --- /dev/null +++ b/changelog/unreleased/checksums.md @@ -0,0 +1,8 @@ +Enhancement: Checksum support + +We now support checksums on file uploads and PROPFIND results. On uploads, the ocdav service now forwards the `OC-Checksum` (and the similar TUS `Upload-Checksum`) header to the storage provider. We added an internal http status code that allows storage drivers to return checksum errors. On PROPFINDs, ocdav now renders the `` header in a bug compatible way for oc10 backward compatability with existing clients. Finally, GET and HEAD requests now return the `OC-Checksum` header. + +https://github.com/cs3org/reva/pull/1400 +https://github.com/owncloud/core/pull/38304 +https://github.com/owncloud/ocis/issues/1291 +https://github.com/owncloud/ocis/issues/1316 \ No newline at end of file From c216b5920ddd5269af95942ae3b99d96c0974128 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Mon, 18 Jan 2021 13:55:36 +0000 Subject: [PATCH 08/12] cleanup xattr comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- pkg/storage/fs/ocis/ocis.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/storage/fs/ocis/ocis.go b/pkg/storage/fs/ocis/ocis.go index 9ad76e76e4..b3024b51ca 100644 --- a/pkg/storage/fs/ocis/ocis.go +++ b/pkg/storage/fs/ocis/ocis.go @@ -68,8 +68,8 @@ const ( // a temporary etag for a folder that is removed when the mtime propagation happens tmpEtagAttr string = ocisPrefix + "tmp.etag" - referenceAttr string = ocisPrefix + "cs3.ref" // arbitrary metadata - checksumPrefix string = ocisPrefix + "cs." // checksum support + referenceAttr string = ocisPrefix + "cs3.ref" // target of a cs3 reference + checksumPrefix string = ocisPrefix + "cs." // followed by the algorithm, eg. ocis.cs.sha1 trashOriginAttr string = ocisPrefix + "trash.origin" // trash origin // we use a single attribute to enable or disable propagation of both: synctime and treesize From 0f59d58191927a5a3ad3f21433d3648c5a6524a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Mon, 18 Jan 2021 14:03:28 +0000 Subject: [PATCH 09/12] use xml marshaling to render error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../http/services/owncloud/ocdav/error.go | 19 ++++++++++++++++++- .../http/services/owncloud/ocdav/propfind.go | 13 ------------- internal/http/services/owncloud/ocdav/put.go | 16 ++++++++++------ 3 files changed, 28 insertions(+), 20 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/error.go b/internal/http/services/owncloud/ocdav/error.go index 745918d466..e20fbc3aff 100644 --- a/internal/http/services/owncloud/ocdav/error.go +++ b/internal/http/services/owncloud/ocdav/error.go @@ -23,18 +23,23 @@ import ( "net/http" rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" + "github.com/pkg/errors" "github.com/rs/zerolog" ) type code int const ( + + // SabredavMethodBadRequest maps to HTTP 400 + SabredavMethodBadRequest code = iota // SabredavMethodNotAllowed maps to HTTP 405 - SabredavMethodNotAllowed code = iota + SabredavMethodNotAllowed ) var ( codesEnum = []string{ + "Sabre\\DAV\\Exception\\BadRequest", "Sabre\\DAV\\Exception\\MethodNotAllowed", } ) @@ -54,6 +59,18 @@ func Marshal(e exception) ([]byte, error) { }) } +// http://www.webdav.org/specs/rfc4918.html#ELEMENT_error +type errorXML struct { + XMLName xml.Name `xml:"d:error"` + Xmlnsd string `xml:"xmlns:d,attr"` + Xmlnss string `xml:"xmlns:s,attr"` + Exception string `xml:"s:exception"` + Message string `xml:"s:message"` + InnerXML []byte `xml:",innerxml"` +} + +var errInvalidPropfind = errors.New("webdav: invalid propfind") + // HandleErrorStatus checks the status code, logs a Debug or Error level message // and writes an appropriate http status func HandleErrorStatus(log *zerolog.Logger, w http.ResponseWriter, s *rpc.Status) { diff --git a/internal/http/services/owncloud/ocdav/propfind.go b/internal/http/services/owncloud/ocdav/propfind.go index c7c073c324..01c79abc52 100644 --- a/internal/http/services/owncloud/ocdav/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind.go @@ -43,7 +43,6 @@ import ( "github.com/cs3org/reva/pkg/appctx" ctxuser "github.com/cs3org/reva/pkg/user" "github.com/cs3org/reva/pkg/utils" - "github.com/pkg/errors" ) const ( @@ -818,15 +817,3 @@ type propertyXML struct { // even including the DAV: namespace. InnerXML []byte `xml:",innerxml"` } - -// http://www.webdav.org/specs/rfc4918.html#ELEMENT_error -type errorXML struct { - XMLName xml.Name `xml:"d:error"` - Xmlnsd string `xml:"xmlns:d,attr"` - Xmlnss string `xml:"xmlns:s,attr"` - Exception string `xml:"s:exception"` - Message string `xml:"s:message"` - InnerXML []byte `xml:",innerxml"` -} - -var errInvalidPropfind = errors.New("webdav: invalid propfind") diff --git a/internal/http/services/owncloud/ocdav/put.go b/internal/http/services/owncloud/ocdav/put.go index 5f87323ed3..71a8c1b7b3 100644 --- a/internal/http/services/owncloud/ocdav/put.go +++ b/internal/http/services/owncloud/ocdav/put.go @@ -283,12 +283,16 @@ func (s *svc) handlePutHelper(w http.ResponseWriter, r *http.Request, content io } if httpRes.StatusCode == errtypes.StatusChecksumMismatch { w.WriteHeader(http.StatusBadRequest) - xml := ` - - Sabre\DAV\Exception\BadRequest - The computed checksum does not match the one received from the client. -` - _, err := w.Write([]byte(xml)) + b, err := Marshal(exception{ + code: SabredavMethodBadRequest, + message: "The computed checksum does not match the one received from the client.", + }) + if err != nil { + sublog.Error().Msgf("error marshaling xml response: %s", b) + w.WriteHeader(http.StatusInternalServerError) + return + } + _, err = w.Write(b) if err != nil { sublog.Err(err).Msg("error writing response") } From dc2e3c7b2b3c96c7bc1fd0daa15b42319c3d0e2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Mon, 18 Jan 2021 14:12:20 +0000 Subject: [PATCH 10/12] use fmt for better readibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- pkg/storage/fs/ocis/node.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/storage/fs/ocis/node.go b/pkg/storage/fs/ocis/node.go index 30bde6822d..a81ce8c916 100644 --- a/pkg/storage/fs/ocis/node.go +++ b/pkg/storage/fs/ocis/node.go @@ -345,7 +345,7 @@ func calculateEtag(nodeID string, tmTime time.Time) (string, error) { } else { return "", err } - return `"` + hex.EncodeToString(h.Sum(nil)) + `"`, nil + return fmt.Sprintf(`"%x"`, h.Sum(nil)), nil } // SetMtime sets the mtime and atime of a node From 2921f856f3fc79513b33b5dce99e54db69fe60c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 20 Jan 2021 21:17:06 +0000 Subject: [PATCH 11/12] change loglevel, extract method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- pkg/storage/fs/ocis/node.go | 77 ++++++++++++++++++++----------------- 1 file changed, 42 insertions(+), 35 deletions(-) diff --git a/pkg/storage/fs/ocis/node.go b/pkg/storage/fs/ocis/node.go index a81ce8c916..96baf542c7 100644 --- a/pkg/storage/fs/ocis/node.go +++ b/pkg/storage/fs/ocis/node.go @@ -539,42 +539,11 @@ func (n *Node) AsResourceInfo(ctx context.Context, rp *provider.ResourcePermissi } // checksums - if _, ok := mdKeysMap[_checksumsKey]; returnAllKeys || ok { + if _, ok := mdKeysMap[_checksumsKey]; (nodeType == provider.ResourceType_RESOURCE_TYPE_FILE) && returnAllKeys || ok { // TODO which checksum was requested? sha1 adler32 or md5? for now hardcode sha1? - if v, err := xattr.Get(nodePath, checksumPrefix+"sha1"); err == nil { - ri.Checksum = &provider.ResourceChecksum{ - Type: storageprovider.PKG2GRPCXS("sha1"), - Sum: hex.EncodeToString(v), - } - } else { - sublog.Error().Err(err).Str("cstype", "sha1").Msg("could not get checksum value") - } - if v, err := xattr.Get(nodePath, checksumPrefix+"md5"); err == nil { - if ri.Opaque == nil { - ri.Opaque = &types.Opaque{ - Map: map[string]*types.OpaqueEntry{}, - } - } - ri.Opaque.Map["md5"] = &types.OpaqueEntry{ - Decoder: "plain", - Value: []byte(hex.EncodeToString(v)), - } - } else { - sublog.Error().Err(err).Str("cstype", "md5").Msg("could not get checksum value") - } - if v, err := xattr.Get(nodePath, checksumPrefix+"adler32"); err == nil { - if ri.Opaque == nil { - ri.Opaque = &types.Opaque{ - Map: map[string]*types.OpaqueEntry{}, - } - } - ri.Opaque.Map["adler32"] = &types.OpaqueEntry{ - Decoder: "plain", - Value: []byte(hex.EncodeToString(v)), - } - } else { - sublog.Error().Err(err).Str("cstype", "adler32").Msg("could not get checksum value") - } + readChecksumIntoResourceChecksum(ctx, nodePath, storageprovider.XSSHA1, ri) + readChecksumIntoOpaque(ctx, nodePath, storageprovider.XSMD5, ri) + readChecksumIntoOpaque(ctx, nodePath, storageprovider.XSAdler32, ri) } // only read the requested metadata attributes @@ -612,6 +581,44 @@ func (n *Node) AsResourceInfo(ctx context.Context, rp *provider.ResourcePermissi return ri, nil } +func readChecksumIntoResourceChecksum(ctx context.Context, nodePath, algo string, ri *provider.ResourceInfo) { + v, err := xattr.Get(nodePath, checksumPrefix+algo) + switch { + case err == nil: + ri.Checksum = &provider.ResourceChecksum{ + Type: storageprovider.PKG2GRPCXS(algo), + Sum: hex.EncodeToString(v), + } + case isNoData(err): + appctx.GetLogger(ctx).Debug().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("checksum not set") + case isNotFound(err): + appctx.GetLogger(ctx).Error().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("file not fount") + default: + appctx.GetLogger(ctx).Error().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("could not read checksum") + } +} +func readChecksumIntoOpaque(ctx context.Context, nodePath, algo string, ri *provider.ResourceInfo) { + v, err := xattr.Get(nodePath, checksumPrefix+algo) + switch { + case err == nil: + if ri.Opaque == nil { + ri.Opaque = &types.Opaque{ + Map: map[string]*types.OpaqueEntry{}, + } + } + ri.Opaque.Map[algo] = &types.OpaqueEntry{ + Decoder: "plain", + Value: []byte(hex.EncodeToString(v)), + } + case isNoData(err): + appctx.GetLogger(ctx).Debug().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("checksum not set") + case isNotFound(err): + appctx.GetLogger(ctx).Error().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("file not fount") + default: + appctx.GetLogger(ctx).Error().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("could not read checksum") + } +} + // HasPropagation checks if the propagation attribute exists and is set to "1" func (n *Node) HasPropagation() (propagation bool) { if b, err := xattr.Get(n.lu.toInternalPath(n.ID), propagationAttr); err == nil { From 44075eec6f91fb520102f0bc173f03bf4917e4b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Sat, 23 Jan 2021 22:02:33 +0000 Subject: [PATCH 12/12] incorporate review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- internal/http/services/owncloud/ocdav/head.go | 4 +- .../http/services/owncloud/ocdav/propfind.go | 7 +- pkg/storage/fs/ocis/node.go | 5 +- pkg/storage/fs/ocis/upload.go | 93 +++++++++---------- 4 files changed, 57 insertions(+), 52 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/head.go b/internal/http/services/owncloud/ocdav/head.go index 8784629685..250da299bb 100644 --- a/internal/http/services/owncloud/ocdav/head.go +++ b/internal/http/services/owncloud/ocdav/head.go @@ -71,7 +71,9 @@ func (s *svc) handleHead(w http.ResponseWriter, r *http.Request, ns string) { w.Header().Set("ETag", info.Etag) w.Header().Set("OC-FileId", wrapResourceID(info.Id)) w.Header().Set("OC-ETag", info.Etag) - w.Header().Set("OC-Checksum", fmt.Sprintf("%s:%s", strings.ToUpper(string(storageprovider.GRPC2PKGXS(info.Checksum.Type))), info.Checksum.Sum)) + if info.Checksum != nil { + w.Header().Set("OC-Checksum", fmt.Sprintf("%s:%s", strings.ToUpper(string(storageprovider.GRPC2PKGXS(info.Checksum.Type))), info.Checksum.Sum)) + } t := utils.TSToTime(info.Mtime).UTC() lastModifiedString := t.Format(time.RFC1123Z) w.Header().Set("Last-Modified", lastModifiedString) diff --git a/internal/http/services/owncloud/ocdav/propfind.go b/internal/http/services/owncloud/ocdav/propfind.go index 01c79abc52..a4a0716472 100644 --- a/internal/http/services/owncloud/ocdav/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind.go @@ -106,6 +106,11 @@ func (s *svc) handlePropfind(w http.ResponseWriter, r *http.Request, ns string) metadataKeys := []string{} if pf.Allprop != nil { + // TODO this changes the behavior and returns all properties if allprops has been set, + // but allprops should only return some default properties + // see https://tools.ietf.org/html/rfc4918#section-9.1 + // the description of arbitrary_metadata_keys in https://cs3org.github.io/cs3apis/#cs3.storage.provider.v1beta1.ListContainerRequest an others may need clarification + // tracked in https://github.com/cs3org/cs3apis/issues/104 metadataKeys = append(metadataKeys, "*") } else { for i := range pf.Prop { @@ -410,7 +415,7 @@ func (s *svc) mdToPropResponse(ctx context.Context, pf *propfindXML, md *provide checksums.WriteString(string(e.Value)) } } - if checksums.Len() > 13 { + if checksums.Len() > 0 { checksums.WriteString("") response.Propstat[0].Prop = append(response.Propstat[0].Prop, s.newProp("oc:checksums", checksums.String())) } diff --git a/pkg/storage/fs/ocis/node.go b/pkg/storage/fs/ocis/node.go index 96baf542c7..f79a07e0ed 100644 --- a/pkg/storage/fs/ocis/node.go +++ b/pkg/storage/fs/ocis/node.go @@ -23,6 +23,7 @@ import ( "crypto/md5" "encoding/hex" "fmt" + "hash" "io" "os" "path/filepath" @@ -642,8 +643,8 @@ func (n *Node) SetTMTime(t time.Time) (err error) { } // SetChecksum writes the checksum with the given checksum type to the extended attributes -func (n *Node) SetChecksum(csType string, bytes []byte) (err error) { - return xattr.Set(n.lu.toInternalPath(n.ID), checksumPrefix+csType, bytes) +func (n *Node) SetChecksum(csType string, h hash.Hash) (err error) { + return xattr.Set(n.lu.toInternalPath(n.ID), checksumPrefix+csType, h.Sum(nil)) } // UnsetTempEtag removes the temporary etag attribute diff --git a/pkg/storage/fs/ocis/upload.go b/pkg/storage/fs/ocis/upload.go index c8c727a79e..5d0261e569 100644 --- a/pkg/storage/fs/ocis/upload.go +++ b/pkg/storage/fs/ocis/upload.go @@ -25,6 +25,7 @@ import ( "encoding/hex" "encoding/json" "fmt" + "hash" "hash/adler32" "io" "io/ioutil" @@ -42,6 +43,7 @@ import ( "github.com/cs3org/reva/pkg/user" "github.com/google/uuid" "github.com/pkg/errors" + "github.com/rs/zerolog" tusd "github.com/tus/tusd/pkg/handler" ) @@ -428,25 +430,27 @@ func (upload *fileUpload) FinishUpload(ctx context.Context) (err error) { sublog := appctx.GetLogger(upload.ctx).With().Interface("info", upload.info).Str("binPath", upload.binPath).Str("targetPath", targetPath).Logger() // calculate the checksum of the written bytes - // TODO only calculate in sync if checksum was sent? + // they will all be written to the metadata later, so we cannot omit any of them + // TODO only calculate the checksum in sync that was requested to match, the rest could be async ... but the tests currently expect all to be present + // TODO the hashes all implement BinaryMarshaler so we could try to persist the state for resumable upload. we would neet do keep track of the copied bytes ... sha1h := sha1.New() md5h := md5.New() adler32h := adler32.New() + { + f, err := os.Open(upload.binPath) + if err != nil { + sublog.Err(err).Msg("ocisfs: could not open file for checksumming") + // we can continue if no oc checksum header is set + } + defer f.Close() - f, err := os.Open(upload.binPath) - if err != nil { - sublog.Err(err).Msg("ocisfs: could not open file for checksumming") - // we can continue if no oc checksum header is set - } - defer f.Close() - - r1 := io.TeeReader(f, sha1h) - r2 := io.TeeReader(r1, md5h) + r1 := io.TeeReader(f, sha1h) + r2 := io.TeeReader(r1, md5h) - if _, err := io.Copy(adler32h, r2); err != nil { - sublog.Err(err).Msg("ocisfs: could not copy bytes for checksumming") + if _, err := io.Copy(adler32h, r2); err != nil { + sublog.Err(err).Msg("ocisfs: could not copy bytes for checksumming") + } } - // compare if they match the sent checksum // TODO the tus checksum extension would do this on every chunk, but I currently don't see an easy way to pass in the requested checksum. for now we do it in FinishUpload which is also called for chunked uploads if upload.info.MetaData["checksum"] != "" { @@ -456,22 +460,16 @@ func (upload *fileUpload) FinishUpload(ctx context.Context) (err error) { } switch parts[0] { case "sha1": - if parts[1] != hex.EncodeToString(sha1h.Sum(nil)) { - upload.discardChunk() - return errtypes.ChecksumMismatch(fmt.Sprintf("invalid checksum: expected %s got %x", upload.info.MetaData["checksum"], sha1h.Sum(nil))) - } + err = upload.checkHash(parts[1], sha1h) case "md5": - if parts[1] != hex.EncodeToString(md5h.Sum(nil)) { - upload.discardChunk() - return errtypes.ChecksumMismatch(fmt.Sprintf("invalid checksum: expected %s got %x", upload.info.MetaData["checksum"], md5h.Sum(nil))) - } + err = upload.checkHash(parts[1], md5h) case "adler32": - if parts[1] != hex.EncodeToString(adler32h.Sum(nil)) { - upload.discardChunk() - return errtypes.ChecksumMismatch(fmt.Sprintf("invalid checksum: expected %s got %x", upload.info.MetaData["checksum"], adler32h.Sum(nil))) - } + err = upload.checkHash(parts[1], adler32h) default: - return errtypes.BadRequest("unsupported checksum algorithm: " + parts[0]) + err = errtypes.BadRequest("unsupported checksum algorithm: " + parts[0]) + } + if err != nil { + return err } } @@ -500,28 +498,10 @@ func (upload *fileUpload) FinishUpload(ctx context.Context) (err error) { return } - // now write the checksums - if err := n.SetChecksum("sha1", sha1h.Sum(nil)); err != nil { - sublog.Err(err). - Str("csType", "sha1"). - Bytes("hash", sha1h.Sum(nil)). - Msg("ocisfs: could not write checksum") - // this is not critical, the bytes are there so we will continue - } - if err := n.SetChecksum("md5", md5h.Sum(nil)); err != nil { - sublog.Err(err). - Str("csType", "md5"). - Bytes("hash", md5h.Sum(nil)). - Msg("ocisfs: could not write checksum") - // this is not critical, the bytes are there so we will continue - } - if err := n.SetChecksum("adler32", adler32h.Sum(nil)); err != nil { - sublog.Err(err). - Str("csType", "adler32"). - Bytes("hash", adler32h.Sum(nil)). - Msg("ocisfs: could not write checksum") - // this is not critical, the bytes are there so we will continue - } + // now try write all checksums + tryWritingChecksum(&sublog, n, "sha1", sha1h) + tryWritingChecksum(&sublog, n, "md5", md5h) + tryWritingChecksum(&sublog, n, "adler32", adler32h) // who will become the owner? the owner of the parent actually ... not the currently logged in user err = n.writeMetadata(&userpb.UserId{ @@ -574,6 +554,23 @@ func (upload *fileUpload) FinishUpload(ctx context.Context) (err error) { return upload.fs.tp.Propagate(upload.ctx, n) } +func (upload *fileUpload) checkHash(expected string, h hash.Hash) error { + if expected != hex.EncodeToString(h.Sum(nil)) { + upload.discardChunk() + return errtypes.ChecksumMismatch(fmt.Sprintf("invalid checksum: expected %s got %x", upload.info.MetaData["checksum"], h.Sum(nil))) + } + return nil +} +func tryWritingChecksum(log *zerolog.Logger, n *Node, algo string, h hash.Hash) { + if err := n.SetChecksum(algo, h); err != nil { + log.Err(err). + Str("csType", algo). + Bytes("hash", h.Sum(nil)). + Msg("ocisfs: could not write checksum") + // this is not critical, the bytes are there so we will continue + } +} + func (upload *fileUpload) discardChunk() { if err := os.Remove(upload.binPath); err != nil { if !os.IsNotExist(err) {