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-1524: Importing LDAPS certificate into docker container #694

Closed
wants to merge 1 commit into from

Conversation

sirux88
Copy link
Contributor

@sirux88 sirux88 commented Jan 30, 2022

Copy link
Contributor

@mike-jumper mike-jumper left a comment

Choose a reason for hiding this comment

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

Overall:

  • I really think this is not the right approach. If the container needs to be able to perform a privileged action involving system-wide certificates, we need to find a safe way to do that, not just open up the cert store to modification by the webapp.
  • Please provide a more meaningful commit message than "commit". Commit messages should capture the high-level purpose of the change being made, and need to reference the JIRA issue (see established git history). A good blog post on this: https://www.codelord.net/2015/03/16/bad-commit-messages-hall-of-shame/

Comment on lines +65 to +66
# allow guacamole user to import certificates into default java keystore file cacerts
run chown guacamole /usr/local/openjdk-8/jre/lib/security/cacerts && chmod +w /usr/local/openjdk-8/jre/lib/security/cacerts
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be done. Should something go awry in the web application, this would extend the malicious potential of that to installing certificates.

The limited-privilege user that runs the webapp shouldn't be able to alter certs.

Copy link
Contributor Author

@sirux88 sirux88 Feb 2, 2022

Choose a reason for hiding this comment

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

i don't like that solution either because of the same security reasons you mentioned.

To sum up my finding about java and ceritifcates:

  • Only a keystorefile can be used as a certificate source within java.
  • You can't load a single cert file into you application
  • Importing into default keystore file for unprivileged users is not possible by default

A solution that only affects the container and provides sufficient security is not possible as far as I can assume:

  • You can't run multiple CMD/ENTRYPOINT-commands within a dockerfile with different users
  • Importing certifcates with RUN-Commands is nonsense since this would be done while building the image (and not starting a container)

A possible solution would be:

  • create a new keystore file with all the necessary certs
  • make the file read only after creating and importing
  • merge it on the fly within the application with the system wide keystore (maybe with https://github.com/1and1/CompositeJKS)

But since this topic only affects containers and the approach above requires some changes to the application itself I don't know if it should be done like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mike-jumper:
I reviewed all the stuff once again and came to the conclusion that there's a easier and more overall solution.
It ended up in allow-jsk-usage

I would suggest closing this topic as well as the JIRA issue and creating new ones.
Please give me a short hint if this is fine for you.
I'll then do all the steps

Choose a reason for hiding this comment

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

@sirux88 did you create a new PR for that new allow-jsk-usage ? I'm interested by this feature but cant find anything related to the announcement you made here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the maintainers never gave some feedback on this I haven't created a PR.

Since contributing is, at least in my eyes, not that easy within in this project I'm not really interested in doing so.

If you want to feel free to copy my approach and put a PR on this yourself

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fair - sorry for being so unresponsive on this and other PRs.

Copy link

Choose a reason for hiding this comment

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

@sirux88 since there's an answer from @mike-jumper, are you going to keep on working on the PR ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry but no @daftaupe I'll close this PR and will not open a new one. Reasons are the same as above and without any offense to anyone mike's comment is not an answer to the question it is an excuse.

Your options are:

  1. You could use the startup.sh from my branch allow-jsk-usage and mount it into your container. Then set the appropriate environment variables and use your own keystore file.
  2. Copy my approach and open a new PR on yourself
  3. There's another PR that deals with loading custom keystore files GUACAMOLE-1746: Docker Allow usage of custom keystore and custom certificat #805. The PR's reason is different but the solution may fit for your needs aswell.

Copy link

Choose a reason for hiding this comment

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

thanks @sirux88 I'll think about what is the best option.

@sirux88
Copy link
Contributor Author

sirux88 commented Oct 1, 2023

For anyone stumbling over this and need the solution discussed above:

add the following to startup.sh right before start_guacamole. Then set the corresponding environment variables and use your custom keystore file

Details

# Set java truststore and truststore password if specified
if [ -n "$JAVA_TRUSTSTORE_FILE" ]; then
    
    if [ -z "$JAVA_TRUSTSTORE_PASSWORD" ]; then
        cat <<END
FATAL: Missing required environment variables
-----------------------------------------------------------------------------------
If using a java truststore file, you must provide each of the
following environment variables:

    JAVA_TRUSTSTORE_FILE        The truststore file to be used.

    JAVA_TRUSTSTORE_PASSWORD    The password for the specified truststore file
END
        exit 1;
    fi

    CATALINA_OPTS="$CATALINA_OPTS -Djavax.net.ssl.trustStore=$JAVA_TRUSTSTORE_FILE"
    CATALINA_OPTS="$CATALINA_OPTS -Djavax.net.ssl.trustStorePassword=$JAVA_TRUSTSTORE_PASSWORD"

    if [ -n "$JAVA_TRUSTSTORE_TYPE" ]; then
        CATALINA_OPTS="$CATALINA_OPTS -Djavax.net.ssl.trustStoreType=$JAVA_TRUSTSTORE_TYPE"
    fi

    export CATALINA_OPTS
fi

@sirux88 sirux88 closed this Oct 1, 2023
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