opendev
/
gerrit
Code Issues Proposed changes

Revert "Hide sensitive data from audit and gerrit logs" 

There is concern that this new feature might introduce regressions on
the stable branch. Revert it so that it can be done again on master.

This reverts commit a8a7a84ae5.
This reverts commit dfa74b835f.

Change-Id: I21e2046dfcdbba7d5f4c2704b620ea6ea689abc8
This commit is contained in:
David Pursehouse 2018-02-07 10:46:46 +09:00
parent ec89c1dd35
commit a5bea0ba14
10 changed files with 15 additions and 245 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
Documentation/config-gerrit.txt
View File

@ -144,17 +144,6 @@ be ordered as ranked by the plugins (if there are any).
+
By default 1.
[[audit]]
=== Section audit
[[audit.maskSensitiveData]]audit.maskSensitiveData::
+
If true, command parameters marked as sensitive are masked in audit logs.
+
This option only affects audit. Other means of logging will always be masked.
+
By default `false`.
[[auth]]
=== Section auth

69
gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java
View File

@ -42,10 +42,6 @@ import java.io.OutputStreamWriter;
import java.io.PrintWriter;
import java.io.StringWriter;
import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.concurrent.Future;
import java.util.concurrent.atomic.AtomicReference;
import org.apache.sshd.common.SshException;
@ -67,8 +63,6 @@ public abstract class BaseCommand implements Command {
static final int STATUS_NOT_FOUND = PRIVATE_STATUS | 2;
public static final int STATUS_NOT_ADMIN = PRIVATE_STATUS | 3;
private static final String MASK = "***";
@Option(name = "--", usage = "end of options", handler = EndOfOptionsHandler.class)
private boolean endOfOptions;
@ -90,8 +84,6 @@ public abstract class BaseCommand implements Command {
@Inject private SshScope.Context context;
@Inject private SshCommandSensitiveFieldsCache cache;
/** Commands declared by a plugin can be scoped by the plugin name. */
@Inject(optional = true)
@PluginName
@ -106,10 +98,6 @@ public abstract class BaseCommand implements Command {
/** Unparsed command line options. */
private String[] argv;
private List<String> maskedArgv = new ArrayList<>();
private Set<String> sensitiveParameters = new HashSet<>();
public BaseCommand() {
task = Atomics.newReference();
}
@ -155,22 +143,6 @@ public abstract class BaseCommand implements Command {
this.argv = argv;
}
public List<String> getMaskedArguments() {
return maskedArgv;
}
public String getFormattedMaskedArguments(String delimiter) {
return String.join(delimiter, maskedArgv);
}
public void setMaskedArguments(List<String> argv) {
this.maskedArgv = argv;
}
public boolean isSensitiveParameter(String param) {
return sensitiveParameters.contains(param);
}
@Override
public void destroy() {
Future<?> future = task.getAndSet(null);
@ -353,7 +325,7 @@ public abstract class BaseCommand implements Command {
m.append(")");
}
m.append(" during ");
m.append(getFormattedMaskedArguments(" "));
m.append(context.getCommandLine());
log.error(m.toString(), e);
}
@ -399,7 +371,7 @@ public abstract class BaseCommand implements Command {
protected String getTaskDescription() {
StringBuilder m = new StringBuilder();
m.append(getFormattedMaskedArguments(" "));
m.append(context.getCommandLine());
return m.toString();
}
@ -413,49 +385,12 @@ public abstract class BaseCommand implements Command {
return m.toString();
}
private void maskSensitiveParameters() {
if (argv == null) {
return;
}
sensitiveParameters = cache.get(this.getClass());
maskedArgv = new ArrayList<>();
maskedArgv.add(commandName);
boolean maskNext = false;
for (int i = 0; i < argv.length; i++) {
if (maskNext) {
maskedArgv.add(MASK);
maskNext = false;
continue;
}
String arg = argv[i];
String key = extractKey(arg);
if (isSensitiveParameter(key)) {
maskNext = arg.equals(key);
// When arg != key then parameter contains '=' sign and we mask them right away.
// Otherwise we mask the next parameter as indicated by maskNext.
if (!maskNext) {
arg = key + "=" + MASK;
}
}
maskedArgv.add(arg);
}
}
private String extractKey(String arg) {
int eqPos = arg.indexOf('=');
if (eqPos > 0) {
return arg.substring(0, eqPos);
}
return arg;
}
private final class TaskThunk implements CancelableRunnable, ProjectRunnable {
private final CommandRunnable thunk;
private final String taskName;
private Project.NameKey projectName;
private TaskThunk(final CommandRunnable thunk) {
maskSensitiveParameters();
this.thunk = thunk;
this.taskName = getTaskName();
}

7
gerrit-sshd/src/main/java/com/google/gerrit/sshd/CommandFactoryProvider.java
View File

@ -29,7 +29,6 @@ import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
@ -160,7 +159,7 @@ class CommandFactoryProvider implements Provider<CommandFactory>, LifecycleListe
} catch (Exception e) {
logger.warn(
"Cannot start command \""
+ cmd.getFormattedMaskedArguments(" ")
+ ctx.getCommandLine()
+ "\" for user "
+ ctx.getSession().getUsername(),
e);
@ -180,10 +179,6 @@ class CommandFactoryProvider implements Provider<CommandFactory>, LifecycleListe
try {
cmd = dispatcher.get();
cmd.setArguments(argv);
cmd.setMaskedArguments(
argv.length > 0
? Arrays.asList(argv[0])
: Arrays.asList(ctx.getCommandLine().split(" ")[0]));
cmd.setInputStream(in);
cmd.setOutputStream(out);
cmd.setErrorStream(err);

4
gerrit-sshd/src/main/java/com/google/gerrit/sshd/DispatchCommand.java
View File

@ -100,10 +100,6 @@ final class DispatchCommand extends BaseCommand {
atomicCmd.set(cmd);
cmd.start(env);
if (cmd instanceof BaseCommand) {
setMaskedArguments(((BaseCommand) cmd).getMaskedArguments());
}
} catch (UnloggedFailure e) {
String msg = e.getMessage();
if (!msg.endsWith("\n")) {

28
gerrit-sshd/src/main/java/com/google/gerrit/sshd/SensitiveData.java
View File

@ -1,28 +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.sshd;
import static java.lang.annotation.ElementType.FIELD;
import static java.lang.annotation.RetentionPolicy.RUNTIME;
import java.lang.annotation.Retention;
import java.lang.annotation.Target;
/**
* Annotation tagged on a field of an ssh command to indicate the value must be hidden from logs.
*/
@Target({FIELD})
@Retention(RUNTIME)
public @interface SensitiveData {}

24
gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCache.java
View File

@ -1,24 +0,0 @@
// 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.
package com.google.gerrit.sshd;
import java.util.Set;
/** Keeps data about ssh commands' parameters that have extra secure annotation. */
public interface SshCommandSensitiveFieldsCache {
Set<String> get(Class<?> command);
void evictAll();
}

76
gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCacheImpl.java
View File

@ -1,76 +0,0 @@
// 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.
package com.google.gerrit.sshd;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.gerrit.server.cache.CacheModule;
import com.google.inject.Inject;
import com.google.inject.Module;
import com.google.inject.TypeLiteral;
import com.google.inject.name.Named;
import java.lang.reflect.Field;
import java.util.HashSet;
import java.util.Set;
import org.kohsuke.args4j.Option;
public class SshCommandSensitiveFieldsCacheImpl implements SshCommandSensitiveFieldsCache {
private static final String CACHE_NAME = "sshd_sensitive_command_params";
private final LoadingCache<Class<?>, Set<String>> sshdCommandsCache;
static Module module() {
return new CacheModule() {
@Override
protected void configure() {
cache(CACHE_NAME, new TypeLiteral<Class<?>>() {}, new TypeLiteral<Set<String>>() {})
.loader(Loader.class);
bind(SshCommandSensitiveFieldsCache.class).to(SshCommandSensitiveFieldsCacheImpl.class);
}
};
}
@Inject
SshCommandSensitiveFieldsCacheImpl(@Named(CACHE_NAME) LoadingCache<Class<?>, Set<String>> cache) {
sshdCommandsCache = cache;
}
@Override
public Set<String> get(Class<?> cmd) {
return sshdCommandsCache.getUnchecked(cmd);
}
@Override
public void evictAll() {
sshdCommandsCache.invalidateAll();
}
static class Loader extends CacheLoader<Class<?>, Set<String>> {
@Override
public Set<String> load(Class<?> cmd) throws Exception {
Set<String> datas = new HashSet<>();
for (Field field : cmd.getDeclaredFields()) {
if (field.isAnnotationPresent(SensitiveData.class)) {
Option option = field.getAnnotation(Option.class);
datas.add(option.name());
for (String opt : option.aliases()) {
datas.add(opt);
}
}
}
return datas;
}
}
}

33
gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java
View File

@ -48,12 +48,9 @@ class SshLog implements LifecycleListener {
private static final String P_STATUS = "status";
private static final String P_AGENT = "agent";
private static final String MASK = "***";
private final Provider<SshSession> session;
private final Provider<Context> context;
private final AsyncAppender async;
private final boolean auditMask;
private final AuditService auditService;
@Inject
@ -67,7 +64,6 @@ class SshLog implements LifecycleListener {
this.context = context;
this.auditService = auditService;
auditMask = config.getBoolean("audit", "maskSensitiveData", false);
if (!config.getBoolean("sshd", "requestLog", true)) {
async = null;
return;
@ -125,7 +121,8 @@ class SshLog implements LifecycleListener {
final Context ctx = context.get();
ctx.finished = TimeUtil.nowMs();
String cmd = extractWhat(dcmd, true);
String cmd = extractWhat(dcmd);
final LoggingEvent event = log(cmd);
event.setProperty(P_WAIT, (ctx.started - ctx.created) + "ms");
event.setProperty(P_EXEC, (ctx.finished - ctx.started) + "ms");
@ -157,11 +154,7 @@ class SshLog implements LifecycleListener {
if (async != null) {
async.append(event);
}
if (!auditMask) {
cmd = extractWhat(dcmd, false);
}
audit(ctx, status, cmd, extractParameters(dcmd));
audit(context.get(), status, dcmd);
}
private ListMultimap<String, ?> extractParameters(DispatchCommand dcmd) {
@ -184,10 +177,7 @@ class SshLog implements LifecycleListener {
// --param=value
int eqPos = arg.indexOf('=');
if (arg.startsWith("--") && eqPos > 0) {
String param = arg.substring(0, eqPos);
String value =
auditMask && dcmd.isSensitiveParameter(param) ? MASK : arg.substring(eqPos + 1);
parms.put(param, value);
parms.put(arg.substring(0, eqPos), arg.substring(eqPos + 1));
continue;
}
// -p value or --param value
@ -202,7 +192,7 @@ class SshLog implements LifecycleListener {
if (paramName == null) {
parms.put("$" + argPos++, arg);
} else {
parms.put(paramName, auditMask && dcmd.isSensitiveParameter(paramName) ? MASK : arg);
parms.put(paramName, arg);
paramName = null;
}
}
@ -266,6 +256,10 @@ class SshLog implements LifecycleListener {
audit(ctx, result, cmd, null);
}
void audit(Context ctx, Object result, DispatchCommand cmd) {
audit(ctx, result, extractWhat(cmd), extractParameters(cmd));
}
private void audit(Context ctx, Object result, String cmd, ListMultimap<String, ?> params) {
String sessionId;
CurrentUser currentUser;
@ -283,16 +277,11 @@ class SshLog implements LifecycleListener {
auditService.dispatch(new SshAuditEvent(sessionId, currentUser, cmd, created, params, result));
}
private String extractWhat(DispatchCommand dcmd, boolean hideSensitive) {
private String extractWhat(DispatchCommand dcmd) {
if (dcmd == null) {
return "Command was already destroyed";
}
return hideSensitive ? dcmd.getFormattedMaskedArguments(".") : extractWhat(dcmd);
}
private String extractWhat(DispatchCommand dcmd) {
String name = dcmd.getCommandName();
StringBuilder commandName = new StringBuilder(name == null ? "" : name);
StringBuilder commandName = new StringBuilder(dcmd.getCommandName());
String[] args = dcmd.getArguments();
for (int i = 1; i < args.length; i++) {
commandName.append(".").append(args[i]);

1
gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java
View File

@ -63,7 +63,6 @@ public class SshModule extends LifecycleModule {
configureRequestScope();
install(new AsyncReceiveCommits.Module());
configureAliases();
install(SshCommandSensitiveFieldsCacheImpl.module());
bind(SshLog.class);
bind(SshInfo.class).to(SshDaemon.class).in(SINGLETON);

7
gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshPluginStarterCallback.java
View File

@ -30,14 +30,10 @@ class SshPluginStarterCallback implements StartPluginListener, ReloadPluginListe
private static final Logger log = LoggerFactory.getLogger(SshPluginStarterCallback.class);
private final DispatchCommandProvider root;
private final SshCommandSensitiveFieldsCache cache;
@Inject
SshPluginStarterCallback(
@CommandName(Commands.ROOT) DispatchCommandProvider root,
SshCommandSensitiveFieldsCache cache) {
SshPluginStarterCallback(@CommandName(Commands.ROOT) DispatchCommandProvider root) {
this.root = root;
this.cache = cache;
}
@Override
@ -54,7 +50,6 @@ class SshPluginStarterCallback implements StartPluginListener, ReloadPluginListe
if (cmd != null) {
newPlugin.add(root.replace(Commands.named(newPlugin.getName()), cmd));
}
cache.evictAll();
}
private Provider<Command> load(Plugin plugin) {