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

fix(RemoteCmdRunner): fix scp destination string #10585

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dimakr
Copy link
Contributor

@dimakr dimakr commented Apr 3, 2025

Scp command behaviour has changed on systems that have newer openssh version (v9.0+,
like in Ubuntu24) compared to older version (e.g. v8.*, Like in Ubuntu22). Newer openssh
versions use SFTP protocol by default. This affected the behavior of RemoteCmdRunner
send_files and receive_files API, where we quote path portion of desctination to be sure
that we can run the command with multiple remote paths.

This change removes single quotes around the destination parameter in scp command.
This helps to avoid improper shell expansion of remote path portion of destination.

Fixes: #10584

Testing

  • pr-provision-test
  • executed a test locally (i.e environment uses local machine as sct-runner and runs test on remote scylla cluster) - the issue was observed on such setup

PR pre-checks (self review)

  • I added the relevant backport labels
  • I didn't leave commented-out/debugging code

Reminders

  • Add New configuration option and document them (in sdcm/sct_config.py)
  • Add unit tests to cover my changes (under unit-test/ folder)
  • Update the Readme/doc folder relevant to this change (if needed)

@dimakr dimakr added the backport/none Backport is not required label Apr 3, 2025
@dimakr dimakr added the test-provision-aws Run provision test on AWS label Apr 3, 2025
@dimakr dimakr marked this pull request as ready for review April 3, 2025 10:04
@dimakr dimakr requested review from Copilot and soyacz April 3, 2025 10:04
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the scp command in RemoteCmdRunner to enforce usage of the legacy scp protocol by adding the '-O' flag, addressing differences between Ubuntu 24.04 and 22.04.

  • Updated scp command to include '-O'.
  • Ensures compatibility with both newer openssh versions (using SFTP by default) and older versions (ignoring the flag).

@dimakr dimakr marked this pull request as draft April 3, 2025 22:03
@dimakr
Copy link
Contributor Author

dimakr commented Apr 3, 2025

Turns out not all flavors of scp ignore unknown option, like the package for Ubuntu for instance

@@ -411,7 +411,7 @@ def _make_scp_cmd(self, src: str, dst: str, connect_timeout: int = 300, alive_in
key_option = ''
if self.key_file:
key_option = '-i %s' % os.path.expanduser(self.key_file)
command = ("scp -r -o StrictHostKeyChecking=no -o BatchMode=yes "
command = ("scp -O -r -o StrictHostKeyChecking=no -o BatchMode=yes "
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add doc for this -O param, so in future we'll be able to drop it (when all align to new scp version)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually, some implementations of scp silently ignore unknown options, but others do not. For instance pr-provision-test failed with:

00:31:14  Command: 'scp -O -r -o StrictHostKeyChecking=no -o BatchMode=yes -o ConnectTimeout=300 -o ServerAliveInterval=300 -o UserKnownHostsFile=/tmp/tmpa1njsnq7 -P 22 -i /home/ubuntu/.ssh/scylla_test_id_ed25519  scyllaadm@[10.4.0.217]:"/etc/scylla/scylla.yaml" \'/tmp/scthitllgp8/scylla.yaml\''
00:31:14  
00:31:14  Exit code: 1
00:31:14  Stdout:
00:31:14  Stderr:
00:31:14  
00:31:14  unknown option -- O
00:31:14  usage: scp [-346ABCpqrTv] [-c cipher] [-F ssh_config] [-i identity_file]
00:31:14              [-J destination] [-l limit] [-o ssh_option] [-P port]
00:31:14              [-S program] source ... target

I will change the fix - will remove unnecessary quotes to improper avoid shell expansion.

Copy link
Contributor

Choose a reason for hiding this comment

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

how about the case it was added for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean the original problem described in #10584?

Just removing the single quotes around destination in scp command was enough. Double quotes that were previously inside those single ones are interpreted OK now, on systems with newer and older openssh.
Why these single quotes were around the scp destination in the command string - I don't know, they are in the code of remote_base.py:RemoteCmdRunnerBase._make_scp_cmd from the very beginning, when it was added in 2020.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant it was added in 5bcddb7 - so we need to ensure it still works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Single quotes, which are the problem here, were not added in that commit, but they are from the very beginning in the code.
The 5bcddb7, just added proxy parameter into scp command.

Let me test it - the proxy parameter was added for cases where scylladb cluster is not reachable without proxy, basically the cases of using local dev machine as sct/hydra-runner. I would need to test from local machine again. Will be back soon.

Copy link
Contributor Author

@dimakr dimakr Apr 7, 2025

Choose a reason for hiding this comment

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

@soyacz Tested the flow of updating db packages on remote scylla DB node from local machine, i.e. the case when we need/use the proxy host parameter:

> ssh -i ~/.ssh/scylla_test_id_ed25519 -A [email protected] scylla --version
Warning: Permanently added '108.129.183.202' (ED25519) to the list of known hosts.
6.2.3-0.20250119.bff9ddde1283

> ./sct.py update-scylla-packages --test-id 086f2da3-952c-4d19-9125-5dd8da3f03ac --backend aws -p ../test_upd_pkg_dir
logged in as arn:aws:sts::797456418907:assumed-role/DeveloperAccessRole/[email protected]
? Select machine:  [aws - PR-provision-test-master-db-node-086f2da3-3 - 108.129.183.202 10.4.0.183 - eu-west-1]
...
Installed .deb packages before replacing with new .DEB files
['scylla\t6.2.3-0.20250119.bff9ddde1283-1', 'scylla-conf\t6.2.3-0.20250119.bff9ddde1283-1', 'scylla-cqlsh\t6.2.3-0.20250119.bff9ddde1283-1', 'scylla-kernel-conf\t6.2.3-0.20250119.bff9ddde1283-1', 'scylla-machine-image\t6.2.3-20250119.5cd334b-1', 'scylla-manager-agent\t3.5.0~0.20250402.c599d2025', 'scylla-node-exporter\t6.2.3-0.20250119.bff9ddde1283-1', 'scylla-python3\t6.2.3-0.20250119.bff9ddde1283-1', 'scylla-server\t6.2.3-0.20250119.bff9ddde1283-1', 'scylla-server-dbg\t6.2.3-0.20250119.bff9ddde1283-1', 'scylla-tools\t']
Installed .deb packages after replacing with new .DEB files
['scylla\t6.2.4-0.20250406.fe7132ac083d-1', 'scylla-conf\t6.2.4-0.20250406.fe7132ac083d-1', 'scylla-cqlsh\t6.2.4-0.20250406.fe7132ac083d-1', 'scylla-kernel-conf\t6.2.4-0.20250406.fe7132ac083d-1', 'scylla-machine-image\t6.2.3-20250119.5cd334b-1', 'scylla-manager-agent\t3.5.0~0.20250402.c599d2025', 'scylla-node-exporter\t6.2.4-0.20250406.fe7132ac083d-1', 'scylla-python3\t6.2.4-0.20250406.fe7132ac083d-1', 'scylla-server\t6.2.4-0.20250406.fe7132ac083d-1', 'scylla-server-dbg\t6.2.4-0.20250406.fe7132ac083d-1', 'scylla-tools\t6.2.4-0.20250406.fe7132ac083d-1', 'scylla-tools-core\t6.2.4-0.20250406.fe7132ac083d-1']
Update DB packages duration -> 62 s

> ssh -i ~/.ssh/scylla_test_id_ed25519 -A [email protected] scylla --version
6.2.4-0.20250406.fe7132ac083d

> git status
On branch fix_scp_command_remote_path

Scp command behaviour has changed on systems that have newer openssh version (v9.0+,
like in Ubuntu24) compared to older version (e.g. v8.*, Like in Ubuntu22). Newer openssh
versions use SFTP protocol by default. This affected the behavior of RemoteCmdRunner
send_files and receive_files API, where we quote path portion of desctination to be sure
that we can run the command with multiple remote paths.

This change removes single quotes around the destination parameter in scp command.
This helps to avoid improper shell expansion of remote path portion of destination.

Fixes: scylladb#10584
@dimakr dimakr force-pushed the fix_scp_command_remote_path branch from 9f7f0c7 to c6926f5 Compare April 4, 2025 12:17
@dimakr dimakr marked this pull request as ready for review April 4, 2025 18:38
@dimakr dimakr requested a review from soyacz April 4, 2025 18:38
@dimakr dimakr changed the title fix(RemoteCmdRunner): run scp with legacy protocol if applicable fix(RemoteCmdRunner): fix scp destination string Apr 4, 2025
Copy link
Contributor

@soyacz soyacz left a comment

Choose a reason for hiding this comment

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

Seems should be fine - we have 2 usages of this and anyway we pass double-quoted path for remote and using quote() for local destination, so looks like we added another single-quotes unnecessarily and it worked only because we never had special chars/space in local dest (which is usually /tmp).
LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/none Backport is not required test-provision-aws Run provision test on AWS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(send|receive)_files API of RemoteCmdRunner is failing on ubuntu24 when scp is used as transport
2 participants