From e15fff00a6404319d01e30b0a673a1a2ce289ca9 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 1 Mar 2019 09:45:41 -0800
Subject: [PATCH] Use "LogLevel=ERROR" to try to improve "ssh" hostkey behavior
 without doing anything extreme/hacky

Summary:
Ref T13121. When you connect to a host with SSH, don't already know the host key, and don't have strict host key checking, it prints "Permanently adding host X to known hosts". This is super un-useful.

In a perfect world, we'd probably always have strict host key checking, but this is a significant barrier to configuration/setup and I think not hugely important (MITM attacks against SSH hosts are hard/rare and probably not hugely valuable). I'd imagine a more realistic long term approach is likely optional host key checking.

For now, try using `LogLevel=ERROR` instead of `LogLevel=quiet` to suppress this error. This should be strictly better (since at least some messages we want to see are ERROR or better), although it may not be perfect (there may be other INFO messages we would still like to see).

Test Plan:
  - Ran `ssh -o LogLevel=... -o 'StrictHostKeyChecking=no' -o 'UserKnownHostsFile=/dev/null'` with bad credentials, for "ERROR", "quiet", and default ("INFO") log levels.
  - With `INFO`, got a warning about adding the key, then an error about bad credentials (bad: don't want the key warning).
  - With `quiet`, got nothing (bad: we want the credential error).
  - With `ERROR`, got no warning but did get an error (good!).

Not sure this always gives us exactly what we want, but it seems like an improvement over "quiet".

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13121

Differential Revision: https://secure.phabricator.com/D20240
---
 src/applications/diffusion/ssh/DiffusionSSHWorkflow.php      | 4 ++--
 .../drydock/interface/command/DrydockSSHCommandInterface.php | 5 ++++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php
index b2d1d25f44..57dc83953d 100644
--- a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php
+++ b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php
@@ -127,9 +127,9 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow {
     // This is suppressing "added <address> to the list of known hosts"
     // messages, which are confusing and irrelevant when they arise from
     // proxied requests. It might also be suppressing lots of useful errors,
-    // of course. Ideally, we would enforce host keys eventually.
+    // of course. Ideally, we would enforce host keys eventually. See T13121.
     $options[] = '-o';
-    $options[] = 'LogLevel=quiet';
+    $options[] = 'LogLevel=ERROR';
 
     // NOTE: We prefix the command with "@username", which the far end of the
     // connection will parse in order to act as the specified user. This
diff --git a/src/applications/drydock/interface/command/DrydockSSHCommandInterface.php b/src/applications/drydock/interface/command/DrydockSSHCommandInterface.php
index 1aab14b57b..b1eebd92a1 100644
--- a/src/applications/drydock/interface/command/DrydockSSHCommandInterface.php
+++ b/src/applications/drydock/interface/command/DrydockSSHCommandInterface.php
@@ -30,8 +30,11 @@ final class DrydockSSHCommandInterface extends DrydockCommandInterface {
     $full_command = call_user_func_array('csprintf', $argv);
 
     $flags = array();
+
+    // See T13121. Attempt to suppress the "Permanently added X to list of
+    // known hosts" message without suppressing anything important.
     $flags[] = '-o';
-    $flags[] = 'LogLevel=quiet';
+    $flags[] = 'LogLevel=ERROR';
 
     $flags[] = '-o';
     $flags[] = 'StrictHostKeyChecking=no';