diff --git a/awscli/customizations/sessionmanager.py b/awscli/customizations/sessionmanager.py index a0a15222f401..663edf7ef6b9 100644 --- a/awscli/customizations/sessionmanager.py +++ b/awscli/customizations/sessionmanager.py @@ -116,6 +116,23 @@ class StartSessionCaller(CLIOperationCaller): RECOMMENDED_MINIMUM_PLUGIN_VERSION = "1.2.764.0" DEFAULT_SSM_ENV_NAME = "AWS_SSM_START_SESSION_RESPONSE" + def _get_plugin_version(self): + """Return the session-manager-plugin version string. + + Raises a ValueError with a helpful message when the plugin is not + installed instead of letting the raw OSError propagate. + """ + try: + return check_output( + ["session-manager-plugin", "--version"], text=True + ) + except OSError as ex: + if ex.errno == errno.ENOENT: + logger.debug('SessionManagerPlugin is not present', + exc_info=True) + raise ValueError(''.join(ERROR_MESSAGE)) + raise + def _warn_if_plugin_version_is_outdated(self, plugin_version): """Warn when the plugin is older than the recommended minimum.""" version_requirement = VersionRequirement( @@ -139,6 +156,14 @@ def invoke(self, service_name, operation_name, parameters, service_name, region_name=parsed_globals.region, endpoint_url=parsed_globals.endpoint_url, verify=parsed_globals.verify_ssl) + # Check that the session-manager-plugin is installed before starting + # the session. Previously the plugin was only checked after the + # session had been started; when it was missing, the CLI called + # terminate_session to clean up, and if the caller lacked the + # ssm:TerminateSession permission that AccessDenied error masked the + # real "plugin not found" error. Failing fast also avoids creating a + # session that is immediately torn down. + plugin_version = self._get_plugin_version() response = client.start_session(**parameters) session_id = response['SessionId'] region_name = client.meta.region_name @@ -159,9 +184,6 @@ def invoke(self, service_name, operation_name, parameters, } start_session_response = json.dumps(session_parameters) - plugin_version = check_output( - ["session-manager-plugin", "--version"], text=True - ) env = os.environ.copy() # Warn, but do not fail, when the plugin is older than the @@ -207,6 +229,14 @@ def invoke(self, service_name, operation_name, parameters, # session-manager-plugin. If plugin is not present, terminate # is called so that service and ssm-agent terminates the # session to avoid zombie session active on ssm-agent for - # default self terminate time - client.terminate_session(SessionId=session_id) + # default self terminate time. A failure of this cleanup + # call (e.g. the caller lacks ssm:TerminateSession + # permission) must not mask the plugin-not-found error + # below, which is the actionable message for the user. + try: + client.terminate_session(SessionId=session_id) + except Exception: + logger.debug('Failed to terminate session %s after the ' + 'session-manager-plugin was not found.', + session_id, exc_info=True) raise ValueError(''.join(ERROR_MESSAGE)) diff --git a/tests/functional/ssm/test_start_session.py b/tests/functional/ssm/test_start_session.py index 2ea8b3c4e0f4..d23c9754c29f 100644 --- a/tests/functional/ssm/test_start_session.py +++ b/tests/functional/ssm/test_start_session.py @@ -331,24 +331,15 @@ def test_start_session_fails(self, mock_check_output, mock_check_call): def test_start_session_when_get_plugin_version_fails( self, mock_check_output, mock_check_call ): + # The plugin is checked before the session is started: when it is + # not installed the command fails fast with the plugin-not-found + # error and no API calls are made. cmdline = 'ssm start-session --target instance-id' mock_check_output.side_effect = OSError(errno.ENOENT, 'some error') - self.parsed_responses = [ - { - "SessionId": "session-id", - "TokenValue": "token-value", - "StreamUrl": "stream-url", - } - ] - self.run_cmd(cmdline, expected_rc=255) - self.assertEqual(self.operations_called[0][0].name, - 'StartSession') - self.assertEqual(self.operations_called[0][1], - {'Target': 'instance-id'}) - self.assertEqual(self.operations_called[1][0].name, - 'TerminateSession') - self.assertEqual(self.operations_called[1][1], - {'SessionId': 'session-id'}) + stdout, stderr, rc = self.run_cmd(cmdline, expected_rc=255) + self.assertEqual(self.operations_called, []) + self.assertIn('SessionManagerPlugin is not found', stderr) + mock_check_call.assert_not_called() class TestHelpOutput(BaseAWSHelpOutputTest): diff --git a/tests/unit/customizations/test_sessionmanager.py b/tests/unit/customizations/test_sessionmanager.py index 781a11c7407b..bb808bc81d6f 100644 --- a/tests/unit/customizations/test_sessionmanager.py +++ b/tests/unit/customizations/test_sessionmanager.py @@ -38,7 +38,10 @@ def setUp(self): self.parsed_globals = mock.Mock() self.parsed_globals.profile = 'user_profile' - def test_start_session_when_non_custom_start_session_fails(self): + @mock.patch("awscli.customizations.sessionmanager.check_output") + def test_start_session_when_non_custom_start_session_fails( + self, mock_check_output): + mock_check_output.return_value = "1.2.500.0\n" self.client.start_session.side_effect = Exception('some exception') params = {} with self.assertRaisesRegex(Exception, 'some exception'): @@ -226,9 +229,11 @@ def test_start_session_with_env_variable_success_scenario( @mock.patch("awscli.customizations.sessionmanager.check_call") @mock.patch("awscli.customizations.sessionmanager.check_output") - def test_start_session_when_check_output_fails( + def test_start_session_fails_fast_when_plugin_version_check_fails( self, mock_check_output, mock_check_call ): + # The plugin version is checked before the session is started, so a + # failure of that check must surface without any API calls. mock_check_output.side_effect = subprocess.CalledProcessError( returncode=1, cmd="session-manager-plugin", output="some error" ) @@ -236,13 +241,7 @@ def test_start_session_when_check_output_fails( start_session_params = { "Target": "i-123456789" } - start_session_response = { - "SessionId": "session-id", - "TokenValue": "token-value", - "StreamUrl": "stream-url", - } - self.client.start_session.return_value = start_session_response with self.assertRaises(subprocess.CalledProcessError): self.caller.invoke( "ssm", @@ -251,13 +250,79 @@ def test_start_session_when_check_output_fails( self.parsed_globals ) - self.client.start_session.assert_called_with(**start_session_params) + self.client.start_session.assert_not_called() self.client.terminate_session.assert_not_called() mock_check_output.assert_called_with( ["session-manager-plugin", "--version"], text=True ) mock_check_call.assert_not_called() + @mock.patch("awscli.customizations.sessionmanager.check_call") + @mock.patch("awscli.customizations.sessionmanager.check_output") + def test_start_session_when_plugin_not_installed( + self, mock_check_output, mock_check_call + ): + # When the plugin is not installed the CLI must fail fast with the + # actionable plugin-not-found error: no session may be started (which + # would then require a terminate_session cleanup call that can fail + # with a misleading AccessDenied error of its own). + mock_check_output.side_effect = OSError( + errno.ENOENT, 'session-manager-plugin not found' + ) + + start_session_params = { + "Target": "i-123456789" + } + + with self.assertRaisesRegex( + ValueError, 'SessionManagerPlugin is not found'): + self.caller.invoke( + "ssm", + "StartSession", + start_session_params, + self.parsed_globals + ) + + self.client.start_session.assert_not_called() + self.client.terminate_session.assert_not_called() + mock_check_call.assert_not_called() + + @mock.patch("awscli.customizations.sessionmanager.check_call") + @mock.patch("awscli.customizations.sessionmanager.check_output") + def test_start_session_plugin_not_found_error_not_masked_by_cleanup( + self, mock_check_output, mock_check_call + ): + # If the plugin disappears after the pre-flight check, the + # terminate_session cleanup of the started session must not mask + # the plugin-not-found error (e.g. when the caller lacks the + # ssm:TerminateSession permission). + mock_check_output.return_value = "1.2.500.0\n" + mock_check_call.side_effect = OSError(errno.ENOENT, 'some error') + self.client.terminate_session.side_effect = Exception( + 'AccessDenied') + + start_session_params = { + "Target": "i-123456789" + } + start_session_response = { + "SessionId": "session-id", + "TokenValue": "token-value", + "StreamUrl": "stream-url", + } + self.client.start_session.return_value = start_session_response + + with self.assertRaisesRegex( + ValueError, 'SessionManagerPlugin is not found'): + self.caller.invoke( + "ssm", + "StartSession", + start_session_params, + self.parsed_globals + ) + + self.client.terminate_session.assert_called_with( + SessionId="session-id") + @mock.patch("awscli.customizations.sessionmanager.check_call") @mock.patch("awscli.customizations.sessionmanager.check_output") def test_start_session_when_response_not_json(