Persist jwt_secret in config file

[tofarr]

This PR modifies the jwt_secret field in AppConfig to persist the value in a file:

• Moves jwt_secret generation to a module-level function
• Stores the secret in a .jwt_secret file in the config directory
• Reuses the existing secret if available, generates new one if not
• Improves code organization by moving the function outside the class

This is follow on work from https://github.com/All-Hands-AI/OpenHands/pull/5305

---

To run this PR locally, use the following command:

docker run -it --rm   -p 3000:3000   -v /var/run/docker.sock:/var/run/docker.sock   --add-host host.docker.internal:host-gateway   -e SANDBOX_RUNTIME_CONTAINER_IMAGE=docker.all-hands.dev/all-hands-ai/runtime:f4b7a2a-nikolaik   --name openhands-app-f4b7a2a   docker.all-hands.dev/all-hands-ai/openhands:f4b7a2a

[tofarr]

Moving this back to draft until I can test.

[enyst]

Allow me to explain the reason for holding it a little bit. I hope it's just, like, delayed for a bit: to give people time to see it and give feedback.

Personally, I agree with this solution, I mentioned it in the previous PR around this issue; it's just so obvious, I'd love to just do this, if everyone is on the same page.

This is a long-standing issue, and this solution has been suggested by tobitege and others for a long time. (https://github.com/All-Hands-AI/OpenHands/issues/2276#issuecomment-2151502351)

Mamoodi is just working on documenting our current behavior, and he may want to know we're just about to change it. Moreover, @mamoodi you may want to take a look at the issue linked in the OP, where Tim explained very well what's going on here.

I think we should solve it as Tim suggests. I'd love to give @rbren time for coffee, too, and see if he's okay with this.

(I recently gave rbren time for coffee on another too-obvious-how-come issue, and it proved a good idea because he came up with something I hadn't considered. Hey coffee ☕️ is always a good ideal!)

[amanape]

Not much I can comment here but I do agree with this change as it makes sense to me

[rbren]

I do think this is the right change, but agree w/ enyst that it won't fix it for docker run

This seems worth getting in as-is, but I think it's still an open issue for most users

[diwu-sf]

I think we should use a solution that works for both docker, and for non-docker, and it shouldn't try to persist the secret to a file.

Prefer that we do this instead:

from uuid import getnode as get_mac
mac = get_mac()

I tested this on local docker and the docker containers will have the same node id. It does NOT work for Kubernetes, but that's fine since in production, these JWT secrets should come from vault secrets.

[rbren]

That method seems a bit unsafe to me, since the mac isn't a secret.

I'm being a little bit of a pain about this one (we could also just hard-code a default like abcd) because those kind of insecure-by-default options can cause serious pain.

[diwu-sf]

This persisted file wouldn't be for production deployment anyways, so it's just to ease local development instances. In our deployments, the JWT secret come from kubernetes secret and just get passed in via env.

So I don't think the safety should be primary design objective here, it feels like we just want convenience for the local dev instances, and get some persistent randomization beyond using the same defaults on every computer.

[enyst]

Great points, @diwu-sf! I think you hit on exactly the problem: this is for local users, but we're also judging it in some way by the criteria of remote/shared/enterprise.

I do think that the local users should be able to trust that the app won't lose data or state by design, and waste their time. They should be able to do work that matters to them. Hey, not just toy projects.

That implies that they can hit, for example, max iterations or budget or any error, change it, restart, and just be back to what they were doing. I do think we should have sane defaults for that.

This PR's solution goes some way towards it, but only for development mode. Your solution clearly covers all, if we can live with it. (and it works with WSL?)

[diwu-sf]

I dont have a windows machine to test WSL but from reading the WSL architecture, its a VM underneath so that VM should have consistent getnode output.

[enyst]

This persisted file wouldn't be for production deployment anyways

Yes, on the other hand the fact that it wouldn't work (one may hope it's read only!) also means those folks have to RFTM.

Let's move on with this solution, we now have a new issue for docker run.

[tofarr]

So I updated this PR with the suggestions @rbren made, and tested locally and with the docker run -it command.

There is one additional change: In order to get this working with the the file_store, I had to change the default implementation of the file store from memory to local. (So that files are persisted between restarts)

Does anybody foresee this causing problems?

[rbren]

@tofarr looks like we need to update the readme?

[enyst]

There is one additional change: In order to get this working with the the file_store, I had to change the default implementation of the file store from memory to local. (So that files are persisted between restarts)

This will also save the event stream I think. It's a nice change IMO.