Conversation
|
|
||
| newSecret, err := randomHex(32) | ||
| // delete the JWT secret so it will be re-generated on next unlock (to log all sessions out on password change) | ||
| err = tx.Where(&db.UserConfig{Key: "JWTSecret"}).Delete(&db.UserConfig{}).Error |
There was a problem hiding this comment.
we currently create the secret in the Unlock function, don't we? so it's a bit of a side effect and not explicit and easy noticeable.
is the deletion here enough? or maybe could it be RegenerateJWTSecret() or something?
There was a problem hiding this comment.
I understand there is a side effect, but I think this is inevitable because now the JWT secret is stored decrypted in memory - and that is a side effect that we have to keep in mind. Also this code is inside a DB transaction, so the code for generating a new JWT secret had to be duplicated and slightly different than the other generation code.
I covered this quite well with tests I think. Do you think it's ok as-is?
| // NOTE: the config is also unlocked as part of the start | ||
| // goroutine below. But since we execute start asynchronously it's hard to | ||
| // know when the config has been unlocked before being able to create the JWT token | ||
| err := httpSvc.cfg.Unlock(startRequest.UnlockPassword) |
There was a problem hiding this comment.
so it runs twice potentially in parallel?
There was a problem hiding this comment.
No, it's not in parallel - this first call here is synchronous
There was a problem hiding this comment.
Actually it doesn't make sense to call it in start. I can just call it here since it's only needed for the HTTP mode. So maybe the name of this function config.Unlock should also be renamed to be UnlockJWTSecret)
There was a problem hiding this comment.
Update: this doesn't work so well with auto-unlock. I will leave this as a follow-up improvement
2 participants
Fixes #1986
NOTES
TODOs:
Tests