add: secure connection for docker compose

[VietND96]

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

add: secure connection for docker compose

Motivation and Context

Simplify to enable secure connection for Selenium Grid server when deploying via docker compose

Secure connection

By default, there are default self-signed certificates available in the image in location /opt/selenium/secrets includes

• server.jks: truststore file to configure for JVM via system property javax.net.ssl.trustStore when start the server.
• server.pass: file contains the truststore password for JVM via system property javax.net.ssl.trustStorePassword.
• tls.crt: Server certificate for https connection is set to Selenium option --https-certificate.
• tls.key: Server private key (in PKCS8 format) for https connection is set to Selenium option --https-private-key.

There are environment variables to configure the secure connection:

Environment variables	Default	Option of	Description
SE_ENABLE_TLS	false		Enable secure connection with default configs
SE_JAVA_SSL_TRUST_STORE	/opt/selenium/secrets/server.jks	JVM
SE_JAVA_SSL_TRUST_STORE_PASSWORD	/opt/selenium/secrets/server.pass	JVM
SE_JAVA_DISABLE_HOSTNAME_VERIFICATION	true	JVM	Disable host checks for components internally
SE_HTTPS_CERTIFICATE	/opt/selenium/secrets/tls.crt	Selenium	Set to CLI option --https-certificate
SE_HTTPS_PRIVATE_KEY	/opt/selenium/secrets/tls.key	Selenium	Set to CLI option --https-private-key

Via volume mount, you can replace the default certificates with your own certificates.

The self-signed certificate also needs to be trusted by the client (add to system widely bundle trusted CA) to avoid error message relates to SSL handshake when creating RemoteWebDriver.

Refer to sample: docker-compose-v3-full-grid-secure.yml

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[qodo-merge-pro]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 No relevant tests

🔒 Security concerns Sensitive information exposure: The PR introduces potential security risks by including sensitive data such as trust store passwords in configuration files (charts/selenium-grid/templates/server-configmap.yaml and charts/selenium-grid/values.yaml). It is recommended to use Kubernetes secrets or other secure storage mechanisms to handle such sensitive information.

⚡ Key issues to review Configuration Error The environment variable SE_JAVA_SSL_TRUST_STORE_PASSWORD should not expose the password directly for security reasons. It's recommended to use secrets management features provided by Kubernetes to handle sensitive data securely. Configuration Error The trustStorePassword should not be set directly in the values.yaml file for security reasons. It is recommended to use more secure methods to handle sensitive data.

[qodo-merge-pro]

PR Code Suggestions ✨

Category	Suggestion	Score
Security	Ensure secure handling and storage of private keys Ensure that the private key is stored and handled securely, avoiding exposure in the repository. Consider using a secure vault or environment variables to reference keys, and ensure they are not hardcoded in scripts or configuration files. charts/selenium-grid/certs/tls.key [1-40] ------BEGIN PRIVATE KEY----- -MIIG/QIBADANBgkqhkiG9w0BAQEFAASCBucwggbjAgEAAoIBgQDFoKw132Ojd2Zt -... ------END PRIVATE KEY----- +# Load private key from a secure location +PRIVATE_KEY=$(< /secure/location/private.key) Suggestion importance[1-10]: 10 Why: This suggestion addresses a critical security concern by recommending the use of a secure vault or environment variables to handle private keys, thus preventing potential exposure in the repository.	10
	Enhance security by using secure temporary file handling Use more secure temporary file handling mechanisms to avoid potential race conditions and security vulnerabilities with predictable file names. charts/selenium-grid/certs/add-cert-helper.sh [60] -APPEND_CRT_KEY="/tmp/tls.crt" +APPEND_CRT_KEY=$(mktemp -p /tmp tls.XXXXXX.crt) Apply this suggestion Suggestion importance[1-10]: 9 Why: Using mktemp for temporary file handling significantly enhances security by avoiding potential race conditions and security vulnerabilities associated with predictable file names.	9
	Remove or conditionally apply the disabling of hostname verification to enhance security It is a security risk to disable hostname verification as it makes the SSL/TLS connection vulnerable to man-in-the-middle attacks. Consider removing this option or setting it conditionally based on a secure configuration flag. Hub/start-selenium-grid-hub.sh [54-55] -echo "Appending Java options: -Djdk.internal.httpclient.disableHostnameVerification=${SE_JAVA_DISABLE_HOSTNAME_VERIFICATION}" -SE_JAVA_OPTS="$SE_JAVA_OPTS -Djdk.internal.httpclient.disableHostnameVerification=${SE_JAVA_DISABLE_HOSTNAME_VERIFICATION}" +# Consider removing or conditionally setting the following line if security is a concern +# echo "Appending Java options: -Djdk.internal.httpclient.disableHostnameVerification=${SE_JAVA_DISABLE_HOSTNAME_VERIFICATION}" +# SE_JAVA_OPTS="$SE_JAVA_OPTS -Djdk.internal.httpclient.disableHostnameVerification=${SE_JAVA_DISABLE_HOSTNAME_VERIFICATION}" Apply this suggestion Suggestion importance[1-10]: 9 Why: Disabling hostname verification can expose the system to man-in-the-middle attacks. This suggestion addresses a significant security concern by recommending the removal or conditional application of this setting.	9
	Add a security warning about using self-signed certificates in production Update the documentation to include a warning about the security risks of using self-signed certificates in production environments and suggest the use of certificates from trusted Certificate Authorities for such scenarios. charts/selenium-grid/README.md [631] -In the chart, there is directory [certs](./certs) contains utility scripts, the default self-signed certificate, private key (as PKCS8 format), and Java Keystore (JKS) to teach Java about secure connection (since we are using a non-standard CA) for your trial, local testing purpose. +In the chart, there is directory [certs](./certs) contains utility scripts, the default self-signed certificate, private key (as PKCS8 format), and Java Keystore (JKS) to teach Java about secure connection (since we are using a non-standard CA) for your trial, local testing purpose. **Note**: Using self-signed certificates in production can expose you to security risks. It is recommended to use certificates from trusted Certificate Authorities in production environments. Apply this suggestion Suggestion importance[1-10]: 9 Why: This suggestion enhances security awareness by warning users about the risks of using self-signed certificates in production and recommending the use of trusted Certificate Authorities.	9
Possible issue	Improve the certificate validation process Replace the use of openssl verify with a more specific check for certificate validity to avoid potential issues with certificate chain verification. charts/selenium-grid/certs/add-cert-helper.sh [37] -openssl verify -CAfile $bundle_file $cert_file +openssl verify -CAfile $bundle_file -purpose sslclient $cert_file Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion to use -purpose sslclient with openssl verify is a good improvement as it makes the certificate validation more specific and avoids potential issues with certificate chain verification.	8
	Add error handling after the certificate generation command to ensure robust script execution The script should handle potential failures in the certificate generation process. Consider adding error handling after the certificate generation command to ensure the script does not proceed if there is an error. tests/charts/make/chart_test.sh [225] ADD_IP_ADDRESS=hostname ./${CHART_PATH}/certs/gen-cert-helper.sh -d ${cert_dir} +if [ $? -ne 0 ]; then + echo "Certificate generation failed" + exit 1 +fi Apply this suggestion Suggestion importance[1-10]: 7 Why: Adding error handling after the certificate generation command ensures that the script does not proceed if there is an error, which enhances the reliability of the script.	7
	Verify the new default path for ROUTER_CONFIG_DIRECTORY to ensure it is correct and accessible The default value for ROUTER_CONFIG_DIRECTORY has been changed from $SE_OPT_BIN to /opt/bin. Ensure this path is correct and accessible in your deployment environment to avoid runtime issues. charts/selenium-grid/configs/distributor/distributorProbe.sh [7] +# Ensure the directory path is correct and accessible ROUTER_CONFIG_DIRECTORY=${ROUTER_CONFIG_DIRECTORY:-"/opt/bin"} Apply this suggestion Suggestion importance[1-10]: 6 Why: Ensuring the new default path for ROUTER_CONFIG_DIRECTORY is correct and accessible helps prevent potential runtime issues, although it is a minor change.	6
Best practice	Add error handling for directory creation to improve script robustness Ensure consistent error handling by adding an error check after the sudo mkdir -p command to handle potential failures gracefully. charts/selenium-grid/certs/add-cert-helper.sh [63] -sudo mkdir -p ${TARGET_CERT_DIR} +sudo mkdir -p ${TARGET_CERT_DIR} || { echo "Failed to create directory ${TARGET_CERT_DIR}"; exit 1; } Apply this suggestion Suggestion importance[1-10]: 8 Why: Adding an error check after the sudo mkdir -p command ensures that potential failures are handled gracefully, improving the robustness of the script.	8
	Prevent word splitting and globbing issues in the script Add double quotes around variable expansions to prevent word splitting and globbing issues, which can lead to script failures or unexpected behavior. charts/selenium-grid/certs/add-cert-helper.sh [24] -shift $((OPTIND-1)) +shift "$((OPTIND-1))" Apply this suggestion Suggestion importance[1-10]: 7 Why: Adding double quotes around variable expansions is a best practice to prevent word splitting and globbing issues, which can lead to script failures or unexpected behavior.	7
Possible bug	Add a check for readability of the truststore password file to prevent runtime errors Ensure that the file path in SE_JAVA_SSL_TRUST_STORE_PASSWORD is correctly checked before reading the password. The current check might lead to unexpected behavior if the variable is a directory or inaccessible. Hub/start-selenium-grid-hub.sh [46-48] -if [ -f "${SE_JAVA_SSL_TRUST_STORE_PASSWORD}" ]; then +if [ -f "${SE_JAVA_SSL_TRUST_STORE_PASSWORD}" ] && [ -r "${SE_JAVA_SSL_TRUST_STORE_PASSWORD}" ]; then echo "Getting Truststore password from ${SE_JAVA_SSL_TRUST_STORE_PASSWORD} to set Java options: -Djavax.net.ssl.trustStorePassword" SE_JAVA_SSL_TRUST_STORE_PASSWORD="$(cat ${SE_JAVA_SSL_TRUST_STORE_PASSWORD})" +else + echo "Error: Truststore password file not found or not readable" fi Apply this suggestion Suggestion importance[1-10]: 8 Why: Adding a readability check for the truststore password file ensures that the script does not fail unexpectedly if the file is not accessible, improving robustness.	8
Maintainability	Replace hardcoded directory path with a configurable environment variable Replace the hardcoded directory path with a more flexible environment variable approach or a configuration file parameter. This change enhances the script's flexibility and maintainability, allowing for easier adjustments in different environments without modifying the script directly. charts/selenium-grid/configs/node/nodePreStop.sh [5] -NODE_CONFIG_DIRECTORY=${NODE_CONFIG_DIRECTORY:-"/opt/bin"} +NODE_CONFIG_DIRECTORY=${NODE_CONFIG_DIRECTORY:-$DEFAULT_NODE_CONFIG_DIRECTORY} Apply this suggestion Suggestion importance[1-10]: 8 Why: This suggestion improves maintainability by allowing the directory path to be configured via an environment variable, making the script more flexible and adaptable to different environments.	8
Documentation	Improve clarity and usability of script usage instructions in the documentation Clarify the usage instructions for the gen-cert-helper.sh script by providing explicit examples of how to set environment variables and their effects on the script's behavior, improving the documentation's clarity and usability. charts/selenium-grid/README.md [635-645] Usage of [certs/gen-cert-helper.sh](./certs/gen-cert-helper.sh) script: ```bash -# Generate self-signed to target directory (by default output in same directory with script) +# Generate self-signed certificate to target directory (default output is the same directory as the script) ./certs/gen-cert-helper.sh -d /path/to/your/ -# Add current host IP to the certificate -ADD_IP_ADDRESS=hostname ./certs/gen-cert-helper -d /path/to/your/ -# Add multiple IP addresses to the certificate (comma-separated) -ADD_IP_ADDRESS=",IP:10.10.10.10,IP:10.10.11.11" ./certs/gen-cert-helper.sh -d /path/to/your/ -# Other environment variables that script consumes -# CERTNAME, STOREPASS, KEYPASS, ALIAS, SERVER_KEYSTORE, BASE64_ONLY +# Example: Add current host IP to the certificate +ADD_IP_ADDRESS=$(hostname) ./certs/gen-cert-helper.sh -d /path/to/your/ +# Example: Add multiple IP addresses to the certificate (comma-separated) +ADD_IP_ADDRESS="IP:10.10.10.10,IP:10.10.11.11" ./certs/gen-cert-helper.sh -d /path/to/your/ +# Explanation of environment variables: +# CERTNAME - Name of the certificate +# STOREPASS - Password for the store +# KEYPASS - Password for the key +# ALIAS - Alias for the key in the store +# SERVER_KEYSTORE - Path to the server keystore +# BASE64_ONLY - Set to true to output only base64 encoded certificate - [ ] **Apply this suggestion** <!-- /improve --apply_suggestion=11 --> <details><summary>Suggestion importance[1-10]: 7</summary> Why: This suggestion improves the documentation by providing clearer examples and explanations of environment variables, making it easier for users to understand and use the script effectively. </details></details></td><td align=center>7 </td></tr></tr></tbody></table>