Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions jupyter_server/auth/identity.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import os
import re
import sys
import time
import typing as t
import uuid
from dataclasses import asdict, dataclass
Expand Down Expand Up @@ -350,13 +351,28 @@ def user_to_cookie(self, user: User) -> str:
"display_name": user.display_name,
"initials": user.initials,
"color": user.color,
"cookie_creation_time": time.time(),
}
)
return cookie

def user_from_cookie(self, cookie_value: str) -> User | None:
"""Inverse of user_to_cookie"""
user = json.loads(cookie_value)
cookie_creation_time = user.get("cookie_creation_time", None)

if not cookie_creation_time:
self.clear_login_cookie()
raise ValueError("No cookie_creation_time in cookie; must recreate cookie")
secret_creation_time = self.parent._cookie_secret_creation_time
if not secret_creation_time:
raise ValueError("Secret creation time not set")
if cookie_creation_time < secret_creation_time:
raise ValueError(
f"Stale cookie created at {cookie_creation_time};"
f" but server secret created {secret_creation_time}"
)

return User(
user["username"],
user["name"],
Expand Down Expand Up @@ -448,6 +464,7 @@ def get_user_cookie(
)
if not _user_cookie:
return None

user_cookie = _user_cookie.decode()
# TODO: try/catch in case of change in config?
try:
Expand Down Expand Up @@ -722,6 +739,8 @@ def process_login_form(self, handler: web.RequestHandler) -> User | None:
config_file = os.path.join(config_dir, "jupyter_server_config.json")
self.hashed_password = set_password(new_password, config_file=config_file)
self.log.info(_i18n("Wrote hashed password to {file}").format(file=config_file))
self.parent._write_cookie_secret_file(self.parent.cookie_secret)
self.log.info(_i18n("Touched cookie secret file to update server secret time"))

return user

Expand Down
1 change: 0 additions & 1 deletion jupyter_server/auth/login.py
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,6 @@ def get_user(cls, handler):
# because that can erroneously log you out (see gh-3365)
if handler.get_cookie(handler.cookie_name) is not None:
handler.log.warning("Clearing invalid/expired login cookie %s", handler.cookie_name)
handler.clear_login_cookie()
if not handler.login_available:
# Completely insecure! No authentication at all.
# No need to warn here, though; validate_security will have already done that.
Expand Down
13 changes: 13 additions & 0 deletions jupyter_server/serverapp.py
Original file line number Diff line number Diff line change
Expand Up @@ -610,7 +610,11 @@ def start(self):
"""Start the password app."""
from jupyter_server.auth.security import set_password

if self.parent is None:
raise ValueError("Unable to change password without parent app")
set_password(config_file=self.config_file)
self.parent._write_cookie_secret_file(self.parent.cookie_secret)
self.log.info(_i18n("Touched cookie secret file to update server secret time"))
self.log.info("Wrote hashed password to %s" % self.config_file)


Expand Down Expand Up @@ -1159,6 +1163,13 @@ def _default_cookie_secret_file(self) -> str:
""",
)

# If the server side cookie secret is changed/created then we should
# not trust cookies created before that. We can use this as a way
# to invalidate old cookies when a password is changed by rewriting
# the cookie secret again (without necessarily changing it).
# See https://github.com/jupyter-server/jupyter_server/issues/1566
_cookie_secret_creation_time = 0

@default("cookie_secret")
def _default_cookie_secret(self) -> bytes:
if os.path.exists(self.cookie_secret_file):
Expand All @@ -1167,6 +1178,7 @@ def _default_cookie_secret(self) -> bytes:
else:
key = encodebytes(os.urandom(32))
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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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_hook or 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:

  1. 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.
  2. 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.
  3. 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_file to 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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

I'm not sure if you're just providing background or suggesting a change to the PR. I don't feel comfortable interacting with a deprecated value as a fix and don't know enough about the code to know where to fix otherwise.

The current PR does fix what is a significant security hole. I think it would be reasonable to accept the PR and maybe raise alternative fixes as another fix.

Otherwise, I need more explicit feedback if there is something you want me to change.

Thanks.

return h.digest()
Expand All @@ -1183,6 +1195,7 @@ def _write_cookie_secret_file(self, secret: bytes) -> None:
self.cookie_secret_file,
e,
)
self._cookie_secret_creation_time = os.stat(self.cookie_secret_file).st_mtime

_token_set = False

Expand Down
Loading