Fix audit issue logging by default peer address #1485
Conversation
By default log format include remote address that is taken from headers.
This is very easy to replace making log untrusted.
Changing default log format value `%a` to peer address we are getting
this trusted data always. Also, remote address option is maintianed and
relegated to `%{r}a` value.
Related kanidm/kanidm#191.
|
So if I have this right, you take issue with the fact that the current %a replacement uses the derived "Client IP" which could be taken from X-Forwarded-For headers which are obviously easy to replace if your proxy is misconfigured (or you don't have a proxy and the header is being sent manually). There's a couple bits of terminology I want to nail down, document, and be strict to use in the right places.
Could you elaborate on the above so there is no confusion in reviewing? Might be worth adding doc comments to the ConnectionInfo fields to describe when and what they are populated with. |
Codecov Report
@@ Coverage Diff @@
## master #1485 +/- ##
==========================================
+ Coverage 47.58% 47.66% +0.07%
==========================================
Files 137 137
Lines 12925 12952 +27
==========================================
+ Hits 6151 6174 +23
- Misses 6774 6778 +4
Continue to review full report at Codecov.
|
Change names to avoid naming confusions. I choose this accord to Nginx variables and [ngx_http_realip_module](https://nginx.org/en/docs/http/ngx_http_realip_module.html). Add more specific documentation about security concerns of using Real IP in logger.
|
@robjtede thanks for your comment! |
|
@JohnTitor both done. Must I rebase all changes in one? |
|
Any update? |
By default log format include remote address that is taken from headers.
This is very easy to replace making log untrusted.
Changing default log format value
%ato peer address we are gettingthis trusted data always. Also, remote address option is maintianed and
relegated to
%{r}avalue.Related kanidm/kanidm#191.