Allow SSH authentication via GSS. #950
Merged
Conversation
This file contains hidden or 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
|
Thanks for looking into this @Carreau. I am sort of coming back from vacation and will take a look at this... and sorry for the delay... |
akchinSTC
reviewed
Apr 16, 2021
No problem, take your time. |
|
This works great for the initial connection! When I restart a kernel, I see the following error: This makes it impossible to change kernels in that same notebooks: Even worse, this makes the gateway unusable after for future remote kernels (i.e. make a new notebook): |
|
Oops, pushed a commit that should fix the unbound local; hopefully it fix the rest, I'll read in more details. |
|
Now those errors do not appear, but restart never completes. I can change kernels and go back to the kernel again, which does cause it to start. Messages in the server output: |
|
I'm not too sure why using gss would affect the rest of the functionality. I've pushed additional changes that remove use of the globals and made them instance variables, plus added a number of debug statements, which I hope will help. |
|
This looks good to me now. I upgraded all things jupyter and now restart just works. I suspect the bug was unrelated to gss and more related to
|
atringham
reviewed
Apr 22, 2021
| ) # this should use password-less ssh | ||
| self.remote_user = os.getenv("EG_REMOTE_USER", getpass.getuser()) | ||
|
|
||
| if use_gss and (self.remote_pwd or self.remote_user): |
There was a problem hiding this comment.
As you are defaulting remote_user to getpass.getuser() then when use_gss is True it seems this will always print the warning even if neither EG_REMOTE_PWD or EG_REMOTE_USER have been set.
There was a problem hiding this comment.
i've move the logic for the warning to __init__ that might not be exactly the same but I doubt there are use case where use change the value of env variable after the object have been constructed.
That should take care of the problem.
|
Gentle nudge; I believe I have addressed all the review points, if not I'm happy to iterate more. |
akchinSTC
approved these changes
May 3, 2021
There was a problem hiding this comment.
@Carreau - I'm good with these changes. Can we also update some of the relevant documentation sections? At a quick glance
- https://github.com/jupyter/enterprise_gateway/blob/master/docs/source/getting-started-security.md#ssh-tunneling
could use a few sentences for the Kerberos uninitiated with a quick link your config_options markdown updates
|
Added on commit with docs and be stricter on bool env variable. In SSH tunneling section add information about GSS and Kerberos. Thus only allow "True", "False", empty string and unset (case Store the raw variable and its boolean interpretation separately for Also one typo |
kevin-bates
reviewed
May 6, 2021
kevin-bates
reviewed
May 6, 2021
akchinSTC
reviewed
May 14, 2021
This attempt to implement GSS as requested in jupyter-server#946. I've tried to also document the other environment variable, though I couldn't find where or how they are supposed to be used. I'm also currently trying to find a deployment that could use GSS to test this, but haven't so far. I'm assuming that if GSS is enabled then it takes priority over username/password.
The globals used to be loaded at module level, which i not the case anymore; move them to instance values.
In SSH tunneling section add information about GSS and Kerberos. I realised that this is bool env, and that we likely want to be stricter on which values are allowed, we don't want "0" to mean enable. Thus only allow "True", "False", empty string and unset (case insensitive) as possible values. Store the raw variable and its boolean interpretation separately for error messages.
Co-authored-by: Kevin Bates <[email protected]>
Co-authored-by: Kevin Bates <[email protected]>
|
Rebased on master – Is there anything else I can do to push this forward ? |
|
I believe this is good to go (thank you @Carreau) but I'd like to finalize with Alan first. @akchinSTC - is there anything remaining here? If not, please go ahead and merge. Thanks. |
|
Thanks ! |
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.
This attempt to implement GSS as requested in #946.
I've tried to also document the other environment variable, though I
couldn't find where or how they are supposed to be used.
I'm also currently trying to find a deployment that could use GSS to
test this, but haven't so far.
I'm assuming that if GSS is enabled then it takes priority over
username/password.