Merge tracking 'stable-2.14' into stable-2.15

* stable-2.14:
  Move downloaded artifact cache from buck-cache to bazel-cache
  Revert "Hide sensitive data from audit and gerrit logs"
  dev-plugins: Improve formatting of reviewer suggestion documentation
  ReviewerSuggestion: Reword Javadoc
  ReviewerRecommender: Add debug log of plugin provided weight
  Handle deleted project in ReindexIfStaleTask
  Handle the ReindexIfStale event when a change is deleted
  PolyGerrit: Fix gr-diff-view arrows to use html code
  Migrate metrics-core to 4.0.2 version
  CreateChange: Fix appending Signed-off-by line after Change-Id
  CreateChange: Only insert Change-Id if there isn't already one
  CreateChangeIT: Disable "Insert Signed-off-by" after test

Change-Id: Ieee9ee045a2ec6e82bc9f46273ee3ebc7420f3fa
This commit is contained in:
Dave Borowitz 2018-02-07 15:00:46 -05:00
commit ec96085b78
17 changed files with 138 additions and 284 deletions

View File

@ -147,17 +147,6 @@ be ordered as ranked by the plugins (if there are any).
+ +
By default 1. 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]] [[auth]]
=== Section auth === Section auth

View File

@ -2646,15 +2646,18 @@ new RestApi("accounts").id("self").view("username")
Gerrit provides an extension point that enables Plugins to rank Gerrit provides an extension point that enables Plugins to rank
the list of reviewer suggestion a user receives upon clicking "Add Reviewer" on the list of reviewer suggestion a user receives upon clicking "Add Reviewer" on
the change screen. the change screen.
Gerrit supports both a default suggestion that appears when the user has not yet Gerrit supports both a default suggestion that appears when the user has not yet
typed anything and a filtered suggestion that is shown as the user starts typed anything and a filtered suggestion that is shown as the user starts
typing. typing.
Plugins receive a candidate list and can return a Set of suggested reviewers
containing the Account.Id and a score for each reviewer. Plugins receive a candidate list and can return a `Set` of suggested reviewers
The candidate list is non-binding and plugins can choose to return reviewers not containing the `Account.Id` and a score for each reviewer. The candidate list is
initially contained in the candidate list. non-binding and plugins can choose to return reviewers not initially contained in
Server administrators can configure the overall weight of each plugin using the the candidate list.
weight config parameter on [addreviewer "<pluginName-exportName>"].
Server administrators can configure the overall weight of each plugin by setting
the `addreviewer.pluginName-exportName.weight` value in `gerrit.config`.
[source, java] [source, java]
---- ----

View File

@ -609,8 +609,8 @@ maven_jar(
maven_jar( maven_jar(
name = "dropwizard_core", name = "dropwizard_core",
artifact = "io.dropwizard.metrics:metrics-core:3.2.4", artifact = "io.dropwizard.metrics:metrics-core:4.0.2",
sha1 = "36af4975e38bb39686a63ba5139dce8d3f410669", sha1 = "ec9878842d510cabd6bd6a9da1bebae1ae0cd199",
) )
# When updading Bouncy Castle, also update it in bazlets. # When updading Bouncy Castle, also update it in bazlets.

View File

@ -107,9 +107,32 @@ public class CreateChangeIT extends AbstractDaemonTest {
"invalid Change-Id line format in commit message footer"); "invalid Change-Id line format in commit message footer");
} }
@Test
public void createEmptyChange_InvalidSubject() throws Exception {
ChangeInput ci = newChangeInput(ChangeStatus.NEW);
ci.subject = "Change-Id: I1234000000000000000000000000000000000000";
assertCreateFails(
ci,
ResourceConflictException.class,
"missing subject; Change-Id must be in commit message footer");
}
@Test @Test
public void createNewChange() throws Exception { public void createNewChange() throws Exception {
assertCreateSucceeds(newChangeInput(ChangeStatus.NEW)); ChangeInfo info = assertCreateSucceeds(newChangeInput(ChangeStatus.NEW));
assertThat(info.revisions.get(info.currentRevision).commit.message)
.contains("Change-Id: " + info.changeId);
}
@Test
public void createNewChangeWithChangeId() throws Exception {
ChangeInput ci = newChangeInput(ChangeStatus.NEW);
String changeId = "I1234000000000000000000000000000000000000";
String changeIdLine = "Change-Id: " + changeId;
ci.subject = "Subject\n\n" + changeIdLine;
ChangeInfo info = assertCreateSucceeds(ci);
assertThat(info.changeId).isEqualTo(changeId);
assertThat(info.revisions.get(info.currentRevision).commit.message).contains(changeIdLine);
} }
@Test @Test
@ -137,14 +160,38 @@ public class CreateChangeIT extends AbstractDaemonTest {
@Test @Test
public void createNewChangeSignedOffByFooter() throws Exception { public void createNewChangeSignedOffByFooter() throws Exception {
setSignedOffByFooter(); setSignedOffByFooter(true);
try {
ChangeInfo info = assertCreateSucceeds(newChangeInput(ChangeStatus.NEW));
String message = info.revisions.get(info.currentRevision).commit.message;
assertThat(message)
.contains(
String.format(
"%sAdministrator <%s>", SIGNED_OFF_BY_TAG, admin.getIdent().getEmailAddress()));
} finally {
setSignedOffByFooter(false);
}
}
ChangeInfo info = assertCreateSucceeds(newChangeInput(ChangeStatus.NEW)); @Test
String message = info.revisions.get(info.currentRevision).commit.message; public void createNewChangeSignedOffByFooterWithChangeId() throws Exception {
assertThat(message) setSignedOffByFooter(true);
.contains( try {
String.format( ChangeInput ci = newChangeInput(ChangeStatus.NEW);
"%sAdministrator <%s>", SIGNED_OFF_BY_TAG, admin.getIdent().getEmailAddress())); String changeId = "I1234000000000000000000000000000000000000";
String changeIdLine = "Change-Id: " + changeId;
ci.subject = "Subject\n\n" + changeIdLine;
ChangeInfo info = assertCreateSucceeds(ci);
assertThat(info.changeId).isEqualTo(changeId);
String message = info.revisions.get(info.currentRevision).commit.message;
assertThat(message).contains(changeIdLine);
assertThat(message)
.contains(
String.format(
"%sAdministrator <%s>", SIGNED_OFF_BY_TAG, admin.getIdent().getEmailAddress()));
} finally {
setSignedOffByFooter(false);
}
} }
@Test @Test
@ -383,7 +430,7 @@ public class CreateChangeIT extends AbstractDaemonTest {
ChangeInfo out = gApi.changes().create(in).get(); ChangeInfo out = gApi.changes().create(in).get();
assertThat(out.project).isEqualTo(in.project); assertThat(out.project).isEqualTo(in.project);
assertThat(out.branch).isEqualTo(in.branch); assertThat(out.branch).isEqualTo(in.branch);
assertThat(out.subject).isEqualTo(in.subject); assertThat(out.subject).isEqualTo(in.subject.split("\n")[0]);
assertThat(out.topic).isEqualTo(in.topic); assertThat(out.topic).isEqualTo(in.topic);
assertThat(out.status).isEqualTo(in.status); assertThat(out.status).isEqualTo(in.status);
assertThat(out.isPrivate).isEqualTo(in.isPrivate); assertThat(out.isPrivate).isEqualTo(in.isPrivate);
@ -403,17 +450,21 @@ public class CreateChangeIT extends AbstractDaemonTest {
} }
// TODO(davido): Expose setting of account preferences in the API // TODO(davido): Expose setting of account preferences in the API
private void setSignedOffByFooter() throws Exception { private void setSignedOffByFooter(boolean value) throws Exception {
RestResponse r = adminRestSession.get("/accounts/" + admin.email + "/preferences"); RestResponse r = adminRestSession.get("/accounts/" + admin.email + "/preferences");
r.assertOK(); r.assertOK();
GeneralPreferencesInfo i = newGson().fromJson(r.getReader(), GeneralPreferencesInfo.class); GeneralPreferencesInfo i = newGson().fromJson(r.getReader(), GeneralPreferencesInfo.class);
i.signedOffBy = true; i.signedOffBy = value;
r = adminRestSession.put("/accounts/" + admin.email + "/preferences", i); r = adminRestSession.put("/accounts/" + admin.email + "/preferences", i);
r.assertOK(); r.assertOK();
GeneralPreferencesInfo o = newGson().fromJson(r.getReader(), GeneralPreferencesInfo.class); GeneralPreferencesInfo o = newGson().fromJson(r.getReader(), GeneralPreferencesInfo.class);
assertThat(o.signedOffBy).isTrue(); if (value) {
assertThat(o.signedOffBy).isTrue();
} else {
assertThat(o.signedOffBy).isNull();
}
} }
private ChangeInput newMergeChangeInput(String targetBranch, String sourceRef, String strategy) { private ChangeInput newMergeChangeInput(String targetBranch, String sourceRef, String strategy) {

View File

@ -139,21 +139,16 @@ public class ReviewerRecommender {
changeNotes.getChangeId(), changeNotes.getChangeId(),
query, query,
reviewerScores.keySet())); reviewerScores.keySet()));
String pluginWeight = String key = plugin.getPluginName() + "-" + plugin.getExportName();
config.getString( String pluginWeight = config.getString("addReviewer", key, "weight");
"addReviewer", plugin.getPluginName() + "-" + plugin.getExportName(), "weight");
if (Strings.isNullOrEmpty(pluginWeight)) { if (Strings.isNullOrEmpty(pluginWeight)) {
pluginWeight = "1"; pluginWeight = "1";
} }
log.debug("weight for {}: {}", key, pluginWeight);
try { try {
weights.add(Double.parseDouble(pluginWeight)); weights.add(Double.parseDouble(pluginWeight));
} catch (NumberFormatException e) { } catch (NumberFormatException e) {
log.error( log.error("Exception while parsing weight for {}", key, e);
"Exception while parsing weight for "
+ plugin.getPluginName()
+ "-"
+ plugin.getExportName(),
e);
weights.add(1d); weights.add(1d);
} }
} }

View File

@ -16,6 +16,7 @@ package com.google.gerrit.server.change;
import static org.eclipse.jgit.lib.Constants.SIGNED_OFF_BY_TAG; import static org.eclipse.jgit.lib.Constants.SIGNED_OFF_BY_TAG;
import com.google.common.base.Joiner;
import com.google.common.base.MoreObjects; import com.google.common.base.MoreObjects;
import com.google.common.base.Strings; import com.google.common.base.Strings;
import com.google.common.collect.Iterables; import com.google.common.collect.Iterables;
@ -226,13 +227,22 @@ public class CreateChange
AccountState account = accountCache.get(me.getAccountId()); AccountState account = accountCache.get(me.getAccountId());
GeneralPreferencesInfo info = account.getAccount().getGeneralPreferencesInfo(); GeneralPreferencesInfo info = account.getAccount().getGeneralPreferencesInfo();
ObjectId treeId = mergeTip == null ? emptyTreeId(oi) : mergeTip.getTree(); // Add a Change-Id line if there isn't already one
ObjectId id = ChangeIdUtil.computeChangeId(treeId, mergeTip, author, author, input.subject); String commitMessage = input.subject;
String commitMessage = ChangeIdUtil.insertId(input.subject, id); if (ChangeIdUtil.indexOfChangeId(commitMessage, "\n") == -1) {
ObjectId treeId = mergeTip == null ? emptyTreeId(oi) : mergeTip.getTree();
ObjectId id = ChangeIdUtil.computeChangeId(treeId, mergeTip, author, author, commitMessage);
commitMessage = ChangeIdUtil.insertId(commitMessage, id);
}
if (Boolean.TRUE.equals(info.signedOffBy)) { if (Boolean.TRUE.equals(info.signedOffBy)) {
commitMessage += commitMessage =
String.format( Joiner.on("\n")
"%s%s", SIGNED_OFF_BY_TAG, account.getAccount().getNameEmail(anonymousCowardName)); .join(
commitMessage.trim(),
String.format(
"%s%s",
SIGNED_OFF_BY_TAG, account.getAccount().getNameEmail(anonymousCowardName)));
} }
RevCommit c; RevCommit c;

View File

@ -24,7 +24,7 @@ import java.util.Set;
/** /**
* Listener to provide reviewer suggestions. * Listener to provide reviewer suggestions.
* *
* <p>Invoked by Gerrit a user who is searching for a reviewer to add to a change. * <p>Invoked by Gerrit when a user clicks "Add Reviewer" on a change.
*/ */
@ExtensionPoint @ExtensionPoint
public interface ReviewerSuggestion { public interface ReviewerSuggestion {

View File

@ -33,6 +33,7 @@ import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.index.IndexExecutor; import com.google.gerrit.server.index.IndexExecutor;
import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.util.RequestContext; import com.google.gerrit.server.util.RequestContext;
import com.google.gerrit.server.util.ThreadLocalRequestContext; import com.google.gerrit.server.util.ThreadLocalRequestContext;
@ -53,6 +54,7 @@ import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future; import java.util.concurrent.Future;
import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.atomic.AtomicReference;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Config;
import org.slf4j.Logger; import org.slf4j.Logger;
import org.slf4j.LoggerFactory; import org.slf4j.LoggerFactory;
@ -443,11 +445,23 @@ public class ChangeIndexer {
@Override @Override
public Boolean callImpl(Provider<ReviewDb> db) throws Exception { public Boolean callImpl(Provider<ReviewDb> db) throws Exception {
if (!stalenessChecker.isStale(id)) { try {
return false; if (stalenessChecker.isStale(id)) {
index(newChangeData(db.get(), project, id));
return true;
}
} catch (NoSuchChangeException nsce) {
log.debug("Change {} was deleted, aborting reindexing the change.", id.get());
} catch (Exception e) {
if (!isCausedByRepositoryNotFoundException(e)) {
throw e;
}
log.debug(
"Change {} belongs to deleted project {}, aborting reindexing the change.",
id.get(),
project.get());
} }
index(newChangeData(db.get(), project, id)); return false;
return true;
} }
@Override @Override
@ -456,6 +470,16 @@ public class ChangeIndexer {
} }
} }
private boolean isCausedByRepositoryNotFoundException(Throwable throwable) {
while (throwable != null) {
if (throwable instanceof RepositoryNotFoundException) {
return true;
}
throwable = throwable.getCause();
}
return false;
}
// Avoid auto-rebuilding when reindexing if reading is disabled. This just // Avoid auto-rebuilding when reindexing if reading is disabled. This just
// increases contention on the meta ref from a background indexing thread // increases contention on the meta ref from a background indexing thread
// with little benefit. The next actual write to the entity may still incur a // with little benefit. The next actual write to the entity may still incur a

View File

@ -48,10 +48,6 @@ import java.io.OutputStreamWriter;
import java.io.PrintWriter; import java.io.PrintWriter;
import java.io.StringWriter; import java.io.StringWriter;
import java.nio.charset.Charset; 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.Future;
import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.atomic.AtomicReference;
@ -74,8 +70,6 @@ public abstract class BaseCommand implements Command {
static final int STATUS_NOT_FOUND = PRIVATE_STATUS | 2; static final int STATUS_NOT_FOUND = PRIVATE_STATUS | 2;
public static final int STATUS_NOT_ADMIN = PRIVATE_STATUS | 3; public static final int STATUS_NOT_ADMIN = PRIVATE_STATUS | 3;
private static final String MASK = "***";
@Option(name = "--", usage = "end of options", handler = EndOfOptionsHandler.class) @Option(name = "--", usage = "end of options", handler = EndOfOptionsHandler.class)
private boolean endOfOptions; private boolean endOfOptions;
@ -98,8 +92,6 @@ public abstract class BaseCommand implements Command {
@Inject private SshScope.Context context; @Inject private SshScope.Context context;
@Inject private SshCommandSensitiveFieldsCache cache;
/** Commands declared by a plugin can be scoped by the plugin name. */ /** Commands declared by a plugin can be scoped by the plugin name. */
@Inject(optional = true) @Inject(optional = true)
@PluginName @PluginName
@ -118,10 +110,6 @@ public abstract class BaseCommand implements Command {
/** Unparsed command line options. */ /** Unparsed command line options. */
private String[] argv; private String[] argv;
private List<String> maskedArgv = new ArrayList<>();
private Set<String> sensitiveParameters = new HashSet<>();
public BaseCommand() { public BaseCommand() {
task = Atomics.newReference(); task = Atomics.newReference();
} }
@ -167,22 +155,6 @@ public abstract class BaseCommand implements Command {
this.argv = argv; 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 @Override
public void destroy() { public void destroy() {
Future<?> future = task.getAndSet(null); Future<?> future = task.getAndSet(null);
@ -353,7 +325,7 @@ public abstract class BaseCommand implements Command {
m.append(")"); m.append(")");
} }
m.append(" during "); m.append(" during ");
m.append(getFormattedMaskedArguments(" ")); m.append(context.getCommandLine());
log.error(m.toString(), e); log.error(m.toString(), e);
} }
@ -399,7 +371,7 @@ public abstract class BaseCommand implements Command {
protected String getTaskDescription() { protected String getTaskDescription() {
StringBuilder m = new StringBuilder(); StringBuilder m = new StringBuilder();
m.append(getFormattedMaskedArguments(" ")); m.append(context.getCommandLine());
return m.toString(); return m.toString();
} }
@ -413,49 +385,12 @@ public abstract class BaseCommand implements Command {
return m.toString(); 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 class TaskThunk implements CancelableRunnable, ProjectRunnable {
private final CommandRunnable thunk; private final CommandRunnable thunk;
private final String taskName; private final String taskName;
private Project.NameKey projectName; private Project.NameKey projectName;
private TaskThunk(CommandRunnable thunk) { private TaskThunk(CommandRunnable thunk) {
maskSensitiveParameters();
this.thunk = thunk; this.thunk = thunk;
this.taskName = getTaskName(); this.taskName = getTaskName();
} }

View File

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

View File

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

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

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

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

View File

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

View File

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

View File

@ -33,16 +33,13 @@ class SshPluginStarterCallback implements StartPluginListener, ReloadPluginListe
private final DispatchCommandProvider root; private final DispatchCommandProvider root;
private final DynamicMap<DynamicOptions.DynamicBean> dynamicBeans; private final DynamicMap<DynamicOptions.DynamicBean> dynamicBeans;
private final SshCommandSensitiveFieldsCache cache;
@Inject @Inject
SshPluginStarterCallback( SshPluginStarterCallback(
@CommandName(Commands.ROOT) DispatchCommandProvider root, @CommandName(Commands.ROOT) DispatchCommandProvider root,
DynamicMap<DynamicOptions.DynamicBean> dynamicBeans, DynamicMap<DynamicOptions.DynamicBean> dynamicBeans) {
SshCommandSensitiveFieldsCache cache) {
this.root = root; this.root = root;
this.dynamicBeans = dynamicBeans; this.dynamicBeans = dynamicBeans;
this.cache = cache;
} }
@Override @Override
@ -59,7 +56,6 @@ class SshPluginStarterCallback implements StartPluginListener, ReloadPluginListe
if (cmd != null) { if (cmd != null) {
newPlugin.add(root.replace(Commands.named(newPlugin.getName()), cmd)); newPlugin.add(root.replace(Commands.named(newPlugin.getName()), cmd));
} }
cache.evictAll();
} }
private Provider<Command> load(Plugin plugin) { private Provider<Command> load(Plugin plugin) {