opendev
/
gerrit
Code Issues Proposed changes

Filter out access_token when logging exception 

When the user uses OAuth for accessing Gerrit, the query parameters
contain an access token. This token is used for authenticating the user
with Gerrit. It should never be logged as a logged token could be used
to impersonate the user the user when sending requests to Gerrit.

Previously, we already redacted the access token from Jetty logs, now
we move this logic to RestApiServlet to redact it in other environments
as well. We'll leave it in the Jetty logger for now as the filtering is
cheap and there might be other logging sources besides RestApiServlet.

Change-Id: Id7c207f697f53f319ff9f959754a95a3f5f92409
This commit is contained in:
Patrick Hiesel 2018-10-08 09:05:33 +02:00
parent 7adc8f25a7
commit 76e588b688
5 changed files with 97 additions and 83 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

57
java/com/google/gerrit/httpd/restapi/LogRedactUtil.java Normal file
View File

@ -0,0 +1,57 @@
// Copyright (C) 2018 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// WARNING: NoteDbUpdateManager cares about the package name RestApiServlet lives in.
package com.google.gerrit.httpd.restapi;
import static com.google.gerrit.httpd.restapi.RestApiServlet.XD_AUTHORIZATION;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.extensions.restapi.Url;
import java.util.Iterator;
public class LogRedactUtil {
private static final ImmutableSet<String> REDACT_PARAM = ImmutableSet.of(XD_AUTHORIZATION);
private LogRedactUtil() {}
/**
* Redacts sensitive information such as an access token from the query string to make it suitable
* for logging.
*/
@VisibleForTesting
public static String redactQueryString(String qs) {
StringBuilder b = new StringBuilder();
for (String kvPair : Splitter.on('&').split(qs)) {
Iterator<String> i = Splitter.on('=').limit(2).split(kvPair).iterator();
String key = i.next();
if (b.length() > 0) {
b.append('&');
}
b.append(key);
if (i.hasNext()) {
b.append('=');
if (REDACT_PARAM.contains(Url.decode(key))) {
b.append('*');
} else {
b.append(i.next());
}
}
}
return b.toString();
}
}

3
java/com/google/gerrit/httpd/restapi/RestApiServlet.java
View File

@ -1408,10 +1408,9 @@ public class RestApiServlet extends HttpServlet {
Throwable err, HttpServletRequest req, HttpServletResponse res) throws IOException {
String uri = req.getRequestURI();
if (!Strings.isNullOrEmpty(req.getQueryString())) {
uri += "?" + req.getQueryString();
uri += "?" + LogRedactUtil.redactQueryString(req.getQueryString());
}
logger.atSevere().withCause(err).log("Error in %s %s", req.getMethod(), uri);
if (!res.isCommitted()) {
res.reset();
return replyError(req, res, SC_INTERNAL_SERVER_ERROR, "Internal server error", err);

39
java/com/google/gerrit/pgm/http/jetty/HttpLog.java
View File

@ -14,18 +14,12 @@
package com.google.gerrit.pgm.http.jetty;
import static com.google.gerrit.httpd.restapi.RestApiServlet.XD_AUTHORIZATION;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Splitter;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.httpd.GetUserFilter;
import com.google.gerrit.httpd.restapi.LogRedactUtil;
import com.google.gerrit.server.util.SystemLog;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.Inject;
import java.util.Iterator;
import org.apache.log4j.AsyncAppender;
import org.apache.log4j.Level;
import org.apache.log4j.Logger;
@ -39,7 +33,6 @@ import org.eclipse.jetty.util.component.AbstractLifeCycle;
class HttpLog extends AbstractLifeCycle implements RequestLog {
private static final Logger log = Logger.getLogger(HttpLog.class);
private static final String LOG_NAME = "httpd_log";
private static final ImmutableSet<String> REDACT_PARAM = ImmutableSet.of(XD_AUTHORIZATION);
interface HttpLogFactory {
HttpLog get();
@ -87,8 +80,9 @@ class HttpLog extends AbstractLifeCycle implements RequestLog {
);
String uri = req.getRequestURI();
uri = redactQueryString(uri, req.getQueryString());
if (!Strings.isNullOrEmpty(req.getQueryString())) {
uri += "?" + LogRedactUtil.redactQueryString(req.getQueryString());
}
String user = (String) req.getAttribute(GetUserFilter.USER_ATTR_KEY);
if (user != null) {
event.setProperty(P_USER, user);
@ -106,31 +100,6 @@ class HttpLog extends AbstractLifeCycle implements RequestLog {
async.append(event);
}
@VisibleForTesting
static String redactQueryString(String uri, String qs) {
if (Strings.isNullOrEmpty(qs)) {
return uri;
}
StringBuilder b = new StringBuilder(uri);
boolean first = true;
for (String kvPair : Splitter.on('&').split(qs)) {
Iterator<String> i = Splitter.on('=').limit(2).split(kvPair).iterator();
String key = i.next();
b.append(first ? '?' : '&').append(key);
first = false;
if (i.hasNext()) {
b.append('=');
if (REDACT_PARAM.contains(Url.decode(key))) {
b.append('*');
} else {
b.append(i.next());
}
}
}
return b.toString();
}
private static void set(LoggingEvent event, String key, String val) {
if (val != null && !val.isEmpty()) {
event.setProperty(key, val);

35
javatests/com/google/gerrit/httpd/restapi/HttpLogRedactTest.java Normal file
View File

@ -0,0 +1,35 @@
// Copyright (C) 2017 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.httpd.restapi;
import static com.google.common.truth.Truth.assertThat;
import org.junit.Test;
public class HttpLogRedactTest {
@Test
public void redactAuth() {
assertThat(LogRedactUtil.redactQueryString("query=status:open")).isEqualTo("query=status:open");
assertThat(LogRedactUtil.redactQueryString("query=status:open&access_token=foo"))
.isEqualTo("query=status:open&access_token=*");
assertThat(LogRedactUtil.redactQueryString("access_token=foo")).isEqualTo("access_token=*");
assertThat(
LogRedactUtil.redactQueryString("query=status:open&access_token=foo&access_token=bar"))
.isEqualTo("query=status:open&access_token=*&access_token=*");
}
}

46
javatests/com/google/gerrit/pgm/http/jetty/HttpLogRedactTest.java
View File

@ -1,46 +0,0 @@
// Copyright (C) 2017 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.pgm.http.jetty;
import static com.google.common.truth.Truth.assertThat;
import org.junit.Test;
public class HttpLogRedactTest {
@Test
public void includeQueryString() {
assertThat(HttpLog.redactQueryString("/changes/", null)).isEqualTo("/changes/");
assertThat(HttpLog.redactQueryString("/changes/", "")).isEqualTo("/changes/");
assertThat(HttpLog.redactQueryString("/changes/", "x")).isEqualTo("/changes/?x");
assertThat(HttpLog.redactQueryString("/changes/", "x=y")).isEqualTo("/changes/?x=y");
}
@Test
public void redactAuth() {
assertThat(HttpLog.redactQueryString("/changes/", "query=status:open"))
.isEqualTo("/changes/?query=status:open");
assertThat(HttpLog.redactQueryString("/changes/", "query=status:open&access_token=foo"))
.isEqualTo("/changes/?query=status:open&access_token=*");
assertThat(HttpLog.redactQueryString("/changes/", "access_token=foo"))
.isEqualTo("/changes/?access_token=*");
assertThat(
HttpLog.redactQueryString(
"/changes/", "query=status:open&access_token=foo&access_token=bar"))
.isEqualTo("/changes/?query=status:open&access_token=*&access_token=*");
}
}