You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by tylermauthe <gi...@git.apache.org> on 2016/05/27 03:29:21 UTC
[GitHub] incubator-guacamole-server pull request: GUACAMOLE-39: Do not abor...
GitHub user tylermauthe opened a pull request:
https://github.com/apache/incubator-guacamole-server/pull/11
GUACAMOLE-39: Do not abort VNC/RDP connection if SFTP fails
Please review and let me know if you'd like to suggest any changes.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Hatsize/incubator-guacamole-server graceful-sftp-connection
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-guacamole-server/pull/11.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 #11
----
commit 35d272bc8f380897e412dc7ab2bdd02109063ed4
Author: Tyler Mauthe <tm...@hatsize.com>
Date: 2016-05-27T03:15:59Z
GUACAMOLE-39: Do not abort VNC/RDP connection if SFTP fails
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #11: GUACAMOLE-39: Do not abort VNC/...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/11#discussion_r71967184
--- Diff: src/protocols/rdp/rdp.c ---
@@ -1003,36 +1003,27 @@ void* guac_rdp_client_thread(void* data) {
/* Attempt SSH connection */
rdp_client->sftp_session =
guac_common_ssh_create_session(client, settings->sftp_hostname,
- settings->sftp_port, rdp_client->sftp_user);
-
- /* Fail if SSH connection does not succeed */
- if (rdp_client->sftp_session == NULL) {
- /* Already aborted within guac_common_ssh_create_session() */
- guac_common_ssh_destroy_user(rdp_client->sftp_user);
- return NULL;
- }
-
- /* Load and expose filesystem */
- rdp_client->sftp_filesystem =
- guac_common_ssh_create_sftp_filesystem(
- rdp_client->sftp_session, "/");
-
- /* Expose filesystem to connection owner */
- guac_client_for_owner(client,
- guac_common_ssh_expose_sftp_filesystem,
- rdp_client->sftp_filesystem);
-
- /* Abort if SFTP connection fails */
- if (rdp_client->sftp_filesystem == NULL) {
- guac_common_ssh_destroy_session(rdp_client->sftp_session);
- guac_common_ssh_destroy_user(rdp_client->sftp_user);
- guac_client_abort(client, GUAC_PROTOCOL_STATUS_UPSTREAM_ERROR,
- "SFTP connection failed.");
- return NULL;
+ settings->sftp_port, rdp_client->sftp_user, 0);
+
+ /* Try to initialize SFTP if connection succeeded */
+ if (rdp_client->sftp_session != NULL) {
+ /* Load filesystem */
+ rdp_client->sftp_filesystem =
+ guac_common_ssh_create_sftp_filesystem(
+ rdp_client->sftp_session, "/");
+
+ /* Expose filesystem to connection owner */
+ guac_client_for_owner(client,
+ guac_common_ssh_expose_sftp_filesystem,
+ rdp_client->sftp_filesystem);
+
+ /* Verify that SFTP connection succeeded */
+ if (rdp_client->sftp_filesystem != NULL) {
+ guac_client_log(client, GUAC_LOG_DEBUG,
+ "SFTP connection succeeded.");
+ }
}
--- End diff --
I think you've accidentally killed the required calls to `guac_common_ssh_destroy_user()`, as well as the other cleanup in the case that the filesystem object creation fails, etc.
If the session fails to start, the user object will be leaked. If the filesystem fails, the user and session objects will be leaked.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #11: GUACAMOLE-39: Do not abort VNC/...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/11#discussion_r71967234
--- Diff: src/protocols/vnc/vnc.c ---
@@ -283,43 +283,32 @@ void* guac_vnc_client_thread(void* data) {
/* Attempt SSH connection */
vnc_client->sftp_session =
guac_common_ssh_create_session(client, settings->sftp_hostname,
- settings->sftp_port, vnc_client->sftp_user);
-
- /* Fail if SSH connection does not succeed */
- if (vnc_client->sftp_session == NULL) {
- /* Already aborted within guac_common_ssh_create_session() */
- guac_common_ssh_destroy_user(vnc_client->sftp_user);
- return NULL;
- }
-
- /* Load filesystem */
- vnc_client->sftp_filesystem =
- guac_common_ssh_create_sftp_filesystem(
- vnc_client->sftp_session, "/");
-
- /* Expose filesystem to connection owner */
- guac_client_for_owner(client,
- guac_common_ssh_expose_sftp_filesystem,
- vnc_client->sftp_filesystem);
-
- /* Abort if SFTP connection fails */
- if (vnc_client->sftp_filesystem == NULL) {
- guac_common_ssh_destroy_session(vnc_client->sftp_session);
- guac_common_ssh_destroy_user(vnc_client->sftp_user);
- guac_client_abort(client, GUAC_PROTOCOL_STATUS_UPSTREAM_ERROR,
- "SFTP connection failed.");
- return NULL;
+ settings->sftp_port, vnc_client->sftp_user, 0);
+
+ /* Try to initialize SFTP if connection succeeded */
+ if (vnc_client->sftp_session != NULL) {
+ /* Load filesystem */
+ vnc_client->sftp_filesystem =
+ guac_common_ssh_create_sftp_filesystem(
+ vnc_client->sftp_session, "/");
+
+ /* Expose filesystem to connection owner */
+ guac_client_for_owner(client,
+ guac_common_ssh_expose_sftp_filesystem,
+ vnc_client->sftp_filesystem);
+
+ /* Verify SFTP connection succeeded */
+ if (vnc_client->sftp_filesystem != NULL) {
+ /* If specified, configure destination for basic uploads */
+ if (settings->sftp_directory != NULL)
+ guac_common_ssh_sftp_set_upload_path(
+ vnc_client->sftp_filesystem,
+ settings->sftp_directory);
+
+ guac_client_log(client, GUAC_LOG_DEBUG,
+ "SFTP connection succeeded.");
+ }
--- End diff --
Same deal here. It looks like the user and filesystem object cleanup was deleted without being re-added.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request: GUACAMOLE-39: Do not abor...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/11#discussion_r64854314
--- Diff: src/common-ssh/guac_ssh.c ---
@@ -274,19 +275,36 @@ static void guac_common_ssh_kbd_callback(const char *name, int name_len,
}
+void ssh_client_error(bool abortOnError, guac_client* client, guac_protocol_status status,
+ const char* format, ...) {
+ va_list args;
--- End diff --
The `stdarg.h` header required for `va_list` and related functions is missing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #11: GUACAMOLE-39: Do not abort VNC/...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/11#discussion_r71967090
--- Diff: src/common-ssh/guac_ssh.h ---
@@ -76,10 +76,10 @@ void guac_common_ssh_uninit();
/**
* Connects to the SSH server running at the given hostname and port, and
* authenticates as the given user. If an error occurs while connecting or
- * authenticating, the Guacamole client will automatically and fatally abort.
- * The user object provided must eventually be explicitly destroyed, but should
- * not be destroyed until this session is destroyed, assuming the session is
- * successfully created.
+ * authenticating, the Guacamole client will only automatically and fatally
+ * abort if the abort_on_error parameter is set to 1 and errors will always
--- End diff --
As mentioned earlier, requiring `1` is odd for C. Better for this to simply be non-zero. Other than that, still looking good.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #11: GUACAMOLE-39: Do not abort VNC/...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/11#discussion_r71967043
--- Diff: src/common-ssh/guac_ssh.c ---
@@ -275,11 +275,43 @@ static void guac_common_ssh_kbd_callback(const char *name, int name_len,
}
/**
+ * A handler for SSH client errors which logs the error and can optionally
+ * abort the given guac_client with the given guac_protocol_status.
+ *
+ * @param client
+ * Guacamole client instance to use when logging or aborting.
+ *
+ * @param status
+ * Protocol status to return if aborting the connection. This is only
+ * used if abort_on_error is set to true.
+ *
+ * @param abort_on_error
+ * Whether to abort the connection as well as log the error.
+ *
+ * @param format
+ * Format string for log messages. This is also used when aborting
+ * the connection.
+ *
+ */
+static void guac_common_ssh_client_error(guac_client* client,
+ guac_protocol_status status, int abort_on_error, const char* format, ...) {
+ va_list args;
+ va_start(args, format);
+ if(abort_on_error == 1) {
--- End diff --
Two things here:
1. Direct comparisons against `1` are a little bit funky in C. Normally, you use non-zero to represent true, as this is what C uses internally for `if` tests, etc. Things will work more as expected with:
```
if (abort_on_error) {
...
}
```
2. `if` is not a function. There should be a space between the `if` and the opening paren.
Beyond that, I think this is OK. The function seems a bit crowded to me due to the lack of blank lines / "code paragraphs", but that's not a deal-breaker. I'd like you to consider omitting the braces where unnecessary (as that's our preference - see the [style guidelines](http://guacamole.incubator.apache.org/guac-style/)), but as we say in that doc, these aren't hard rules - use your own best judgement.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #11: GUACAMOLE-39: Do not abort VNC/...
Posted by tylermauthe <gi...@git.apache.org>.
Github user tylermauthe closed the pull request at:
https://github.com/apache/incubator-guacamole-server/pull/11
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request: GUACAMOLE-39: Do not abor...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/11#discussion_r64854256
--- Diff: src/common-ssh/guac_ssh.c ---
@@ -274,19 +275,36 @@ static void guac_common_ssh_kbd_callback(const char *name, int name_len,
}
+void ssh_client_error(bool abortOnError, guac_client* client, guac_protocol_status status,
--- End diff --
The are a few issues here:
1. The other functions defined in this part of guacamole-server all have the namespace/prefix `guac_common_ssh_`. Should be the same here.
2. If this is not used externally (and not declared in a header file), it should be `static`.
3. Documentation of the function and its parameters is missing.
4. The variable `abortOnError` does not follow the naming style of the rest of this file or guacamole-server. Please user lowercase, separating words by underscores.
As a minor nitpick, it's strange to me to a flag variable as the first argument in a function. Normally such things are toward the end of the parameter list. As this is essentially a selective wrapper for `guac_client_abort()` and `guac_client_log()`, I would recommend following the same ordering to avoid confusion when people try to develop against this in the future.
I also notice that the `status` parameter is ignored in the case that the abort flag is set. I don't immediately see a way to avoid that with this approach, but perhaps that is a sign that a different approach would be better.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request: GUACAMOLE-39: Do not abor...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/11#discussion_r64854778
--- Diff: src/common-ssh/guac_ssh.c ---
@@ -274,19 +275,36 @@ static void guac_common_ssh_kbd_callback(const char *name, int name_len,
}
+void ssh_client_error(bool abortOnError, guac_client* client, guac_protocol_status status,
+ const char* format, ...) {
+ va_list args;
+ va_start(args, format);
+ if(abortOnError) {
+ vguac_client_abort(client, status, format, args);
+ }
+ else {
+ vguac_client_log(client, GUAC_LOG_ERROR, format, args);
+ }
+ va_end(args);
+}
+
/**
* Authenticates the user associated with the given session over SSH. All
* required credentials must already be present within the user object
* associated with the given session.
*
- * @param session
+ * @param abortOnError
+ * If set to true, the connection will be aborted. Otherwise, the error
+ * will be logged.
+ *
+ * @param common_session
* The session associated with the user to be authenticated.
*
* @return
* Zero if authentication succeeds, or non-zero if authentication has
* failed.
*/
-static int guac_common_ssh_authenticate(guac_common_ssh_session* common_session) {
+static int guac_common_ssh_authenticate(bool abortOnError, guac_common_ssh_session* common_session) {
--- End diff --
Since the function is already defined as returning zero / non-zero depending on whether the operation succeeds, why not leverage that and let the caller dictate whether to abort?
It seems like we shouldn't have had `guac_common_ssh_authenticate()` automatically handle the full blown abort in the first place - way out of scope for what it's suppose to do / be aware of.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request: GUACAMOLE-39: Do not abor...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/11#discussion_r64854575
--- Diff: src/common-ssh/guac_ssh.h ---
@@ -101,6 +101,33 @@ guac_common_ssh_session* guac_common_ssh_create_session(guac_client* client,
const char* hostname, const char* port, guac_common_ssh_user* user);
/**
+ * Used within VNC and RDP connections to create an SFTP connection using the
--- End diff --
Dictating exactly where this function is used within the documentation of the function seems questionable to me. Can this not instead be described at a high level, strictly from the perspective of the common SSH client implemented here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request: GUACAMOLE-39: Do not abor...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/11#discussion_r64854416
--- Diff: src/common-ssh/guac_ssh.h ---
@@ -101,6 +101,33 @@ guac_common_ssh_session* guac_common_ssh_create_session(guac_client* client,
const char* hostname, const char* port, guac_common_ssh_user* user);
/**
+ * Used within VNC and RDP connections to create an SFTP connection using the
+ * given hostname and port, and authenticates as the given user. If an error
+ * occurs while connecting or authenticating, the Guacamole client will log
+ * the error encountered and continue without aborting. The user object provided
+ * must eventually be explicitly destroyed, but should not be destroyed until the
+ * entire VNC or RDP session is terminated.
+ *
+ * @param client
+ * The Guacamole client that will be using SSH.
+ *
+ * @param hostname
+ * The hostname of the SSH server to connect to.
+ *
+ * @param port
+ * The port to connect to on the given hostname.
+ *
+ * @param user
+ * The user to authenticate as, once connected.
+ *
+ * @return
+ * A new SSH session if the connection and authentication succeed, or NULL
+ * if the connection or authentication were not successful.
+ */
+guac_common_ssh_session* guac_common_ssh_create_secondary_session(guac_client* client,
--- End diff --
Why define a whole new function for this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---