diff --git a/doc/source/admin/data.md b/doc/source/admin/data.md index f4732057fba..482e225c4a3 100644 --- a/doc/source/admin/data.md +++ b/doc/source/admin/data.md @@ -448,13 +448,11 @@ Example configuration for EBI SRA downloads: retry_base_delay: 2.0 retry_max_delay: 60.0 enable_resume: true - # SSH key - use either ssh_key_file OR ssh_key_content - ssh_key_file: "/path/to/ssh_key_file" # path to key file - # OR - # ssh_key_content: | # embed key content - # -----BEGIN RSA PRIVATE KEY----- - # ... - # -----END RSA PRIVATE KEY----- + # SSH key content (required) - embed the key directly in the configuration + ssh_key_content: | + -----BEGIN RSA PRIVATE KEY----- + + -----END RSA PRIVATE KEY----- ``` The plugin is **download-only** and supports automatic retry with exponential backoff for transient diff --git a/lib/galaxy/config/sample/file_sources_conf.yml.sample b/lib/galaxy/config/sample/file_sources_conf.yml.sample index be9e1660b97..59c0d971ad4 100644 --- a/lib/galaxy/config/sample/file_sources_conf.yml.sample +++ b/lib/galaxy/config/sample/file_sources_conf.yml.sample @@ -83,13 +83,11 @@ retry_max_delay: 60.0 # Maximum delay between retries enable_resume: true # Enable resume for interrupted transfers # SSH private key for authentication (download ENA key from: https://www.ebi.ac.uk/ena/browser/about/ascp) - # Option 1: Provide path to key file (recommended) - ssh_key_file: "/path/to/your/ssh_key_file" - # Option 2: Embed key content directly (use ssh_key_content instead of ssh_key_file) - # ssh_key_content: | - # -----BEGIN RSA PRIVATE KEY----- - # - # -----END RSA PRIVATE KEY----- + # Embed the key content directly in the configuration + ssh_key_content: | + -----BEGIN RSA PRIVATE KEY----- + + -----END RSA PRIVATE KEY----- # Note: This plugin is download-only (writable: false, browsable: false) # Example: Custom Aspera endpoint with encryption enabled diff --git a/lib/galaxy/files/sources/ascp.py b/lib/galaxy/files/sources/ascp.py index 32737acb42e..ac46d133fd8 100644 --- a/lib/galaxy/files/sources/ascp.py +++ b/lib/galaxy/files/sources/ascp.py @@ -11,10 +11,7 @@ The implementation is extensible to support future enhancements such as: """ import logging -from typing import ( - Optional, - Union, -) +from typing import Union from galaxy.files.models import ( FilesSourceRuntimeContext, @@ -45,12 +42,11 @@ class AscpFilesSourceTemplateConfiguration(FsspecBaseFileSourceTemplateConfigura This configuration supports template expansion for all fields, allowing dynamic configuration based on user context or other variables. - Note: Exactly one of ssh_key_file or ssh_key_content must be provided. + Note: ssh_key_content is required for SSH authentication. """ ascp_path: Union[str, TemplateExpansion] = "ascp" - ssh_key_file: Union[str, TemplateExpansion, None] = None # Path to SSH key file - ssh_key_content: Union[str, TemplateExpansion, None] = None # SSH key content as string + ssh_key_content: Union[str, TemplateExpansion] # SSH key content as string (required) user: Union[str, TemplateExpansion] # Required field host: Union[str, TemplateExpansion] # Required field rate_limit: Union[str, TemplateExpansion] = "300m" @@ -67,12 +63,11 @@ class AscpFilesSourceConfiguration(FsspecBaseFileSourceConfiguration): This configuration contains the actual values after template expansion. - Note: Exactly one of ssh_key_file or ssh_key_content must be provided. + Note: ssh_key_content is required for SSH authentication. """ ascp_path: str = "ascp" - ssh_key_file: Optional[str] = None # Path to SSH key file - ssh_key_content: Optional[str] = None # SSH key content as string + ssh_key_content: str # SSH key content as string (required) user: str # Required field host: str # Required field rate_limit: str = "300m" @@ -138,18 +133,9 @@ class AscpFilesSource(FsspecFilesSource[AscpFilesSourceTemplateConfiguration, As config = context.config - # Validate that exactly one of ssh_key_file or ssh_key_content is provided - if config.ssh_key_file and config.ssh_key_content: - raise ValueError("Cannot specify both ssh_key_file and ssh_key_content. Please provide only one.") - if not config.ssh_key_file and not config.ssh_key_content: - raise ValueError("Must specify either ssh_key_file or ssh_key_content for SSH authentication.") - - # Determine which key parameter to use - ssh_key = config.ssh_key_file if config.ssh_key_file else config.ssh_key_content - return AscpFileSystem( ascp_path=config.ascp_path, - ssh_key=ssh_key, + ssh_key=config.ssh_key_content, user=config.user, host=config.host, rate_limit=config.rate_limit, diff --git a/lib/galaxy/files/sources/ascp_fsspec.py b/lib/galaxy/files/sources/ascp_fsspec.py index 6b1439cff45..95cd0820c82 100644 --- a/lib/galaxy/files/sources/ascp_fsspec.py +++ b/lib/galaxy/files/sources/ascp_fsspec.py @@ -178,35 +178,19 @@ class AscpFileSystem(AbstractFileSystem): if not self.ssh_key: raise MessageException("SSH key is required for ascp authentication.") - # Determine if ssh_key is a file path or key content - # If it looks like a path and the file exists, use it directly - # Otherwise, treat it as key content and create a temporary file - key_is_file = False + # Create temporary file for key content key_fd = None key_path = None - # Check if ssh_key is a file path - if not self.ssh_key.startswith("-----BEGIN") and os.path.isfile(self.ssh_key): - # It's a file path - use it directly - key_path = self.ssh_key - key_is_file = True - - # Verify permissions (should be 0600 or 0400) - stat_info = os.stat(key_path) - mode = stat_info.st_mode & 0o777 - if mode not in (0o600, 0o400): - log.warning(f"SSH key file {key_path} has permissions {oct(mode)}, ascp may require 0600 or 0400") - try: - # If not using an existing file, create temporary file for key content - if not key_is_file: - key_fd, key_path = tempfile.mkstemp(suffix=".key", text=True) + # Create temporary file for key content + key_fd, key_path = tempfile.mkstemp(suffix=".key", text=True) - # Write key with proper permissions (required by ascp) - os.chmod(key_path, 0o600) - with os.fdopen(key_fd, "w") as key_file: - key_file.write(self.ssh_key) - key_fd = None # Prevent double-close + # Write key with proper permissions (required by ascp) + os.chmod(key_path, 0o600) + with os.fdopen(key_fd, "w") as key_file: + key_file.write(self.ssh_key) + key_fd = None # Prevent double-close # Type narrowing: key_path is guaranteed to be set by this point assert key_path is not None @@ -255,18 +239,17 @@ class AscpFileSystem(AbstractFileSystem): except OSError as e: raise MessageException(f"File system error during ascp transfer: {e}") from e finally: - # Only cleanup temporary key file (not user-provided files) - if not key_is_file: - if key_fd is not None: - try: - os.close(key_fd) - except Exception: - pass # Best effort - if key_path is not None: - try: - os.unlink(key_path) - except Exception: - pass # Best effort + # Cleanup temporary key file + if key_fd is not None: + try: + os.close(key_fd) + except Exception: + pass # Best effort + if key_path is not None: + try: + os.unlink(key_path) + except Exception: + pass # Best effort def _is_retryable_error(self, exception: MessageException) -> bool: """Determine if an error is worth retrying. diff --git a/test/unit/files/test_ascp.py b/test/unit/files/test_ascp.py index df336deed3e..6e5d1a83a2e 100644 --- a/test/unit/files/test_ascp.py +++ b/test/unit/files/test_ascp.py @@ -380,46 +380,6 @@ class TestAscpFilesSource: finally: os.unlink(config_file) - def test_ssh_key_file_usage(self): - """Test that ssh_key_file is properly used when provided.""" - with tempfile.NamedTemporaryFile(mode="w", delete=False, suffix=".key") as key_file: - key_file.write(TEST_SSH_KEY) - key_file_path = key_file.name - - try: - os.chmod(key_file_path, 0o600) - - config = { - "type": "ascp", - "id": "test_ascp", - "label": "Test Aspera", - "ssh_key_file": key_file_path, - "user": "test-user", - "host": "test.example.com", - } - - with patch("shutil.which", return_value="/usr/bin/ascp"): - from ._util import configured_file_sources - - with tempfile.NamedTemporaryFile(mode="w", suffix=".yml", delete=False) as f: - import yaml - - yaml.dump([config], f) - config_file = f.name - - try: - file_sources = configured_file_sources(config_file) - # Should load successfully - assert ( - file_sources.get_file_source_path("ascp://test.example.com/path/to/file").file_source.id - == "test_ascp" - ) - finally: - os.unlink(config_file) - finally: - if os.path.exists(key_file_path): - os.unlink(key_file_path) - class TestAscpRetryLogic: """Tests for retry and resume functionality.""" @@ -683,42 +643,6 @@ class TestAscpRetryLogic: assert fs.retry_max_delay == 60.0 assert fs.enable_resume is True - def test_ssh_key_as_file_path(self): - """Test that ssh_key can be provided as a file path.""" - with patch("shutil.which", return_value="/usr/bin/ascp"): - with tempfile.NamedTemporaryFile(mode="w", delete=False, suffix=".key") as key_file: - key_file.write(TEST_SSH_KEY) - key_file_path = key_file.name - - try: - # Set proper permissions - os.chmod(key_file_path, 0o600) - - fs = AscpFileSystem( - ssh_key=key_file_path, # Pass file path instead of content - user="test-user", - host="test.example.com", - ) - - # Mock subprocess to verify the key file path is used directly - with patch("subprocess.run") as mock_run: - mock_run.return_value = Mock(returncode=0, stderr="", stdout="") - - with patch("os.unlink") as mock_unlink: - fs._get_file("/remote/file.txt", "/local/file.txt") - - # Verify the original key file was NOT deleted - # (only temporary files should be deleted) - mock_unlink.assert_not_called() - - # Verify ascp was called with the original key file path - call_args = mock_run.call_args[0][0] - assert key_file_path in call_args - - finally: - if os.path.exists(key_file_path): - os.unlink(key_file_path) - def test_ssh_key_as_content(self): """Test that ssh_key can be provided as key content (original behavior).""" with patch("shutil.which", return_value="/usr/bin/ascp"):