You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by necouchman <gi...@git.apache.org> on 2018/04/06 11:15:18 UTC
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
GitHub user necouchman opened a pull request:
https://github.com/apache/guacamole-server/pull/164
GUACAMOLE-527: Implement SSH host key checking against known hosts
This series of commits implements changes to guacd necessary to perform checks of the SSH host keys.
Aside from some specific comments on some of the code, it's worth noting that this is a fairly significant change in behavior, as connections will fail, by default, if a matching host key is not found. I believe this is the preferred operation, but current users will need to be aware of the significance of this change.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/necouchman/guacamole-server jira/527
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/guacamole-server/pull/164.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #164
----
commit 5bda4854f7ceaa66a8ff48ddc6e5fea52d07a972
Author: Nick Couchman <vn...@...>
Date: 2018-03-25T21:34:29Z
GUACAMOLE-527: Add basic check for known hosts file for SSH connections.
commit ac00192da970b657767abc73b71dbae40d232183
Author: Nick Couchman <vn...@...>
Date: 2018-04-05T11:36:37Z
GUACAMOLE-527: Add host key and type settings.
commit 3955319ad9a627fb3f693f89a7af18f2af877a93
Author: Nick Couchman <vn...@...>
Date: 2018-04-05T12:52:16Z
GUACAMOLE-527: Enable host key setting for SFTP connections.
commit 0c29570127e776938300f3d6feca4dcba5be57cf
Author: Nick Couchman <vn...@...>
Date: 2018-04-05T15:26:50Z
GUACAMOLE-527: Add error logging for known host checks.
commit dc59506512c7c7c319f8dabf466b4ce4015cc29a
Author: Nick Couchman <vn...@...>
Date: 2018-04-05T15:28:32Z
GUACAMOLE-527: Fix issue with null host_key variable.
commit 6d7bc8351dd757b8e8fea267434b6437e475a656
Author: Nick Couchman <vn...@...>
Date: 2018-04-06T09:45:14Z
GUACAMOLE-527: Order SSH handshake correctly, and remove unnecessary logging.
commit 866999e062ce7de55b77891b37643094e63ac1eb
Author: Nick Couchman <vn...@...>
Date: 2018-04-06T10:33:24Z
GUACAMOLE-527: Correct names of parameters coming from client.
commit 275ce34cc8815cd81c3c6e506f9cf1bf95a3b1bd
Author: Nick Couchman <vn...@...>
Date: 2018-04-06T10:47:35Z
GUACAMOLE-527: Clean up memory and logging.
----
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r190593831
--- 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 --
Okay, I've gone with the approach that known_hosts of the user running guacd will be read and checked against (in addition to any provided host key in the configuration parameters). If there is a mismatch, the connection will fail. If there is no match, guacd will warn but continue the connection. Is that acceptable behavior? Or would you still prefer an explicit enable parameter for checking the known_hosts file at all?
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r179728057
--- 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 --
I'm not dead set on this change, here, but basically, in addition to allowing the client to provide a host key, I've also put this code in that reads in the .ssh/known_hosts file from the user running guacd. There may be situations where admins do not want to individually enter every host key in the Guacamole interface and would rather just copy the known_hosts file from a known good location. There are a couple of alternatives we could consider to having it check the .ssh/known_hosts file from the guacd user's home directory:
- Use /etc/guacamole/known_hosts, instead, or wherever guacd.conf is located.
- Have the client pass through the location of a known_hosts file in addition to an actual host key.
Thoughts?
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r179728655
--- Diff: src/protocols/rdp/rdp_settings.c ---
@@ -822,6 +838,32 @@ guac_rdp_settings* guac_rdp_parse_args(guac_user* user,
guac_user_parse_args_string(user, GUAC_RDP_CLIENT_ARGS, argv,
IDX_SFTP_HOSTNAME, settings->hostname);
+ /* The public SSH host key. */
+ settings->sftp_host_key =
+ guac_user_parse_args_string(user, GUAC_RDP_CLIENT_ARGS, argv,
+ IDX_SFTP_HOST_KEY, NULL);
+
+ if(settings->sftp_host_key) {
+ /* Type of public SSH host key. */
+ char* str_host_key_type = guac_user_parse_args_string(user, GUAC_RDP_CLIENT_ARGS, argv,
+ IDX_SFTP_HOST_KEY_TYPE, "ssh-rsa");
+
+ if (strcmp(str_host_key_type, "ssh-rsa") == 0)
+ settings->sftp_host_key_type = LIBSSH2_KNOWNHOST_KEY_SSHRSA;
+ else if (strcmp(str_host_key_type, "ssh-dss") == 0)
+ settings->sftp_host_key_type = LIBSSH2_KNOWNHOST_KEY_SSHDSS;
+ else if (strcmp(str_host_key_type, "rsa1") == 0)
+ settings->sftp_host_key_type = LIBSSH2_KNOWNHOST_KEY_RSA1;
+ else {
--- End diff --
Not 100% certain this is the best way to deal with translating this setting from the string provided by the client to the libssh2 bitmask?
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r197978466
--- Diff: src/common-ssh/ssh.c ---
@@ -518,6 +520,43 @@ guac_common_ssh_session* guac_common_ssh_create_session(guac_client* client,
return NULL;
}
+ /* Get host key of remote system we're connecting to */
+ size_t remote_hostkey_len;
+ const char *remote_hostkey = libssh2_session_hostkey(session, &remote_hostkey_len, NULL);
+
+ /* Failure to retrieve a host key means we should abort */
+ if (!remote_hostkey) {
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
+ "Failed to get host key for %s", hostname);
+ free(common_session);
+ close(fd);
+ return NULL;
+ }
+
+ /* SSH known host key checking. */
+ int known_host_check = guac_common_ssh_verify_host_key(session, client, host_key,
+ hostname, atoi(port), remote_hostkey,
+ remote_hostkey_len);
+
+ /* Abort on any error codes */
+ if (known_host_check != 0) {
+ char* err_msg;
+ int err_len;
+ libssh2_session_last_error(session, &err_msg, &err_len, 0);
--- End diff --
NULLed.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r191081345
--- Diff: src/common-ssh/ssh.c ---
@@ -518,6 +520,63 @@ 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");
--- End diff --
According to the documentation of `strcat()`:
> ... The strings may not overlap, and the `dest` string must have enough space for the result. If `dest` is not large enough, program behavior is unpredictable; buffer overruns are a favorite avenue for attacking secure programs.
Unless we know that `pw->pw_dir` will have sufficient space for the additional "/.ssh/known_hosts", this may not be safe as-written.
What about using a location specific to guacd like `/etc/guacamole/ssh/known_hosts`? That would avoid inheriting known hosts from the user running the process, and would effectively require explicit steps to enable the checking.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r191087041
--- 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 --
> Any reason behind checking against two sources? If a host key is specified via the connection parameters, shouldn't that override a local known_hosts entirely?
> What about only checking local known_hosts if (1) the host key parameter is not provided and (2) the file exists?
Sounds good to me...will make it so.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r197972780
--- Diff: src/common-ssh/ssh.c ---
@@ -518,6 +520,43 @@ guac_common_ssh_session* guac_common_ssh_create_session(guac_client* client,
return NULL;
}
+ /* Get host key of remote system we're connecting to */
+ size_t remote_hostkey_len;
+ const char *remote_hostkey = libssh2_session_hostkey(session, &remote_hostkey_len, NULL);
+
+ /* Failure to retrieve a host key means we should abort */
+ if (!remote_hostkey) {
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
+ "Failed to get host key for %s", hostname);
+ free(common_session);
+ close(fd);
+ return NULL;
+ }
+
+ /* SSH known host key checking. */
+ int known_host_check = guac_common_ssh_verify_host_key(session, client, host_key,
+ hostname, atoi(port), remote_hostkey,
+ remote_hostkey_len);
+
+ /* Abort on any error codes */
+ if (known_host_check != 0) {
+ char* err_msg;
+ int err_len;
+ libssh2_session_last_error(session, &err_msg, &err_len, 0);
--- End diff --
As `err_len` is not used anywhere, this should just be `NULL`. There's no need to provide space for storing the length of the error message when that value is ultimately discarded. From the [documentation for `libssh2_session_lasterror()`](https://www.libssh2.org/libssh2_session_last_error.html):
> errmsg_len - If not NULL, is populated by reference with the length of errmsg. (The string is NUL-terminated, so the length is only useful as an optimization, to avoid calling strlen.)
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r197886978
--- Diff: src/common-ssh/ssh.c ---
@@ -518,6 +520,40 @@ guac_common_ssh_session* guac_common_ssh_create_session(guac_client* client,
return NULL;
}
+ /* Get fingerprint of host we're connecting to */
+ size_t fp_len;
+ int fp_type;
+ const char *fingerprint = libssh2_session_hostkey(session, &fp_len, &fp_type);
+
+ /* Failure to generate a fingerprint means we should abort */
+ if (!fingerprint) {
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
+ "Failed to get fingerprint for host %s", hostname);
+ return NULL;
--- End diff --
Leak plugged.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r191104521
--- Diff: src/common-ssh/ssh.c ---
@@ -518,6 +520,71 @@ guac_common_ssh_session* guac_common_ssh_create_session(guac_client* client,
return NULL;
}
+ /* SSH known host key checking. */
+ LIBSSH2_KNOWNHOSTS *ssh_known_hosts = libssh2_knownhost_init(session);
+ int num_known_hosts = 0;
+
+ /* Add host key provided from settings */
+ if (host_key && strcmp(host_key, "") != 0) {
+
+ int kh_add = libssh2_knownhost_readline(ssh_known_hosts, host_key, strlen(host_key),
+ LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+ num_known_hosts++;
+
+ if (kh_add)
+ guac_client_log(client, GUAC_LOG_WARNING, "Failed to add provided host key"
+ " to known hosts store for %s. Error was %d", hostname, kh_add);
+
+ }
+
+ /* Otherwise, we look for a ssh_known_hosts file within GUACAMOLE_HOME and read that in. */
+ else {
+ const char *known_hosts = "/etc/guacamole/ssh_known_hosts";
+ num_known_hosts = libssh2_knownhost_readfile(ssh_known_hosts, known_hosts, LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+ }
+
+ /* If we've found a provided set of host keys, check against them. */
+ if (num_known_hosts > 0) {
+ /* Get fingerprint of host we're connecting to */
+ size_t fp_len;
+ int fp_type;
+ const char *fingerprint = libssh2_session_hostkey(session, &fp_len, &fp_type);
+
+ if (!fingerprint)
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
+ "Failed to get fingerprint for host %s", hostname);
+
+ /* Check fingerprint against known hosts */
+ struct libssh2_knownhost *host;
+ int kh_check = libssh2_knownhost_checkp(ssh_known_hosts, hostname, atoi(port),
--- End diff --
The check for `fingerprint` aborts the connection if it is null, so this should never be reached in that case, correct? The thought is that, if the fingerprint of the server cannot be calculated, something else worse than just a host key check has gone wrong and we should bail out entirely.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r197882935
--- Diff: src/common-ssh/common-ssh/key.h ---
@@ -166,5 +169,52 @@ void guac_common_ssh_key_free(guac_common_ssh_key* key);
int guac_common_ssh_key_sign(guac_common_ssh_key* key, const char* data,
int length, unsigned char* sig);
+/**
+ * Verifies the fingerprint for the given hostname/port combination against
+ * one or more known_hosts entries. The known_host entries can either be a
+ * single host_key, provided by the client, or a set of known_hosts entries
+ * provided in the /etc/guacamole/ssh_known_hosts file. Failure to correctly
+ * load the known_hosts entries will result in a connection abort and a returned
+ * error code. A return code of zero indiciates that either no known_hosts entries
+ * were provided, or that the verification succeeded (match). Negative values
+ * indicate internal libssh2 error codes; positive values indicate a failure
+ * during verification of the fingerprint against the known hosts.
+ *
+ * @param session
+ * A pointer to the LIBSSH2_SESSION structure of the SSH connection already
+ * in progress.
+ *
+ * @param client
+ * The current guac_client instance for which the known_hosts checking is
+ * being performed.
+ *
+ * @param host_key
+ * The known host entry provided by the client. If this is non-null and not
+ * empty, it will be the only host key loaded and used for verification. If
+ * this is null or empty an attempt will be made to read the
+ * /etc/guacamole/ssh_known_hosts file and load entries from it.
+ *
+ * @param hostname
+ * The hostname or IP of the server that is being verified.
+ *
+ * @param port
+ * The port number of the server being verified.
+ *
+ * @param fingerprint
+ * The fingering of the server being verified.
--- End diff --
fingerprint*, I believe, assuming this is indeed a fingerprint and not a key (see below).
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r197707629
--- Diff: src/common-ssh/key.c ---
@@ -245,3 +246,84 @@ int guac_common_ssh_key_sign(guac_common_ssh_key* key, const char* data,
}
+int guac_common_ssh_verify_host_key(LIBSSH2_SESSION* session, guac_client* client,
+ const char* host_key, const char* hostname, int port, const char* fingerprint,
+ const size_t fp_len) {
+
+ LIBSSH2_KNOWNHOSTS* ssh_known_hosts = libssh2_knownhost_init(session);
+ int known_hosts = 0;
+
+ /* Add host key provided from settings */
+ if (host_key && strcmp(host_key, "") != 0) {
+
+ known_hosts = libssh2_knownhost_readline(ssh_known_hosts, host_key, strlen(host_key),
+ LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+
+ /* readline function returns 0 on success, so we increment to indicate a valid entry */
+ if (known_hosts == 0)
+ known_hosts++;
+
+ }
+
+ /* Otherwise, we look for a ssh_known_hosts file within GUACAMOLE_HOME and read that in. */
+ else {
+
+ const char *guac_known_hosts = "/etc/guacamole/ssh_known_hosts";
+ if (access(guac_known_hosts, F_OK) != -1)
+ known_hosts = libssh2_knownhost_readfile(ssh_known_hosts, guac_known_hosts, LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+
+ }
+
+ /* If there's an error provided, abort connection and return that. */
+ if (known_hosts < 0) {
+
+ guac_client_log(client, GUAC_LOG_ERROR,
+ "Failure trying to load SSH host keys.");
--- End diff --
Is there any way to log what the specific failure is?
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r190594153
--- Diff: src/protocols/rdp/rdp_settings.c ---
@@ -822,6 +838,32 @@ guac_rdp_settings* guac_rdp_parse_args(guac_user* user,
guac_user_parse_args_string(user, GUAC_RDP_CLIENT_ARGS, argv,
IDX_SFTP_HOSTNAME, settings->hostname);
+ /* The public SSH host key. */
+ settings->sftp_host_key =
+ guac_user_parse_args_string(user, GUAC_RDP_CLIENT_ARGS, argv,
+ IDX_SFTP_HOST_KEY, NULL);
+
+ if(settings->sftp_host_key) {
+ /* Type of public SSH host key. */
+ char* str_host_key_type = guac_user_parse_args_string(user, GUAC_RDP_CLIENT_ARGS, argv,
+ IDX_SFTP_HOST_KEY_TYPE, "ssh-rsa");
+
+ if (strcmp(str_host_key_type, "ssh-rsa") == 0)
+ settings->sftp_host_key_type = LIBSSH2_KNOWNHOST_KEY_SSHRSA;
+ else if (strcmp(str_host_key_type, "ssh-dss") == 0)
+ settings->sftp_host_key_type = LIBSSH2_KNOWNHOST_KEY_SSHDSS;
+ else if (strcmp(str_host_key_type, "rsa1") == 0)
+ settings->sftp_host_key_type = LIBSSH2_KNOWNHOST_KEY_RSA1;
+ else {
--- End diff --
Good call - switched over to this, which makes configuration a little more straight-forward.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r197749922
--- Diff: src/common-ssh/key.c ---
@@ -245,3 +246,84 @@ int guac_common_ssh_key_sign(guac_common_ssh_key* key, const char* data,
}
+int guac_common_ssh_verify_host_key(LIBSSH2_SESSION* session, guac_client* client,
+ const char* host_key, const char* hostname, int port, const char* fingerprint,
+ const size_t fp_len) {
+
+ LIBSSH2_KNOWNHOSTS* ssh_known_hosts = libssh2_knownhost_init(session);
+ int known_hosts = 0;
+
+ /* Add host key provided from settings */
+ if (host_key && strcmp(host_key, "") != 0) {
+
+ known_hosts = libssh2_knownhost_readline(ssh_known_hosts, host_key, strlen(host_key),
+ LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+
+ /* readline function returns 0 on success, so we increment to indicate a valid entry */
+ if (known_hosts == 0)
+ known_hosts++;
+
+ }
+
+ /* Otherwise, we look for a ssh_known_hosts file within GUACAMOLE_HOME and read that in. */
+ else {
+
+ const char *guac_known_hosts = "/etc/guacamole/ssh_known_hosts";
+ if (access(guac_known_hosts, F_OK) != -1)
+ known_hosts = libssh2_knownhost_readfile(ssh_known_hosts, guac_known_hosts, LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+
+ }
+
+ /* If there's an error provided, abort connection and return that. */
+ if (known_hosts < 0) {
+
+ guac_client_log(client, GUAC_LOG_ERROR,
+ "Failure trying to load SSH host keys.");
--- End diff --
I'll have a look - I believe there is a function in the libssh2 API for translating the error to text. I think I actually had it implemented that way at one point, but it must have gotten lost in one of my changes.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r197788208
--- Diff: src/common-ssh/key.c ---
@@ -245,3 +246,84 @@ int guac_common_ssh_key_sign(guac_common_ssh_key* key, const char* data,
}
+int guac_common_ssh_verify_host_key(LIBSSH2_SESSION* session, guac_client* client,
+ const char* host_key, const char* hostname, int port, const char* fingerprint,
+ const size_t fp_len) {
+
+ LIBSSH2_KNOWNHOSTS* ssh_known_hosts = libssh2_knownhost_init(session);
+ int known_hosts = 0;
+
+ /* Add host key provided from settings */
+ if (host_key && strcmp(host_key, "") != 0) {
+
+ known_hosts = libssh2_knownhost_readline(ssh_known_hosts, host_key, strlen(host_key),
+ LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+
+ /* readline function returns 0 on success, so we increment to indicate a valid entry */
+ if (known_hosts == 0)
+ known_hosts++;
+
+ }
+
+ /* Otherwise, we look for a ssh_known_hosts file within GUACAMOLE_HOME and read that in. */
+ else {
+
+ const char *guac_known_hosts = "/etc/guacamole/ssh_known_hosts";
+ if (access(guac_known_hosts, F_OK) != -1)
+ known_hosts = libssh2_knownhost_readfile(ssh_known_hosts, guac_known_hosts, LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+
+ }
+
+ /* If there's an error provided, abort connection and return that. */
+ if (known_hosts < 0) {
+
+ guac_client_log(client, GUAC_LOG_ERROR,
+ "Failure trying to load SSH host keys.");
--- End diff --
Okay, updated it and tested out some errors, and it looks like it shows the correct error message.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r179728475
--- 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);
+
+ /* Add host key provided from settings */
+ if (host_key && strcmp(host_key, "") > 0) {
+
+ int kh_add = libssh2_knownhost_addc(ssh_known_hosts, hostname, NULL, host_key, strlen(host_key),
+ NULL, 0, LIBSSH2_KNOWNHOST_TYPE_PLAIN|LIBSSH2_KNOWNHOST_KEYENC_BASE64|
+ host_key_type, NULL);
+
+ if (kh_add)
+ guac_client_log(client, GUAC_LOG_WARNING, "Failed to add provided host key"
+ " to known hosts store for %s. Error was %d", hostname, kh_add);
+
+ }
+
+ /* Get fingerprint of host we're connecting to */
+ size_t fp_len;
+ int fp_type;
+ const char *fingerprint = libssh2_session_hostkey(session, &fp_len, &fp_type);
+
+ if (!fingerprint)
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
+ "Failed to get fingerprint for host %s", hostname);
+
+ /* Check fingerprint against known hosts */
+ struct libssh2_knownhost *host;
+ int kh_check = libssh2_knownhost_checkp(ssh_known_hosts, hostname, atoi(port),
+ fingerprint, fp_len,
+ LIBSSH2_KNOWNHOST_TYPE_PLAIN|
+ LIBSSH2_KNOWNHOST_KEYENC_RAW,
+ &host);
+
+ libssh2_knownhost_free(ssh_known_hosts);
+
+ switch (kh_check) {
+ case LIBSSH2_KNOWNHOST_CHECK_MATCH:
+ guac_client_log(client, GUAC_LOG_DEBUG,
+ "Host key match found for %s", hostname);
+ break;
+ case LIBSSH2_KNOWNHOST_CHECK_NOTFOUND:
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
--- End diff --
Do I need to pass a more specific error back to the client to indicate this failure, or is that one of the things that falls under the category of "leave it in the log files for the admin to deal with, but don't bother the user with the details?"
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r191088179
--- 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 --
Okay, I've implemented it as suggested and verified that it works. The code seems a little clunky to me, but let me know what you think. I was trying to avoid duplicating a lot of code checking it, but maybe the host key checking needs to be split into its own function?
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r191217635
--- Diff: src/common-ssh/ssh.c ---
@@ -518,6 +520,71 @@ guac_common_ssh_session* guac_common_ssh_create_session(guac_client* client,
return NULL;
}
+ /* SSH known host key checking. */
+ LIBSSH2_KNOWNHOSTS *ssh_known_hosts = libssh2_knownhost_init(session);
+ int num_known_hosts = 0;
+
+ /* Add host key provided from settings */
+ if (host_key && strcmp(host_key, "") != 0) {
+
+ int kh_add = libssh2_knownhost_readline(ssh_known_hosts, host_key, strlen(host_key),
+ LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+ num_known_hosts++;
+
+ if (kh_add)
+ guac_client_log(client, GUAC_LOG_WARNING, "Failed to add provided host key"
+ " to known hosts store for %s. Error was %d", hostname, kh_add);
+
+ }
+
+ /* Otherwise, we look for a ssh_known_hosts file within GUACAMOLE_HOME and read that in. */
+ else {
+ const char *known_hosts = "/etc/guacamole/ssh_known_hosts";
+ num_known_hosts = libssh2_knownhost_readfile(ssh_known_hosts, known_hosts, LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+ }
+
+ /* If we've found a provided set of host keys, check against them. */
+ if (num_known_hosts > 0) {
+ /* Get fingerprint of host we're connecting to */
+ size_t fp_len;
+ int fp_type;
+ const char *fingerprint = libssh2_session_hostkey(session, &fp_len, &fp_type);
+
+ if (!fingerprint)
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
+ "Failed to get fingerprint for host %s", hostname);
+
+ /* Check fingerprint against known hosts */
+ struct libssh2_knownhost *host;
+ int kh_check = libssh2_knownhost_checkp(ssh_known_hosts, hostname, atoi(port),
--- End diff --
Got it. Returning NULL now with the aborts in those areas.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r191088199
--- Diff: src/common-ssh/ssh.c ---
@@ -518,6 +520,63 @@ 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);
+
+ /* Add host key provided from settings */
+ if (host_key && strcmp(host_key, "") > 0) {
--- End diff --
Fixed.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r191081186
--- 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 --
> Okay, I've gone with the approach that known_hosts of the user running guacd will be read and checked against (in addition to any provided host key in the configuration parameters).
Any reason behind checking against two sources? If a host key is specified via the connection parameters, shouldn't that override a local `known_hosts` entirely?
> Or would you still prefer an explicit enable parameter for checking the known_hosts file at all?
What about only checking local `known_hosts` if (1) the host key parameter is not provided and (2) the file exists?
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by mike-jumper <gi...@git.apache.org>.
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.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r197881188
--- Diff: src/common-ssh/ssh.c ---
@@ -518,6 +520,40 @@ guac_common_ssh_session* guac_common_ssh_create_session(guac_client* client,
return NULL;
}
+ /* Get fingerprint of host we're connecting to */
+ size_t fp_len;
+ int fp_type;
+ const char *fingerprint = libssh2_session_hostkey(session, &fp_len, &fp_type);
+
+ /* Failure to generate a fingerprint means we should abort */
+ if (!fingerprint) {
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
+ "Failed to get fingerprint for host %s", hostname);
+ return NULL;
+ }
+
+ /* SSH known host key checking. */
+ int known_host_check = guac_common_ssh_verify_host_key(session, client, host_key,
+ hostname, atoi(port), fingerprint,
+ fp_len);
+
+ /* Abort on any error codes */
+ if (known_host_check != 0) {
+ char* err_msg;
+ int err_len;
+ libssh2_session_last_error(session, &err_msg, &err_len, 0);
+
+ if (known_host_check < 0)
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
+ "Error occurred attempting to check host key: %s", err_msg);
+
+ if (known_host_check > 0)
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
+ "Host fingerprint did not match any provided known host keys. %s", err_msg);
+
+ return NULL;
--- End diff --
Same here - `common_session` and `fd` are resources allocated by this function which will not be freed elsewhere if `NULL` is returned. They need to be freed prior to return.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r190593972
--- 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);
+
+ /* Add host key provided from settings */
+ if (host_key && strcmp(host_key, "") > 0) {
+
+ int kh_add = libssh2_knownhost_addc(ssh_known_hosts, hostname, NULL, host_key, strlen(host_key),
+ NULL, 0, LIBSSH2_KNOWNHOST_TYPE_PLAIN|LIBSSH2_KNOWNHOST_KEYENC_BASE64|
+ host_key_type, NULL);
+
+ if (kh_add)
+ guac_client_log(client, GUAC_LOG_WARNING, "Failed to add provided host key"
+ " to known hosts store for %s. Error was %d", hostname, kh_add);
+
+ }
+
+ /* Get fingerprint of host we're connecting to */
+ size_t fp_len;
+ int fp_type;
+ const char *fingerprint = libssh2_session_hostkey(session, &fp_len, &fp_type);
+
+ if (!fingerprint)
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
+ "Failed to get fingerprint for host %s", hostname);
+
+ /* Check fingerprint against known hosts */
+ struct libssh2_knownhost *host;
+ int kh_check = libssh2_knownhost_checkp(ssh_known_hosts, hostname, atoi(port),
+ fingerprint, fp_len,
+ LIBSSH2_KNOWNHOST_TYPE_PLAIN|
+ LIBSSH2_KNOWNHOST_KEYENC_RAW,
+ &host);
+
+ libssh2_knownhost_free(ssh_known_hosts);
+
+ switch (kh_check) {
+ case LIBSSH2_KNOWNHOST_CHECK_MATCH:
+ guac_client_log(client, GUAC_LOG_DEBUG,
+ "Host key match found for %s", hostname);
+ break;
+ case LIBSSH2_KNOWNHOST_CHECK_NOTFOUND:
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
--- End diff --
I'll just leave it as it is.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r197887124
--- Diff: src/common-ssh/key.c ---
@@ -245,3 +246,86 @@ int guac_common_ssh_key_sign(guac_common_ssh_key* key, const char* data,
}
+int guac_common_ssh_verify_host_key(LIBSSH2_SESSION* session, guac_client* client,
+ const char* host_key, const char* hostname, int port, const char* fingerprint,
+ const size_t fp_len) {
+
+ LIBSSH2_KNOWNHOSTS* ssh_known_hosts = libssh2_knownhost_init(session);
+ int known_hosts = 0;
+
+ /* Add host key provided from settings */
+ if (host_key && strcmp(host_key, "") != 0) {
+
+ known_hosts = libssh2_knownhost_readline(ssh_known_hosts, host_key, strlen(host_key),
+ LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+
+ /* readline function returns 0 on success, so we increment to indicate a valid entry */
+ if (known_hosts == 0)
+ known_hosts++;
+
+ }
+
+ /* Otherwise, we look for a ssh_known_hosts file within GUACAMOLE_HOME and read that in. */
+ else {
+
+ const char *guac_known_hosts = "/etc/guacamole/ssh_known_hosts";
+ if (access(guac_known_hosts, F_OK) != -1)
+ known_hosts = libssh2_knownhost_readfile(ssh_known_hosts, guac_known_hosts, LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+
+ }
+
+ /* If there's an error provided, abort connection and return that. */
+ if (known_hosts < 0) {
+
+ char* errmsg;
+ int errval = libssh2_session_last_error(session, &errmsg, NULL, 0);
+ guac_client_log(client, GUAC_LOG_ERROR,
+ "Error %d trying to load SSH host keys: %s", errval, errmsg);
+
+ libssh2_knownhost_free(ssh_known_hosts);
+ return known_hosts;
+
+ }
+
+ /* No host keys were loaded, so we bail out checking and continue the connection. */
+ else if (known_hosts == 0) {
+ guac_client_log(client, GUAC_LOG_WARNING,
+ "No known host keys provided, host identity will not be verified.");
+ libssh2_knownhost_free(ssh_known_hosts);
+ return known_hosts;
+ }
+
+
+ /* Check fingerprint against known hosts */
--- End diff --
Changed the nomenclature, here.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r191093916
--- Diff: src/common-ssh/ssh.c ---
@@ -518,6 +520,71 @@ guac_common_ssh_session* guac_common_ssh_create_session(guac_client* client,
return NULL;
}
+ /* SSH known host key checking. */
+ LIBSSH2_KNOWNHOSTS *ssh_known_hosts = libssh2_knownhost_init(session);
+ int num_known_hosts = 0;
+
+ /* Add host key provided from settings */
+ if (host_key && strcmp(host_key, "") != 0) {
+
+ int kh_add = libssh2_knownhost_readline(ssh_known_hosts, host_key, strlen(host_key),
+ LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+ num_known_hosts++;
+
+ if (kh_add)
+ guac_client_log(client, GUAC_LOG_WARNING, "Failed to add provided host key"
+ " to known hosts store for %s. Error was %d", hostname, kh_add);
+
+ }
+
+ /* Otherwise, we look for a ssh_known_hosts file within GUACAMOLE_HOME and read that in. */
+ else {
+ const char *known_hosts = "/etc/guacamole/ssh_known_hosts";
+ num_known_hosts = libssh2_knownhost_readfile(ssh_known_hosts, known_hosts, LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+ }
+
+ /* If we've found a provided set of host keys, check against them. */
+ if (num_known_hosts > 0) {
+ /* Get fingerprint of host we're connecting to */
+ size_t fp_len;
+ int fp_type;
+ const char *fingerprint = libssh2_session_hostkey(session, &fp_len, &fp_type);
+
+ if (!fingerprint)
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
+ "Failed to get fingerprint for host %s", hostname);
+
+ /* Check fingerprint against known hosts */
+ struct libssh2_knownhost *host;
+ int kh_check = libssh2_knownhost_checkp(ssh_known_hosts, hostname, atoi(port),
--- End diff --
What happens here if `fingerprint` is `NULL`, as checked earlier?
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r189492258
--- Diff: src/common-ssh/ssh.c ---
@@ -414,7 +415,8 @@ static int guac_common_ssh_authenticate(guac_common_ssh_session* common_session)
}
guac_common_ssh_session* guac_common_ssh_create_session(guac_client* client,
- const char* hostname, const char* port, guac_common_ssh_user* user, int keepalive) {
+ const char* hostname, const char* port, guac_common_ssh_user* user, int keepalive,
+ const int host_key_type, const char* host_key) {
--- End diff --
Perhaps the new connection parameter should be in the same format as the `known_hosts` file (via [`libssh2_knownhost_readline()`](https://www.libssh2.org/libssh2_knownhost_readline.html))? That would eliminate the need for specifying the key type, and would provide for specifying multiple keys.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r197887137
--- Diff: src/common-ssh/common-ssh/key.h ---
@@ -166,5 +169,52 @@ void guac_common_ssh_key_free(guac_common_ssh_key* key);
int guac_common_ssh_key_sign(guac_common_ssh_key* key, const char* data,
int length, unsigned char* sig);
+/**
+ * Verifies the fingerprint for the given hostname/port combination against
+ * one or more known_hosts entries. The known_host entries can either be a
+ * single host_key, provided by the client, or a set of known_hosts entries
+ * provided in the /etc/guacamole/ssh_known_hosts file. Failure to correctly
+ * load the known_hosts entries will result in a connection abort and a returned
+ * error code. A return code of zero indiciates that either no known_hosts entries
+ * were provided, or that the verification succeeded (match). Negative values
+ * indicate internal libssh2 error codes; positive values indicate a failure
+ * during verification of the fingerprint against the known hosts.
+ *
+ * @param session
+ * A pointer to the LIBSSH2_SESSION structure of the SSH connection already
+ * in progress.
+ *
+ * @param client
+ * The current guac_client instance for which the known_hosts checking is
+ * being performed.
+ *
+ * @param host_key
+ * The known host entry provided by the client. If this is non-null and not
+ * empty, it will be the only host key loaded and used for verification. If
+ * this is null or empty an attempt will be made to read the
+ * /etc/guacamole/ssh_known_hosts file and load entries from it.
+ *
+ * @param hostname
+ * The hostname or IP of the server that is being verified.
+ *
+ * @param port
+ * The port number of the server being verified.
+ *
+ * @param fingerprint
+ * The fingering of the server being verified.
--- End diff --
Renamed.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r191081389
--- Diff: src/common-ssh/ssh.c ---
@@ -518,6 +520,63 @@ 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);
+
+ /* Add host key provided from settings */
+ if (host_key && strcmp(host_key, "") > 0) {
--- End diff --
When testing for inequality, `strcmp(...) > 0` is odd. Did you mean `strcmp(...) != 0`?
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r197880942
--- Diff: src/common-ssh/ssh.c ---
@@ -518,6 +520,40 @@ guac_common_ssh_session* guac_common_ssh_create_session(guac_client* client,
return NULL;
}
+ /* Get fingerprint of host we're connecting to */
+ size_t fp_len;
+ int fp_type;
+ const char *fingerprint = libssh2_session_hostkey(session, &fp_len, &fp_type);
+
+ /* Failure to generate a fingerprint means we should abort */
+ if (!fingerprint) {
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
+ "Failed to get fingerprint for host %s", hostname);
+ return NULL;
--- End diff --
This will leak `common_session` and `fd`, allocated earlier in this function. For example, see:
https://github.com/apache/guacamole-server/blob/c120aa0274ee2d11465c80e1d3052c725db44a17/src/common-ssh/ssh.c#L512-L519
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r197887031
--- Diff: src/common-ssh/ssh.c ---
@@ -518,6 +520,40 @@ guac_common_ssh_session* guac_common_ssh_create_session(guac_client* client,
return NULL;
}
+ /* Get fingerprint of host we're connecting to */
+ size_t fp_len;
+ int fp_type;
+ const char *fingerprint = libssh2_session_hostkey(session, &fp_len, &fp_type);
+
+ /* Failure to generate a fingerprint means we should abort */
+ if (!fingerprint) {
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
+ "Failed to get fingerprint for host %s", hostname);
+ return NULL;
+ }
+
+ /* SSH known host key checking. */
+ int known_host_check = guac_common_ssh_verify_host_key(session, client, host_key,
+ hostname, atoi(port), fingerprint,
+ fp_len);
+
+ /* Abort on any error codes */
+ if (known_host_check != 0) {
+ char* err_msg;
+ int err_len;
+ libssh2_session_last_error(session, &err_msg, &err_len, 0);
+
+ if (known_host_check < 0)
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
+ "Error occurred attempting to check host key: %s", err_msg);
+
+ if (known_host_check > 0)
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
+ "Host fingerprint did not match any provided known host keys. %s", err_msg);
+
+ return NULL;
--- End diff --
Lead plugged.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r191104171
--- Diff: src/common-ssh/ssh.c ---
@@ -518,6 +520,71 @@ guac_common_ssh_session* guac_common_ssh_create_session(guac_client* client,
return NULL;
}
+ /* SSH known host key checking. */
+ LIBSSH2_KNOWNHOSTS *ssh_known_hosts = libssh2_knownhost_init(session);
+ int num_known_hosts = 0;
+
+ /* Add host key provided from settings */
+ if (host_key && strcmp(host_key, "") != 0) {
+
+ int kh_add = libssh2_knownhost_readline(ssh_known_hosts, host_key, strlen(host_key),
+ LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+ num_known_hosts++;
+
+ if (kh_add)
+ guac_client_log(client, GUAC_LOG_WARNING, "Failed to add provided host key"
+ " to known hosts store for %s. Error was %d", hostname, kh_add);
--- End diff --
Yeah, looks like maybe I can use `libssh2_session_last_error()`:
https://libssh2.org/libssh2_session_last_error.html
We'll have to see how useful the error messages really are - some of them will probably be near as meaningless as the error code - but it'll at least get closer to the error.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r189494030
--- 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);
+
+ /* Add host key provided from settings */
+ if (host_key && strcmp(host_key, "") > 0) {
+
+ int kh_add = libssh2_knownhost_addc(ssh_known_hosts, hostname, NULL, host_key, strlen(host_key),
+ NULL, 0, LIBSSH2_KNOWNHOST_TYPE_PLAIN|LIBSSH2_KNOWNHOST_KEYENC_BASE64|
+ host_key_type, NULL);
+
+ if (kh_add)
+ guac_client_log(client, GUAC_LOG_WARNING, "Failed to add provided host key"
+ " to known hosts store for %s. Error was %d", hostname, kh_add);
+
+ }
+
+ /* Get fingerprint of host we're connecting to */
+ size_t fp_len;
+ int fp_type;
+ const char *fingerprint = libssh2_session_hostkey(session, &fp_len, &fp_type);
+
+ if (!fingerprint)
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
+ "Failed to get fingerprint for host %s", hostname);
+
+ /* Check fingerprint against known hosts */
+ struct libssh2_knownhost *host;
+ int kh_check = libssh2_knownhost_checkp(ssh_known_hosts, hostname, atoi(port),
+ fingerprint, fp_len,
+ LIBSSH2_KNOWNHOST_TYPE_PLAIN|
+ LIBSSH2_KNOWNHOST_KEYENC_RAW,
+ &host);
+
+ libssh2_knownhost_free(ssh_known_hosts);
+
+ switch (kh_check) {
+ case LIBSSH2_KNOWNHOST_CHECK_MATCH:
+ guac_client_log(client, GUAC_LOG_DEBUG,
+ "Host key match found for %s", hostname);
+ break;
+ case LIBSSH2_KNOWNHOST_CHECK_NOTFOUND:
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
--- End diff --
To the extent that the failure to validate the host is due to misconfiguration, `GUAC_PROTOCOL_STATUS_SERVER_ERROR` is probably correct. That is the status code we use elsewhere when an error with connection parameters is encountered.
Other options for the case where the host key doesn't match could be:
* `GUAC_PROTOCOL_STATUS_UPSTREAM_ERROR` (the documentation states "The operation was unsuccessful due to an error or otherwise unexpected condition of the upstream server", and this certainly seems an unexpected condition of the upstream server).
* `GUAC_PROTOCOL_STATUS_UPSTREAM_NOT_FOUND` (we were not able to find the expected remote desktop server - we found some other server instead)
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r179728179
--- 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);
+
+ /* Add host key provided from settings */
+ if (host_key && strcmp(host_key, "") > 0) {
+
+ int kh_add = libssh2_knownhost_addc(ssh_known_hosts, hostname, NULL, host_key, strlen(host_key),
+ NULL, 0, LIBSSH2_KNOWNHOST_TYPE_PLAIN|LIBSSH2_KNOWNHOST_KEYENC_BASE64|
+ host_key_type, NULL);
+
+ if (kh_add)
+ guac_client_log(client, GUAC_LOG_WARNING, "Failed to add provided host key"
+ " to known hosts store for %s. Error was %d", hostname, kh_add);
+
--- End diff --
I decided to make errors adding the known hosts key provided from the client non-fatal, as, if the host key is missing, it will ultimately fail below.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r189586135
--- 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 --
I agree that this is a departure from backward-compatibility, but I have two arguments for why I think it should be done that way, anyway:
- 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. This gets closer to that, though without prompting within the client, we can't ask if it doesn't initially exist.
- It makes SSH support in Guacamole behave more like RDP. In RDP, you have to explicitly tell Guacamole to ignore server certificates, else it fails the connection unless you have correctly configured SSL on the system running guacd such that the certificates are trusted. So, the behavior is more consistent within itself.
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. If the host key exists but does not match, fail.
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.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r189492193
--- 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 --
Verifying against `~/.ssh/known_hosts` by default will break current behavior. In the interest of backward compatibility, host checking should be enforced only when explicitly enabled.
I agree that a server-side `known_hosts` file may be desirable, though loading that file (and host key checking in general) should be disabled unless explicitly enabled through connection parameters. Requiring the location to be explicitly specified in connection parameters would be one way of doing that. Defaulting to `~/.ssh/known_hosts` or `/etc/guacamole/known_hosts` or `/etc/guacamole/ssh/known_hosts` and providing a boolean connection parameter to enable checking against that file would be another.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/guacamole-server/pull/164
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r191094035
--- Diff: src/common-ssh/ssh.c ---
@@ -518,6 +520,71 @@ guac_common_ssh_session* guac_common_ssh_create_session(guac_client* client,
return NULL;
}
+ /* SSH known host key checking. */
+ LIBSSH2_KNOWNHOSTS *ssh_known_hosts = libssh2_knownhost_init(session);
+ int num_known_hosts = 0;
+
+ /* Add host key provided from settings */
+ if (host_key && strcmp(host_key, "") != 0) {
+
+ int kh_add = libssh2_knownhost_readline(ssh_known_hosts, host_key, strlen(host_key),
+ LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+ num_known_hosts++;
+
+ if (kh_add)
+ guac_client_log(client, GUAC_LOG_WARNING, "Failed to add provided host key"
+ " to known hosts store for %s. Error was %d", hostname, kh_add);
--- End diff --
Is there any way to get a human-readable error? The integer value of `kh_add` isn't likely to have any meaning to the user reading the logs, let alone to us when reported as a bug.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r197882126
--- Diff: src/common-ssh/key.c ---
@@ -245,3 +246,86 @@ int guac_common_ssh_key_sign(guac_common_ssh_key* key, const char* data,
}
+int guac_common_ssh_verify_host_key(LIBSSH2_SESSION* session, guac_client* client,
+ const char* host_key, const char* hostname, int port, const char* fingerprint,
+ const size_t fp_len) {
+
+ LIBSSH2_KNOWNHOSTS* ssh_known_hosts = libssh2_knownhost_init(session);
+ int known_hosts = 0;
+
+ /* Add host key provided from settings */
+ if (host_key && strcmp(host_key, "") != 0) {
+
+ known_hosts = libssh2_knownhost_readline(ssh_known_hosts, host_key, strlen(host_key),
+ LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+
+ /* readline function returns 0 on success, so we increment to indicate a valid entry */
+ if (known_hosts == 0)
+ known_hosts++;
+
+ }
+
+ /* Otherwise, we look for a ssh_known_hosts file within GUACAMOLE_HOME and read that in. */
+ else {
+
+ const char *guac_known_hosts = "/etc/guacamole/ssh_known_hosts";
+ if (access(guac_known_hosts, F_OK) != -1)
+ known_hosts = libssh2_knownhost_readfile(ssh_known_hosts, guac_known_hosts, LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+
+ }
+
+ /* If there's an error provided, abort connection and return that. */
+ if (known_hosts < 0) {
+
+ char* errmsg;
+ int errval = libssh2_session_last_error(session, &errmsg, NULL, 0);
+ guac_client_log(client, GUAC_LOG_ERROR,
+ "Error %d trying to load SSH host keys: %s", errval, errmsg);
+
+ libssh2_knownhost_free(ssh_known_hosts);
+ return known_hosts;
+
+ }
+
+ /* No host keys were loaded, so we bail out checking and continue the connection. */
+ else if (known_hosts == 0) {
+ guac_client_log(client, GUAC_LOG_WARNING,
+ "No known host keys provided, host identity will not be verified.");
+ libssh2_knownhost_free(ssh_known_hosts);
+ return known_hosts;
+ }
+
+
+ /* Check fingerprint against known hosts */
--- End diff --
Is this the fingerprint? I see that the variable is named `fingerprint`, but so far the functions receiving that value seem to deal only with keys, or at least that's what I gather from the libssh2 documentation.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r191086955
--- Diff: src/common-ssh/ssh.c ---
@@ -518,6 +520,63 @@ 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");
--- End diff --
> What about using a location specific to guacd like /etc/guacamole/ssh/known_hosts?
I'm willing to go that route. Will rewrite it as such.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r189595826
--- Diff: src/common-ssh/ssh.c ---
@@ -414,7 +415,8 @@ static int guac_common_ssh_authenticate(guac_common_ssh_session* common_session)
}
guac_common_ssh_session* guac_common_ssh_create_session(guac_client* client,
- const char* hostname, const char* port, guac_common_ssh_user* user, int keepalive) {
+ const char* hostname, const char* port, guac_common_ssh_user* user, int keepalive,
+ const int host_key_type, const char* host_key) {
--- End diff --
Yeah, probably a better option.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r189494122
--- Diff: src/protocols/rdp/rdp_settings.c ---
@@ -822,6 +838,32 @@ guac_rdp_settings* guac_rdp_parse_args(guac_user* user,
guac_user_parse_args_string(user, GUAC_RDP_CLIENT_ARGS, argv,
IDX_SFTP_HOSTNAME, settings->hostname);
+ /* The public SSH host key. */
+ settings->sftp_host_key =
+ guac_user_parse_args_string(user, GUAC_RDP_CLIENT_ARGS, argv,
+ IDX_SFTP_HOST_KEY, NULL);
+
+ if(settings->sftp_host_key) {
+ /* Type of public SSH host key. */
+ char* str_host_key_type = guac_user_parse_args_string(user, GUAC_RDP_CLIENT_ARGS, argv,
+ IDX_SFTP_HOST_KEY_TYPE, "ssh-rsa");
+
+ if (strcmp(str_host_key_type, "ssh-rsa") == 0)
+ settings->sftp_host_key_type = LIBSSH2_KNOWNHOST_KEY_SSHRSA;
+ else if (strcmp(str_host_key_type, "ssh-dss") == 0)
+ settings->sftp_host_key_type = LIBSSH2_KNOWNHOST_KEY_SSHDSS;
+ else if (strcmp(str_host_key_type, "rsa1") == 0)
+ settings->sftp_host_key_type = LIBSSH2_KNOWNHOST_KEY_RSA1;
+ else {
--- End diff --
Leveraging [`libssh2_knownhost_readline()`](https://www.libssh2.org/libssh2_knownhost_readline.html) might be a better alternative.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r191086961
--- Diff: src/common-ssh/ssh.c ---
@@ -518,6 +520,63 @@ 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);
+
+ /* Add host key provided from settings */
+ if (host_key && strcmp(host_key, "") > 0) {
--- End diff --
Yeah, not sure how I arrived at that. Will change it.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r191088192
--- Diff: src/common-ssh/ssh.c ---
@@ -518,6 +520,63 @@ 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");
--- End diff --
Okay, implemented as suggested. Again, this might be a little clunky - let me know if you think it's worth moving this to its own function - maybe even in the key.h/key.c files within the SSH code?
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/164#discussion_r191107001
--- Diff: src/common-ssh/ssh.c ---
@@ -518,6 +520,71 @@ guac_common_ssh_session* guac_common_ssh_create_session(guac_client* client,
return NULL;
}
+ /* SSH known host key checking. */
+ LIBSSH2_KNOWNHOSTS *ssh_known_hosts = libssh2_knownhost_init(session);
+ int num_known_hosts = 0;
+
+ /* Add host key provided from settings */
+ if (host_key && strcmp(host_key, "") != 0) {
+
+ int kh_add = libssh2_knownhost_readline(ssh_known_hosts, host_key, strlen(host_key),
+ LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+ num_known_hosts++;
+
+ if (kh_add)
+ guac_client_log(client, GUAC_LOG_WARNING, "Failed to add provided host key"
+ " to known hosts store for %s. Error was %d", hostname, kh_add);
+
+ }
+
+ /* Otherwise, we look for a ssh_known_hosts file within GUACAMOLE_HOME and read that in. */
+ else {
+ const char *known_hosts = "/etc/guacamole/ssh_known_hosts";
+ num_known_hosts = libssh2_knownhost_readfile(ssh_known_hosts, known_hosts, LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+ }
+
+ /* If we've found a provided set of host keys, check against them. */
+ if (num_known_hosts > 0) {
+ /* Get fingerprint of host we're connecting to */
+ size_t fp_len;
+ int fp_type;
+ const char *fingerprint = libssh2_session_hostkey(session, &fp_len, &fp_type);
+
+ if (!fingerprint)
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
+ "Failed to get fingerprint for host %s", hostname);
+
+ /* Check fingerprint against known hosts */
+ struct libssh2_knownhost *host;
+ int kh_check = libssh2_knownhost_checkp(ssh_known_hosts, hostname, atoi(port),
--- End diff --
The connection will be aborted, yes, but `guac_client_abort()` does not stop the remainder of the function from executing. It's a combination of `guac_client_stop()`, `guac_client_log()`, and `guac_protocol_send_error()`, flagging the `guac_client` as stopped (which eventually terminates the loops involved), reporting the error to the client, and logging an error to guacd's logs.
It's a cooperative signal, coupled with logging:
https://github.com/apache/guacamole-server/blob/master/src/libguac/guacamole/client.h#L314-L317
---