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

Fix SAML read Certificate and private key #990

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

Conversation

kalaxcortez
Copy link

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

@necouchman
Copy link
Contributor

@kalaxcortez Thank you for contributing, here. Before we can accept this contribution, a number of things need to be done:

  • You need to request a Jira account and open a Jira issue, and your pull request and commit messages need to be tagged with the Jira issue. You can look at any of the existing closed, and many of the open pull requests for examples of this.
  • You need to follow established style within the code. For example, you're cuddling braces (} catch () {) - they should be on new lines. Please adjust your style to conform to the rest of the code.
  • I think the IOException handling would actually be better placed in the FileGuacamoleProperty class (in guacamole-ext), where it could easily be used by any and all users of the class, rather than just these two properties. It does not seem to be a situation unique to these particular usages of that property.
  • You're catching both IOException and GuacamoleException, here, and printing the stack trace - this defeats the purpose of having this particular function throw the GuacamoleException. If you're going to catch IOException, it should be re-thrown as a GuacamoleException, and the throwing of GuacamoleException should be left alone.

please validate and if you do not like the structure, change it but solve it

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();
Copy link
Contributor

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();
Copy link
Contributor

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();
Copy link
Contributor

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();
Copy link
Contributor

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.

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