Allow filtering of private-token in gitlab-workhorse

Overview

The private-token is retracted from the GitLab rails application logs, however it is still contained in NGINX and gitlab-workhorse logs. Can we enable scrubbing of the private token in these logs as well.

This is a security related issue for GitLab.com and Logging to https://log.gitlap.com/app/kibana (marked as confidential for that reason)

//cc @jacobvosmaer-gitlab @stanhu

I think the real issue here is that we allow these private tokens to be passed as query parameters. That means they can get logged anywhere along the chain. (On gitlab.com that also means HAproxy, for instance.)

Scrubbing these tokens in gitlab-workhorse is doable but it will probably not be pretty. I think the only reasonable way to hide these tokens from NGINX access logs is to stop logging query strings altogether. That would then also be the preferred solution in gitlab-workhorse.

FYI, Rails already does scrubbing of specific query string tokens.

I think the only sane option, if we want this, is to not print the query string in the log at all, both in NGINX and gitlab-workhorse. Which of course does not help any HTTP load balancers higher up the chain (such as HAProxy on gitlab.com).

We could at least plug this leak in gitlab-workhorse and our NGINX template.

http://stackoverflow.com/questions/19265766/how-to-not-log-a-get-request-parameter-in-the-nginx-access-logs

Moved from gitlab-org/gitlab-ce#19203

Would you like to work on this @nick.thomas ?

I can, but I'm on another security issue at the moment so it'll have to wait until that's finished. I'll leave it unassigned but TODO until then, in case someone else wants to grab it in the meantime.

Added ~901417 label

This has come up again as part of the Risk Assessment and is one of the top 10 priorities. I've added this issue to the Risk Assessment as a course of action.

@nick.thomas Any time to take another look?

Whoops, sorry, I completely forgot to return to this.

I'll try to get it done for %9.3 . I agree that completely removing the querystring is better than trying to filter it.

assigned to @nick.thomas

changed milestone to %9.3

changed title from Allow filtering of private-token in NIGNX and gitlab-workhorse to Allow filtering of private-token in NGINX and gitlab-workhorse

changed the description

We log net/http.RequestURI in a number of places; these can be replaced with Path.

We also log the HTTP Referer header in a few places; it's possible these could include the private token. I'll change those to log the referer without a query string.

I agree with @jacobvosmaer-gitlab that we should stop using the private token in query strings more generally. Its security characteristics are very poor. However, it's still worth cleaning the logs at present, I think.

mentioned in merge request !165 (merged)

@nick.thomas Agreed on the token use in query strings. I'd prefer if we didn't completely strip the query strings from referrers and request logs unless it's the only way to do it. There might be useful stuff in there. Is it possible to replace the token with [token] like we do with the Rails logs? Or is it worth the effort?

@briann It's definitely possible, including in NGINX, but I question whether it's worthwhile. Do we often make use of the query strings in debugging sessions?

Essentially, we'd just run a replace of /[\?&]private-token=.*/ on the string before outputting it. It's not very expensive.

Are there other query-string parameters that might be sensitive?

@nick.thomas The CSRF/authenticity_token shows up in request logs a lot and should be considered sensitive.

I just want to make sure that the query string is logged somewhere, with the appropriate filtering. We do use it for security purposes and probably debugging as well. If that's only in NGINX and not anywhere else that's fine. The referrer field is nice to have as well but also doesn't need to be saved in more than one place.

OK, so that's known as authenticity_token: https://github.com/rails/rails/blob/v4.2.8/actionpack/lib/action_controller/metal/request_forgery_protection.rb#L62

Yes. It's not anywhere near as sensitive as a private or access token, though. So it's not a priority.

changed title from Allow filtering of private-token in NGINX and gitlab-workhorse to Allow filtering of private-token in gitlab-workhorse

changed the description

NGINX turns out to be much more difficult to filter, so I propose we just remove the query string from referer and request. We'll have them (filtered) in the workhorse logs.

mentioned in merge request omnibus-gitlab!1585 (merged)

NGINX MR: https://gitlab.com/gitlab-org/omnibus-gitlab/merge_requests/1585

@nick.thomas What about something like this: https://github.com/propublica/sunlight-congress/tree/master/config/nginx#suppressing-user-latitudelongitude-in-our-logs

I'd hate to strip the query string from the nginx logs completely, as I bet that's the primary way people monitor traffic to their GitLab instances.

We're tracking NGINX elsewhere and !165 (merged) is merged, so let's close this.

@briann are you OK for us to mark this issue as non-confidential now? Or do we need to wait until we've got this deployed to gitlab.com and purged historic kibana data?

closed

@nick.thomas I don't have any problem with making this issue public. I do feel a bit weird closing it before it's been resolved.

Typically, issues get closed when the fix makes it to master - not when the fix makes it to a release.

We have https://gitlab.com/gitlab-org/gitlab-ce/issues/19203 to track NGINX; perhaps we need another issue to track any action required by infrastructure?

made the issue visible to everyone

No, you're right. I got this issue confused with the nginx issue.