Bugfix for issue 1566. #1567
Bugfix for issue 1566. #1567
Conversation
This commit provides a fix for the bug in issue jupyter-server#1566. Basically we keep track of when the server side cookie secret was generated and only trust cookies created after that point. When a password change occurs, we touch the cookie secret file as a way of invalidating cookies created before the password change. We could also generate a new cookie secret which might be better but I am not familiar enough with the code to know the implications. This patch seems like the minimal way to get the bug fixed.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
| self._write_cookie_secret_file(key) | ||
| self._cookie_secret_creation_time = os.stat(self.cookie_secret_file).st_mtime | ||
| h = hmac.new(key, digestmod=hashlib.sha256) | ||
| h.update(self.password.encode()) |
There was a problem hiding this comment.
I see that changing the password actually is taken into account in the cookie secret here, so changing the password should invalidate all cookies. But this wasn't updated with the move to IdentityProvider.hashed_password in Jupyter Server 2.
Maybe what we should add rather than timestamping the cookie secret, which can have clock issues, is a cookie_secret_hook or something on the IdentityProvider, to take into account? And the PasswordIdentityProvider should implement this to call update(self.hashed_password.encode()?
There was a problem hiding this comment.
I see that changing the password actually is taken into account in the cookie secret here, so changing the password should invalidate all cookies. But this wasn't updated with the move to
IdentityProvider.hashed_passwordin Jupyter Server 2.
I agree that it looks like the password should influence the cookie secret. But when I tested, changing the password did NOT invalidate all cookies even after a restart. So I'm not sure what's going on here or where that password in the code you reference comes from. I don't see a good reason to try to use the password in the cookie secret. In principle, these are difference concepts but I defer to people who understand the code better.
Maybe what we should add rather than timestamping the cookie secret, which can have clock issues, is a
cookie_secret_hookor something on the IdentityProvider, to take into account?
Yes, timestamping the cookie is not the best approach. Note that in principle, if the password changes or the cookie secret changes then no cookies generated before that time should be considered valid. I believe time.time() and os.stat are using the same clock so not sure what issues that might generate but I'm not an expert on clocks.
If you're OK with changing the cookie secret, we could just create a method to generate a new cookie secret, write it to disk, and update the cookie secret in the running server. That would be better, but I don't know the code well enough to know if that might break other things. For example, if anything is encrypted with the cookie secret, the server may lose the ability to read such encrypted data.
I'm happy to make the changes to write a new cookie secret on password change if you want.
And the PasswordIdentityProvider should implement this to call
update(self.hashed_password.encode()?
I'm not familiar enough with the code to understand what you mean in the above. For example, I'm not sure what you're updating and why. Are you saying update the hmac for the cookie secret based on the new password? I don't think that by itself will solve the problem.
If you don't like the cookie creation time tracking, I think an alternative would be:
- Make a method to generate a cookie secret (e.g., called
generate_cookie_secret). It could use the password or something from the config or just something random. - Create a method to call to change the cookie secret on the running server (e.g., called
set_cookie_secret). Someone who knows the code better than I should confirm that its OK to change the secret on the running server. - When the password is changed, call the method in step 1 to generate a new cookie secret and then call the method in step 2 to update the cookie secret for the currently running server, and then call the existing
_write_cookie_secret_fileto write the secret to disk.
More knowledgeable developers could then debate/refine the details of exactly how to generate the cookie secret (e.g., don't bother including the password since it may not exist or use it because maybe it adds more randomness or whatever).
There was a problem hiding this comment.
But when I tested, changing the password did NOT invalidate all cookies even after a restart.
Yes, that's what I was referring to with the switch to IdentityProvider. The issue is ServerApp.password, which is what's consumed here, is not how you set the password, it's IdentityProvider.hashed_password. If you use the deprecated ServerApp.password config, cookies are absolutely invalidated when this value changes, but when you change the correct config (IdentityProvider.hashed_password), it doesn't.
2 participants
This commit provides a fix for the bug in issue #1566. Basically we keep track of when the server side cookie secret was generated and only trust cookies created after that point.
When a password change occurs, we touch the cookie secret file as a way of invalidating cookies created before the password change.
We could also generate a new cookie secret which might be better but I am not familiar enough with the code to know the implications. For example, I don't know if the server caches the cookie secret in ways that might not get updated without a server restart. Also, changing the cookie secret might make it impossible to read other things which are encrypted with that secret. If someone more knowledgeable with the code thinks it is OK, changing the cookie secret would be safer and preferable (please advise). This patch seems like the minimal risk way to get the bug fixed.