Jupyterhub: periodically check whether the logged-in user still have permission to access #402
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
component/jupyterhub
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2226/Result : failure BIRDHOUSE_DEPLOY_BRANCH : periodically-check-jupyterub-access DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-90.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1407/NOTEBOOK TEST RESULTS |
fmigneault
reviewed
Nov 11, 2023
| if os.getenv("JUPYTERHUB_CRYPT_KEY"): | ||
| c.MagpieAuthenticator.enable_auth_state = True | ||
| c.MagpieAuthenticator.refresh_pre_spawn = True | ||
| c.MagpieAuthenticator.auth_refresh_age = int("${JUPYTERHUB_AUTHENTICATOR_REFRESH_AGE}") |
There was a problem hiding this comment.
Might need some alignment with MAGPIE_COOKIE_EXPIRE.
There was a problem hiding this comment.
Can you explain why these need to be aligned?
This variable checks how often jupyterhub checks whether the magpie cookie has expired. It doesn't set the age of the jupyterhub cookie.
There was a problem hiding this comment.
In case the value is set higher here than the actual cookie age. It would give the false feedback that the cookie/session is still valid until the next refresh happens, while Magpie would actually log the user out. The refresh rate should happen at least a few times per cookie-age/refresh cycle to provide a relevant status.
There was a problem hiding this comment.
Ok, I understand.
Since the default MAGPIE_COOKIE_EXPIRE value is 24 hours and this checks every minute now. That should be more that sufficient to align the values
| # Jupyterhub will check if the current logged in user still has permission to access jupyterhub (according to Magpie) | ||
| # if their authentication information is older that this value (in seconds). This value is only applied if | ||
| # JUPYTERHUB_CRYPT_KEY is set. | ||
| export JUPYTERHUB_AUTHENTICATOR_REFRESH_AGE=300 |
There was a problem hiding this comment.
It looks like Jupyter checks its own session much more frequently (every second or so).
This doesn't need to be as often, but I think it could still be slightly decreased, like every minute.
There was a problem hiding this comment.
This is the default that jupyterhub uses (https://jupyterhub.readthedocs.io/en/latest/reference/api/auth.html#jupyterhub.auth.Authenticator.auth_refresh_age)
I'm happy to shorten it to one minute. I'll update that shortly
| @@ -64,6 +64,15 @@ export JUPYTERHUB_CONFIG_OVERRIDE="" | |||
| # recommended as it may permit unauthorized users from accessing jupyterhub. | |||
| export JUPYTERHUB_AUTHENTICATOR_AUTHORIZATION_URL='http://twitcher:8000/ows/verify/jupyterhub' | |||
|
|
|||
| # 32 byte hex-encoded key used to encrypt a user's authentication state in the juptyerhub database. | |||
| # If set, jupyterhub will periodically check if the user still has permission to access jupyterhub (according to Magpie) | |||
| export JUPYTERHUB_CRYPT_KEY= | |||
There was a problem hiding this comment.
What happens if the value is changed between restarts, causing a different state to be obtained?
Does Jupyter perform a new check with Magpie to verify if an updated value works, and update the state transparently with the new encryption key?
In other words, once this is set, must it remain the same, or we can automatically regenerate it each time?
There was a problem hiding this comment.
I wouldn't recommend updating it at every restart. Jupyterhub does not update the state transparently if it finds a new key. So if a new key is found, all currently logged in users would need to log in again.
Jupyterhub does allow us to specify multiple keys (; delimited) but I don't think that will help us since we'd still need to include the old key with the new key (https://jupyterhub.readthedocs.io/en/latest/reference/api/auth.html#jupyterhub.auth.Authenticator.enable_auth_state)
tlvu
approved these changes
Nov 13, 2023
There was a problem hiding this comment.
Approuve the spirit of this PR.
Please test for upgrade scenario of existing production deployments where multiples users are already logged in, already have their Jupyter session opened and potentially running some calculations.
Do they have to re-login or this is completely transparent to them? If they have to re-login, please note that in the CHANGES.md and I will have to notify our users before rolling this change out.
| @@ -5,7 +5,7 @@ | |||
| # are applied and must be added to the list of DELAYED_EVAL. | |||
|
|
|||
| export JUPYTERHUB_DOCKER=pavics/jupyterhub | |||
| export JUPYTERHUB_VERSION=4.0.2-20231002 | |||
| export JUPYTERHUB_VERSION=4.0.2-20231024 | |||
There was a problem hiding this comment.
I am guessing the JUPYTERHUB_VERSION will change again after Ouranosinc/jupyterhub#23 is merged an a new build is triggered on DockerHub?
There was a problem hiding this comment.
Yeah, I'll update it shortly.
Yes, it is transparent to them. This feature will only start applying once the user has logged out and logged back in again. If a user is already logged in, they will not have any authentication information added to the database yet, so the check that was added here (Ouranosinc/jupyterhub#23) will allow them to remain logged in. |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2305/Result : failure BIRDHOUSE_DEPLOY_BRANCH : periodically-check-jupyterub-access DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-154.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1435/NOTEBOOK TEST RESULTS |
Very nice to ensure transparent upgrade. Please add a note to the CHANGES.md that each existing Jupyter user have to logout and re-login for this new change to take effect. This is to ensure node admin is not falsely thinking the security enhancement is effective simply with the new deployment. For us, we probably will kill all user sessions somewhere during Xmas holidays so everyone is forced to re-login so we are sure this will take effect. One way I found to force everyone to re-login again is to Please also document in CHANGES.md one way to force everyone to re-login. |
|
Ok I've updated the CHANGES.md file with the relevant information. Please have a look to see if that makes sense |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2313/Result : failure BIRDHOUSE_DEPLOY_BRANCH : periodically-check-jupyterub-access DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-154.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1442/NOTEBOOK TEST RESULTS |
fmigneault
approved these changes
Nov 29, 2023
Good me for me, thanks. |
tlvu
approved these changes
Nov 29, 2023
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2319/Result : failure BIRDHOUSE_DEPLOY_BRANCH : periodically-check-jupyterub-access DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-154.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1448/NOTEBOOK TEST RESULTS |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
By setting the
JUPYTERHUB_CRYPT_KEYenvironment variable in theenv.localfile, jupyterhub will store user's authentication information (session cookie) in the database. This allows jupyterhub to periodically check whether the user still has permission to access jupyterhub (the session cookie is not expired and the permission have not changed).The minimum duration between checks can be set with the
JUPYTERHUB_AUTHENTICATOR_REFRESH_AGEvariable which is an integer (in seconds).Note that users who are already logged in to jupyterhub will need to log out and log in for these changes to take effect.
To forcibly log out all users currently logged in to jupyterhub you can run the following command to force the recreation of the cookie secret:
First discussed here: #358 (comment)
Changes
Non-breaking changes
Breaking changes
Related Issue / Discussion
Related to #334
Additional Information
birdhouse_daccs_configs_branch: master
birdhouse_skip_ci: false