diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index d577470634..5739f1a232 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -147,6 +147,17 @@ 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 diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java index 634c47c8e6..620d6b4932 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java @@ -48,6 +48,10 @@ 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.ScheduledThreadPoolExecutor; import java.util.concurrent.atomic.AtomicReference; @@ -70,6 +74,8 @@ 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; @@ -92,6 +98,8 @@ 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 @@ -110,6 +118,10 @@ public abstract class BaseCommand implements Command { /** Unparsed command line options. */ private String[] argv; + private List maskedArgv = new ArrayList<>(); + + private Set sensitiveParameters = new HashSet<>(); + public BaseCommand() { task = Atomics.newReference(); } @@ -155,6 +167,22 @@ public abstract class BaseCommand implements Command { this.argv = argv; } + public List getMaskedArguments() { + return maskedArgv; + } + + public String getFormattedMaskedArguments(String delimiter) { + return String.join(delimiter, maskedArgv); + } + + public void setMaskedArguments(List argv) { + this.maskedArgv = argv; + } + + public boolean isSensitiveParameter(String param) { + return sensitiveParameters.contains(param); + } + @Override public void destroy() { Future future = task.getAndSet(null); @@ -325,7 +353,7 @@ public abstract class BaseCommand implements Command { m.append(")"); } m.append(" during "); - m.append(context.getCommandLine()); + m.append(getFormattedMaskedArguments(" ")); log.error(m.toString(), e); } @@ -371,7 +399,7 @@ public abstract class BaseCommand implements Command { protected String getTaskDescription() { StringBuilder m = new StringBuilder(); - m.append(context.getCommandLine()); + m.append(getFormattedMaskedArguments(" ")); return m.toString(); } @@ -385,12 +413,49 @@ 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(CommandRunnable thunk) { + maskSensitiveParameters(); this.thunk = thunk; this.taskName = getTaskName(); } diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/CommandFactoryProvider.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/CommandFactoryProvider.java index 0287ceb5e0..d0615351d7 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/CommandFactoryProvider.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/CommandFactoryProvider.java @@ -29,6 +29,7 @@ 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; @@ -159,7 +160,7 @@ class CommandFactoryProvider implements Provider, LifecycleListe } catch (Exception e) { logger.warn( "Cannot start command \"" - + ctx.getCommandLine() + + cmd.getFormattedMaskedArguments(" ") + "\" for user " + ctx.getSession().getUsername(), e); @@ -179,6 +180,10 @@ class CommandFactoryProvider implements Provider, 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); diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DispatchCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DispatchCommand.java index 3f2e258537..62f80c9ab2 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DispatchCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DispatchCommand.java @@ -107,6 +107,10 @@ 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")) { diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SensitiveData.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SensitiveData.java new file mode 100644 index 0000000000..1dd7896013 --- /dev/null +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SensitiveData.java @@ -0,0 +1,28 @@ +// 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 {} diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCache.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCache.java new file mode 100644 index 0000000000..8c79299394 --- /dev/null +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCache.java @@ -0,0 +1,24 @@ +// 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 get(Class command); + + void evictAll(); +} diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCacheImpl.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCacheImpl.java new file mode 100644 index 0000000000..b5933886b4 --- /dev/null +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCacheImpl.java @@ -0,0 +1,76 @@ +// 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, Set> sshdCommandsCache; + + static Module module() { + return new CacheModule() { + @Override + protected void configure() { + cache(CACHE_NAME, new TypeLiteral>() {}, new TypeLiteral>() {}) + .loader(Loader.class); + bind(SshCommandSensitiveFieldsCache.class).to(SshCommandSensitiveFieldsCacheImpl.class); + } + }; + } + + @Inject + SshCommandSensitiveFieldsCacheImpl(@Named(CACHE_NAME) LoadingCache, Set> cache) { + sshdCommandsCache = cache; + } + + @Override + public Set get(Class cmd) { + return sshdCommandsCache.getUnchecked(cmd); + } + + @Override + public void evictAll() { + sshdCommandsCache.invalidateAll(); + } + + static class Loader extends CacheLoader, Set> { + + @Override + public Set load(Class cmd) throws Exception { + Set 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; + } + } +} diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java index 12064c83fa..5241627cd1 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java @@ -48,9 +48,12 @@ 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 session; private final Provider context; private final AsyncAppender async; + private final boolean auditMask; private final AuditService auditService; @Inject @@ -64,6 +67,7 @@ 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; @@ -121,8 +125,7 @@ class SshLog implements LifecycleListener { final Context ctx = context.get(); ctx.finished = TimeUtil.nowMs(); - String cmd = extractWhat(dcmd); - + String cmd = extractWhat(dcmd, true); final LoggingEvent event = log(cmd); event.setProperty(P_WAIT, (ctx.started - ctx.created) + "ms"); event.setProperty(P_EXEC, (ctx.finished - ctx.started) + "ms"); @@ -154,7 +157,11 @@ class SshLog implements LifecycleListener { if (async != null) { async.append(event); } - audit(context.get(), status, dcmd); + + if (!auditMask) { + cmd = extractWhat(dcmd, false); + } + audit(ctx, status, cmd, extractParameters(dcmd)); } private ListMultimap extractParameters(DispatchCommand dcmd) { @@ -177,7 +184,10 @@ class SshLog implements LifecycleListener { // --param=value int eqPos = arg.indexOf('='); if (arg.startsWith("--") && eqPos > 0) { - parms.put(arg.substring(0, eqPos), arg.substring(eqPos + 1)); + String param = arg.substring(0, eqPos); + String value = + auditMask && dcmd.isSensitiveParameter(param) ? MASK : arg.substring(eqPos + 1); + parms.put(param, value); continue; } // -p value or --param value @@ -192,7 +202,7 @@ class SshLog implements LifecycleListener { if (paramName == null) { parms.put("$" + argPos++, arg); } else { - parms.put(paramName, arg); + parms.put(paramName, auditMask && dcmd.isSensitiveParameter(paramName) ? MASK : arg); paramName = null; } } @@ -256,10 +266,6 @@ 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 params) { String sessionId; CurrentUser currentUser; @@ -277,11 +283,16 @@ class SshLog implements LifecycleListener { auditService.dispatch(new SshAuditEvent(sessionId, currentUser, cmd, created, params, result)); } - private String extractWhat(DispatchCommand dcmd) { + private String extractWhat(DispatchCommand dcmd, boolean hideSensitive) { if (dcmd == null) { return "Command was already destroyed"; } - StringBuilder commandName = new StringBuilder(dcmd.getCommandName()); + return hideSensitive ? dcmd.getFormattedMaskedArguments(".") : extractWhat(dcmd); + } + + private String extractWhat(DispatchCommand dcmd) { + String name = dcmd.getCommandName(); + StringBuilder commandName = new StringBuilder(name == null ? "" : name); String[] args = dcmd.getArguments(); for (int i = 1; i < args.length; i++) { commandName.append(".").append(args[i]); diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java index 748277e266..c672b00b94 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java @@ -65,6 +65,7 @@ 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); diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshPluginStarterCallback.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshPluginStarterCallback.java index 9ae1814f56..047690ce74 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshPluginStarterCallback.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshPluginStarterCallback.java @@ -33,13 +33,16 @@ class SshPluginStarterCallback implements StartPluginListener, ReloadPluginListe private final DispatchCommandProvider root; private final DynamicMap dynamicBeans; + private final SshCommandSensitiveFieldsCache cache; @Inject SshPluginStarterCallback( @CommandName(Commands.ROOT) DispatchCommandProvider root, - DynamicMap dynamicBeans) { + DynamicMap dynamicBeans, + SshCommandSensitiveFieldsCache cache) { this.root = root; this.dynamicBeans = dynamicBeans; + this.cache = cache; } @Override @@ -56,6 +59,7 @@ class SshPluginStarterCallback implements StartPluginListener, ReloadPluginListe if (cmd != null) { newPlugin.add(root.replace(Commands.named(newPlugin.getName()), cmd)); } + cache.evictAll(); } private Provider load(Plugin plugin) {