-
Notifications
You must be signed in to change notification settings - Fork 648
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-2012: Fix SSH connection to FIPS servers which only offer AES GCM #572
base: patch
Are you sure you want to change the base?
Conversation
eugen-keeper
commented
Jan 7, 2025
- Added AES GCM ciphers to the FIPS compliant cipher list
- Added some code to check if the hardcoded FIPS compliant ciphers are really supported by libssh2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me - the only thing that I see is an opportunity for some comments in the new function you implemented - the existing code around that has bits of documentation throughout, so maybe just brush that up a bit in the new code?
src/common-ssh/ssh.c
Outdated
} | ||
} | ||
guac_mem_free(preferred_algs); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't cuddle the else
. See: https://cwiki.apache.org/confluence/display/GUAC/Contribution+and+Style+Guidelines#ContributionandStyleGuidelines-Braces
@eugen-keeper Please also:
|
ea80b45
to
c9b341e
Compare
c9b341e
to
af6a5bb
Compare
"libssh2 reports that no ciphers/algorithms are supported. This may be a bug in libssh2." | ||
"If the SSH connection fails, it may not be possible to log the cause here."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A space is needed at the end of the first string literal here. Without that, the relevant portion of the resulting string will be libssh2.If
and not the intended libssh2. If
.
/* Request the list of supported algorithms/cyphers from libssh2. */ | ||
const char** supported_algs; | ||
int supported_algs_count = | ||
libssh2_session_supported_algs(session, method_type, &supported_algs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the example at https://libssh2.org/libssh2_session_supported_algs.html, the memory pointed to by supported_algs
after a successful call to libssh2_session_supported_algs()
must eventually be freed by a call to libssh2_free()
.