Skip to content

Conversation

@necouchman
Copy link
Contributor

Okay, here we go...this pull request implements support for two major components of getting Guacamole to support prompting:

  • Support for sending the "required" instruction to the client, along with whatever is being required.
  • Support for reading back the required input from the client via the argv instruction, including various handlers for the protocols to handle (mostly authentication) data sent back.

I've tested where I can to try to make sure everything works - RDP and SSH seem to be good (along with some client changes that I'll need to clean up and submit, soon). I don't have a great VNC setup to try that out with, but I tested what I could.

Also, at this moment this change does not handle collecting and sending multiple required parameters, and then reading them back concurrently - it does things one at a time. I know that was part of the discussion somewhere (other PR, mailing list - something), so I'm happy to take a stab at that, too, but wanted to get some other eyes on this code, first.

Also, I'm not sure if this particular change warrants a rev of the protocol version introduced in 1.1.0 - it does add the required instruction??

@mike-jumper
Copy link
Contributor

Should the new required instruction allow for multiple parameters, similar in principle to args?

It feels awkward to request credentials one at a time, particularly in the common username+password case.

@necouchman
Copy link
Contributor Author

@mike-jumper: Yep, definitely. I'll continue to work that...

@necouchman necouchman force-pushed the jira/221 branch 4 times, most recently from 2bb69b3 to 1bc13da Compare August 10, 2019 01:36
@necouchman
Copy link
Contributor Author

@mike-jumper: Okay, protocol side has been reworked to act like send_args where the type is a const char** and it loops through a NULL-terminated array gather up everything.

I retooled the RDP credential request function to leverage it - it feels a bit clunky and probably needs some work.

I'm not sure how feasible it is on the SSH side to gather up all of the required credentials and send them all at once - the various requests for credentials are spread out quite a bit through different pieces of the code. Or maybe it's just an opportunity to do things differently :-).

@necouchman
Copy link
Contributor Author

@mike-jumper: Well, most of that was pretty straight-forward on the server side, but not entirely sure how to handle multiple values being sent. The RDP code is probably the best current example of this - the way it's constructed right now:

  • guacd sends parameters to client that need to be input.
  • RDP authentication callback kicks off pthread_cond_wait waiting for request to be filled.
  • Prompts are presented to the client, which sends them back one at a time.
  • argv handler triggers completion via the pthread_cond_broadcast as soon one of the parameters is received.

I don't think it's going to work the way I currently have it implemented because the thread is going to continue as soon as the first value is received, and not wait for the remaining ones. Any suggestions on how I should proceed that would satisfy all of the following:

  • Allow multiple parameters to be both requested (guacd -> client) and received (client->guacd) in a single shot
  • Allow callback functions of the various libraries to be used.
  • Allow for the returned username, password, and/or domain (in the case of RDP) to be empty strings.

??

@necouchman
Copy link
Contributor Author

Okay, think I figured something out for this - implemented some flags for the conditional stuff, maybe that'll work...

Copy link
Contributor

@mike-jumper mike-jumper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you go - an initial review. :)

Beware this will also need rebasing because of #243. It definitely will conflict as-is with all those changes to RDP having been merged.

@necouchman necouchman force-pushed the jira/221 branch 4 times, most recently from 164dd1f to 8002b26 Compare January 26, 2020 09:36
@necouchman necouchman force-pushed the jira/221 branch 3 times, most recently from 609e313 to df08daa Compare March 19, 2020 18:24
@necouchman
Copy link
Contributor Author

Okay, made some changes, fixed up some style and such, and has been rebased after RDP updates.

@necouchman necouchman force-pushed the jira/221 branch 4 times, most recently from e9d1043 to 43c1879 Compare June 28, 2020 19:57
@necouchman necouchman force-pushed the jira/221 branch 7 times, most recently from af81acb to e76e772 Compare September 18, 2020 11:14
@necouchman necouchman force-pushed the jira/221 branch 3 times, most recently from 8ca9bab to 12f64f0 Compare September 19, 2020 02:20
@necouchman necouchman force-pushed the jira/221 branch 3 times, most recently from f55182d to 321af24 Compare September 20, 2020 01:56
Copy link
Contributor

@mike-jumper mike-jumper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost ... there ...

@necouchman
Copy link
Contributor Author

Yeah, it's a bit of a process with me...lol.

@necouchman necouchman force-pushed the jira/221 branch 2 times, most recently from 945a3fe to 1b4f125 Compare September 20, 2020 18:38
@necouchman
Copy link
Contributor Author

Okay, did another swipe through the code and fixed up a handful of things.

@mike-jumper mike-jumper changed the base branch from master to staging/1.3.0 September 20, 2020 20:19
Copy link
Contributor

@mike-jumper mike-jumper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏆

@mike-jumper mike-jumper merged commit aa870de into apache:staging/1.3.0 Sep 21, 2020
@necouchman necouchman deleted the jira/221 branch June 18, 2022 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants