Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GUACAMOLE-1627: Enforce TLS1.2 #390

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sivagudivadaz
Copy link

This is a one-off issue when communicating with Windows 2019 server that has both TLS1.3 and TLS1.2 enabled.
Win 2019 has partial/experimental support for TLS1.3 - although it advertises this during version negotiation, it doesn't allow the client (FreeRDP) to make a successful RDP connection.

A new version of FreeRDP has been released https://github.com/FreeRDP/FreeRDP/releases/tag/2.8.0 that supports TLS Version control for this use case, the changes in this PR utilize this feature to establish a successful connection.

@necouchman
Copy link
Contributor

I'm wondering if, rather than forcing a setting given the FreeRDP library version, it makes more sense to make this a configurable option that can be driven from the client-side, perhaps with a default value chosen based on the FreeRDP library and/or OpenSSL library version. This would incorporate some of what you've done, but also allow admins to drive particular versions if they so desire, and they've correctly configured the RDP server.

Also, regarding your commit message:

  • The commit message needs to include the GUACAMOLE-1627 prefix.
  • The commit message should be a bit more descriptive than that - Enforce TLS1.2 in what situations? Why?

@sivagudivadaz
Copy link
Author

Regarding commit messages: will reword the commit message and provide better descriptive info for the changes.

it makes more sense to make this a configurable option that can be driven from the client-side, perhaps with a default value chosen based on the FreeRDP library and/or OpenSSL library version.

Will look into this, thank you!

…vers that have experimental support for TLS1.3
@sivagudivadaz
Copy link
Author

sivagudivadaz commented Aug 27, 2022

make this a configurable option that can be driven from the client-side

Are you referring to the guacamole-client project?

@necouchman
Copy link
Contributor

@sivagudivadaz : It would require changes in both guacamole-server and guacamole-client, yes. You'd need to make the changes in this code to allow a connection parameter to be provided that would allow you to override the TLS version. The changes in the guacamole-client code would be required to allow connections to provide that parameter.

@sivagudivadaz
Copy link
Author

sivagudivadaz commented Oct 20, 2022

Hi @necouchman , Does this look like a good spot to introduce a parameter to version control TLS ?
image

I don't think the user will run into TLS1.3 issues that often or at least this new UI setting will not be touched as frequently as other settings in the Authentication section. So, my preference would be to introduce a guacd configuration setting rather than making an UI change.

Thoughts ?

@necouchman
Copy link
Contributor

@sivagudivadaz : Yes, that seems to be a reasonable place for it.

@sivagudivadaz
Copy link
Author

Thank you for confirming, Nick. I will raise a PR soon.

// Note that older versions of FreeRDP that relied on older versions of Openssl that didn't have TLS1.3 don't run into
// this issue as the max TLS version supported by those clients is TLS1.2.
rdp_settings->TLSMinVersion = 0;
rdp_settings->TLSMaxVersion = TLS1_2_VERSION;
Copy link

Choose a reason for hiding this comment

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

Rather than hard-coding this for 1.2 only, wouldn't it be better to add configuration options for defining min and max versions? For example, Redemption uses such configuration options.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jclc: Yes, I mentioned this in my original comment, above, and most of the conversation between @sivagudivadaz and me has been regarding the best place to put such an option or options.

I suppose we can talk about whether it makes more sense to force a single specific version or provide options for minimum and maximum versions. I'm not opposed to a version range, per se, but I wonder how valuable it will be - if you have a connection to a single server, it may not be all that useful to have a range of options, as the server is usually going to consistently support the same level, and you ought to be able to just enforce that supported level.

Copy link

Choose a reason for hiding this comment

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

What about using the Guacamole protocol to pass these as optional arguments? Then you could do it on a connection-by-connection basis.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's exactly what we're already talking about. The conversation and screenshot above deals with making these TLS versions configurable on a per-connection basis - if they are added as connection parameters they would be passed along with the rest of the options (hostname, port, username, password, etc.) within the Guacamole Protocol as part of establishing the connection.

@jclc
Copy link

jclc commented Dec 19, 2023

Is there any progress on this? This is a blocker for a feature we require, so I'd be willing to contribute.

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