Bugfix for issue 1566.

[emin63]

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.

[minrk]

Thanks for suggesting a fix! Looking at the context where you suggested changes, I think maybe there's an easier fix with what we already have.

[minrk]

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

[emin63]

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).

[minrk]

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.

[emin63]

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.

[minrk]

Sorry, I should have been clearer.

In previous versions, the configuration for the password was ServerApp.password. ServerApp.password is taken into account when computing the cookie secret here, so changing ServerApp.password changes the cookie secret, and thus revokes all cookies.

The problem comes in a refactor in Jupyter Server 2, where password handling was moved from ServerApp to the IdentityProvider, which deprecated the ServerApp.password config in favor of a new PasswordIdentityProvider.hashed_password, which means the same thing. But PasswordIdentityProvider.hashed_password is not taken into account for the cookie secret, which means that changing the password no longer affects the accepted cookies, but changing the deprecated ServerApp.password config can still be used as a workaround.

The simple fix is to change self.password to self.identity_provider.hashed_password here, and that would be it. But this isn't quite enough because not all IdentityProvider implementations have that attribute. The more general fix needs to give IdentityProvider implementations a hook to process the

I think it would be reasonable to accept the PR and maybe raise alternative fixes as another fix.

Unfortunately, I think the approach in this PR introduces potential issues related to filesystem timestamps that may cause complications e.g. in remote filesystems and container environments where file modification times can be wonky, unlike a local system. So I don't think this is a simple straightforward improvement to take and then move on.

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

Yes, absolutely.

Since including the password in the hash is a pattern already in place that already solves #1566, and the problem is that the current storage of the password is not properly taken into account, I would like to remove the timestamping and only fix this to take the correct password into account. As I suggested above, I think a cookie_secret_hook method on IdentityProvider, where the default implementation in PasswordIdentityProvider is just what we do now, but with self.hashed_password. Then ServerApp calls this new hook where it is currently consuming self.password:

class IdentityProvider:
    ...
    def cookie_secret_hook(self, h: hashlib._Hash) -> hashlib._Hash:
        """Update cookie secret input

        Subclasses may call `h.update()` with any credentials that,
when changed, should invalidate existing cookies,
        such as a password.

        The updated hashlib object should be returned.
        """
        return h

class PasswordIdentityProvider:
    ...    
    def cookie_secret_hook(self, h: hashlib._Hash) -> hashlib._Hash:
        """
        Changing the password invalidates cookies.
        """
        h.update(self.hashed_password)
        return h

Would you like to take a stab at that? If not, I can give it a go.

[emin63]

OK, I will implement as you suggest. When it's ready I will open a new PR and close this one.