Skip to content

Conversation

@Satellite-QE
Copy link
Collaborator

Cherrypick of PR: #20552

Problem Statement

Template sync tests that use SSH for transport are failing on IPv6 while on IPv4 they pass:

    Could not export:
      git '--git-dir=/tmp/d20251201-23437-yapn4p/.git' '--work-tree=/tmp/d20251201-23437-yapn4p' '-c' 'core.quotePath=true' '-c' 'color.ui=false' 'fetch' '--' 'origin'  2>&1
      status: pid 33515 exit 128
      output: "Host key verification failed.\r\nfatal: Could not read from remote repository.\n\nPlease make sure you have the correct access rights\nand the repository exists.\n"

Further investigation showed that ssh-keyscan is used with wrong parameter -4:

[D 251218 12:12:36 hosts:198] <SAT_FQDN> executing command: ssh-keyscan -4 -t rsa -p 50126 infra-podman-ipv6.infra > /usr/share/foreman/.ssh/known_hosts
[D 251218 12:12:36 hosts:200] <SAT_FQDN> command result:
    stdout:
    
    stderr:
    getaddrinfo infra-podman-ipv6.infra: Name or service not known
    
    status: 1

Solution

Sanitize ssh-keyscan command:

  • -4 - don't always use ipv4, especially on ipv6
  • sudo -u foreman ... - .ssh/known_hosts file should be owned by user foreman not root
  • >> {key_path}/known_hosts - .ssh/known_hosts file may contain multiple entries, append new entry

Related Issues

SAT-41275

@Satellite-QE Satellite-QE added 6.16.z Auto_Cherry_Picked Automatically cherrypicked PR using GHA No-CherryPick PR doesnt need CherryPick to previous branches labels Jan 8, 2026
@Satellite-QE
Copy link
Collaborator Author

trigger: test-robottelo
pytest: >-
  -v
  tests/foreman/cli/test_templatesync.py::TestTemplateSyncTestCase::test_positive_update_templates_in_git[non_empty_repo-ssh]
  tests/foreman/cli/test_templatesync.py::TestTemplateSyncTestCase::test_positive_export_filtered_templates_to_git[non_empty_repo-ssh]
  tests/foreman/cli/test_templatesync.py::TestTemplateSyncTestCase::test_positive_export_filtered_templates_to_git[empty_repo-ssh]
  tests/foreman/ui/test_templatesync.py::test_positive_export_filtered_templates_to_git[non_empty_repo-ssh] 
  tests/foreman/ui/test_templatesync.py::test_positive_export_filtered_templates_to_git[empty_repo-ssh] 
  tests/foreman/api/test_templatesync.py::TestTemplateSyncTestCase::test_positive_export_all_templates_to_repo[auth_http_proxy-do_not_use_proxy-non_empty_repo-ssh]
  tests/foreman/api/test_templatesync.py::TestTemplateSyncTestCase::test_positive_export_all_templates_to_repo[auth_http_proxy-do_not_use_proxy-empty_repo-ssh]
network_type: ipv6

@Satellite-QE Satellite-QE added the AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing label Jan 8, 2026
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've found 1 security issue, and left some high level feedback:

Security issues:

  • Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option. (link)

General comments:

  • The new sudo -u foreman sh -c '...' command builds a single-quoted shell string inside a Python f-string; if git.hostname or key_path ever contain characters like single quotes or shell metacharacters this will break or become unsafe—consider passing arguments without extra shell interpolation (e.g., using sudo -u foreman sh -c with carefully escaped parameters or a helper that avoids manual quoting).
  • This change introduces a hard dependency on sudo and the foreman user being present on the target system; if this fixture is used in different environments, it may be safer to assert or detect their availability before running the command and fail fast with a clear error when they are missing.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The new `sudo -u foreman sh -c '...'` command builds a single-quoted shell string inside a Python f-string; if `git.hostname` or `key_path` ever contain characters like single quotes or shell metacharacters this will break or become unsafe—consider passing arguments without extra shell interpolation (e.g., using `sudo -u foreman sh -c` with carefully escaped parameters or a helper that avoids manual quoting).
- This change introduces a hard dependency on `sudo` and the `foreman` user being present on the target system; if this fixture is used in different environments, it may be safer to assert or detect their availability before running the command and fail fast with a clear error when they are missing.

## Individual Comments

### Comment 1
<location> `pytest_fixtures/component/templatesync.py:68-70` </location>
<code_context>
    session_target_sat.execute(
        f"sudo -u foreman sh -c 'ssh-keyscan -t rsa -p {git.ssh_port} {git.hostname} >> {key_path}/known_hosts'"
    )
</code_context>

<issue_to_address>
**security (python.sqlalchemy.security.sqlalchemy-execute-raw-query):** Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.

*Source: opengrep*
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@Gauravtalreja1
Copy link
Member

trigger: test-robottelo
pytest: >-
  -v
  tests/foreman/cli/test_templatesync.py::TestTemplateSyncTestCase::test_positive_update_templates_in_git[non_empty_repo-ssh]
  tests/foreman/cli/test_templatesync.py::TestTemplateSyncTestCase::test_positive_export_filtered_templates_to_git[non_empty_repo-ssh]
  tests/foreman/cli/test_templatesync.py::TestTemplateSyncTestCase::test_positive_export_filtered_templates_to_git[empty_repo-ssh]
  tests/foreman/ui/test_templatesync.py::test_positive_export_filtered_templates_to_git[non_empty_repo-ssh] 
  tests/foreman/ui/test_templatesync.py::test_positive_export_filtered_templates_to_git[empty_repo-ssh] 
  tests/foreman/api/test_templatesync.py::TestTemplateSyncTestCase::test_positive_export_all_templates_to_repo[auth_http_proxy-do_not_use_proxy-non_empty_repo-ssh]
  tests/foreman/api/test_templatesync.py::TestTemplateSyncTestCase::test_positive_export_all_templates_to_repo[auth_http_proxy-do_not_use_proxy-empty_repo-ssh]

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

Labels

6.16.z Auto_Cherry_Picked Automatically cherrypicked PR using GHA AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing No-CherryPick PR doesnt need CherryPick to previous branches

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants