Use SecretStr type for api key #5939
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5939 +/- ##
==========================================
+ Coverage 75.54% 75.77% +0.22%
==========================================
Files 191 191
Lines 13100 13100
==========================================
+ Hits 9897 9926 +29
+ Misses 3203 3174 -29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
To prevent accidental export of API keys
| assert serialized_config | ||
| assert "sk-password" not in serialized_config | ||
| client2 = AnthropicChatCompletionClient.load_component(config) | ||
| assert client2 |
There was a problem hiding this comment.
Hold on, did you assert that api_key==sk-password. How would the model know how to load it back?
There was a problem hiding this comment.
The intention is to prevent the key from exported and loaded back directly without user specifying the API key on the config first.
There was a problem hiding this comment.
That makes sense in theory, but what about autogenstudio if a user specifies a key it will be lost as soon as it's saved.
There was a problem hiding this comment.
Ok .. let us step back for a second.
- Core issue that his PR addresses is that whenever
dump_component()is called, fields like keys (for some model clients) get dumped too. This can lead to key security issues. - This PR, makes those field Secrets and hence they are not dumped.
The side effect is that anyone who is loading a dumped component (with secrets) will get an error now when they load the component (secrets no longer available).
by @EItanya For the meantime I think we need to revert until a better solution is reached, unless we just tell everyone they have to use the OPENAI_API_KEY env var
A potential middle ground is that we parameterize dump_component(secrets=true) and it is false by default. If people really want to dump their secrets, they set it to true. WDYT @EItanya @ekzhu ?
This seems to be slightly different from the error in #5944 (it seems somwhere down the stack we will need to convert SecretStr to str before giving it to the model client). I am still investigating.
Edit: fix for #5944 is in #5939
There was a problem hiding this comment.
Having a parameter secrets=True is a good middle ground. Right now, it is a security issue that we need to fix for the default case.
There was a problem hiding this comment.
I totally understand where the requirement is coming from.
I agree that in the default case this will be totally fine because the model is loaded in from some datastore, run, and thrown away.
But I still think there are questions that need to be answered here. Let's say a GroupChat is paused, so save_state is called, how will it pick back up? You're saying in that instance we pass secrets=true so that it can be picked back up?
Checkpointing is one of the coolest potential features of Autogen so just wanna make sure we account for it.
There was a problem hiding this comment.
Just to throw this in to the mix, I feel like secrets should be able to be specified externally and "referenced" in a config. For example in home assistant you can use the !secret directive to load a secret https://www.home-assistant.io/docs/configuration/secrets/
|
I think this was prematurely merged, I thought about doing this as well, but realized it was a problem for loading components |
Perhaps we should make it a parameter? |
Secrets in config are always difficult, but they still need to be able to be loaded. I've seen other projects create For the meantime I think we need to revert until a better solution is reached, unless we just tell everyone they have to use the |
To prevent accidental export of API keys