Merge branch 'stable-2.15'

* stable-2.15:
  Fix error dialog not honoring line breaks
  Protect against null pointer for BaseCommand
  Hide sensitive data from audit and gerrit logs

Change-Id: I9803bd3f0129ff32cd7e1790b80502d84dc013a5

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
 

gerrit-gwtui/src/main/java/com/google/gerrit/client/ErrorDialog.java

@@ -88,7 +88,7 @@ public class ErrorDialog extends PopupPanel {
   /** Create a dialog box to show a single message string. */
   public ErrorDialog(String message) {
     this();
-    body.add(new Label(message));
+    body.add(createErrorMsgLabel(message));
   }
 
   /** Create a dialog box to show a single message string. */
@@ -145,12 +145,16 @@ public class ErrorDialog extends PopupPanel {
     }
 
     if (msg != null) {
-      final Label m = new Label(msg);
-      m.getElement().getStyle().setProperty("whiteSpace", "pre");
-      body.add(m);
+      body.add(createErrorMsgLabel(msg));
     }
   }
 
+  private Label createErrorMsgLabel(String message) {
+    Label m = new Label(message);
+    m.getElement().getStyle().setProperty("white-space", "pre");
+    return m;
+  }
+
   public ErrorDialog setText(String t) {
     text.setText(t);
     return this;

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;
 
@@ -93,6 +99,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
@@ -111,6 +119,10 @@ 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();
   }
@@ -156,6 +168,22 @@ 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);
@@ -326,7 +354,7 @@ public abstract class BaseCommand implements Command {
         m.append(")");
       }
       m.append(" during ");
-      m.append(context.getCommandLine());
+      m.append(getFormattedMaskedArguments(" "));
       log.error(m.toString(), e);
     }
 
@@ -372,7 +400,7 @@ public abstract class BaseCommand implements Command {
 
   protected String getTaskDescription() {
     StringBuilder m = new StringBuilder();
-    m.append(context.getCommandLine());
+    m.append(getFormattedMaskedArguments(" "));
     return m.toString();
   }
 
@@ -388,12 +416,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();
     }

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<CommandFactory>, 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<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);

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")) {

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 {}

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<String> get(Class<?> command);
+
+  void evictAll();
+}

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<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;
+    }
+  }
+}

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<SshSession> session;
   private final Provider<Context> 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<String, ?> 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<String, ?> 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]);

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);

java/com/google/gerrit/sshd/SshPluginStarterCallback.java

@@ -33,13 +33,16 @@ class SshPluginStarterCallback implements StartPluginListener, ReloadPluginListe
 
   private final DispatchCommandProvider root;
   private final DynamicMap<DynamicOptions.DynamicBean> dynamicBeans;
+  private final SshCommandSensitiveFieldsCache cache;
 
   @Inject
   SshPluginStarterCallback(
       @CommandName(Commands.ROOT) DispatchCommandProvider root,
-      DynamicMap<DynamicOptions.DynamicBean> dynamicBeans) {
+      DynamicMap<DynamicOptions.DynamicBean> 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<Command> load(Plugin plugin) {