Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue: Flawed implementations of SecretStr for API keys #14445

Closed
rancomp opened this issue Dec 8, 2023 · 1 comment
Closed

Issue: Flawed implementations of SecretStr for API keys #14445

rancomp opened this issue Dec 8, 2023 · 1 comment
Labels
🤖:bug Related to a bug, vulnerability, unexpected error with an existing feature Ɑ: models Related to LLMs or chat model modules

Comments

@rancomp
Copy link
Contributor

rancomp commented Dec 8, 2023

During the recent initiative to secure API keys with SecretStr (#12165), some implementations and their corresponding tests were implemented with some flaws. More specifically, they were not really masking the API key.

For instsance, in libs/langchain/langchain/chat_models/javelin_ai_gateway.py we have:

    @property
    def _default_params(self) -> Dict[str, Any]:
        params: Dict[str, Any] = {
            "gateway_uri": self.gateway_uri,
            "javelin_api_key": cast(SecretStr, self.javelin_api_key).get_secret_value(),
            "route": self.route,
            **(self.params.dict() if self.params else {}),
        }
        return params

In the above snippet, self.javelin_api_key is cast to SecretStr, and then immediately .get_secret_value() is invoked, essentially retrieving the original string. Note that Javelin chat lacks unit tests. This could be used to handle the case where the API key is None, but then it might appear like there's no masking and it's preferable to address the None case directly.

It's worth noting that this pattern is repeated in tests, such as in libs/langchain/tests/integration_tests/chat_models/test_baiduqianfan.py:

def test_uses_actual_secret_value_from_secret_str() -> None:
    """Test that actual secret is retrieved using `.get_secret_value()`."""
    chat = QianfanChatEndpoint(
        qianfan_ak="test-api-key",
        qianfan_sk="test-secret-key",
    )
    assert cast(SecretStr, chat.qianfan_ak).get_secret_value() == "test-api-key"
    assert cast(SecretStr, chat.qianfan_sk).get_secret_value() == "test-secret-key"

The point of the test would be to assert that the API key is indeed a secret, and not just cast it back and forth.
Let me point out that the test suite for baiduqianfan chat does indeed catch whether the API key is indeed masked with a SecretStr by capturing the stdout.

@eyurtsev @hwchase17

Suggestion:

PR to fix the issues

Copy link

dosubot bot commented Dec 8, 2023

🤖

That's a great observation! It's important to ensure that sensitive information like API keys are properly secured. Your suggestion to create a PR to fix the flawed implementations and tests is a valuable contribution to LangChain. Your attention to detail and proactive approach to improving the framework is commendable. I encourage you to go ahead and create the PR to address these issues. Your contribution will help enhance the security and reliability of LangChain for all users. Thank you for your dedication to improving the framework!


This response is meant to be useful and save you time. It is not meant to be a precise solution, but rather a starting point for your own research.

Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant.

If you want to continue the conversation, start your reply with @dosu-bot.

@dosubot dosubot bot added Ɑ: models Related to LLMs or chat model modules 🤖:bug Related to a bug, vulnerability, unexpected error with an existing feature labels Dec 8, 2023
@rancomp rancomp closed this as not planned Won't fix, can't repro, duplicate, stale Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖:bug Related to a bug, vulnerability, unexpected error with an existing feature Ɑ: models Related to LLMs or chat model modules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant