Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support running .nbgitpuller.script.{init,update} on start #291

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jimdigriz
Copy link

@jimdigriz jimdigriz commented Feb 22, 2023

Proposal for fixing #122

.nbgitpuller.script.{init,update} placed at the top of the repo contains a list of shell commands to run when the project is checked out or refreshed respectively.

Let me know what you think, and if this has legs I can add some spit, polish and documentation to this.

@welcome
Copy link

welcome bot commented Feb 22, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@yuvipanda
Copy link
Contributor

Ah, this is an interesting idea with fun security implications (see autorun.inf) :D I would say we can probably do this in the following cases:

  1. It should be off by default, and the jupyterhub admin would need to turn it on (perhaps via traitlet / env var)
  2. The person making the link should also opt-in, with an extra param required in the URL
  3. Display a clear message that this is executing in the progress UI
  4. Don't make it a hidden file - I think it's important for end users to know this is happening too.

I think under these conditions, I would be thrilled to add this functionality!

@jimdigriz
Copy link
Author

Sounds reasonable, I'll take a stab at this over the next few weeks when I find some time and get back to you. Thanks for the fast feedback and suggestions!

@jimdigriz
Copy link
Author

jimdigriz commented Mar 21, 2023

  1. Don't make it a hidden file - I think it's important for end users to know this is happening too.

@yuvipanda Would you be willing to drop this requirement?

Asking as (for us) it is also important to not litter the project with files students do not need to interact with or be aware of; we already have .hidden directory containing various utilities that are run by our notebooks in our git projects.

In the case of a bad actor, this countermeasure would be trivially neutered with an rm "$0"; git --rewrite-history ... at the top of the script to hide themselves so not sure it actually adds any real value.

For visibility I think this would be covered by (3).

What do you think?

Just negotiating here, as if I don't ask... :)

@yuvipanda
Copy link
Contributor

I think I'd like the default to not be hidden, but can be allowed to be hidden with a server-side setting via a traitlet. That way, admins can opt-in to it being hidden, but by default it's visible. I think AUTORUN.INF and similar things on Windows have scarred me enough that I don't want automatic code execution without full visibility :)

@jimdigriz
Copy link
Author

I think I'd like the default to not be hidden, but can be allowed to be hidden with a server-side setting via a traitlet.

Works for Me(tm)

Thanks

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Hey!

I'm triaging some PRs pending a release and see that there are review requests in #291 (comment) not yet implemented.


My take is that this can be a useful feature, but I agree with Yuvi that this must be very secure to be accepted.

I figure nbgitpuller is not jupyterhub aware, only the link generator is. So how could the jupyterhub admin disable this by default or similar? Also if the jupyterhub admin would enable this, how would users know its enabled - are we relying on the jupyterhub admin to successfully communicate this to all users?

I think I lean towards not wanting to accept a feature like this unless the individual user that has clicked the link and pulled content is prompted to do something after pulling. The complexity of supporting that may not be something I think is in scope for nbgitpuller as a project, depending on how its proposed to be accomplished.

@yuvipanda do you think my proposed security requirement to have a user confirm they want to execute something after they have clicked a link is too strict? And if not, do you think it is in scope to pursue for this project that needs to maintain such functionality long term if added?

@jimdigriz
Copy link
Author

jimdigriz commented Aug 1, 2023

Trying to understand the actual risk here. I acutely understand the 'security' problem and the proposed solutions that could be applied to mitigate this, but struggling to understand how this ultimately applies to the environments it will be deployed to.

Understand, I am outlining my assumptions here and my reasoning, not dictating this is how this should be.

My understanding for the majority of JupyterHub deployments the user is not the administrator of the deployment. The user is choosing to use the service offered in the same way they choose to use an educational institution or company provided workstation. The user is not the decision maker on what runs on those environments. Right?

Should we also be considering extending these proposals to users attempting to run pip install ...?

My aim is to provide an option for a course author to use nbgitpuller for their deployment of JupyterHub where they need a bootstrapping script to be executed to get things working. For my platform, we have tried clear instructions, but for some reason people still keep tripping up and making a mess of cut'n'pasting. The only solution I see here is that the service solves this for them...somehow.

I find it hard to believe that a user is going to be generating nbgitpuller links and passing them around when they can also ask people to type git clone .... A bad actor could send a email with the link in it, but what is at risk here? The users home directory on a learning platform? Unencrypted SSH/GPG private keys? A map showing the location of the loot?

I think @yuvipanda's suggestion of placing this behind an administrator flag (disabled by default) is prudent. Asking the user to make a choice on a platform as a guest is peculiar, especially when those same users struggle with "please cut'n'paste verbatim...".

Did no one else suffer Windows 7 and those security popups? :)

Maybe what is needed here is to make the administrative flag behave more like c.Authenticator.username_pattern but holding a list of URI regexp's on when to allow use of the bootstrapping script otherwise it just fetches the repo as usual.

@jimdigriz jimdigriz force-pushed the script branch 4 times, most recently from b99e625 to bc09520 Compare April 7, 2024 16:41
@yuvipanda
Copy link
Contributor

I don't think it's necessary for the end user to make the choice here. In #291 (comment) I described this as being behind a traitlet flag, that can be enabled by JupyterHub admin (or end user, if they're using it without JupyterHub). I think that's the right place for this security setup. It should be opt-in as well.

So I'm happy to accept this with the following security requirements:

  1. It's disabled by default
  2. Requires explicit action by someone who can modify jupyter server config (so JupyterHub admin or end user) to enable this feature

disabled by default, and configurable via trailets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants