You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
Motivation and Context
Refactoring config keys in the chart to enable secure connection Fix#2293
Configuration of Secure Communication
Selenium Grid supports secure communication between components. Refer to the instructions and options are able to configure the secure communication. Below is the details on how to enable secure communication in Selenium Grid chart.
In the chart, there is directory certs contains 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. You can generate your own self-signed certificate put them in that default directory by using script certs/cert.sh with adjust needed information. The certificate, private key, truststore are mounted to the components via Secret.
# Generate self-signed to target directory
./certs/cert.sh -d /path/to/your/
# Add current host IP to the certificate
ADD_IP_ADDRESS=hostname ./certs/cert.sh -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/cert.sh -d /path/to/your/
# Other environment variables that script consumes# CERTNAME, STOREPASS, KEYPASS, ALIAS, SERVER_KEYSTORE, BASE64_ONLY
Create TLS Secret
There are multiple ways to insert your certificate, private key, truststore to the components. You can choose one of following ways:
Replace your certificate, private key, truststore to the default directory certs in chart with the same name before deploying the chart.
Use Helm CLI to pass your certificate, private key, truststore via --set-file when deploying the chart. For example (replace $RELEASENAME and $NAMESPACE with your values):
Create your own TLS Secret with your certificate, private key, truststore and pass the Secret name via tls.nameOverride when deploying the chart. For example (replace $RELEASENAME and $NAMESPACE with your values):
# Steps to prepare your self-signed certificate
./certs/cert.sh -d /path/to/your/
# Create TLS Secret with your certificate, private key, truststore
kubectl create secret generic -n $NAMESPACE my-external-tls-secret \
--from-file=tls.crt=/path/to/your/tls.crt \
--from-file=tls.key=/path/to/your/tls.key \
--from-file=server.jks=/path/to/your/server.jks
# Deploy chart with your external TLS Secret
helm upgrade -i $RELEASENAME -n $NAMESPACE docker-selenium/selenium-grid \
--set tls.enabled=true --set tls.nameOverride=my-external-tls-secret
In case your external secret contains key file names are different with default, you can instruct server to use them via following values:
When enabling secure communication between Selenium Grid server components, you need to set the following values:
tls:
enabled: true
In additional, if the ingress is enabled, and approach SSL Passthrough is used to ensure the request forwards to the backend components via an encrypted connection.
With ingress.hostname is set, the default server TLS secret is also used for hosts TLS secretName when ingress.tls is empty. Once you specify ingress.tls, your specified secret will be used for hosts TLS secretName.
Moreover, when sub-chart ingress-nginx is enabled (deploy Ingress NGINX Controller together), the default server TLS secret can also be assigned via ingress-nginx.controller.extraArgs.default-ssl-certificate.
For example (replace $RELEASENAME and $NAMESPACE with your values):
Below is an example of Grid UI accessible via NodePort with secure connection, and using external TLS Secret (replace $RELEASENAME and $NAMESPACE with your values):
Grid UI can be accessed via HTTPS address https://your.host.public.ip:30444.
Secure Connection to the Ingress proxy
When enabling secure communication via HTTPS/TLS between the client and the Ingress proxy only (SSL Offloading / aka SSL Termination). The proxy will terminate the TLS connection, decrypt incoming HTTPS traffic and send it to the backend components without encryption. The backend Selenium Grid components doesn't need to understand HTTPS. To enable this mode, you need to set the following values:
tls:
ingress:
enabled: true
In additional, a self-signed certificate and private key can be generated runtime during the chart deployment for Ingress TLS by setting these values (replace $RELEASENAME with your value):
tls:
ingress:
generateTLS: truedefaultName: "MySelfSignedCert"defaultDays: 3650defaultCN: "www.domain.com"# Common NamedefaultSANList:
- selenium-grid.prod.domain.com # Subject Alternative Name
- selenium-grid.staging.domain.comdefaultIPList:
- 10.87.99.100 # Public IP of the host running K8s or LoadBalancer IP
- 10.87.100.101ingress-ngnix:
enabled: truecontroller:
extraArgs:
default-ssl-certificate: $(POD_NAMESPACE)/$RELEASENAME-selenium-tls-secret
You can get the tls.crt and tls.key from the Secret after the chart is deployed. For example (replace $RELEASENAME and $NAMESPACE with your values):
Below is an example of Grid UI accessible via secure connection to the Ingress proxy with self-signed certificate in external TLS Secret (replace $RELEASENAME and $NAMESPACE with your values):
Grid UI can be accessed via HTTPS address https://selenium-grid.prod.domain.com.
Node Registration
To enable secure in the node registration to make sure that the node is one you control and not a rouge node, you can enable and provide a registration secret string to Distributor, Router and
Node servers in config registrationSecret. For example:
No specific security vulnerabilities are introduced in the PR code itself, but the script modifications in 'cert.sh' could potentially be exploited if not handled carefully, especially regarding file operations and external command executions.
⚡ Key issues to review
Script Robustness The script lacks robust error handling for file operations and external command failures (e.g., keytool, openssl). Consider adding checks after each critical operation to ensure the script exits with an appropriate error message and status if something goes wrong.
Security Concern The script uses predictable names for temporary files which can lead to race conditions or security vulnerabilities if multiple instances of the script run concurrently. Consider using mktemp or similar to create secure, unique temporary file names.
Code Duplication The logic to determine if the server connection should be secure is duplicated in multiple template definitions. Consider refactoring this into a single reusable template function or variable.
Handle potential unset configuration for hostname verification explicitly
To avoid potential security issues and ensure that the configuration is explicit, consider handling the case where $.Values.tls.disableHostnameVerification might not be set, by providing a default value.
Why: Providing a default value for $.Values.tls.disableHostnameVerification ensures that the configuration is explicit, avoiding potential security issues.
10
Enhance security by not hardcoding sensitive information like passwords
Instead of hardcoding the trustStore password, consider using a more secure approach by fetching it from a secret management service or environment variable.
Why: Fetching the trustStore password from a values file instead of hardcoding it enhances security by preventing sensitive information from being exposed in the codebase.
10
Best practice
Add error handling to exit the script on any command failure
To ensure that the script handles errors properly and exits when a command fails, consider adding set -e at the beginning of the script. This command causes the script to exit immediately if a command exits with a non-zero status.
Why: Adding set -e at the beginning of the script is a best practice for bash scripts to ensure that the script exits immediately if any command fails, preventing potential issues from cascading errors.
9
Add robustness to protocol determination logic
It's a good practice to ensure that the protocol switching logic is robust against potential configuration errors. Adding a default case or validation can prevent runtime issues if the configuration is not set correctly.
Why: Adding a default case or validation in the protocol determination logic ensures robustness against potential configuration errors, preventing runtime issues.
9
Use variable configuration instead of hardcoded values for conditions
Replace the hardcoded "true" string with a reference to the .Values.tls.enabled to maintain flexibility and consistency with other configuration options.
-{{- if eq (include "seleniumGrid.server.secureConnection" $) "true" }}+{{- if .Values.tls.enabled }}
Apply this suggestion
Suggestion importance[1-10]: 9
Why: Replacing the hardcoded "true" with .Values.tls.enabled enhances flexibility and consistency with other configuration options, following best practices.
9
Ensure explicit default value handling in ternary operations
Consider adding a default value for the ternary operation to ensure that the value is explicitly set when $.Values.tls.enabled is false. This can prevent unintended behavior in configurations where TLS is not enabled.
Why: Adding an explicit default value in the ternary operation improves code robustness by preventing unintended behavior when $.Values.tls.enabled is false.
8
Use the configured volume mount path from the values file to ensure consistency
Ensure that the volume mount path for TLS certificates is fetched from the values file to maintain consistency across templates.
Why: Ensuring that the volume mount path for TLS certificates is fetched from the values file maintains consistency across templates and makes the configuration more flexible and easier to manage.
8
Possible bug
Improve the condition check for empty values to handle unexpected types
Use a more specific condition to check for empty values in the secret files to avoid potential issues with unexpected null or false values.
Why: Using a more specific condition to check for empty values avoids potential issues with unexpected null or false values, which can prevent bugs and improve the robustness of the code.
9
Possible issue
Validate that the directory path is not empty after parsing options
The script should validate that the DIRECTORY_PATH is not empty after parsing options, to ensure that a valid directory is provided for storing certificates. This can prevent potential issues where no directory is specified, leading to unexpected behavior.
case ${opt} in
d )
DIRECTORY_PATH=$OPTARG
+ if [ -z "$DIRECTORY_PATH" ]; then+ echo "Error: Directory path must not be empty."+ exit 1+ fi
;;
Apply this suggestion
Suggestion importance[1-10]: 8
Why: Validating that the DIRECTORY_PATH is not empty after parsing options ensures that a valid directory is provided, preventing potential issues where no directory is specified, leading to unexpected behavior.
8
Add error handling for the file deletion command to ensure it succeeds
It's recommended to check if the rm command successfully deletes the files before proceeding. This can be done by checking the exit status of the rm command. If the deletion fails, the script should handle this gracefully, possibly by logging an error and exiting.
Why: Checking if the rm command successfully deletes the files before proceeding ensures that the script handles any failure gracefully, preventing further operations on non-deleted files.
8
Conditionally include the secretName attribute to avoid configuration errors
Ensure that the secretName attribute is included within the conditional block to prevent potential issues when the condition for secure connection is not met.
-secretName: {{ include "seleniumGrid.tls.fullname" . | quote }}+{{- if eq (include "seleniumGrid.server.secureConnection" $) "true" }}+ secretName: {{ include "seleniumGrid.tls.fullname" . | quote }}+{{- end }}
Apply this suggestion
Suggestion importance[1-10]: 8
Why: This suggestion prevents potential issues by ensuring the secretName attribute is only included when the secure connection condition is met, improving reliability.
8
Handle potential multiple IP addresses returned by hostname -I
The script uses hostname -I which can potentially list multiple IP addresses. It's safer to explicitly handle the possibility of multiple IPs by either documenting the expected behavior or modifying the script to handle multiple IPs appropriately.
-ADD_IP_ADDRESS=",IP:$(hostname -I | awk '{print $1}')"+IP_ADDRESSES=$(hostname -I | awk '{print $1}')+if [[ $(echo $IP_ADDRESSES | wc -w) -gt 1 ]]; then+ echo "Error: Multiple IP addresses found. Please specify which one to use."+ exit 1+fi+ADD_IP_ADDRESS=",IP:$IP_ADDRESSES"
Apply this suggestion
Suggestion importance[1-10]: 7
Why: Handling the possibility of multiple IP addresses returned by hostname -I ensures that the script does not proceed with ambiguous IP addresses, which could lead to unexpected behavior.
7
Maintainability
Use a variable for the common port to enhance maintainability
Consider using a variable for the common port 443 used in multiple targets to ensure consistency and simplify future changes.
Why: Using a variable for the common port 443 improves maintainability by ensuring consistency and simplifying future changes. This is a good practice for managing configuration values that are used in multiple places.
8
Use helper variables for complex ternary operations to enhance readability
To improve the readability and maintainability of the code, consider using a helper variable to store the result of the complex ternary operation. This will make the code easier to understand and modify in the future.
Why: Using a helper variable for the complex ternary operation enhances readability and maintainability, making the code easier to understand and modify.
7
Ensure the readOnly attribute is conditionally applied
Ensure that the readOnly attribute is included within the conditional block to maintain consistency and avoid potential misconfigurations when the condition is not met.
-readOnly: true+{{- if eq (include "seleniumGrid.server.secureConnection" $) "true" }}+ readOnly: true+{{- end }}
Apply this suggestion
Suggestion importance[1-10]: 7
Why: This suggestion improves maintainability by ensuring the readOnly attribute is only applied when the secure connection condition is met, preventing potential misconfigurations.
7
Enhancement
Improve readability and maintainability by using descriptive variable names
Use a more descriptive variable name for the condition to enable secure connections, enhancing readability and maintainability.
-{{- if eq (include "seleniumGrid.server.secureConnection" $) "true" }}+{{- if .Values.enableSecureConnection }}
Apply this suggestion
Suggestion importance[1-10]: 6
Why: Using a more descriptive variable name enhances readability and maintainability, although it is a minor improvement compared to functional changes.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
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
Motivation and Context
Refactoring config keys in the chart to enable secure connection
Fix #2293
Configuration of Secure Communication
Selenium Grid supports secure communication between components. Refer to the instructions and options are able to configure the secure communication. Below is the details on how to enable secure communication in Selenium Grid chart.
In the chart, there is directory certs contains 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. You can generate your own self-signed certificate put them in that default directory by using script certs/cert.sh with adjust needed information. The certificate, private key, truststore are mounted to the components via
Secret.Usage of certs/cert.sh script:
Create TLS Secret
There are multiple ways to insert your certificate, private key, truststore to the components. You can choose one of following ways:
Replace your certificate, private key, truststore to the default directory certs in chart with the same name before deploying the chart.
Use Helm CLI to pass your certificate, private key, truststore via
--set-filewhen deploying the chart. For example (replace$RELEASENAMEand$NAMESPACEwith your values):Create your own TLS Secret with your certificate, private key, truststore and pass the Secret name via
tls.nameOverridewhen deploying the chart. For example (replace$RELEASENAMEand$NAMESPACEwith your values):In case your external secret contains key file names are different with default, you can instruct server to use them via following values:
Secure Connection to Selenium Grid components
When enabling secure communication between Selenium Grid server components, you need to set the following values:
In additional, if the ingress is enabled, and approach SSL Passthrough is used to ensure the request forwards to the backend components via an encrypted connection.
With
ingress.hostnameis set, the default server TLS secret is also used for hosts TLS secretName wheningress.tlsis empty. Once you specifyingress.tls, your specified secret will be used for hosts TLS secretName.Moreover, when sub-chart
ingress-nginxis enabled (deploy Ingress NGINX Controller together), the default server TLS secret can also be assigned viaingress-nginx.controller.extraArgs.default-ssl-certificate.For example (replace
$RELEASENAMEand$NAMESPACEwith your values):Below is an example of Grid UI accessible via NodePort with secure connection, and using external TLS Secret (replace
$RELEASENAMEand$NAMESPACEwith your values):Grid UI can be accessed via HTTPS address
https://your.host.public.ip:30444.Secure Connection to the Ingress proxy
When enabling secure communication via HTTPS/TLS between the client and the Ingress proxy only (SSL Offloading / aka SSL Termination). The proxy will terminate the TLS connection, decrypt incoming HTTPS traffic and send it to the backend components without encryption. The backend Selenium Grid components doesn't need to understand HTTPS. To enable this mode, you need to set the following values:
In additional, a self-signed certificate and private key can be generated runtime during the chart deployment for Ingress TLS by setting these values (replace
$RELEASENAMEwith your value):You can get the
tls.crtandtls.keyfrom the Secret after the chart is deployed. For example (replace$RELEASENAMEand$NAMESPACEwith your values):Below is an example of Grid UI accessible via secure connection to the Ingress proxy with self-signed certificate in external TLS Secret (replace
$RELEASENAMEand$NAMESPACEwith your values):Grid UI can be accessed via HTTPS address
https://selenium-grid.prod.domain.com.Node Registration
To enable secure in the node registration to make sure that the node is one you control and not a rouge node, you can enable and provide a registration secret string to Distributor, Router and
Node servers in config
registrationSecret. For example:Types of changes
Checklist
PR Type
enhancement, tests, documentation
Description
cert.shscript to support new options and improved file handling.Changes walkthrough 📝
8 files
cert.sh
Enhance certificate generation script with new options and cleanupcharts/selenium-grid/certs/cert.sh
_helpers.tpl
Add and update helper functions for secure connectionscharts/selenium-grid/templates/_helpers.tpl
Dockerfile
Add Netty version argument and update classpath fetchBase/Dockerfile
tls.crt
Add new self-signed certificate filecharts/selenium-grid/certs/tls.crt
tls.key
Add new private key filecharts/selenium-grid/certs/tls.key
distributor-deployment.yaml
Update distributor deployment for secure connectionscharts/selenium-grid/templates/distributor-deployment.yaml
event-bus-deployment.yaml
Update event bus deployment for secure connectionscharts/selenium-grid/templates/event-bus-deployment.yaml
hub-deployment.yaml
Update hub deployment for secure connectionscharts/selenium-grid/templates/hub-deployment.yaml
4 files
chart_cluster_setup.sh
Simplify Minikube startup command in test setuptests/charts/make/chart_cluster_setup.sh
chart_test.sh
Update test script for secure connection configurationstests/charts/make/chart_test.sh
bootstrap.sh
Update certificate file extensions in bootstrap scripttests/customCACert/bootstrap.sh
.pemto.crt.Makefile
Add and update test targets for secure connectionsMakefile
1 files
README.md
Update documentation for secure communication and TLS secretscharts/selenium-grid/README.md
2 files
selenium.pem
Remove old self-signed certificate filecharts/selenium-grid/certs/selenium.pem
selenium.pkcs8.base64
Remove old base64 encoded private key filecharts/selenium-grid/certs/selenium.pkcs8.base64