Log user from basic authentication requests

[carlssonia]

depends on my pull request to snap-core.

[hvr]

Just a few remarks:

1. Why only support basic access authentication and not digest access authentication as well?
2. There doesn't seem to be any sanitation of the username string for the purpose of logging, this way an attacker could inject malicious bytestrings into the access logfile
3. @gregorycollins are there any performance considerations w.r.t. to the overhead required to extract the authentication headers of all HTTP requests just for logging purposes, even if only very few urls of the web-app have support for http-auth -- what I'm trying to say here: imho the username should only be extracted and logged, if the respective snap handler cares about the authentication

[carlssonia]

1. I only implemented basic authentication since that's what I use in a REST application (the requests are over SSL).
2. If one is worried about content in the log file, doesn't one also need to sanitize the other elements? For example, control characters in the user agent or the url are passed through to the log.

[gregorycollins]

I'm not sure we want to add this overhead to every request, no. If you're going to make every thread pay a tax here, at least make it a cheaper tax -- add a field rqUser to the Request type (and a setRqUser :: ByteString -> Request -> Request) and update this field when you authenticate the user.

If you do it that way, logging the user name is a Maybe decons (and other authentication methods can reuse the field) instead of a HashMap lookup that usually fails + a bunch of http-basic-auth-specific string twiddling we probably did once already.

[carlssonia]

@gregorycollins I could add the rqUser field, but then I would want to change the semantics of logging in Snap.Internal.Http.Server.httpSession. Currently, it logs the unmodified request, and not the request returned by the handler. Apart from a semantic change, it means holding on to the unmodified request until after the response has been served. I don't know if this could change the space behavior. What do you think?

More problematic is that at least Snap.Internal.Routing.route runs the handler in localRequest, which means that all changes to the request are thrown away.

It would a lot easier to put a user field in the Response, although it might seem a bit counter-intuitive. How would you feel about that?

[gregorycollins]

@carlssonia I'd have no problem with changing the way anything is logged inside snap-server, logging is something we haven't spent a lot of time on in here. Space behaviour is not an issue here, Request objects are relatively small (and we don't hold on to the request body itself in there, of course).

After reading your message, I actually agree that it might be easier to put a user field in the Response, but I'm not sure it makes semantic sense to do this. We probably want to do some thinking about this before moving in any particular direction; until it feels like we have the right design, I'm not sure that we should move forward.

[carlssonia]

@gregorycollins - Sorry for letting this sit for this long unattended!

Any chance that a patch with a user field in Response would be acceptable? I would love to get something working here since it's difficult to replicate the logging function outside Snap.Internal.Http.Server. This is because stuff like response content length is only available there.

Another approach could be to add a Response field that is a custom logging function, say with the signature of System.Fastlogger.combinedLogEntry. If Snap.Internal.Http.Server.logA' abstracted out its use of combinedLogEntry and used the one in the response (which would default to combinedLogEntry), client code could add arbitrary information to the access log, so it would be more generally useful.

Would you prefer this more general approach?