diff --git a/openhands-agent-server/openhands/agent_server/config.py b/openhands-agent-server/openhands/agent_server/config.py index c7d99ee65c..782a441af6 100644 --- a/openhands-agent-server/openhands/agent_server/config.py +++ b/openhands-agent-server/openhands/agent_server/config.py @@ -10,22 +10,37 @@ # Environment variable constants -SESSION_API_KEY_ENV = "SESSION_API_KEY" +V0_SESSION_API_KEY_ENV = "SESSION_API_KEY" +V1_SESSION_API_KEY_ENV = "OH_SESSION_API_KEYS_0" ENVIRONMENT_VARIABLE_PREFIX = "OH" _logger = logging.getLogger(__name__) def _default_session_api_keys(): - # Legacy fallback for compability with old runtime API + """ + This function exists as a fallback to using this old V0 environment + variable. If new V1_SESSION_API_KEYS_0 environment variable exists, + it is read automatically by the EnvParser and this function is never + called. + """ result = [] - session_api_key = os.getenv(SESSION_API_KEY_ENV) + session_api_key = os.getenv(V0_SESSION_API_KEY_ENV) if session_api_key: result.append(session_api_key) return result def _default_secret_key() -> SecretStr | None: - session_api_key = os.getenv(SESSION_API_KEY_ENV) + """ + If the OH_SECRET_KEY environment variable is present, it is read by the EnvParser + and this function is never called. Otherwise, we fall back to using the first + available session_api_key - which we read from the environment. + We check both the V0 and V1 variables for this. + """ + session_api_key = os.getenv(V0_SESSION_API_KEY_ENV) + if session_api_key: + return SecretStr(session_api_key) + session_api_key = os.getenv(V1_SESSION_API_KEY_ENV) if session_api_key: return SecretStr(session_api_key) return None @@ -77,7 +92,10 @@ class Config(BaseModel): description=( "List of valid session API keys used to authenticate incoming requests. " "Empty list implies the server will be unsecured. Any key in this list " - "will be accepted for authentication." + "will be accepted for authentication. Multiple keys are supported to " + "enable key rotation without service disruption - new keys can be added " + "to the list, then clients are updated with the new key, and finally the " + "old key is removed from the list. " ), ) allow_cors_origins: list[str] = Field( diff --git a/openhands-agent-server/openhands/agent_server/event_service.py b/openhands-agent-server/openhands/agent_server/event_service.py index febed953c9..a3f5b8479e 100644 --- a/openhands-agent-server/openhands/agent_server/event_service.py +++ b/openhands-agent-server/openhands/agent_server/event_service.py @@ -425,6 +425,7 @@ async def start(self): stuck_detection=self.stored.stuck_detection, visualizer=None, secrets=self.stored.secrets, + cipher=self.cipher, ) # Set confirmation mode if enabled diff --git a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py index 9da8fa681a..c5fa7efd7b 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py @@ -40,6 +40,7 @@ from openhands.sdk.security.confirmation_policy import ( ConfirmationPolicyBase, ) +from openhands.sdk.utils.cipher import Cipher from openhands.sdk.workspace import LocalWorkspace @@ -77,6 +78,7 @@ def __init__( type[ConversationVisualizerBase] | ConversationVisualizerBase | None ) = DefaultConversationVisualizer, secrets: Mapping[str, SecretValue] | None = None, + cipher: Cipher | None = None, **_: object, ): """Initialize the conversation. @@ -105,6 +107,10 @@ def __init__( a dict with keys: 'action_observation', 'action_error', 'monologue', 'alternating_pattern'. Values are integers representing the number of repetitions before triggering. + cipher: Optional cipher for encrypting/decrypting secrets in persisted + state. If provided, secrets are encrypted when saving and + decrypted when loading. If not provided, secrets are redacted + (lost) on serialization. """ super().__init__() # Initialize with span tracking # Mark cleanup as initiated as early as possible to avoid races or partially @@ -134,6 +140,7 @@ def __init__( else None, max_iterations=max_iteration_per_run, stuck_detection=stuck_detection, + cipher=cipher, ) # Default callback: persist every event to state diff --git a/openhands-sdk/openhands/sdk/conversation/state.py b/openhands-sdk/openhands/sdk/conversation/state.py index f2d339be0f..cab24be064 100644 --- a/openhands-sdk/openhands/sdk/conversation/state.py +++ b/openhands-sdk/openhands/sdk/conversation/state.py @@ -23,6 +23,7 @@ ConfirmationPolicyBase, NeverConfirm, ) +from openhands.sdk.utils.cipher import Cipher from openhands.sdk.utils.models import OpenHandsModel from openhands.sdk.workspace.base import BaseWorkspace @@ -124,6 +125,7 @@ class ConversationState(OpenHandsModel): # ===== Private attrs (NOT Fields) ===== _fs: FileStore = PrivateAttr() # filestore for persistence _events: EventLog = PrivateAttr() # now the storage for events + _cipher: Cipher | None = PrivateAttr(default=None) # cipher for secret encryption _autosave_enabled: bool = PrivateAttr( default=False ) # to avoid recursion during init @@ -166,8 +168,20 @@ def set_on_state_change(self, callback: ConversationCallbackType | None) -> None def _save_base_state(self, fs: FileStore) -> None: """ Persist base state snapshot (no events; events are file-backed). + + If a cipher is configured, secrets will be encrypted. Otherwise, they + will be redacted (serialized as '**********'). """ - payload = self.model_dump_json(exclude_none=True) + context = {"cipher": self._cipher} if self._cipher else None + # Warn if secrets exist but no cipher is configured + if not self._cipher and self.secret_registry.secret_sources: + logger.warning( + f"Saving conversation state without cipher - " + f"{len(self.secret_registry.secret_sources)} secret(s) will be " + "redacted and lost on restore. Consider providing a cipher to " + "preserve secrets." + ) + payload = self.model_dump_json(exclude_none=True, context=context) fs.write(BASE_STATE, payload) # ===== Factory: open-or-create (no load/save methods needed) ===== @@ -180,6 +194,7 @@ def create( persistence_dir: str | None = None, max_iterations: int = 500, stuck_detection: bool = True, + cipher: Cipher | None = None, ) -> "ConversationState": """Create a new conversation state or resume from persistence. @@ -203,6 +218,10 @@ def create( persistence_dir: Directory for persisting state and events max_iterations: Maximum iterations per run stuck_detection: Whether to enable stuck detection + cipher: Optional cipher for encrypting/decrypting secrets in + persisted state. If provided, secrets are encrypted when + saving and decrypted when loading. If not provided, secrets + are redacted (lost) on serialization. Returns: ConversationState ready for use @@ -224,7 +243,9 @@ def create( # ---- Resume path ---- if base_text: - state = cls.model_validate(json.loads(base_text)) + # Use cipher context for decrypting secrets if provided + context = {"cipher": cipher} if cipher else None + state = cls.model_validate(json.loads(base_text), context=context) # Restore the conversation with the same id if state.id != id: @@ -236,6 +257,7 @@ def create( # Attach event log early so we can read history for tool verification state._fs = file_store state._events = EventLog(file_store, dir_path=EVENTS_DIR) + state._cipher = cipher # Verify compatibility (agent class + tools) agent.verify(state.agent, events=state._events) @@ -272,6 +294,7 @@ def create( ) state._fs = file_store state._events = EventLog(file_store, dir_path=EVENTS_DIR) + state._cipher = cipher state.stats = ConversationStats() state._save_base_state(file_store) # initial snapshot diff --git a/openhands-sdk/openhands/sdk/secret/secrets.py b/openhands-sdk/openhands/sdk/secret/secrets.py index 2e19262626..a1300f8b86 100644 --- a/openhands-sdk/openhands/sdk/secret/secrets.py +++ b/openhands-sdk/openhands/sdk/secret/secrets.py @@ -5,10 +5,14 @@ import httpx from pydantic import Field, SecretStr, field_serializer, field_validator +from openhands.sdk.logger import get_logger from openhands.sdk.utils.models import DiscriminatedUnionMixin from openhands.sdk.utils.pydantic_secrets import serialize_secret, validate_secret +logger = get_logger(__name__) + + class SecretSource(DiscriminatedUnionMixin, ABC): """Source for a named secret which may be obtained dynamically""" @@ -25,9 +29,11 @@ def get_value(self) -> str | None: class StaticSecret(SecretSource): """A secret stored locally""" - value: SecretStr + value: SecretStr | None = None - def get_value(self): + def get_value(self) -> str | None: + if self.value is None: + return None return self.value.get_secret_value() @field_validator("value") @@ -58,7 +64,12 @@ def _validate_secrets(cls, headers: dict[str, str], info): for key, value in headers.items(): if _is_secret_header(key): secret_value = validate_secret(SecretStr(value), info) - assert secret_value is not None + # Skip headers with redacted/empty secret values + if secret_value is None: + logger.debug( + f"Skipping redacted header '{key}' during deserialization" + ) + continue result[key] = secret_value.get_secret_value() else: result[key] = value @@ -70,7 +81,11 @@ def _serialize_secrets(self, headers: dict[str, str], info): for key, value in headers.items(): if _is_secret_header(key): secret_value = serialize_secret(SecretStr(value), info) - assert secret_value is not None + if secret_value is None: + logger.debug( + f"Skipping redacted header '{key}' during serialization" + ) + continue result[key] = secret_value else: result[key] = value diff --git a/tests/sdk/conversation/local/test_state_serialization.py b/tests/sdk/conversation/local/test_state_serialization.py index fd6fe2765d..ea92ee23b4 100644 --- a/tests/sdk/conversation/local/test_state_serialization.py +++ b/tests/sdk/conversation/local/test_state_serialization.py @@ -826,3 +826,342 @@ def test_resume_with_conversation_id_mismatch_raises_error(): agent=agent, id=different_id, ) + + +def test_conversation_state_secrets_serialization_deserialization(): + """Test that secrets are properly serialized and deserialized. + + This is a regression test for issue 1505 where conversations with secrets + would fail to restore because secrets are serialized as '**********' + (redacted) but StaticSecret.value was a required field that couldn't + accept None after validation converted '**********' to None. + """ + with tempfile.TemporaryDirectory() as temp_dir: + llm = LLM( + model="gpt-4o-mini", api_key=SecretStr("test-key"), usage_id="test-llm" + ) + agent = Agent(llm=llm, tools=[]) + conv_id = uuid.UUID("12345678-1234-5678-9abc-123456789099") + persist_path = LocalConversation.get_persistence_dir(temp_dir, conv_id) + + # Create conversation state with secrets + state = ConversationState.create( + workspace=LocalWorkspace(working_dir="/tmp"), + persistence_dir=persist_path, + agent=agent, + id=conv_id, + ) + + # Add secrets to the secret registry + state.secret_registry.update_secrets( + { + "API_KEY": "test-api-key", + "DATABASE_URL": "postgresql://localhost/test", + } + ) + + # Verify secrets are set before save + env_vars = state.secret_registry.get_secrets_as_env_vars("echo $API_KEY") + assert env_vars == {"API_KEY": "test-api-key"} + + # Force save the state (triggers serialization) + state._save_base_state(state._fs) + + # Verify the serialized state has redacted secrets + base_state_path = Path(persist_path) / "base_state.json" + base_state_content = json.loads(base_state_path.read_text()) + assert "secret_registry" in base_state_content + api_key_source = base_state_content["secret_registry"]["secret_sources"][ + "API_KEY" + ] + # Value should be redacted to '**********' in serialization + assert api_key_source["value"] == "**********" + + # Now simulate restoring the conversation state from persisted data + # This was failing before the fix with: + # "pydantic_core._pydantic_core.ValidationError: Field required + # [type=missing, ... for StaticSecret.value" + resumed_state = ConversationState.create( + workspace=LocalWorkspace(working_dir="/tmp"), + persistence_dir=persist_path, + agent=agent, + id=conv_id, + ) + + # The state should load successfully - this was the bug fix + assert resumed_state.id == conv_id + + # The secrets should be None after restore (since they were redacted) + # but the StaticSecret objects should exist + assert "API_KEY" in resumed_state.secret_registry.secret_sources + assert "DATABASE_URL" in resumed_state.secret_registry.secret_sources + + # The values should be None after deserialization of redacted secrets + api_key_source_restored = resumed_state.secret_registry.secret_sources[ + "API_KEY" + ] + assert api_key_source_restored.get_value() is None + + # Getting env vars should return empty since values are None + env_vars = resumed_state.secret_registry.get_secrets_as_env_vars( + "echo $API_KEY" + ) + assert env_vars == {} # No value available + + +def test_conversation_state_secrets_with_cipher(): + """Test that secrets are preserved when using a cipher. + + When a cipher is provided to ConversationState.create(), secrets should + be encrypted during serialization and decrypted during deserialization, + preserving the actual secret values across save/restore cycles. + """ + from openhands.sdk.utils.cipher import Cipher + + with tempfile.TemporaryDirectory() as temp_dir: + llm = LLM( + model="gpt-4o-mini", api_key=SecretStr("test-key"), usage_id="test-llm" + ) + agent = Agent(llm=llm, tools=[]) + conv_id = uuid.UUID("12345678-1234-5678-9abc-1234567890aa") + persist_path = LocalConversation.get_persistence_dir(temp_dir, conv_id) + + # Create a cipher for encryption + cipher = Cipher(secret_key="my-secret-encryption-key") + + # Create conversation state with secrets AND cipher + state = ConversationState.create( + workspace=LocalWorkspace(working_dir="/tmp"), + persistence_dir=persist_path, + agent=agent, + id=conv_id, + cipher=cipher, + ) + + # Add secrets to the secret registry + state.secret_registry.update_secrets( + { + "API_KEY": "test-api-key", + "DATABASE_URL": "postgresql://localhost/test", + } + ) + + # Verify secrets are set before save + env_vars = state.secret_registry.get_secrets_as_env_vars("echo $API_KEY") + assert env_vars == {"API_KEY": "test-api-key"} + + # Force save the state (triggers serialization with encryption) + state._save_base_state(state._fs) + + # Verify the serialized state has encrypted (not redacted) secrets + base_state_path = Path(persist_path) / "base_state.json" + base_state_content = json.loads(base_state_path.read_text()) + assert "secret_registry" in base_state_content + api_key_source = base_state_content["secret_registry"]["secret_sources"][ + "API_KEY" + ] + # Value should be encrypted (not '**********') + assert api_key_source["value"] != "**********" + assert api_key_source["value"] != "test-api-key" # Not plaintext + assert len(api_key_source["value"]) > 20 # Encrypted value is longer + + # Now restore the conversation state with the same cipher + resumed_state = ConversationState.create( + workspace=LocalWorkspace(working_dir="/tmp"), + persistence_dir=persist_path, + agent=agent, + id=conv_id, + cipher=cipher, + ) + + # The state should load successfully + assert resumed_state.id == conv_id + + # The secrets should be PRESERVED after restore + assert "API_KEY" in resumed_state.secret_registry.secret_sources + assert "DATABASE_URL" in resumed_state.secret_registry.secret_sources + + # The values should be decrypted and accessible + api_key_source_restored = resumed_state.secret_registry.secret_sources[ + "API_KEY" + ] + assert api_key_source_restored.get_value() == "test-api-key" + + # Getting env vars should return the actual values + env_vars = resumed_state.secret_registry.get_secrets_as_env_vars( + "echo $API_KEY" + ) + assert env_vars == {"API_KEY": "test-api-key"} + + db_env_vars = resumed_state.secret_registry.get_secrets_as_env_vars( + "echo $DATABASE_URL" + ) + assert db_env_vars == {"DATABASE_URL": "postgresql://localhost/test"} + + +def test_conversation_state_save_with_cipher_load_without(): + """Test loading state saved with cipher but without providing cipher. + + When state is saved with a cipher (secrets encrypted) but loaded without + a cipher, the encrypted values should remain as-is (unusable) but the + conversation should still load successfully. + """ + from openhands.sdk.utils.cipher import Cipher + + with tempfile.TemporaryDirectory() as temp_dir: + llm = LLM( + model="gpt-4o-mini", api_key=SecretStr("test-key"), usage_id="test-llm" + ) + agent = Agent(llm=llm, tools=[]) + conv_id = uuid.UUID("12345678-1234-5678-9abc-1234567890bb") + persist_path = LocalConversation.get_persistence_dir(temp_dir, conv_id) + + # Create a cipher for encryption + cipher = Cipher(secret_key="my-secret-encryption-key") + + # Create conversation state with secrets AND cipher + state = ConversationState.create( + workspace=LocalWorkspace(working_dir="/tmp"), + persistence_dir=persist_path, + agent=agent, + id=conv_id, + cipher=cipher, + ) + + # Add secrets to the secret registry + state.secret_registry.update_secrets({"API_KEY": "test-api-key"}) + + # Force save the state (triggers serialization with encryption) + state._save_base_state(state._fs) + + # Now restore WITHOUT a cipher - should load but secrets are unusable + resumed_state = ConversationState.create( + workspace=LocalWorkspace(working_dir="/tmp"), + persistence_dir=persist_path, + agent=agent, + id=conv_id, + cipher=None, # No cipher provided + ) + + # The state should load successfully + assert resumed_state.id == conv_id + + # The secret source should exist but value is the encrypted string + # (not decrypted, so not usable as the original value) + assert "API_KEY" in resumed_state.secret_registry.secret_sources + api_key_value = resumed_state.secret_registry.secret_sources[ + "API_KEY" + ].get_value() + # Value should be the encrypted string, not the original + assert api_key_value != "test-api-key" + assert api_key_value is not None # It's the encrypted value + + +def test_conversation_state_save_without_cipher_load_with(): + """Test loading state saved without cipher but with cipher provided. + + When state is saved without a cipher (secrets redacted) but loaded with + a cipher, the redacted secrets should deserialize to None values. + """ + from openhands.sdk.utils.cipher import Cipher + + with tempfile.TemporaryDirectory() as temp_dir: + llm = LLM( + model="gpt-4o-mini", api_key=SecretStr("test-key"), usage_id="test-llm" + ) + agent = Agent(llm=llm, tools=[]) + conv_id = uuid.UUID("12345678-1234-5678-9abc-1234567890cc") + persist_path = LocalConversation.get_persistence_dir(temp_dir, conv_id) + + # Create conversation state with secrets but NO cipher + state = ConversationState.create( + workspace=LocalWorkspace(working_dir="/tmp"), + persistence_dir=persist_path, + agent=agent, + id=conv_id, + cipher=None, # No cipher - secrets will be redacted + ) + + # Add secrets to the secret registry + state.secret_registry.update_secrets({"API_KEY": "test-api-key"}) + + # Force save the state (triggers serialization with redaction) + state._save_base_state(state._fs) + + # Now restore WITH a cipher - should load but secrets are already lost + cipher = Cipher(secret_key="my-secret-encryption-key") + resumed_state = ConversationState.create( + workspace=LocalWorkspace(working_dir="/tmp"), + persistence_dir=persist_path, + agent=agent, + id=conv_id, + cipher=cipher, + ) + + # The state should load successfully + assert resumed_state.id == conv_id + + # The secret source should exist but value is None (was redacted) + assert "API_KEY" in resumed_state.secret_registry.secret_sources + api_key_value = resumed_state.secret_registry.secret_sources[ + "API_KEY" + ].get_value() + assert api_key_value is None + + +def test_conversation_state_cipher_mismatch(): + """Test loading state with a different cipher than used for saving. + + When state is saved with cipher A but loaded with cipher B, decryption + fails gracefully - the conversation loads but secrets are set to None + (with a warning logged). + """ + from openhands.sdk.utils.cipher import Cipher + + with tempfile.TemporaryDirectory() as temp_dir: + llm = LLM( + model="gpt-4o-mini", api_key=SecretStr("test-key"), usage_id="test-llm" + ) + agent = Agent(llm=llm, tools=[]) + conv_id = uuid.UUID("12345678-1234-5678-9abc-1234567890dd") + persist_path = LocalConversation.get_persistence_dir(temp_dir, conv_id) + + # Create cipher A for encryption + cipher_a = Cipher(secret_key="cipher-key-a") + + # Create conversation state with secrets AND cipher A + state = ConversationState.create( + workspace=LocalWorkspace(working_dir="/tmp"), + persistence_dir=persist_path, + agent=agent, + id=conv_id, + cipher=cipher_a, + ) + + # Add secrets to the secret registry + state.secret_registry.update_secrets({"API_KEY": "test-api-key"}) + + # Force save the state (triggers serialization with encryption using cipher A) + state._save_base_state(state._fs) + + # Now try to restore with cipher B - decryption fails gracefully + cipher_b = Cipher(secret_key="cipher-key-b") + + # Conversation loads but secrets are lost (set to None with warning) + resumed_state = ConversationState.create( + workspace=LocalWorkspace(working_dir="/tmp"), + persistence_dir=persist_path, + agent=agent, + id=conv_id, + cipher=cipher_b, + ) + + # The state should load successfully + assert resumed_state.id == conv_id + + # The secret source should exist but value is None (decryption failed) + assert "API_KEY" in resumed_state.secret_registry.secret_sources + api_key_value = resumed_state.secret_registry.secret_sources[ + "API_KEY" + ].get_value() + assert api_key_value is None diff --git a/tests/sdk/conversation/test_secret_source.py b/tests/sdk/conversation/test_secret_source.py index e35f9c090a..33f73d9bff 100644 --- a/tests/sdk/conversation/test_secret_source.py +++ b/tests/sdk/conversation/test_secret_source.py @@ -1,8 +1,9 @@ """Tests for SecretSources class.""" import pytest +from pydantic import SecretStr -from openhands.sdk.secret import LookupSecret +from openhands.sdk.secret import LookupSecret, StaticSecret from openhands.sdk.utils.cipher import Cipher @@ -58,3 +59,72 @@ def test_lookup_secret_serialization_encrypt(lookup_secret): dumped = lookup_secret.model_dump(mode="json", context={"cipher": cipher}) validated = LookupSecret.model_validate(dumped, context={"cipher": cipher}) assert validated == lookup_secret + + +def test_lookup_secret_deserialization_redacted_headers(): + """Test LookupSecret can be deserialized with redacted header values. + + This is a regression test for issue 1505 where LookupSecret headers with + redacted (masked) values would fail to deserialize due to assertion errors. + """ + # Simulate the serialized state with redacted headers + serialized = { + "kind": "LookupSecret", + "description": None, + "url": "https://my-oauth-service.com", + "headers": { + "authorization": "**********", # Redacted + "some-key": "**********", # Redacted + "not-sensitive": "hello there", # Not a secret header + }, + } + + # This was failing before the fix with assertion error + validated = LookupSecret.model_validate(serialized) + + # The secret headers should be stripped out since they're redacted + assert validated.url == "https://my-oauth-service.com" + # Secret headers should be removed (since their values were redacted) + assert "authorization" not in validated.headers + assert "some-key" not in validated.headers + # Non-sensitive headers should be preserved + assert validated.headers["not-sensitive"] == "hello there" + + +def test_static_secret_optional_value(): + """Test StaticSecret works with optional value (None default). + + This is a regression test for issue 1505 where StaticSecret.value was + a required field causing deserialization to fail when secrets were + redacted (converted to None). + """ + # Test with value + secret_with_value = StaticSecret(value=SecretStr("test-secret")) + assert secret_with_value.get_value() == "test-secret" + + # Test with None value (default) + secret_without_value = StaticSecret() + assert secret_without_value.value is None + assert secret_without_value.get_value() is None + + # Test deserialization with None value + serialized = {"kind": "StaticSecret", "value": None} + validated = StaticSecret.model_validate(serialized) + assert validated.value is None + assert validated.get_value() is None + + +def test_static_secret_deserialization_redacted(): + """Test StaticSecret can be deserialized from redacted value. + + This is a regression test for issue 1505. + """ + # Simulate the serialized state with redacted value + serialized = {"kind": "StaticSecret", "value": "**********"} + + # This was failing before the fix + validated = StaticSecret.model_validate(serialized) + + # The value should be None since it was redacted + assert validated.value is None + assert validated.get_value() is None