From 476e11e207828811a0f0baf06eaad9e8c96c73ee Mon Sep 17 00:00:00 2001 From: michelligabriele Date: Fri, 20 Mar 2026 21:20:00 +0100 Subject: [PATCH] fix(secret_managers): check vault rotation error return and mask raw API keys in logs --- .../proxy/hooks/key_management_event_hooks.py | 9 +- .../hashicorp_secret_manager.py | 11 ++- tests/litellm_utils_tests/test_hashicorp.py | 5 +- .../hooks/test_key_management_event_hooks.py | 92 ++++++++++++++++++- 4 files changed, 111 insertions(+), 6 deletions(-) diff --git a/litellm/proxy/hooks/key_management_event_hooks.py b/litellm/proxy/hooks/key_management_event_hooks.py index 2d61203ad51..b5798080836 100644 --- a/litellm/proxy/hooks/key_management_event_hooks.py +++ b/litellm/proxy/hooks/key_management_event_hooks.py @@ -322,7 +322,7 @@ class KeyManagementEventHooks: optional_params = await KeyManagementEventHooks._get_secret_manager_optional_params( team_id ) - await litellm.secret_manager_client.async_rotate_secret( + result = await litellm.secret_manager_client.async_rotate_secret( current_secret_name=KeyManagementEventHooks._get_secret_name( current_secret_name ), @@ -332,6 +332,13 @@ class KeyManagementEventHooks: new_secret_value=new_secret_value, optional_params=optional_params, ) + if ( + isinstance(result, dict) + and result.get("status") == "error" + ): + raise ValueError( + f"Secret manager rotation failed: {result.get('message', 'unknown error')}" + ) @staticmethod def _get_secret_name(secret_name: str) -> str: diff --git a/litellm/secret_managers/hashicorp_secret_manager.py b/litellm/secret_managers/hashicorp_secret_manager.py index 8bb3f801a1e..a00e20b7ee6 100644 --- a/litellm/secret_managers/hashicorp_secret_manager.py +++ b/litellm/secret_managers/hashicorp_secret_manager.py @@ -537,12 +537,17 @@ class HashicorpSecretManager(BaseSecretManager): json_resp.get("data", {}).get("data", {}).get(data_key, None) ) if new_secret_value_from_vault != new_secret_value: - verbose_logger.exception( - f"New secret value mismatch. Expected: {new_secret_value}, Got: {new_secret_value_from_vault}" + # Mask key values to avoid logging raw API keys + masked_expected = f"...{new_secret_value[-4:]}" if new_secret_value else "" + masked_got = f"...{new_secret_value_from_vault[-4:]}" if new_secret_value_from_vault else "" + verbose_logger.error( + "New secret value mismatch. Expected: %s, Got: %s", + masked_expected, + masked_got, ) return { "status": "error", - "message": f"New secret value mismatch. Expected: {new_secret_value}, Got: {new_secret_value_from_vault}", + "message": f"New secret value mismatch. Expected: {masked_expected}, Got: {masked_got}", } except httpx.HTTPStatusError as e: if e.response.status_code == 404: diff --git a/tests/litellm_utils_tests/test_hashicorp.py b/tests/litellm_utils_tests/test_hashicorp.py index e4d69da6ac6..9b8ceed488b 100644 --- a/tests/litellm_utils_tests/test_hashicorp.py +++ b/tests/litellm_utils_tests/test_hashicorp.py @@ -759,4 +759,7 @@ async def test_hashicorp_secret_manager_rotate_secret_value_mismatch(hashicorp_s # Verify error response assert response["status"] == "error" assert "mismatch" in response["message"].lower() - assert "expected-value" in response["message"] + # Raw key values should be masked — only last 4 chars shown + assert "expected-value" not in response["message"] + assert "...alue" in response["message"] # last 4 of "expected-value" + assert "...alue" in response["message"] # last 4 of "different-value" diff --git a/tests/test_litellm/proxy/hooks/test_key_management_event_hooks.py b/tests/test_litellm/proxy/hooks/test_key_management_event_hooks.py index f66a65f08f4..5849cceeca5 100644 --- a/tests/test_litellm/proxy/hooks/test_key_management_event_hooks.py +++ b/tests/test_litellm/proxy/hooks/test_key_management_event_hooks.py @@ -385,6 +385,96 @@ class TestRotateVirtualKeyInSecretManager: new_secret_value="sk-new-value", team_id="team-123", ) - + # Verify async_rotate_secret was NOT called mock_secret_manager.async_rotate_secret.assert_not_called() + + +class TestRotateVirtualKeyErrorHandling: + """Tests that _rotate_virtual_key_in_secret_manager propagates error dicts as exceptions.""" + + @pytest.mark.asyncio + async def test_rotate_raises_on_error_dict(self): + """Test that error dict from async_rotate_secret is raised as ValueError.""" + from litellm.types.secret_managers.main import KeyManagementSystem, KeyManagementSettings + from litellm.secret_managers.base_secret_manager import BaseSecretManager + import litellm + + mock_secret_manager = MagicMock(spec=BaseSecretManager) + mock_secret_manager.async_rotate_secret = AsyncMock( + return_value={"status": "error", "message": "New secret value mismatch. Expected: ...3lcA, Got: ...zt4w"} + ) + + litellm.secret_manager_client = mock_secret_manager + litellm._key_management_system = KeyManagementSystem.HASHICORP_VAULT + litellm._key_management_settings = KeyManagementSettings( + store_virtual_keys=True, + prefix_for_stored_virtual_keys="litellm/", + ) + + import builtins + original_isinstance = builtins.isinstance + + def mock_isinstance(obj, cls): + if cls == BaseSecretManager and obj == mock_secret_manager: + return True + return original_isinstance(obj, cls) + + with patch.object( + KeyManagementEventHooks, + "_get_secret_manager_optional_params", + return_value=None, + ), patch( + "litellm.proxy.hooks.key_management_event_hooks.isinstance", + side_effect=mock_isinstance, + ): + with pytest.raises(ValueError, match="Secret manager rotation failed"): + await KeyManagementEventHooks._rotate_virtual_key_in_secret_manager( + current_secret_name="old-key", + new_secret_name="new-key", + new_secret_value="sk-new-value", + team_id=None, + ) + + @pytest.mark.asyncio + async def test_rotate_success_dict_does_not_raise(self): + """Test that a success response from async_rotate_secret does not raise.""" + from litellm.types.secret_managers.main import KeyManagementSystem, KeyManagementSettings + from litellm.secret_managers.base_secret_manager import BaseSecretManager + import litellm + + mock_secret_manager = MagicMock(spec=BaseSecretManager) + mock_secret_manager.async_rotate_secret = AsyncMock( + return_value={"request_id": "abc", "data": {}} + ) + + litellm.secret_manager_client = mock_secret_manager + litellm._key_management_system = KeyManagementSystem.HASHICORP_VAULT + litellm._key_management_settings = KeyManagementSettings( + store_virtual_keys=True, + prefix_for_stored_virtual_keys="litellm/", + ) + + import builtins + original_isinstance = builtins.isinstance + + def mock_isinstance(obj, cls): + if cls == BaseSecretManager and obj == mock_secret_manager: + return True + return original_isinstance(obj, cls) + + with patch.object( + KeyManagementEventHooks, + "_get_secret_manager_optional_params", + return_value=None, + ), patch( + "litellm.proxy.hooks.key_management_event_hooks.isinstance", + side_effect=mock_isinstance, + ): + # Should not raise + await KeyManagementEventHooks._rotate_virtual_key_in_secret_manager( + current_secret_name="old-key", + new_secret_name="new-key", + new_secret_value="sk-new-value", + team_id=None, + )