mnutt / davros

Personal file storage server
Apache License 2.0
298 stars 35 forks source link

Davros should respond with a DAV header on an unauthorized OPTIONS request. #74

Open russellmcc opened 7 years ago

russellmcc commented 7 years ago

Many webdav clients check if the server supports webdav by sending an unauthorized OPTIONS requests. Currently, when running in sandstorm, we fail this test because our response does not include the DAV header.

Here's the results of a simple test on an instance in oasis:

curl -IL -X OPTIONS https://api-302626db96182d25f56dc3813d4640fc.oasis.sandstorm.io/remote.php/webdav

response:

HTTP/1.1 200 OK
Server: nginx/1.10.1
Date: Sun, 16 Oct 2016 20:42:38 GMT
Transfer-Encoding: chunked
Connection: keep-alive
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: GET, HEAD, POST, PUT, PATCH, DELETE
Access-Control-Max-Age: 3600
Strict-Transport-Security: max-age=31536000

Note that authorized OPTIONS work just fine:

curl -IL -X OPTIONS https://testing-123:<password>@api-302626db96182d25f56dc3813d4640fc.oasis.sandstorm.io/remote.php/webdav
HTTP/1.1 200 OK
Server: nginx/1.10.1
Date: Sun, 16 Oct 2016 20:43:43 GMT
Content-Length: 0
Connection: keep-alive
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: GET, HEAD, POST, PUT, PATCH, DELETE
Access-Control-Max-Age: 3600
DAV: 1, 2, 3, extended-mkcol
Access-Control-Expose-Headers: DAV
Strict-Transport-Security: max-age=31536000

It seems that sandstorm has some logic for supporting DAV headers on unauthorized OPTIONS requests (@paulproteus on irc helped me find this). See https://github.com/sandstorm-io/sandstorm/blob/master/shell/server/proxy.js#L1091 . This functionality was introduced in https://github.com/sandstorm-io/sandstorm/commit/b87c7670bf3d239a5e7a4bde7b11fd2d6612ba40 . I'm not sure how to populate the static DAV header option in an app, but if davros could do that it seems it would be able to support many more DAV clients.

paulproteus commented 7 years ago

BTW @kentonv, as the author of https://github.com/sandstorm-io/sandstorm/commit/b87c7670bf3d239a5e7a4bde7b11fd2d6612ba40 , can you explain how Davros should be integrating with that codepath? Or is it doing things correctly, at the moment?

Thanks for any insight you can provide. I can write up docs for docs.sandstorm.io if that makes it easier or more compelling for you to answer here.

mnutt commented 7 years ago

We use unauthenticated resources here: https://github.com/mnutt/davros/blob/84f22eba6b44c6663839e164989110166f5b696a/app/lib/owncloud.js#L11-L19 and could probably add one for /remote.php/webdav.

Is the intention to send back a 401 Unauthorized with Dav: 1, 2, 3, extended-mkcol? And do you know of some clients that do the unauthenticated OPTIONS check?

russellmcc commented 7 years ago

Transmit iOS app and the mac os finder (which uses WebDAVLibs: https://opensource.apple.com/source/webdavfs/webdavfs-293/webdavlib/webdavlib.c) use unauthenticated OPTIONS.

russellmcc commented 7 years ago

Note that this seems to be a sandstorm-specific issue, all these clients respond correctly when running davros outside of sandstorm.

russellmcc commented 7 years ago

Reading the apple code, it seems like it would accept either 401 unauthorized or 200 with the proper DAV headers. Responding with 200 with no dav headers makes apple think it's not a dav server, which is reasonable.

mnutt commented 7 years ago

Sadly, in testing it with the extra options added to the offer template, I now get a "401 unauthorized" from Transmit. My guess at this point, based on reading that apple code (search for ctx-serverAuth), is that by returning a 200 initially the webdav client now no longer thinks any of the requests need authentication.

russellmcc commented 7 years ago

Uh-oh! What's the right behavior for a DAV server that requires auth? Should we return 401 on OPTIONS?

kentonv commented 7 years ago

So, does the unauthenticated resources API need to be extended to support returning a different kind of response in some situations? (I don't totally follow what's going on here...)

mnutt commented 7 years ago

I'm not sure what the "correct" behavior is, but at least according to apple's source, they assume that a server will either be authenticated or unauthenticated based on the first request, and follow through with that assumption on all subsequent requests. I would tend to agree with them in that the authorization mechanism should be consistent across http types, but other clients have gone the other direction. Notably OwnCloud Android (among others) has a totally convoluted authentication flow that requires a 200 OPTIONS response even for servers that provide credentials. I believe OPTIONS requests in CORS may have a reason for being unauthenticated as well?

kentonv commented 7 years ago

So you are saying that different clients have incompatible requirements? Does this mean there's no way to be compatible with both clients at once?

russellmcc commented 7 years ago

So, transmit as far as I know is totally closed source. Reading the code more deeply, the apple webdavfs library does seem to work so long as we never return 200 without DAV headers.

There could just be a bug in Transmit's auth logic that is making it incorrectly bail out early.

@mnutt I'd be happy to try test out whether the apple finder works after adding the DAV options to the offer template if you can guide me through how to do it or point me to a branch.

mnutt commented 7 years ago

I've pushed up my branch that adds unauthenticated DAV headers here:

https://github.com/mnutt/davros/compare/mn/unauthenticated-dav-headers

From what I've seen so far, they send an unauthorized 200 OPTIONS (expected), they see the DAV headers, then send a PROPFIND without Authorization header (also expected, as generally http client libs will try without an Authorization header first, then retry with header when challenged) and we present a 401 as expected, then they give up rather than sending the Authorization header.

mnutt commented 7 years ago

@kentonv I think it's may be possible to support both OwnCloud Mobile and Transmit. OwnCloud has a proprietary /status.php which needs to respond unauthenticated, but doesn't explicitly depend on any other OPTIONS requests being unauthenticated. I think we can make /status.php unauthenticated via the offerTemplate without making all OPTIONS unauthenticated, but was there another application that needed all OPTIONS responses to be unauthenticated?

russellmcc commented 7 years ago

Definitely any browser-based clients want a 200 response to unauthenticated OPTIONS since CORS pre-flights don't send authentications.

kentonv commented 7 years ago

If all else fails, I suppose you could always ask the user what client they are using before generating the offer template.

russellmcc commented 7 years ago

Hey, I've been looking at this a little bit more, focusing on the Mac Finder. It seems like auth is actually okay on the branch with the unauthenticated DAV headers, however something else is going wrong. The error I get from the console is:

default 19:40:07.341284 -0400   webdavfs_agent  network_mount: URL is not a collection resource (directory); file: /BuildRoot/Library/Caches/com.apple.xbs/Sources/webdavfs/webdavfs-373/mount.tproj/webdav_network.c; line: 2775

It looks like this is, in turn caused by the fact that the response to the PROPFIND request is gzip-encoded, even though the request has no Accept-Encoding header and the davros app returns un-gzipped content. So, it looks like on that branch the only barrier to getting the apple client to work is sandstorm over-eagerly gzipping.

russellmcc commented 7 years ago

I've spent some time now debugging the Transmit client. It looks like they have an issue where they never send auth headers if the auth realm in the response has a space in it. I've contacted them about this and hopefully they fix it on their end.

The following header makes Transmit choke:

                WWW-Authenticate: Basic realm='Sandstorm API'

This one makes Transmit work perfectly:

                WWW-Authenticate: Basic realm='SandstormAPI'
mnutt commented 7 years ago

Oh weird, that's a great find! Thanks!

SailReal commented 3 years ago

I have also just stumbled across this issue. The OkHttp client does not allow spaces when the realm value is single quoted.

"WWW-Authenticate: Basic realm=\"Sandstorm API\"" and "WWW-Authenticate: Basic realm='SandstormAPI'" works but "WWW-Authenticate: Basic realm='Sandstorm API'" not.

The RFC7617#section-2 only says that it can be a token or a quoted string with the example "WallyWorld". I just tested it with a few servers from https://community.cryptomator.org/t/webdav-urls-of-common-cloud-storage-services/75 which all responds with the double quoted variant. But other clients also accepts blanks in single quoted string. So I really don't know exactly which party I should ask to fix this. I will also open an issue with the OkHttp client and link it here.

Just a short update: As a collaborator of OkHttp mentioned in https://github.com/square/okhttp/issues/6743#issuecomment-873639272, the value of the realm must be a double quoted string according to https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.6.