On 19 Nov 2011, at 4:49 AM, William A. Rowe Jr. wrote:
Nevermind that you failed to be consistent in tag values between logging schemas...
Can you confirm in more detail what you're referring to? There is only one logging line in the patch, and this was based on existing logging lines in the same module. Patches should ideally change just one thing at a time, but if there are things to fix we should fix them.
nothing in this proposal addresses the reason that I myself had implemented mod_remoteip, which was authn/authz control. In the limited scenario you have considered, authn is pretty much a noop on the physical address (no public client would ever be routable to that server anyways) so access control is shifted to the consumer of the web resources. This patch would have a long way to go before being considered... and is certainly not a 2.4.x candidate.
The way I approached this was to hunt through the code for instances of c->remote_ip and c->remote_addr and address each part of the code they touched. In the aaa code I only found reference to them in mod_authz_host.c, there was no reference anywhere else. Can you be more specific about the authn functionality you believe was missed?
The very design of mod_remoteip keeps the precious values that you are concerned about losing as request notes suitable for logging. But Stefan also points out some flaws in the current approach.
Having built a module like this before, I fully appreciate just how hard it is to keep the overridden IP address alive long enough to be logged given that logging happens in a cleanup, and the module manages to do it, but the way it's done now is definitely wrong. We allow dangling pointers allocated from a destroyed r->pool to exist in c- >pool structures in the hope that the next request will clean it up, and we hope that nobody tries to read c->remote_ip before we do. We also slowly leak from c->pool. I recognise this is done to work around restrictions to the existing v2.2 API, but as new code, we should fix the API and do this properly - none of this should be released in v2.4 in it's current form.
The correction is simple; promote the remote_ip up to the request rec and log/use for authentication that r->remote_ip throughout httpd. Introduce a wire client logging tag for c->remote_ip.
This is a lot simpler and cleaner I think, let me come up with an alternative patch.
Regards, Graham --
