Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/net/webdav: moving a locked file fails unless destination is locked #43556

Open
klarose opened this issue Jan 6, 2021 · 4 comments · May be fixed by golang/net#93
Open

x/net/webdav: moving a locked file fails unless destination is locked #43556

klarose opened this issue Jan 6, 2021 · 4 comments · May be fixed by golang/net#93

Comments

@klarose
Copy link

@klarose klarose commented Jan 6, 2021

What version of Go are you using (go version)?

$ go version
go version go1.15 linux/amd64

Does this issue reproduce with the latest release?

Haven't tried, but it doesn't look like the code in question has changed.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/kyle/.cache/go-build"
GOENV="/home/kyle/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/kyle/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/kyle/"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/kyle/src/klarose/golang/net/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build211380463=/tmp/go-build -gno-record-gcc-switches"

What did you do?

While running a webdav server using x/net/webdav, I found that any operations which tried to rename a locked file failed unless the client held a lock on the destination file. This seems wrong.

Reproduction steps:

Compile https://github.com/hacdias/webdav (this uses x/net/webdav as its server).

Run the following:

cd $(mktemp -d)
cat > config.yaml <<EOF 
# Server related settings
address: 127.0.0.1
port: 8080
auth: false
tls: false
prefix: /

scope: .
modify: true
rules: []
EOF

webdav -p 8080 --auth=false

Download/build/install/etc cadaver (a simple tool for running webdav commands)

Run cadaver http://localhost:8080

In its command prompt, run the following commands:

A

lock foo.yaml
mv foo.yaml foo.yaml.2

Then run:

B

lock foo.yaml.2
mv foo.yaml foo.yaml.2

What did you expect to see?

A should have succeeded -- foo.yaml shoud be renamed to foo.yaml.2:

dav:/> lock foo.yaml
Locking `foo.yaml': succeeded.
dav:/> mv foo.yaml foo.yaml.2
Moving `/foo.yaml' to `/foo.yaml.2':  succeeded.

What did you see instead?

A fails, but B succeeds:

dav:/> lock foo.yaml
Locking `foo.yaml': succeeded.
dav:/> mv foo.yaml foo.yaml.2
Moving `/foo.yaml' to `/foo.yaml.2':  failed:
412 Precondition Failed
dav:/> lock foo.yaml.2
Locking `foo.yaml.2': succeeded.
dav:/> mv foo.yaml foo.yaml.2
Moving `/foo.yaml' to `/foo.yaml.2':  succeeded.
dav:/> 
@gopherbot gopherbot added this to the Unreleased milestone Jan 6, 2021
@klarose
Copy link
Author

@klarose klarose commented Jan 6, 2021

Some follow up thoughts/comments:

My findings imply that you can only move a source to a locked destination, which is not correct. The locked resource should simply be renamed, with the lock discarded as per the webdav RFC:

   A successful MOVE request on a write locked resource MUST NOT move
   the write lock with the resource.  However, if there is an existing
   lock at the destination, the server MUST add the moved resource to
   the destination lock scope.  For example, if the MOVE makes the
   resource a child of a collection that has a depth-infinity lock, then
   the resource will be added to that collection's lock.  Additionally,
   if a resource with a depth-infinity lock is moved to a destination
   that is within the scope of the same lock (e.g., within the URL
   namespace tree covered by the lock), the moved resource will again be
   added to the lock.  In both these examples, as specified in
   Section 7.5, an If header must be submitted containing a lock token
   for both the source and destination.

From inspecting the code, it looks like the lock subsystem takes an optional src/dst parameter. If both are set, it tries to take locks on both resources. If only one is set, it takes only that one. In none are set, it takes temporary locks to maintain locking consistency with other clients.

In the move handler, since there is a src and dst, it looks for locks in both places, even if dst is not present or unlocked:

	release, status, err := h.confirmLocks(r, src, dst)

So, even though we've only locked src, and dst doesn't exist, the request will fail because we don't have a valid lock on dst.

Really, the check should be "if there is no condition for a given resource, take a temporary lock on it", rather than "if there are no conditions, take a temporary lock on all".

If there's interest, I can look at implementing something to fix this (as well as #42839). Both issues are currently blocking us, so we need a solution soon (i.e. the next few weeks). I can look at putting up a PR with the fix. I wouldn't mind some guidance from those experienced with this codebase, particularly for this issue, since I think the fix may not be as simple as #42839.

@toothrot
Copy link
Contributor

@toothrot toothrot commented Jan 8, 2021

/cc @bradfitz

klarose added a commit to Agilicus/golang-net that referenced this issue Jan 21, 2021
The WebDAV RFC is pretty clear that if the source and destination of a
MOVE request are locked, then the request must include lock tokens for
both. It doesn't explicitly state that if only one is locked, the
request need only include a token for the one, but that seems fairly
obvious: if the destination of an operation is not locked, a client
should be able to copy or move a locked resource over top of it,
particularly if it doesn't exist in the first place.

Prior to this commit, the lock validation logic required that both the
source and destination of an operation hold a lock if any locks were
presented. Consequently, binary operations where only one of the
resources were locked would always fail. For example, the following
sequence of events would fail with a 412 Precondition Failed:
 - LOCK /foo
 - MOVE /foo (Destination: /bar)

This commit changes the lock validation to only require locks for
resources which are actually locked. It takes some care to ensure that
invalid lock tokens will cause a failure, even if the resources did not
require locks, since both the litmus test and RFC imply that if there
are any conditions, and all conditions fail, the request should fail.

Fixes golang/go#43556
@klarose
Copy link
Author

@klarose klarose commented Jan 21, 2021

I have a potential fix for this. Going to wait for my CLA to go through, but feel free to take a look.

@klarose klarose linked a pull request that will close this issue Jan 22, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Jan 22, 2021

Change https://golang.org/cl/285754 mentions this issue: webdav: only require locks when necessary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants