-
Notifications
You must be signed in to change notification settings - Fork 711
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
Fix SAML read Certificate and private key #990
base: main
Are you sure you want to change the base?
Conversation
@kalaxcortez Thank you for contributing, here. Before we can accept this contribution, a number of things need to be done:
I believe your suggested changes have merit, and, with some tweaks, would be quite valuable to have added to the project. I'd highly encourage you to take the time to do the steps mentioned above and clean this up for inclusion - and, if you're so inclined, find other ways to contribute to the project. |
return environment.getProperty(SAML_X509_CERT_PATH); | ||
File certificate = null; | ||
try { | ||
certificate = environment.getProperty(SAML_X509_CERT_PATH).getCanonicalFile(); |
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.
This will throw a NullPointerException
if getProperty()
returns null
(if the property is not present). Same for the other call to getCanonicalFile()
.
return environment.getProperty(SAML_X509_CERT_PATH); | ||
File certificate = null; | ||
try { | ||
certificate = environment.getProperty(SAML_X509_CERT_PATH).getCanonicalFile(); |
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.
What is the problem you're encountering that is fixed by calling getCanonicalFile()
?
try { | ||
certificate = environment.getProperty(SAML_X509_CERT_PATH).getCanonicalFile(); | ||
} catch (IOException | GuacamoleException e) { | ||
e.printStackTrace(); |
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.
If an error should be logged, it should be logged by a Logger
instance with the stack trace logged only at the debug level. We typically do this by logging things twice, with an admin-facing message at the error level and a developer-facing message at the debug level.
That said, I think the proper thing to do here would be to just rethrow the IOException
as a GuacamoleException
, with the GuacamoleException
adding a meaningful message.
@@ -480,7 +492,7 @@ public Saml2Settings getSamlSettings() throws GuacamoleException { | |||
readFileContentsIntoString(privateKeyFile, "Private Key")); | |||
|
|||
// If a certificate file is set, load the value into the builder now | |||
File certificateFile = getCertificateFile(); | |||
File certificateFile = getCertificateFile(); |
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.
It looks like this line was changed only by adding several spaces to the end.
Please validate the reading of the certificate and private key files for SAML, it is only one line and it solves the entire signing issue, please validate and if you do not like the structure, change it but solve it, validate it locally and everything works, Greetings