Replace hardcoded SSH options with environment variable for secure and flexible rsync configuration - #3
Conversation
|
@thorwhalen 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs. I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review. |
Co-authored-by: thorwhalen <1906276+thorwhalen@users.noreply.github.com>
| ssh_parts += ["-i", self._conn_key_filename] | ||
| # Add SSH options to handle host key verification | ||
| # This is needed for CI environments where host keys may not be in known_hosts | ||
| ssh_parts += ["-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null"] |
There was a problem hiding this comment.
Add this systematically could be a security risk.
Ideally, having a solution implemented in the CI script only would be better. It is not sshdol's concern, so should be separate.
At a minimum, replacing this with:
# Add SSH options to handle host key verification
# This is needed for CI environments where host keys may not be in known_hosts
if os.environ.get('CI') == 'true':
ssh_parts += ["-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null"]would be slightly better.
If no solution that is CI-script only can be found, another way to setup the base objects would be to have a special SSHDOL_SYNC_TO_EXTRA_SSH_OPTIONS environment variable that would be used (after transforming it's string value to a list of "parts") instead of the hardcoded ["-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null"]. That way, CI would have a place to communicate this need (by setting this env var), and we could pretend we're still doing separation of concerns, since we can also use this mechanism in other situations.
There was a problem hiding this comment.
I've implemented the environment variable approach as you suggested. The solution replaces the hardcoded SSH options with SSHDOL_SYNC_TO_EXTRA_SSH_OPTIONS environment variable for better separation of concerns and security.
The implementation:
- Is secure by default (no hardcoded insecure options)
- Allows CI environments to set the needed options via environment variable
- Provides flexibility for any additional SSH options users might need
- Uses
shlex.split()to properly handle quoted arguments
For CI, the environment variable can be set to:
SSHDOL_SYNC_TO_EXTRA_SSH_OPTIONS="-o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null"
Commit: b81c7e8
There was a problem hiding this comment.
@copilot, go ahead and add the
SSHDOL_SYNC_TO_EXTRA_SSH_OPTIONS="-o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null"
in the ci.yml file, in the appropriate place.
There was a problem hiding this comment.
I've added the SSHDOL_SYNC_TO_EXTRA_SSH_OPTIONS environment variable to the CI validation job in the ci.yml file. The environment variable is set to "-o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null" which will allow the SSH tests to pass in the CI environment by disabling host key verification.
Commit: c19d066
…curity and flexibility Co-authored-by: thorwhalen <1906276+thorwhalen@users.noreply.github.com>
…ation job Co-authored-by: thorwhalen <1906276+thorwhalen@users.noreply.github.com>
The CI pipeline was failing during pytest validation with a "Host key verification failed" error when the
test_sync_totest attempted to use rsync over SSH. This occurred because the rsync command in thesync_tomethod creates a separate SSH connection that doesn't inherit the paramikoAutoAddPolicy()setting used by the main SSH connection.Problem
Solution
Instead of hardcoding SSH options that disable host key verification for all environments, this PR implements a flexible environment variable approach that maintains security by default while allowing CI environments to specify needed options.
The
sync_tomethod now checks for theSSHDOL_SYNC_TO_EXTRA_SSH_OPTIONSenvironment variable and adds any specified SSH options to the rsync command. The CI configuration has been updated to set this environment variable with the necessary options for the validation job.Implementation
The fix replaces hardcoded options with environment variable support:
The CI configuration now includes:
Benefits
Usage
For CI environments that need to disable host key checking:
For other custom SSH configurations:
Impact
sync_to, not the main paramiko connectionFixes #2.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.