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: fails when listing a directory with subdirs that cannot be read #43782

Open
klarose opened this issue Jan 19, 2021 · 3 comments · May be fixed by golang/net#91
Open

x/net/webdav: fails when listing a directory with subdirs that cannot be read #43782

klarose opened this issue Jan 19, 2021 · 3 comments · May be fixed by golang/net#91

Comments

@klarose
Copy link

@klarose klarose commented Jan 19, 2021

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

go version go1.15 linux/amd64

Does this issue reproduce with the latest release?

I assume so, since the code in question hasn't changed.

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

go env Output
$ go env
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"
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-build416020135=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Exposed a webdav fileshare from /tmp, which has a few directories for which I do not have permission. E.g. in my case it's something like this:

drwx------    2 root root    4096 Jan 11 11:02 tel-XXXXXXX

I mounted it with the davfs2 filesystem for linux.

What did you expect to see?

I expect to see listed all files/subdirectories that the server is allowed to access. Failing that, I do not expect to see a failed transaction with a 'success' response code.

What did you see instead?

When mounting the service:

/sbin/mount.davfs: Mounting failed.
XML parse error at line 1: Extra content at the end of the document

From the server:

2021/01/19 15:14:29 http: superfluous response.WriteHeader call from golang.org/x/net/webdav.(*Handler).ServeHTTP (webdav.go:76)

Tracing shows that failure is when handling the initial PROPFIND. The webdav server tries to open a file for which it does not have permission, and it fails. However, it fails after already writing content and returning a response code of 207. The error returned is "Error open /tmp/tel-XXXXXXX: permission denied".

@gopherbot gopherbot added this to the Unreleased milestone Jan 19, 2021
@klarose
Copy link
Author

@klarose klarose commented Jan 19, 2021

Given the existence of #16195, I suspect there are is general class of issues when performing directory listings: things the server cannot recurse into, but which should not cause a failure.

In particular, I think that if the server wouldn't normally be able to access a file or directory, that file or directory should either not be displayed in the response to PROPFIND, or it should be displayed, but not recursed into further (if that's even an option). If a user tries to access a file which the service cannot, return an appropriate error then, but do not block the service from working at all simply because a single file or directory within the share is broken. That just makes it far too fragile.

@klarose
Copy link
Author

@klarose klarose commented Jan 19, 2021

I've read the RFC a bit more closely, and it looks like the proper solution may be to indicate the failure in the propstat elements. In particular, note http://www.webdav.org/specs/rfc4918.html#rfc.section.9.1.p.8 and http://www.webdav.org/specs/rfc4918.html#rfc.section.9.1.2.p.4

I think that implies that if the file is simply unreadable (e.g. a broken link) it should be skipped, and if it is there, but the server does not has permission, it should return a 403 for that particular element.

klarose added a commit to Agilicus/golang-net that referenced this issue Jan 20, 2021
PROPFIND can walk through directories, retrieving information about
each file. Unfortunately, the filesystem may deny access to the WebDAV
server for various reasons, such as the file truly not being readable
(e.g. a broken symlink), or because the server does not have permission
to read the file. PROPFIND should ignore these.

The current behaviour of the WebDAV server when encountering such issues
is to immediately stop its walk, and output an http 500. This leads to
poor behaviour with the builtin golang server, since the walk has likely
already written out its status header as part of serving the previously
walked files' properties. The server closes the response, also emitting
an error log.

While the error log is noisy, the closed response is truly an issue: the
xml returned to the client is invalid, which means that the response is
useless. It is not unreasonable to expect that a directory shared using
WebDAV has files which cannot be read for the reasons given above. The
shared directory becomes useless with the current behavior.

Rather than making directories with unreadable files useless, skip over
anything that is bad. A more nuanced solution to this problem could
likely involve indicating that the requested properties have problems,
or buffering the response prior to returning the failure. However, this
change is simple and a move in the right direction.

Fixes golang/go#16195
Fixes golang/go#43782
@gopherbot
Copy link

@gopherbot gopherbot commented Jan 22, 2021

Change https://golang.org/cl/285752 mentions this issue: webdav: ignore path and perm errors in PROPFIND

drakkan added a commit to drakkan/net that referenced this issue Mar 24, 2021
PROPFIND can walk through directories, retrieving information about
each file. Unfortunately, the filesystem may deny access to the WebDAV
server for various reasons, such as the file truly not being readable
(e.g. a broken symlink), or because the server does not have permission
to read the file. PROPFIND should ignore these.

The current behaviour of the WebDAV server when encountering such issues
is to immediately stop its walk, and output an http 500. This leads to
poor behaviour with the builtin golang server, since the walk has likely
already written out its status header as part of serving the previously
walked files' properties. The server closes the response, also emitting
an error log.

While the error log is noisy, the closed response is truly an issue: the
xml returned to the client is invalid, which means that the response is
useless. It is not unreasonable to expect that a directory shared using
WebDAV has files which cannot be read for the reasons given above. The
shared directory becomes useless with the current behavior.

Rather than making directories with unreadable files useless, skip over
anything that is bad. A more nuanced solution to this problem could
likely involve indicating that the requested properties have problems,
or buffering the response prior to returning the failure. However, this
change is simple and a move in the right direction.

Fixes golang/go#16195
Fixes golang/go#43782
drakkan added a commit to drakkan/net that referenced this issue Mar 28, 2021
PROPFIND can walk through directories, retrieving information about
each file. Unfortunately, the filesystem may deny access to the WebDAV
server for various reasons, such as the file truly not being readable
(e.g. a broken symlink), or because the server does not have permission
to read the file. PROPFIND should ignore these.

The current behaviour of the WebDAV server when encountering such issues
is to immediately stop its walk, and output an http 500. This leads to
poor behaviour with the builtin golang server, since the walk has likely
already written out its status header as part of serving the previously
walked files' properties. The server closes the response, also emitting
an error log.

While the error log is noisy, the closed response is truly an issue: the
xml returned to the client is invalid, which means that the response is
useless. It is not unreasonable to expect that a directory shared using
WebDAV has files which cannot be read for the reasons given above. The
shared directory becomes useless with the current behavior.

Rather than making directories with unreadable files useless, skip over
anything that is bad. A more nuanced solution to this problem could
likely involve indicating that the requested properties have problems,
or buffering the response prior to returning the failure. However, this
change is simple and a move in the right direction.

Fixes golang/go#16195
Fixes golang/go#43782
drakkan added a commit to drakkan/net that referenced this issue Apr 5, 2021
PROPFIND can walk through directories, retrieving information about
each file. Unfortunately, the filesystem may deny access to the WebDAV
server for various reasons, such as the file truly not being readable
(e.g. a broken symlink), or because the server does not have permission
to read the file. PROPFIND should ignore these.

The current behaviour of the WebDAV server when encountering such issues
is to immediately stop its walk, and output an http 500. This leads to
poor behaviour with the builtin golang server, since the walk has likely
already written out its status header as part of serving the previously
walked files' properties. The server closes the response, also emitting
an error log.

While the error log is noisy, the closed response is truly an issue: the
xml returned to the client is invalid, which means that the response is
useless. It is not unreasonable to expect that a directory shared using
WebDAV has files which cannot be read for the reasons given above. The
shared directory becomes useless with the current behavior.

Rather than making directories with unreadable files useless, skip over
anything that is bad. A more nuanced solution to this problem could
likely involve indicating that the requested properties have problems,
or buffering the response prior to returning the failure. However, this
change is simple and a move in the right direction.

Fixes golang/go#16195
Fixes golang/go#43782
drakkan added a commit to drakkan/net that referenced this issue Apr 7, 2021
PROPFIND can walk through directories, retrieving information about
each file. Unfortunately, the filesystem may deny access to the WebDAV
server for various reasons, such as the file truly not being readable
(e.g. a broken symlink), or because the server does not have permission
to read the file. PROPFIND should ignore these.

The current behaviour of the WebDAV server when encountering such issues
is to immediately stop its walk, and output an http 500. This leads to
poor behaviour with the builtin golang server, since the walk has likely
already written out its status header as part of serving the previously
walked files' properties. The server closes the response, also emitting
an error log.

While the error log is noisy, the closed response is truly an issue: the
xml returned to the client is invalid, which means that the response is
useless. It is not unreasonable to expect that a directory shared using
WebDAV has files which cannot be read for the reasons given above. The
shared directory becomes useless with the current behavior.

Rather than making directories with unreadable files useless, skip over
anything that is bad. A more nuanced solution to this problem could
likely involve indicating that the requested properties have problems,
or buffering the response prior to returning the failure. However, this
change is simple and a move in the right direction.

Fixes golang/go#16195
Fixes golang/go#43782
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