You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by mike-jumper <gi...@git.apache.org> on 2018/09/11 21:39:48 UTC
[GitHub] guacamole-server pull request #187: GUACAMOLE-622: Withhold first SSH/telnet...
GitHub user mike-jumper opened a pull request:
https://github.com/apache/guacamole-server/pull/187
GUACAMOLE-622: Withhold first SSH/telnet frame until connection is verified
These changes add a new function, `guac_terminal_start()`, which must be called before the terminal emulator will send the `sync` messages which define frame boundaries. SSH and telnet have been modified to call this function only when the connection has actually been successful.
By withholding frames until the underlying connection is complete, the "Connected. Waiting for response..." status notification becomes an accurate reflection of the connection state. In addition, server-side handling of connection errors via `FailoverGuacamoleSocket` becomes possible, as the instruction filtering code that searches for errors also uses that first `sync` as a sign of success.
As telnet does define an authentication mechanism, there is no standard way to detect whether authentication has succeeded, however these changes define two additional regex parameters which allow success/failure to be detected heuristically. If a username, password, and these two regex are provided, the initial frame is withheld until final success/failure has been determined.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mike-jumper/guacamole-server ssh-telnet-error
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/guacamole-server/pull/187.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 #187
----
commit 61a51df1b29695f6464193b18dd4f24809ef07df
Author: Michael Jumper <mj...@...>
Date: 2018-08-30T17:01:41Z
GUACAMOLE-622: Require guac_terminal_start() to be invoked before the terminal will render frames or accept user input.
commit 0b39b0fc5fa9846ff350cc443b8f432efb4678ee
Author: Michael Jumper <mj...@...>
Date: 2018-08-30T17:06:20Z
GUACAMOLE-622: Implicitly invoke guac_terminal_start() if prompting is required.
commit 4606607309761cca9f69dbd8e15be2376950dac3
Author: Michael Jumper <mj...@...>
Date: 2018-08-30T17:08:30Z
GUACAMOLE-622: Start terminal for SSH only after SSH connection succeeds.
commit 286cbf32a7a178cd6483a52983a33d486edc9129
Author: Michael Jumper <mj...@...>
Date: 2018-09-02T04:26:37Z
GUACAMOLE-622: Ensure connection to guacd is kept alive even if the SSH daemon is taking its time responding. Lengthy connect times due to DNS verification, PAM, etc. are not uncommon.
commit 1178b475dad6b8c567041dda8e3fabd34a584f75
Author: Michael Jumper <mj...@...>
Date: 2018-09-02T04:41:13Z
GUACAMOLE-622: Do not allow STDIN to be redirected if the terminal is not yet started.
commit ab1ca03f9b6dc98fd9dbe614162f6f87641d3f3c
Author: Michael Jumper <mj...@...>
Date: 2018-09-02T21:32:05Z
GUACAMOLE-622: Start terminal for telnet only after login status is known (if login success/failure detection enabled).
commit 55ac0c5b323d8c36d6b09bd99c2d5bc2cd0d043b
Author: Michael Jumper <mj...@...>
Date: 2018-09-03T05:59:18Z
GUACAMOLE-622: Match each line against all regexes.
----
---
[GitHub] guacamole-server pull request #187: GUACAMOLE-622: Withhold first SSH/telnet...
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/187#discussion_r217206655
--- Diff: src/protocols/telnet/telnet.c ---
@@ -82,57 +83,178 @@ static int __guac_telnet_write_all(int fd, const char* buffer, int size) {
}
/**
- * Searches for a line matching the stored password regex, appending the given
- * buffer to the internal pattern matching buffer. The internal pattern match
- * buffer is cleared whenever a newline is read. Returns TRUE if a match is found and the
- * value is sent.
+ * Matches the given line against the given regex, returning true and sending
+ * the given value if a match is found. An enter keypress is automatically
+ * sent after the value is sent.
+ *
+ * @param client
+ * The guac_client associated with the telnet session.
+ *
+ * @param regex
+ * The regex to search for within the given line buffer.
+ *
+ * @param value
+ * The string value to send through STDIN of the telnet session if a
+ * match is found, or NULL if no value should be sent.
+ *
+ * @param line_buffer
+ * The line of character data to test.
+ *
+ * @return
+ * true if a match is found, false otherwise.
*/
-static bool __guac_telnet_regex_search(guac_client* client, regex_t* regex, char* value, const char* buffer, int size) {
+static bool guac_telnet_regex_exec(guac_client* client, regex_t* regex,
+ const char* value, const char* line_buffer) {
- static char line_buffer[1024] = {0};
- static int length = 0;
+ guac_telnet_client* telnet_client = (guac_telnet_client*) client->data;
+
+ /* Send value upon match */
+ if (regexec(regex, line_buffer, 0, NULL, 0) == 0) {
+
+ /* Send value */
+ if (value != NULL) {
+ guac_terminal_send_string(telnet_client->term, value);
+ guac_terminal_send_string(telnet_client->term, "\x0D");
+ }
+
+ /* Stop searching for prompt */
+ return true;
+
+ }
+
+ return false;
+
+}
+
+/**
+ * Matches the given line against the various stored regexes, automatically
+ * sending the configured username, password, or reporting login
+ * success/failure depending on context. If no search is in progress, either
+ * because no regexes have been defined or because all applicable searches have
+ * completed, this function has no effect.
+ *
+ * @param client
+ * The guac_client associated with the telnet session.
+ *
+ * @param line_buffer
+ * The line of character data to test.
+ */
+static void guac_telnet_search_line(guac_client* client, const char* line_buffer) {
guac_telnet_client* telnet_client = (guac_telnet_client*) client->data;
+ guac_telnet_settings* settings = telnet_client->settings;
+
+ /* Continue search for username prompt */
+ if (settings->username_regex != NULL) {
+ if (guac_telnet_regex_exec(client, settings->username_regex,
+ settings->username, line_buffer)) {
+ guac_client_log(client, GUAC_LOG_DEBUG, "Username sent");
+ guac_telnet_regex_free(&settings->username_regex);
+ }
+ }
+
+ /* Continue search for password prompt */
+ if (settings->password_regex != NULL) {
+ if (guac_telnet_regex_exec(client, settings->password_regex,
+ settings->password, line_buffer)) {
- int i;
- const char* current;
+ guac_client_log(client, GUAC_LOG_DEBUG, "Password sent");
- /* Ensure line buffer contains only the most recent line */
- current = buffer;
- for (i = 0; i < size; i++) {
+ /* Do not continue searching for username/password once password is sent */
+ guac_telnet_regex_free(&settings->username_regex);
--- End diff --
I'm guessing I'm just missing something, here, but what's the purpose of freeing the `username_regex` pointer here? Isn't it going to have already been processed by the block above, which checks for its presence? Or is there some situation where it would fail the `if (settings->username_regex != null)` but still actually contain something?
---
[GitHub] guacamole-server pull request #187: GUACAMOLE-622: Withhold first SSH/telnet...
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/187#discussion_r219632398
--- Diff: src/protocols/telnet/settings.c ---
@@ -196,12 +198,31 @@ enum TELNET_ARGS_IDX {
*/
IDX_SCROLLBACK,
+ /**
+ * The regular expression to use when searching for whether login was
+ * successful. This parameter is optional. If given, the
+ * "login-failure-regex" parameter must also be specified, and the first
+ * frame of the Guacamole connection will be withheld until login
+ * success/failure has been determined.
+ */
+ IDX_LOGIN_SUCCESS_REGEX,
+
+ /**
+ * The regular expression to use when searching for whether login was
+ * unsuccessful. This parameter is optional. If given, the
+ * "login-failure-regex" parameter must also be specified, and the first
--- End diff --
Definitely. Fixed and rebased.
---
[GitHub] guacamole-server pull request #187: GUACAMOLE-622: Withhold first SSH/telnet...
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/187#discussion_r219632440
--- Diff: src/protocols/telnet/settings.h ---
@@ -253,6 +267,16 @@ typedef struct guac_telnet_settings {
guac_telnet_settings* guac_telnet_parse_args(guac_user* user,
int argc, const char** argv);
+/**
+ * Frees the regex pointed to by the given pointer, assigning the value NULL to
+ * that pointer once the regex is freed. If the pointer already contains NULL,
+ * this functino has no effect.
--- End diff --
Heh - fixed.
---
[GitHub] guacamole-server pull request #187: GUACAMOLE-622: Withhold first SSH/telnet...
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/187#discussion_r217197430
--- Diff: src/protocols/telnet/settings.h ---
@@ -253,6 +267,16 @@ typedef struct guac_telnet_settings {
guac_telnet_settings* guac_telnet_parse_args(guac_user* user,
int argc, const char** argv);
+/**
+ * Frees the regex pointed to by the given pointer, assigning the value NULL to
+ * that pointer once the regex is freed. If the pointer already contains NULL,
+ * this functino has no effect.
--- End diff --
*function
---
[GitHub] guacamole-server pull request #187: GUACAMOLE-622: Withhold first SSH/telnet...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/guacamole-server/pull/187
---
[GitHub] guacamole-server pull request #187: GUACAMOLE-622: Withhold first SSH/telnet...
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/187#discussion_r219631546
--- Diff: src/protocols/telnet/telnet.c ---
@@ -82,57 +83,178 @@ static int __guac_telnet_write_all(int fd, const char* buffer, int size) {
}
/**
- * Searches for a line matching the stored password regex, appending the given
- * buffer to the internal pattern matching buffer. The internal pattern match
- * buffer is cleared whenever a newline is read. Returns TRUE if a match is found and the
- * value is sent.
+ * Matches the given line against the given regex, returning true and sending
+ * the given value if a match is found. An enter keypress is automatically
+ * sent after the value is sent.
+ *
+ * @param client
+ * The guac_client associated with the telnet session.
+ *
+ * @param regex
+ * The regex to search for within the given line buffer.
+ *
+ * @param value
+ * The string value to send through STDIN of the telnet session if a
+ * match is found, or NULL if no value should be sent.
+ *
+ * @param line_buffer
+ * The line of character data to test.
+ *
+ * @return
+ * true if a match is found, false otherwise.
*/
-static bool __guac_telnet_regex_search(guac_client* client, regex_t* regex, char* value, const char* buffer, int size) {
+static bool guac_telnet_regex_exec(guac_client* client, regex_t* regex,
+ const char* value, const char* line_buffer) {
- static char line_buffer[1024] = {0};
- static int length = 0;
+ guac_telnet_client* telnet_client = (guac_telnet_client*) client->data;
+
+ /* Send value upon match */
+ if (regexec(regex, line_buffer, 0, NULL, 0) == 0) {
+
+ /* Send value */
+ if (value != NULL) {
+ guac_terminal_send_string(telnet_client->term, value);
+ guac_terminal_send_string(telnet_client->term, "\x0D");
+ }
+
+ /* Stop searching for prompt */
+ return true;
+
+ }
+
+ return false;
+
+}
+
+/**
+ * Matches the given line against the various stored regexes, automatically
+ * sending the configured username, password, or reporting login
+ * success/failure depending on context. If no search is in progress, either
+ * because no regexes have been defined or because all applicable searches have
+ * completed, this function has no effect.
+ *
+ * @param client
+ * The guac_client associated with the telnet session.
+ *
+ * @param line_buffer
+ * The line of character data to test.
+ */
+static void guac_telnet_search_line(guac_client* client, const char* line_buffer) {
guac_telnet_client* telnet_client = (guac_telnet_client*) client->data;
+ guac_telnet_settings* settings = telnet_client->settings;
+
+ /* Continue search for username prompt */
+ if (settings->username_regex != NULL) {
+ if (guac_telnet_regex_exec(client, settings->username_regex,
+ settings->username, line_buffer)) {
+ guac_client_log(client, GUAC_LOG_DEBUG, "Username sent");
+ guac_telnet_regex_free(&settings->username_regex);
+ }
+ }
+
+ /* Continue search for password prompt */
+ if (settings->password_regex != NULL) {
+ if (guac_telnet_regex_exec(client, settings->password_regex,
+ settings->password, line_buffer)) {
- int i;
- const char* current;
+ guac_client_log(client, GUAC_LOG_DEBUG, "Password sent");
- /* Ensure line buffer contains only the most recent line */
- current = buffer;
- for (i = 0; i < size; i++) {
+ /* Do not continue searching for username/password once password is sent */
+ guac_telnet_regex_free(&settings->username_regex);
--- End diff --
Yep. There is a situation where `username_regex` will still not be NULL, yet the password regex will match.
While no telnet server has support for sending the password as a dedicated authentication phase, some telnet servers have support for sending the username directly. This is accomplished through sending an environment variable called `USER`:
https://github.com/apache/guacamole-server/blob/332e187813595fc2e769f3e29c0582b7ec726ea1/src/protocols/telnet/telnet.c#L222-L229
As this isn't guaranteed, the heuristics for recognizing username/password prompts have to be somewhat forgiving. It's possible that there will be a username prompt followed by a password prompt, and it's possible that there will be only a password prompt.
---
[GitHub] guacamole-server pull request #187: GUACAMOLE-622: Withhold first SSH/telnet...
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/187#discussion_r217195956
--- Diff: src/protocols/telnet/settings.c ---
@@ -196,12 +198,31 @@ enum TELNET_ARGS_IDX {
*/
IDX_SCROLLBACK,
+ /**
+ * The regular expression to use when searching for whether login was
+ * successful. This parameter is optional. If given, the
+ * "login-failure-regex" parameter must also be specified, and the first
+ * frame of the Guacamole connection will be withheld until login
+ * success/failure has been determined.
+ */
+ IDX_LOGIN_SUCCESS_REGEX,
+
+ /**
+ * The regular expression to use when searching for whether login was
+ * unsuccessful. This parameter is optional. If given, the
+ * "login-failure-regex" parameter must also be specified, and the first
--- End diff --
Copy pasta. I think this should say
> "login-success-regex" pararameter must also be specified...
---