[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
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...
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,
_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, _msg, _len, 0);
--- End diff --
NULLed.
---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
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,
_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, _msg, _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...
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...
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, , 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...
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, _len,
_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, _msg, _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...
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, _len,
_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...
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...
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, _len,
_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, _msg, _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...
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, , 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-client pull request #304: GUACAMOLE-580: Check for null when retri...
Github user asfgit closed the pull request at: https://github.com/apache/guacamole-client/pull/304 ---
[GitHub] guacamole-server pull request #164: GUACAMOLE-527: Implement SSH host key ch...
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...
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...
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?
---
