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

chart(add): Grid scaler use trigger auth to secure GraphQL endpoint #2401

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Sep 20, 2024

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

Set config key basicAuth.embeddedUrl is true will be used to put basic auth credentials in URL again

Motivation and Context

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.

PR Type

enhancement, tests


Description

  • Added basic authentication to various scripts and components to secure GraphQL endpoints.
  • Removed basic authentication credentials from URLs and implemented them as headers.
  • Enhanced error handling and logging for authentication-related issues.
  • Updated Helm templates to support new authentication and autoscaling configurations.
  • Added tests to verify the correct handling of authentication and URL construction.
  • Updated documentation to reflect new features and security recommendations.

Changes walkthrough 📝

Relevant files
Enhancement
12 files
check-grid.sh
Add basic authentication to grid status check                       

Base/check-grid.sh

  • Added basic authentication header to curl command.
  • Encoded username and password in base64 for authentication.
  • +2/-1     
    video.sh
    Enhance video recording with GraphQL authentication           

    Video/video.sh

  • Added handling for GraphQL endpoint authentication and errors.
  • Updated video recording logic with authentication checks.
  • +8/-1     
    video_graphQLQuery.sh
    Secure GraphQL queries with basic authentication                 

    Video/video_graphQLQuery.sh

  • Added basic authentication to GraphQL queries.
  • Returned additional endpoint status and URL information.
  • +6/-1     
    video_gridUrl.sh
    Update grid URL handling with authentication                         

    Video/video_gridUrl.sh

  • Removed basic authentication from grid URL construction.
  • Added basic authentication header to curl command.
  • +2/-5     
    distributorProbe.sh
    Secure distributor probe with authentication                         

    charts/selenium-grid/configs/distributor/distributorProbe.sh

    • Added basic authentication to GraphQL endpoint checks.
    +6/-5     
    nodeGridUrl.sh
    Refactor node grid URL handling                                                   

    charts/selenium-grid/configs/node/nodeGridUrl.sh

  • Removed basic authentication from grid URL.
  • Added basic authentication header to curl command.
  • +9/-13   
    nodePreStop.sh
    Enhance node pre-stop with authentication                               

    charts/selenium-grid/configs/node/nodePreStop.sh

  • Added basic authentication to node pre-stop signal.
  • Improved logging for authentication errors.
  • +13/-5   
    nodeProbe.sh
    Secure node probe with authentication                                       

    charts/selenium-grid/configs/node/nodeProbe.sh

  • Added basic authentication to node probe checks.
  • Improved error handling for authentication issues.
  • +15/-2   
    routerGraphQLUrl.sh
    Simplify router GraphQL URL handling                                         

    charts/selenium-grid/configs/router/routerGraphQLUrl.sh

    • Removed basic authentication from router GraphQL URL.
    +1/-4     
    routerProbe.sh
    Secure router probe with authentication                                   

    charts/selenium-grid/configs/router/routerProbe.sh

    • Added basic authentication to router probe checks.
    +2/-1     
    _helpers.tpl
    Enhance templates with authentication support                       

    charts/selenium-grid/templates/_helpers.tpl

  • Added template for basic authentication secrets.
  • Updated autoscaling authentication reference handling.
  • +14/-4   
    basic-auth-secret.yaml
    Add basic authentication secret template                                 

    charts/selenium-grid/templates/basic-auth-secret.yaml

    • Added new template for basic authentication secret.
    +21/-0   
    Tests
    1 files
    test.py
    Add tests for basic authentication and URL handling           

    tests/charts/templates/test.py

  • Added tests for basic authentication handling.
  • Verified GraphQL URL construction without basic auth in URL.
  • +44/-16 
    Documentation
    2 files
    CONFIGURATION.md
    Update configuration documentation for new features           

    charts/selenium-grid/CONFIGURATION.md

    • Documented new basic authentication and autoscaling options.
    +3/-3     
    README.md
    Update README with new scaler and security info                   

    charts/selenium-grid/README.md

  • Added section for new Selenium Grid Scaler implementation.
  • Updated security recommendations for Grid URL.
  • +8/-2     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 Security concerns

    Sensitive information exposure:
    The changes include modifications to how authentication credentials are handled and stored. While the implementation appears to use secrets and base64 encoding, a thorough review should be conducted to ensure that sensitive information like usernames and passwords are not inadvertently exposed in logs, error messages, or insecure configurations. Special attention should be paid to the new basic-auth-secret.yaml and trigger-auth.yaml files, as well as any changes to environment variables that might contain sensitive data.

    ⚡ Key issues to review

    Error Handling
    New error handling for GraphQL endpoint authentication and 404 errors has been added. This should be carefully reviewed to ensure it covers all possible error scenarios and provides appropriate feedback.

    Security Configuration
    A new template for basic authentication secret has been added. This should be thoroughly reviewed to ensure it's implemented securely and doesn't expose sensitive information.

    Authentication Configuration
    A new TriggerAuthentication resource for KEDA has been added. This should be carefully reviewed to ensure it's correctly configured and securely references the necessary secrets.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add a check for the existence of username and password values before creating the basic auth secret

    Consider adding a check for the existence of username and password values before
    creating the secret. This will prevent creating an empty secret if the values are
    not provided.

    charts/selenium-grid/templates/basic-auth-secret.yaml [17-20]

    -{{- if eq .Values.basicAuth.enabled true }}
    +{{- if and (eq .Values.basicAuth.enabled true) .Values.basicAuth.username .Values.basicAuth.password }}
       SE_ROUTER_USERNAME: {{ .Values.basicAuth.username | b64enc }}
       SE_ROUTER_PASSWORD: {{ .Values.basicAuth.password | b64enc }}
     {{- end }}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion is important as it prevents the creation of an empty secret, which could lead to configuration issues or security concerns if the username and password are not provided.

    9
    Security
    Use a more secure method for handling credentials

    Consider using a more secure method to store and retrieve credentials, such as using
    environment variables directly or a secrets management system, instead of encoding
    them in base64.

    charts/selenium-grid/configs/distributor/distributorProbe.sh [10]

    -BASIC_AUTH="$(echo -n "${SE_ROUTER_USERNAME}:${SE_ROUTER_PASSWORD}" | base64)"
    +BASIC_AUTH="${SE_ROUTER_USERNAME}:${SE_ROUTER_PASSWORD}"
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a security concern by recommending a more secure method for handling credentials, which is crucial for maintaining the integrity and confidentiality of sensitive information.

    8
    Enhancement
    Add a condition to check if basic authentication is enabled before creating the TriggerAuthentication resource

    Consider adding a condition to check if basic authentication is enabled before
    creating the TriggerAuthentication resource. This will prevent potential errors if
    basic auth is disabled but KEDA is enabled.

    charts/selenium-grid/templates/trigger-auth.yaml [1]

    -{{- if and (eq (include "seleniumGrid.useKEDA" $) "true") (not $.Values.autoscaling.authenticationRef.name) }}
    +{{- if and (eq (include "seleniumGrid.useKEDA" $) "true") (not $.Values.autoscaling.authenticationRef.name) (eq $.Values.basicAuth.enabled true) }}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion is valid as it prevents potential errors by ensuring that the TriggerAuthentication resource is only created when basic authentication is enabled, which is a logical enhancement to the existing condition.

    8
    Add conditions to check if the URLs are defined before adding them to the secret

    Consider adding a condition to check if the URLs are defined before adding them to
    the secret. This will prevent adding empty values to the secret if the URLs are not
    set.

    charts/selenium-grid/templates/secrets.yaml [24-25]

    +{{- if (include "seleniumGrid.url" $) }}
     SE_NODE_GRID_URL: {{ include "seleniumGrid.url" $ | b64enc }}
    +{{- end }}
    +{{- if (include "seleniumGrid.graphqlURL" $) }}
     SE_NODE_GRID_GRAPHQL_URL: {{ include "seleniumGrid.graphqlURL" $ | b64enc }}
    +{{- end }}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion is beneficial as it ensures that only defined URLs are added to the secret, preventing the inclusion of empty values which could lead to misconfigurations.

    8
    Improve error handling and logging

    Consider using a more robust method for error handling and logging, such as creating
    separate functions for different error scenarios and using a logging framework.

    charts/selenium-grid/configs/node/nodePreStop.sh [38-42]

    +handle_error() {
    +  local error_code=$1
    +  local error_message=$2
    +  echo "$(date +%FT%T%Z) [${probe_name}] - Error ${error_code}: ${error_message}"
    +}
    +
     if [ "${grid_check}" = "401" ]; then
    -  echo "$(date +%FT%T%Z) [${probe_name}] - Hub/Router requires authentication. Please check SE_ROUTER_USERNAME and SE_ROUTER_PASSWORD."
    +  handle_error 401 "Hub/Router requires authentication. Please check SE_ROUTER_USERNAME and SE_ROUTER_PASSWORD."
     elif [ "${grid_check}" = "404" ]; then
    -  echo "$(date +%FT%T%Z) [${probe_name}] - Hub/Router endpoint could not be found. Please check the endpoint ${grid_url}"
    +  handle_error 404 "Hub/Router endpoint could not be found. Please check the endpoint ${grid_url}"
     fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion enhances code maintainability and readability by proposing a structured approach to error handling and logging, which is beneficial but not critical.

    7
    Improve GraphQL response handling and parsing

    Consider using a more robust method for handling and parsing the GraphQL response,
    such as using a JSON parsing tool like jq for better error handling and data
    extraction.

    Video/video_graphQLQuery.sh [30-34]

    -endpoint_checks=$(curl --noproxy "*" -m ${max_time} -k -X POST \
    +response=$(curl --noproxy "*" -m ${max_time} -k -X POST \
       -H "Content-Type: application/json" \
       -H "Authorization: Basic ${BASIC_AUTH}" \
       --data '{"query":"{ session (id: \"'${SESSION_ID}'\") { id, capabilities, startTime, uri, nodeId, nodeUri, sessionDurationMillis, slot { id, stereotype, lastStarted } } } "}' \
    -  -s "${GRAPHQL_ENDPOINT}" -o "/tmp/graphQL_${SESSION_ID}.json" -w "%{http_code}")
    +  -s "${GRAPHQL_ENDPOINT}")
     
    +endpoint_checks=$(echo "${response}" | jq -r '.errors | length')
    +if [ "${endpoint_checks}" -eq 0 ]; then
    +  echo "${response}" > "/tmp/graphQL_${SESSION_ID}.json"
    +  endpoint_checks=200
    +else
    +  endpoint_checks=500
    +fi
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves the robustness of the code by using a JSON parsing tool for better error handling and data extraction, enhancing reliability but not addressing a critical issue.

    7
    Best practice
    Improve handling of environment variables

    Consider using a more robust method for handling environment variables, such as
    using default values or checking if they are set before using them.

    Video/video.sh [205-209]

     return_list=($(bash ${VIDEO_CONFIG_DIRECTORY}/video_graphQLQuery.sh "$session_id"))
    -caps_se_video_record="${return_list[0]}"
    -video_file_name="${return_list[1]}.mp4"
    -endpoint_status="${return_list[2]}"
    -endpoint_url="${return_list[3]}"
    +caps_se_video_record="${return_list[0]:-false}"
    +video_file_name="${return_list[1]:-unknown}.mp4"
    +endpoint_status="${return_list[2]:-unknown}"
    +endpoint_url="${return_list[3]:-unknown}"
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion improves code robustness by ensuring default values are used for environment variables, which is a good practice but not essential for functionality.

    6

    💡 Need additional feedback ? start a PR chat

    @VietND96
    Copy link
    Member Author

    @Doofus100500, if removing basic auth putting in URL when constructing env var SE_NODE_GRID_URL. Do you think it might cause any issue for Playwright tests rely on CDP Websocket URL in node capabilities?

    @VietND96
    Copy link
    Member Author

    To balance any requirement, config key basicAuth.embeddedUrl will be used to embed basic auth credentials to SE_NODE_GRID_URL if needed

    @Doofus100500
    Copy link
    Contributor

    @VietND96 Removed basic auth from the URL in the SE_NODE_GRID_URL variable in the cluster with basic authentication, the Playwright test passed successfully

    @VietND96
    Copy link
    Member Author

    Thank you for your confirmation. I think we are good to go with this change

    @VietND96 VietND96 merged commit 6c4f76e into SeleniumHQ:trunk Sep 20, 2024
    26 of 33 checks passed
    @VietND96 VietND96 added this to the 4.25.0 milestone Sep 22, 2024
    @farioas
    Copy link

    farioas commented Sep 23, 2024

    Looks like it's breaking change in helm 0.36.0.

    After upgrade, nodes are not scaled, while in KEDA we have this exception:

    2024-09-23T10:46:26Z	ERROR	scale_handler	Error getting scaler metrics and activity, but continue	{"scaledJob.Name": "selenium-grid-selenium-firefox-node", "Scaler": "*scalers.seleniumGridScaler:", "error": "error requesting selenium grid endpoint: selenium grid returned 401"}
    github.com/kedacore/keda/v2/pkg/scaling.(*scaleHandler).getScaledJobMetrics
    	/workspace/pkg/scaling/scale_handler.go:853
    github.com/kedacore/keda/v2/pkg/scaling.(*scaleHandler).isScaledJobActive
    	/workspace/pkg/scaling/scale_handler.go:897
    github.com/kedacore/keda/v2/pkg/scaling.(*scaleHandler).checkScalers
    	/workspace/pkg/scaling/scale_handler.go:262
    github.com/kedacore/keda/v2/pkg/scaling.(*scaleHandler).startScaleLoop
    	/workspace/pkg/scaling/scale_handler.go:182
    

    This is happened because existing implementation of selenium grid scaler in KEDA 2.15.1 expects to receive:

    graphql-url: base64 encoded value of GraphQL URL
    

    Fyi, this change wasn't merged kedacore/keda#6169

    @VietND96
    Copy link
    Member Author

    VietND96 commented Sep 23, 2024

    @farioas, if there is no dependency, can you replace registry and tag to use KEDA components in our build https://github.com/SeleniumHQ/docker-selenium/blob/trunk/.keda/README.md

    @VietND96
    Copy link
    Member Author

    @farioas, can you also try to enable basicAuth.embeddedUrl is true, basic auth will be embedded in GraphQL endpoint as the old format.

    @VietND96
    Copy link
    Member Author

    Ok, looks like basicAuth.embeddedUrl not worked for GraphQL endpoint. I will give a fix in 0.36.1 soon

    @farioas
    Copy link

    farioas commented Sep 23, 2024

    It was 6hrs ago, so I tried to play with embeddedUrl, but it didn't work. So I rolled back to 0.35.2 and pinned image to 4.25.0-20240922.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [🚀 Feature]: Selenium grid Authentication password is stored as plain text in Helm chat values.yaml file
    3 participants