improve authentication

[chripo]

fix #14 in commit b45378c changed the behaviour of the req() method. the auth checks were moved from Connect() to req().

how to improve the code to keep the modification inside the client, because it's a client improvement and to keep the request as small as possible.

suggestions and discussion are welcome!

[chuckwagoncomputing]

Good points. We'd have to add a check on each of [ReadDir, Stat, et. al.]. Maybe add a function to do the checking, initialize auth, and call the calling function again, passed as a parameter? With this approach there will be more code that unnecessarily runs twice.

[MrVine]

If I correctly understand you, @chuckwagoncomputing , you suggest to add a check on each of [ReadDir, Stat, etc] functions? Instead, I want to suggest to create new function which will do the checking and initialize auth, and call it directly from req() function. This function will improve readability of req() to help us to keep the request as small as possible.

What do you think about it?

[chuckwagoncomputing]

From the standpoint of "seperation of concerns", that's essentially what I've done. @chripo's suggestion is that the checking should be the job of the Client part of the library, rather than the Requests part.

[MrVine]

Thanks for explanation. Unfortunately I have no ideas how we can deal with this.

[chuckwagoncomputing]

Further complicating this is the approach I created of duplicating the body in case it needs to be used again. I am ashamed of what I have done but I'm not sure how else to do it. If we do some redesigning, both of these things need to be considered.

[MrVine]

@chripo , to remove TeeReader from req() method we should perform auth checking in separate method. For example, I propose to return c.Connect() method, but use PROPFIND instead of OPTIONS.

[chripo]

From my point of view, sending the body twice is a total waste of performance and time. Adding a strategy for auth would cleanup the code. I worked already on it, but unfinished,i'll share my code soon.

I think there is a way to send the first 6request without a body, need time for a deeper look into the specs / rfcs.

[chuckwagoncomputing]

I haven't actually read the RFCs, but I know some clients do not send the body the first time. SabreDAV has a bug and won't authenticate with an empty body. The maintainer refused to fix it because he claimed it was non-standard to send the first packet without a body. It does have a workaround though.

[MrVine]

I read RFCs, and this things is important for us:

1. We should use application/xml instead of text/xml in Content-Type header inside client.propfind() method:

rs, err := c.req("PROPFIND", path, strings.NewReader(body), func(rq *http.Request) {
	if self {
		rq.Header.Add("Depth", "0")
	} else {
		rq.Header.Add("Depth", "1")
	}
	rq.Header.Add("Content-Type", "text/xml;charset=UTF-8")
	rq.Header.Add("Accept", "application/xml,text/xml")
	rq.Header.Add("Accept-Charset", "utf-8")
	// TODO add support for 'gzip,deflate;q=0.8,q=0.7'
	rq.Header.Add("Accept-Encoding", "")
})

because text/xml is deprecated (RFC4918):

When XML is used for a request or response body, the Content-Type type SHOULD be application/xml. Implementations MUST accept both text/xml and application/xml in request and response bodies. Use of text/xml is deprecated.

2. We must not support Basic authentication method (RFC4918):

Since Basic authentication for HTTP/1.1 performs essentially clear text transmission of a password, Basic authentication MUST NOT be used to authenticate a WebDAV client to a server unless the connection is secure. Furthermore, a WebDAV server MUST NOT send a Basic authentication challenge in a WWW-Authenticate header unless the connection is secure. An example of a secure connection would be a Transport Layer Security (TLS) connection employing a strong cipher suite and server authentication.

3. All information about "Clients desiring to Authenticate" located in Appendix E of RFC2918:

There are a number of ways the client might be able to trigger the server to provide an authentication challenge. This appendix describes a couple approaches that seem particularly likely to work.

I propose to create client.Connect() method, and use the second approach from Appendix E inside it:

The second approach is to use an Authorization header (defined in [RFC2617]), which is likely to be rejected by the server but which will then prompt a proper authentication challenge. For example, the client could start with a PROPFIND request containing an Authorization header containing a made-up Basic userid:password string or with actual plausible credentials.

[chuckwagoncomputing]

@chripo, have you got anywhere with this?

[pdf]

@MrVine wrote:

We must not support Basic authentication method (RFC4918)

That makes no sense for a client library. The RFC states that servers must not present a Basic challenge, unless they're served over a secure connection (ie - TLS). I'd suggest that Basic auth is very widely implemented, as generally connections will be TLS encrypted. I'd be very concerned about any authentication over insecure channels, as leaking bearer tokens or other form of auth is frequently just as bad as leaking a username/password. In fact, Basic auth must be supported, for servers that only support it, and it's likely the only globally supported mechanism.

I propose to ... use the second approach from Appendix E inside it

I see very little value in complicating the auth mechanism in this direction, based on the above, and also based on the caveats listed in the RFC.