Github user mike-jumper commented on a diff in the pull request:

    https://github.com/apache/guacamole-server/pull/164#discussion_r191081131
  
    --- Diff: src/common-ssh/ssh.c ---
    @@ -518,6 +520,64 @@ guac_common_ssh_session* 
guac_common_ssh_create_session(guac_client* client,
             return NULL;
         }
     
    +    /* Check known_hosts, start by getting known_hosts file of user 
running guacd */
    +    struct passwd *pw = getpwuid(getuid());
    +    const char *known_hosts = strcat(pw->pw_dir, "/.ssh/known_hosts");
    +    LIBSSH2_KNOWNHOSTS *ssh_known_hosts = libssh2_knownhost_init(session);
    +    libssh2_knownhost_readfile(ssh_known_hosts, known_hosts, 
LIBSSH2_KNOWNHOST_FILE_OPENSSH);
    +
    --- End diff --
    
    > It makes Guacamole behave more like other SSH clients. Most SSH clients 
require that you at least verify the host key on initial connection and agree 
to add it to the known_hosts file, and throw an error if it doesn't match.
    
    Closer, yes, but breaking SSH functionality for all existing users of 
Guacamole would be bad. The result of upgrading Guacamole from one version to 
another shouldn't be that all existing SSH connections (and VNC/RDP connections 
using SFTP) need to be reconfigured.
    
    > It makes SSH support in Guacamole behave more like RDP.
    
    I don't necessarily think that the built-in host checking of the FreeRDP 
library is the gold standard here, partly because in our case it universally 
requires that the keys be stored on the server running guacd. In fact, we 
should probably look toward implementing our own host checking within RDP, 
overriding FreeRDP's default behavior if a host key is specified, so that 
ignoring the certificate becomes unnecessary.
    
    > One potential compromise might be this: if the host key does not already 
exist in the known_hosts file, add it and allow the connection to continue 
instead of failing.
    
    I believe that would be a security vulnerability. The protections added 
through host checking would be able to be bypassed by a malicious user if they 
happen to use a guacd node which does not yet know about the SSH host in 
question.
    
    > Thoughts? I definitely understand the importance of maintaining 
backward-compatibility; however, this seems like a case where perhaps breaking 
that would be acceptable, in some form or another.
    
    My thoughts are:
    
    * Existing VNC/RDP/SSH connections shouldn't break as a result of these 
changes
    * If host checking is enabled (through providing a `known_hosts` file or 
through specifying the host key as a connection parameter), then strict host 
checking should be performed, with the connection being aborted if the host key 
is unknown or does not match.


---

Reply via email to