-
Notifications
You must be signed in to change notification settings - Fork 109
Fixes for Secret Storage #855
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
Conversation
| payload = self.model_dump_json(exclude_none=True) | ||
| payload = self.model_dump_json( | ||
| exclude_none=True, context={"expose_secrets": True} | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we storing secrets on disk?
This file is in every conversation afaik, so that's a lot of places if I see this right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One use case is when you pause / resume docker containers / K8 sandboxes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Or even just the regular non sandboxed agent server - storing on disk allows you to resume the conversation after a restart)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we resume, could we send it secrets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the fully local case, we are assuming right now that we need to read secrets from env / pass as arguments in python.
Which is kinda possible, kinda annoying at times.
I actually do think we could store on disk, but the problem with dumping base .json is that it's doing it repeatedly everywhere, in all conversations, and I don't think the user expects that. It's very insecure.
One thing that may help is:
We can then store, ok, but once. It's in ~/.openhands/llm-profiles by default, and that's not unusual. It's not awesome, but I do think it's expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could revert the base.json for now - this doesn't include everything required in order to restart a conversation anyway - the agent server uses the meta.json to save / restore state and it is only written when there is a graceful shutdown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meta is also per conversation, so it keeps multiple copies. But at least it's only for users using the server. 🤔
Could you please help me see why we need to eliminate the other alternative, why can't we act at restore time in a similar way we do at start, when we do send over the wire the secrets? What am I missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case, meta.json should work for now, since I'm sure it's urgent, and at least I think it's better than another alternative (the 4th?): some strange "if not local" code. 😓
I do think we will need to give this some thought...
Coverage Report •
|
||||||||||||||||||||
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
|
I am closing this PR in favor of #893 |
Serialization to meta.json was not storing secrets correctly - now it is.
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
golang:1.21-bookwormeclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22Pull (multi-arch manifest)
Run
All tags pushed for this build
The
bb7ff89tag is a multi-arch manifest (amd64/arm64); your client pulls the right arch automatically.