Hide the query string in NGINX access logs by default

Related to https://gitlab.com/gitlab-com/infrastructure/issues/273 https://gitlab.com/gitlab-org/gitlab-workhorse/issues/71

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/19203

/cc @briann

mentioned in issue gitlab-workhorse#71 (closed)

assigned to @ibaum

@ibaum can you review?

approved this merge request

@briann has asked if we can retain the query string using an approach similar to https://github.com/propublica/sunlight-congress/tree/master/config/nginx#suppressing-user-latitudelongitude-in-our-logs

I did look at this approach, but rejected it out of hand due to its use of if - which I always try to avoid in NGINX.

Thoughts?

(Another alternative I looked into was the use of map - but we'd need nginx 1.11.x to make it work)

changed the description

@nick.thomas I had heard if was unreliable in nginx before, so maybe it's not an option.

nginx is already up to 1.13 so maybe an upgrade is in order?

So we are currently on the legacy stable branch of Nginx, and we should probably stick with stable, so 1.12 would be the next version. That does have the map feature I think @nick.thomas is referring to, the ability to mix strings and variables in the results. Unfortunately, that seems like the best option for what we need here.

So we probably need to schedule an nginx upgrade before this can proceed. I'll open an issue.

mentioned in issue #2393 (closed)

If #2393 (closed) happens, I'll change this MR so we just redact rss_token, private_token and authenticity_token from the query string of request and referer. It'll be a long set of maps and it won't protect query strings like ...&private_token=x&private_token=y, but otherwise it'll be fine.

we should probably stick with stable

From the nginx folks themselves:

Note that stable does not mean more reliable or more bug-free. In fact, the mainline is generally regarded as more reliable because we port all bug fixes to it, and not just critical fixes as for the stable branch. On the other hand, changes in the stable branch are very unlikely to affect third-party modules. We don’t make the same commitment concerning the mainline, where new features can affect the operation of third-party modules.

Reference: https://www.nginx.com/blog/nginx-1-6-1-7-released/

@nick.thomas Nginx bump won't be done before the next cycle.

@marin no problem

Looks like we're up to 1.12.1. Can this be merged now?

@briann not yet, this is still using the "remove the whole query string" approach.

I'll look into getting it updated as soon as https://gitlab.com/gitlab-org/gitlab-ee/issues/98 is done.

assigned to @nick.thomas

added 553 commits

• 67b4c306...9f9a0c74 - 552 commits from branch master
• f85579af - Hide the query string in NGINX access logs by default

Compare with previous version

assigned to @marin

added Unscheduled label

@marin here's how it looks with the map approach. We have to run through the request URI several times, once per parameter we want to remove :-S. Perhaps the regexps can be cleaned up somewhat, too?

Since it's quite messy, I decided not to replicate it for Referer. Just removing the query string entirely should be fine there.

Note that query string parameters may still be outputted to the error.log, e.g., when unicorn is down:

2017/08/08 17:26:45 [error] 7620#7620: *1 connect() failed (111: Connection refused) while connecting to upstream, client: 127.0.0.1, server: , request: "GET /foo?other_token=1&private_token=foo HTTP/1.1", upstream: "http://127.0.0.1:3000/foo?other_token=1&private_token=foo", host: "localhost:3443"

/cc @briann

Assuming we decide to go with this approach, do we want to replicate it in the NGINX example files in the gitlab-ce repository as well as have it in omnibus?

Ugh, forced into such an inelegant solution.

It would be nice to update the examples but I don't think it's absolutely necessary.

@nick.thomas This is less than ideal, I agree We don't have other options though as far as I know so this will have to do. Can I ask you to add an example of the query we expect to see coming in and going out as comments above each map call? We have no way of testing this so at least we should have a quick idea of what we expect.

Also, please update the CE examples too. We should aim to keep them as similar as possible.

added 22 commits

• bd5ca88e...afa95f58 - 20 commits from branch master
• 1aea5b38 - Remove sensitive params from the NGINX access logs
• 0580bf91 - Add changelog

Compare with previous version

added 16 commits

• 0580bf91...be5c8be5 - 14 commits from branch master
• 6983fe59 - Remove sensitive params from the NGINX access logs
• 179104aa - Add changelog

Compare with previous version

Thanks @marin

• Add examples of transformation
• Create MR in gitlab-ce adding the maps to lib/support/nginx/... https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/13453

I agree that it's pretty nasty, but if we want to preserve some of the query string I don't know of a better way!

added 99 commits

• 179104aa...fc179702 - 98 commits from branch master
• a5dffe3a - Merge branch 'master' into 'fix-hide-query-string'

Compare with previous version

changed milestone to %10.0

approved this merge request

enabled an automatic merge when the pipeline for a5dffe3a succeeds

merged

mentioned in commit 67210a1c