[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #209: GUACAMOLE-637: Migrate strncpy(), strcat(), etc. to safer libguac implementations.
mike-jumper commented on a change in pull request #209: GUACAMOLE-637: Migrate
strncpy(), strcat(), etc. to safer libguac implementations.
URL: https://github.com/apache/guacamole-server/pull/209#discussion_r272858597
##
File path: src/protocols/rdp/rdp_fs.c
##
@@ -607,11 +608,10 @@ const char* guac_rdp_fs_read_dir(guac_rdp_fs* fs, int
file_id) {
int guac_rdp_fs_normalize_path(const char* path, char* abs_path) {
int i;
-int path_depth = 0;
+int path_depth = 1;
Review comment:
OK - I've cleaned up the SFTP and RDP filesystem normalization logic and
removed the need for setting `path_depth = 1`, which the unit tests I've just
added revealed to be incorrect.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #219: GUACAMOLE-422: Add timezone to client handshake
mike-jumper commented on a change in pull request #219: GUACAMOLE-422: Add timezone to client handshake URL: https://github.com/apache/guacamole-server/pull/219#discussion_r272822672 ## File path: src/libguac/guacamole/user.h ## @@ -537,32 +544,6 @@ void guac_user_free(guac_user* user); */ int guac_user_handle_connection(guac_user* user, int usec_timeout); -/** - * Call the appropriate handler defined by the given user for the given - * instruction. A comparison is made between the instruction opcode and the - * initial handler lookup table defined in user-handlers.c. The intial handlers - * will in turn call the user's handler (if defined). - * - * @param user - * The user whose handlers should be called. - * - * @param opcode - * The opcode of the instruction to pass to the user via the appropriate - * handler. - * - * @param argc - * The number of arguments which are part of the instruction. - * - * @param argv - * An array of all arguments which are part of the instruction. - * - * @return - * Non-negative if the instruction was handled successfully, or negative - * if an error occurred. - */ -int guac_user_handle_instruction(guac_user* user, const char* opcode, Review comment: > Should I just copy it instead of move it ... I think the best route would be: * Keep the new version of `guac_user_handle_instruction()` but rename it such that it can exist as an internal function which doesn't collide with the existing `guac_user_handle_instruction()`. * Invoke the new version from the old function such that `guac_user_handle_instruction()` continues to work, yet primarily exists in one place. > ... and mark this for deprecation? I'm not 100% on that. If we will be removing this function, it does need to be deprecated first, but should it be removed? It seems a useful counterpart to `guac_parser`. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #219: GUACAMOLE-422: Add timezone to client handshake
mike-jumper commented on a change in pull request #219: GUACAMOLE-422: Add
timezone to client handshake
URL: https://github.com/apache/guacamole-server/pull/219#discussion_r272821725
##
File path: src/protocols/kubernetes/settings.c
##
@@ -25,6 +25,14 @@
/* Client plugin arguments */
const char* GUAC_KUBERNETES_CLIENT_ARGS[] = {
+/**
+ * This first argument defines the protocol version in use so that the
+ * client knows how to handle talking to different versions of guacd.
+ * If this is omitted the client may choose not to enable certain
+ * features.
+ */
+"VERSION_1_1_0",
Review comment:
`VERSION_1_1_0` seems fine to me, and would indeed be convenient for parsing
on the Java side.
This would remain `VERSION_1_1_0` even after guacamole-server is at 1.2.0,
1.3.999, etc., being a reference to the protocol variation introduced by the
release having that same version number?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
Re: Guacamole Prompting, Round 4,963,211
On Sat, Apr 6, 2019 at 11:38 PM Nick Couchman wrote: > On Wed, Mar 13, 2019 at 1:18 PM Nick Couchman wrote: > > > > > >> Looks reasonable. Naming convention within libguac for the various > >> instruction sending functions is guac_protocol_send_OPCODE(), though, so > >> this would need to be guac_protocol_send_required(). > >> > > > > I've updated the naming conventions on these... > > > > > >> > >> Perhaps the instruction should accept a list of parameter names rather > >> than > >> just a single name? This is what is done for the related "args" > >> instruction: > >> > >> > https://guacamole.apache.org/doc/1.0.0/libguac/protocol_8h.html#a6047d380b097ebc7d5f35b167e3419e6 > >> > >> > > Yeah, this was my next step, but just thought I should start with getting > > it working, then move on to making the code more efficient. > > > > > >> Client: > >> > > >> > > >> > https://github.com/necouchman/guacamole-client/commit/72de5595880baea46505cf4f9ad49640f16519e7 > >> > >> > >> Same here - convention is "onOPCODE", so this would need to be > >> "onrequired". > >> > > > > Fixed. > > > > > >> > > >> I think you will run into trouble manually parsing the Guacamole > protocol > >> with a guac_parser in a context where inbound instructions are already > >> being parsed received. Is this because the FreeRDP authentication > callback > >> must be synchronous? > >> > >> > > I'm not sure. On the xfreerdp side of things, this code results in a > > Password: prompt on the command line, and the xfreerdp app waits until > you > > enter something before proceeding. > > > > Yeah, looks like this is going to be problematic. Parsing the Guacamole > protocol while it is already running doesn't seem to work - if I use > guac_parser_expect(), it fails because there are other things going on in > the background for the connection and the argv opcode isn't the first thing > through, so it doesn't get what it's expecting. I also tried just looping > on guac_parser_read() and looking at the opcodes as they come across, > ignoring anything but argv, but it still fails. Right. You cannot have two parsers reading from the same socket, nor should protocol support be attempting to parse the Guacamole protocol manually. To handle argv, you need to provide an argv_handler to the guac_user struct. That handler will be called when argv is received. This probably means that the use of the FreeRDP authentication callback > will not work, unless > there's some other way to accomplish this. I could still try to block the > pthread until the input is received, but this might cause other problems on > the client side with an unresponsive connection? > If FreeRDP depends on this being synchronous, yes, the handler will need to block on some sort of pthread condition which is signalled when the credentials are set. Reading of the argv instruction should happen magically as long as argv_handler is properly set, as Guacamole protocol parsing is handled for you by guacd and libguac in a separate thread. If blocking will potentially result in the connection going silent, there's a function for that - guac_socket_require_keep_alive(): http://guacamole.apache.org/doc/libguac/socket_8h.html#a1d8c5111a8234c7785ae23f379f7db03 That will force the guac_socket to send a "nop" instruction roughly every 5 seconds if nothing else is sent during that period. - Mike
Re: Guacamole Prompting, Round 4,963,211
> > >>> > >>> I think you will run into trouble manually parsing the Guacamole protocol >>> with a guac_parser in a context where inbound instructions are already >>> being parsed received. Is this because the FreeRDP authentication >>> callback >>> must be synchronous? >>> >>> >> I'm not sure. On the xfreerdp side of things, this code results in a >> Password: prompt on the command line, and the xfreerdp app waits until you >> enter something before proceeding. >> > > Yeah, looks like this is going to be problematic. Parsing the Guacamole > protocol while it is already running doesn't seem to work - if I use > guac_parser_expect(), it fails because there are other things going on in > the background for the connection and the argv opcode isn't the first thing > through, so it doesn't get what it's expecting. I also tried just looping > on guac_parser_read() and looking at the opcodes as they come across, > ignoring anything but argv, but it still fails. This probably means that > the use of the FreeRDP authentication callback will not work, unless > there's some other way to accomplish this. I could still try to block the > pthread until the input is received, but this might cause other problems on > the client side with an unresponsive connection? > > I could prompt ahead of this, before the RDP connection is actually > established, but the situations where input are going to be required aren't > necessarily predictable. For example, if the server supports either RDP or > TLS security, then the username and password do not have to be entered in > order to establish the connection, so it's possible for the connection to > go to the Windows login screen. If NLA security is in use, then the > credentials must be provided prior to the connection being established. > However, if you have authentication set to "any" (auto-detect), then it's > hard to know before actually connecting to the server what is going to be > required. I could make some assumptions about it (just ask for username > and password if they aren't provided, assuming they'll be used at some > point along the way), but maybe there are situations where people actually > don't want to provide those at all, and don't want to be prompted for it? > > Well, tried moving to the following functions (for RDP) with no joy: - rdp_freerdp_pre_connect() - Behaves identically to using it in the authenticate callback, where the prompt is sent to the client and displayed, but the connection still fails and does not wait for the prompt. - guac_rdp_handle_connection() - This gets me even less far along than putting it in one of the callbacks - now I don't even see the prompt show up on the client side, and it still fails - doesn't wait, and doesn't seem to be getting any opcode. Not sure, but maybe this is too early in the connection? - guac_rdp_client_thread() - Same as in the handle connection function - no required opcode showing up on the client side, and connection just hangs. Sorry, this is probably incredibly dense of me, but right now just trying to get one prompt working... -Nick
Re: Guacamole Prompting, Round 4,963,211
On Wed, Mar 13, 2019 at 1:18 PM Nick Couchman wrote: > > >> Looks reasonable. Naming convention within libguac for the various >> instruction sending functions is guac_protocol_send_OPCODE(), though, so >> this would need to be guac_protocol_send_required(). >> > > I've updated the naming conventions on these... > > >> >> Perhaps the instruction should accept a list of parameter names rather >> than >> just a single name? This is what is done for the related "args" >> instruction: >> >> https://guacamole.apache.org/doc/1.0.0/libguac/protocol_8h.html#a6047d380b097ebc7d5f35b167e3419e6 >> >> > Yeah, this was my next step, but just thought I should start with getting > it working, then move on to making the code more efficient. > > >> Client: >> > >> > >> https://github.com/necouchman/guacamole-client/commit/72de5595880baea46505cf4f9ad49640f16519e7 >> >> >> Same here - convention is "onOPCODE", so this would need to be >> "onrequired". >> > > Fixed. > > >> > >> I think you will run into trouble manually parsing the Guacamole protocol >> with a guac_parser in a context where inbound instructions are already >> being parsed received. Is this because the FreeRDP authentication callback >> must be synchronous? >> >> > I'm not sure. On the xfreerdp side of things, this code results in a > Password: prompt on the command line, and the xfreerdp app waits until you > enter something before proceeding. > Yeah, looks like this is going to be problematic. Parsing the Guacamole protocol while it is already running doesn't seem to work - if I use guac_parser_expect(), it fails because there are other things going on in the background for the connection and the argv opcode isn't the first thing through, so it doesn't get what it's expecting. I also tried just looping on guac_parser_read() and looking at the opcodes as they come across, ignoring anything but argv, but it still fails. This probably means that the use of the FreeRDP authentication callback will not work, unless there's some other way to accomplish this. I could still try to block the pthread until the input is received, but this might cause other problems on the client side with an unresponsive connection? I could prompt ahead of this, before the RDP connection is actually established, but the situations where input are going to be required aren't necessarily predictable. For example, if the server supports either RDP or TLS security, then the username and password do not have to be entered in order to establish the connection, so it's possible for the connection to go to the Windows login screen. If NLA security is in use, then the credentials must be provided prior to the connection being established. However, if you have authentication set to "any" (auto-detect), then it's hard to know before actually connecting to the server what is going to be required. I could make some assumptions about it (just ask for username and password if they aren't provided, assuming they'll be used at some point along the way), but maybe there are situations where people actually don't want to provide those at all, and don't want to be prompted for it? -Nick >
