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.
---