diff --git a/ironic_python_agent/api/app.py b/ironic_python_agent/api/app.py index d44e3793a..f3e6b98df 100644 --- a/ironic_python_agent/api/app.py +++ b/ironic_python_agent/api/app.py @@ -113,6 +113,7 @@ class Application(object): routing.Rule('/commands/', endpoint='run_command', methods=['POST']), ]) + self.security_get_token_support = False def __call__(self, environ, start_response): """WSGI entry point.""" @@ -199,11 +200,27 @@ class Application(object): status = self.agent.get_status() return jsonify(status) + def require_agent_token_for_command(func): + def wrapper(self, request, *args, **kwargs): + token = request.args.get('agent_token', None) + if token: + # TODO(TheJulia): At some point down the road, remove the + # self.security_get_token_support flag and use the same + # decorator for the api_run_command endpoint. + self.security_get_token_support = True + if (self.security_get_token_support + and not self.agent.validate_agent_token(token)): + raise http_exc.Unauthorized('Token invalid.') + return func(self, request, *args, **kwargs) + return wrapper + + @require_agent_token_for_command def api_list_commands(self, request): with metrics_utils.get_metrics_logger(__name__).timer('list_commands'): results = self.agent.list_command_results() return jsonify({'commands': results}) + @require_agent_token_for_command def api_get_command(self, request, cmd): with metrics_utils.get_metrics_logger(__name__).timer('get_command'): result = self.agent.get_command_result(cmd) diff --git a/ironic_python_agent/tests/unit/test_api.py b/ironic_python_agent/tests/unit/test_api.py index c9491493c..a539d1371 100644 --- a/ironic_python_agent/tests/unit/test_api.py +++ b/ironic_python_agent/tests/unit/test_api.py @@ -260,6 +260,32 @@ class TestIronicAPI(ironic_agent_base.IronicAgentTest): ], }, response.json) + def test_list_commands_with_token(self): + agent_token = str('0123456789' * 10) + cmd_result = base.SyncCommandResult('do_things', + {'key': 'value'}, + True, + {'test': 'result'}) + self.mock_agent.list_command_results.return_value = [cmd_result] + self.mock_agent.validate_agent_token.return_value = True + + response = self.get_json('/commands?agent_token=%s' % agent_token) + + self.assertEqual(200, response.status_code) + self.assertEqual(1, self.mock_agent.validate_agent_token.call_count) + self.assertEqual(1, self.mock_agent.list_command_results.call_count) + + def test_list_commands_with_token_invalid(self): + agent_token = str('0123456789' * 10) + self.mock_agent.validate_agent_token.return_value = False + + response = self.get_json('/commands?agent_token=%s' % agent_token, + expect_errors=True) + + self.assertEqual(401, response.status_code) + self.assertEqual(1, self.mock_agent.validate_agent_token.call_count) + self.assertEqual(0, self.mock_agent.list_command_results.call_count) + def test_get_command_result(self): cmd_result = base.SyncCommandResult('do_things', {'key': 'value'}, @@ -274,6 +300,76 @@ class TestIronicAPI(ironic_agent_base.IronicAgentTest): data = response.json self.assertEqual(serialized_cmd_result, data) + def test_get_command_with_token(self): + agent_token = str('0123456789' * 10) + cmd_result = base.SyncCommandResult('do_things', + {'key': 'value'}, + True, + {'test': 'result'}) + self.mock_agent.get_command_result.return_value = cmd_result + self.mock_agent.validate_agent_token.return_value = True + + response = self.get_json( + '/commands/abc123?agent_token=%s' % agent_token) + + self.assertEqual(200, response.status_code) + self.assertEqual(cmd_result.serialize(), response.json) + self.assertEqual(1, self.mock_agent.validate_agent_token.call_count) + self.assertEqual(1, self.mock_agent.get_command_result.call_count) + + def test_get_command_with_token_invalid(self): + agent_token = str('0123456789' * 10) + self.mock_agent.validate_agent_token.return_value = False + + response = self.get_json( + '/commands/abc123?agent_token=%s' % agent_token, + expect_errors=True) + + self.assertEqual(401, response.status_code) + self.assertEqual(1, self.mock_agent.validate_agent_token.call_count) + self.assertEqual(0, self.mock_agent.get_command_result.call_count) + + def test_get_command_locks_out_with_token(self): + """Tests agent backwards compatibility and verifies upgrade lockout.""" + cmd_result = base.SyncCommandResult('do_things', + {'key': 'value'}, + True, + {'test': 'result'}) + cmd_result.serialize() + self.mock_agent.get_command_result.return_value = cmd_result + agent_token = str('0123456789' * 10) + self.mock_agent.validate_agent_token.return_value = False + + # Backwards compatible operation check. + response = self.get_json( + '/commands/abc123') + self.assertEqual(200, response.status_code) + self.assertFalse(self.app.security_get_token_support) + self.assertEqual(1, self.mock_agent.get_command_result.call_count) + self.mock_agent.reset_mock() + + # Check with a newer ironic sending an agent_token upon the command. + # For context, in this case the token is wrong intentionally. + # It doesn't have to be right, but what we're testing is the + # submission of any value triggers the lockout + response = self.get_json( + '/commands/abc123?agent_token=%s' % agent_token, + expect_errors=True) + self.assertTrue(self.app.security_get_token_support) + self.assertEqual(401, response.status_code) + self.assertEqual(1, self.mock_agent.validate_agent_token.call_count) + self.assertEqual(0, self.mock_agent.get_command_result.call_count) + + # Verifying the lockout is now being enforced and that agent token + # is now required by the agent. + response = self.get_json( + '/commands/abc123', expect_errors=True) + self.assertTrue(self.app.security_get_token_support) + self.assertEqual(401, response.status_code) + self.assertEqual(0, self.mock_agent.get_command_result.call_count) + # Verify we still called validate_agent_token + self.assertEqual(2, self.mock_agent.validate_agent_token.call_count) + def test_execute_agent_command_with_token(self): agent_token = str('0123456789' * 10) command = { diff --git a/releasenotes/notes/lockout-command-result-a368187515385270.yaml b/releasenotes/notes/lockout-command-result-a368187515385270.yaml new file mode 100644 index 000000000..ac4f58a73 --- /dev/null +++ b/releasenotes/notes/lockout-command-result-a368187515385270.yaml @@ -0,0 +1,11 @@ +--- +fixes: + - | + Fixes a potential security issue where a third party may be able to + retrieve potentially sensitive data in command result output from + the agent. If a request comes in with an ``agent_token`` to the + command results endpoint, the agent will now require all future + calls to leverage the token to retrieve results and validate + that token's validity. This effectively eliminates the possibility + of a malicious entity with access to the agent's API endpoint from + capturing the command results from agent operations.

AltStyle によって変換されたページ (->オリジナル) /