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-1746: Docker Allow usage of custom keystore and custom certificat #805

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ronansalmon
Copy link
Contributor

The start script handles custom keystore/certificat.

Copy link
Contributor

@necouchman necouchman left a comment

Choose a reason for hiding this comment

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

A few spelling corrections, but I'm also concerned about the nature of the instructions added to the README.md file.

The instructions seem to cover a scenario where you're running guacd natively on a system but you're running guacamole-client in a Docker container. This is a perfectly valid scenario; however, I'm not sure that the README for guacamole-client within Docker should include instructions that are specific to guacd - either Docker-based or natively installed. It might be better to document that in either the gaucamole-server repo or the Docker chapter in the User Guide (guacamole-manual), and just put a note, here, that refers people to one of those two places?

guacamole-docker/README.md Outdated Show resolved Hide resolved
guacamole-docker/README.md Outdated Show resolved Hide resolved
guacamole-docker/README.md Outdated Show resolved Hide resolved
Comment on lines +235 to +237
You need to create the new certificate on the guacd host, see https://github.com/apache/guacamole-server/blob/master/README
or https://github.com/apache/guacamole-server/blob/master/src/guacd-docker/README.md depending
on the version you will use (standalone vs docker).
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect we should document this somewhere in the Guacamole User Guide (Manual), and link to that instead of linking directly to the Github repository.

@necouchman
Copy link
Contributor

@ronansalmon Any chance we can wrap this one up?

@ronansalmon
Copy link
Contributor Author

Hey !
This PR is linked to apache/guacamole-server#419.
I was waiting for the PR 419 to be merged (if accepted) to go any further.

So, how should we proceed ?

@sirux88
Copy link
Contributor

sirux88 commented Oct 26, 2023

I would add some thoughts:
1) What is the environment variabale GUACD_SSL for? As far as I can see it's not used on guacamole-client nor on guacamole-server anywhere (except within the changed files). Seems obsolete to me
2) Maybe the environment variables GUACD_SSL_KEYSTORE_FILE and GUACD_SSL_KEYSTORE_PASS should be renamed to something fitting better to their true purpose like JAVA_KEYSTORE_FILE and JAVA_KEYSTORE_PASS

For reference: I had a similar approach here sirux88@e02d2be. My use case was using a self signed cert for LDAP

@mike-jumper
Copy link
Contributor

mike-jumper commented Oct 26, 2023

The GUACD_SSL environment variable controls the guacd-ssl property, which is used to enable SSL/TLS encryption between the webapp and guacd. It's uncommon to use this, as that communication is typically purely over loopback or at least on a trusted network route, but it's not obsolete.

@sirux88
Copy link
Contributor

sirux88 commented Oct 27, 2023

The GUACD_SSL environment variable controls the guacd-ssl property, which is used to enable SSL/TLS encryption between the webapp and guacd. It's uncommon to use this, as that communication is typically purely over loopback or at least on a trusted network route, but it's not obsolete.

I still can't find it. But if you're saying it's used I'm ok with this

@mike-jumper
Copy link
Contributor

mike-jumper commented Oct 27, 2023

The GUACD_SSL environment variable controls the guacd-ssl property, which is used to enable SSL/TLS encryption between the webapp and guacd. It's uncommon to use this, as that communication is typically purely over loopback or at least on a trusted network route, but it's not obsolete.

I still can't find it. But if you're saying it's used I'm ok with this

For reference, here's the code path:

Property Declaration (standardized as part of guacamole-ext)

/**
* Whether guacd requires SSL/TLS on connections.
*/
public static final BooleanGuacamoleProperty GUACD_SSL = new BooleanGuacamoleProperty() {
@Override
public String getName() { return "guacd-ssl"; }
};

Property Retrieval (via a GuacamoleProxyConfiguration from LocalEnvironment)

@Override
public GuacamoleProxyConfiguration getDefaultGuacamoleProxyConfiguration()
throws GuacamoleException {
// Parse guacd hostname/port/ssl properties
return new GuacamoleProxyConfiguration(
getProperty(Environment.GUACD_HOSTNAME, DEFAULT_GUACD_HOSTNAME),
getProperty(Environment.GUACD_PORT, DEFAULT_GUACD_PORT),
getProperty(Environment.GUACD_SSL, DEFAULT_GUACD_SSL)
);
}

(Voluntary) Usage of Retrieved Values

Implementations aren't strictly required to use getDefaultGuacamoleProxyConfiguration(), but most do, either explicitly or implicitly via SimpleConnection.

GuacamoleProxyConfiguration proxyConfig = environment.getDefaultGuacamoleProxyConfiguration();

GuacamoleProxyConfiguration defaultConfig = environment.getDefaultGuacamoleProxyConfiguration();

etc.

…YSTORE_PASS to JAVA_KEYSTORE_FILE and JAVA_KEYSTORE_PASS
@ronansalmon
Copy link
Contributor Author

ronansalmon commented Oct 30, 2023

I would add some thoughts: 1) What is the environment variabale GUACD_SSL for? As far as I can see it's not used on guacamole-client nor on guacamole-server anywhere (except within the changed files). Seems obsolete to me 2) Maybe the environment variables GUACD_SSL_KEYSTORE_FILE and GUACD_SSL_KEYSTORE_PASS should be renamed to something fitting better to their true purpose like JAVA_KEYSTORE_FILE and JAVA_KEYSTORE_PASS

@sirux88 Done

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.

4 participants