mike-jumper commented on code in PR #392:
URL: https://github.com/apache/guacamole-server/pull/392#discussion_r959000647
##########
src/protocols/rdp/settings.c:
##########
@@ -706,12 +707,31 @@ guac_rdp_settings* guac_rdp_parse_args(guac_user* user,
if (strcmp(argv[IDX_SECURITY], "nla") == 0) {
guac_user_log(user, GUAC_LOG_INFO, "Security mode: NLA");
settings->security_mode = GUAC_SECURITY_NLA;
+
+ /*
+ * NLA is known not to work with FIPS; allow the mode selection but
+ * warn that it will not work.
+ */
+ if (guac_fips_enabled())
+ guac_user_log(user, GUAC_LOG_WARNING,
+ "NLA security mode is selected, "
+ "but is known not to work, as FIPS mode is enabled.");
Review Comment:
I think we should additionally note the anticipated effect and, since we
have it, the expected solution. For example:
> NLA security mode was selected, but is known to be currently incompatible
with FIPS mode (see https://github.com/FreeRDP/FreeRDP/issues/3412). Security
negotiation with the RDP server may fail unless TLS security mode is selected
instead.
##########
src/protocols/rdp/settings.c:
##########
@@ -1529,7 +1549,20 @@ void guac_rdp_push_settings(guac_client* client,
case GUAC_SECURITY_ANY:
rdp_settings->RdpSecurity = TRUE;
rdp_settings->TlsSecurity = TRUE;
- rdp_settings->NlaSecurity = guac_settings->username &&
guac_settings->password;
+
+ /* Explicitly disable NLA if FIPS mode is enabled - it won't work
*/
+ if (guac_fips_enabled()) {
+
+ guac_client_log(client, GUAC_LOG_WARNING,
+ "Disabling NLA security mode when FIPS mode is
enabled.");
+ rdp_settings->NlaSecurity = FALSE;
+
+ }
+
+ /* Enable NLA security mode if both username and password are set
*/
+ else
+ rdp_settings->NlaSecurity = guac_settings->username &&
guac_settings->password;
Review Comment:
I think this is a holdover from back when we didn't support prompting for
credentials and can be safely replaced with just `rdp_settings->NlaSecurity =
TRUE;`.
##########
src/protocols/rdp/settings.c:
##########
@@ -1529,7 +1549,20 @@ void guac_rdp_push_settings(guac_client* client,
case GUAC_SECURITY_ANY:
rdp_settings->RdpSecurity = TRUE;
rdp_settings->TlsSecurity = TRUE;
- rdp_settings->NlaSecurity = guac_settings->username &&
guac_settings->password;
+
+ /* Explicitly disable NLA if FIPS mode is enabled - it won't work
*/
+ if (guac_fips_enabled()) {
+
+ guac_client_log(client, GUAC_LOG_WARNING,
+ "Disabling NLA security mode when FIPS mode is
enabled.");
Review Comment:
1. This should be at the info level. Nothing is failing or anticipated to
fail here (yet); it's just a noteworthy piece of information.
2. As written, this is always true. I suggest rephrasing to explicitly note
the recognition of system state and the action taken: `FIPS mode is enabled.
Excluding NLA security mode from security negotiation (see:
https://github.com/FreeRDP/FreeRDP/issues/3412).`
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
