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

refuse to run as root user #1115

Merged
merged 5 commits into from Feb 22, 2016
Merged

refuse to run as root user #1115

merged 5 commits into from Feb 22, 2016

Conversation

@vivi
Copy link
Contributor

@vivi vivi commented Feb 19, 2016

Hi! I'm a undergraduate at UC Berkeley working with Matthias (@Carreau). This is to resolve #1074. Currently, running the notebook as root will cause the program to terminate, but the --allow-root flag isn't working/being recognized and I'm not sure why.

@@ -445,6 +450,10 @@ def _log_format_default(self):
help="Set the Access-Control-Allow-Credentials: true header"
)

allow_root = Bool(False, config=False,

This comment has been minimized.

@Carreau

Carreau Feb 19, 2016
Member

You just have config=False here, which make it not configurable. Just switch to config=True and it works.

@Carreau
Copy link
Member

@Carreau Carreau commented Feb 19, 2016

Do you want to add a note id docs/sources/changelog.rst ?

@willingc
Copy link
Member

@willingc willingc commented Feb 19, 2016

@Secant Welcome! Matthias is a wonderful mentor. I'm part of the Jupyter team at Cal Poly.

@Carreau beat me to the help. I did restart Travis (our continuous integration and testing service). It is green.

@Carreau
Copy link
Member

@Carreau Carreau commented Feb 19, 2016

It is green.

Yeah, we can try to make a test. Not sure it is worth though.
If we do that, we can monkey patch os.geteuid() to return always Zero. But I'm not sure it's worth testing that.

@willingc
Copy link
Member

@willingc willingc commented Feb 19, 2016

@Carreau No need for tests on this. I was just explaining what Travis was :)

@rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented Feb 20, 2016

Hey this is cool to do.

Does Windows need to be cased off?

@vivi
Copy link
Contributor Author

@vivi vivi commented Feb 20, 2016

@Carreau
Under what version in the changelog would I add this change? 4.0.10? Or a new one? :bowtie:

@takluyver
Copy link
Member

@takluyver takluyver commented Feb 20, 2016

Hi @Secant !

  • In the changelog, start a new section called 'Development' for now - we'll rename it when it actually goes into a release.
  • I think @rgbkrk is right, it needs a separate check for Windows. I guess that the geteuid function is simply not there in Windows, so it would throw AttributeError.
self.log.critical("Running as root is forbidden. Use --allow-root to bypass.")
self.exit(1)
except OSError as e:
pass

This comment has been minimized.

@takluyver

takluyver Feb 20, 2016
Member

Is there some condition that we're expecting to catch here? Why would geteuid() fail? If we're catching this, I think we should at least log the error so that there's an indication that something has gone wrong.

This comment has been minimized.

@Carreau

Carreau Feb 20, 2016
Member

No geteuid raise OSError on windows (according to the docs). So the try/catch is the dealing with windows case.

This comment has been minimized.

This comment has been minimized.

@takluyver

takluyver Feb 20, 2016
Member

@Carreau What docs are those? I rebooted into Windows to check, and it ain't there:

>>> os.geteuid()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'os' has no attribute 'geteuid'

This comment has been minimized.

@vivi

vivi Feb 20, 2016
Author Contributor

@takluyver Whoops, I read the docs too fast

Note All functions in this module raise OSError in the case of invalid or inaccessible file names and paths, or other arguments that have the correct type, but are not accepted by the operating system.

Should be excepting AttributeError, my bad!

This comment has been minimized.

@rgbkrk

rgbkrk Feb 20, 2016
Member

No problem!

is_root = os.geteuid() == 0
except AttributeError as e:
import ctypes
is_root = ctypes.windll.shell32.IsUserAnAdmin() == 1

This comment has been minimized.

@takluyver

takluyver Feb 20, 2016
Member

I wouldn't do this - admin users in Windows aren't really equivalent to root on Unix, and I don't think it makes sense to block them from running the notebook. Admin users on Windows are more like the sudoers group on Linux.

import ctypes
is_root = ctypes.windll.shell32.IsUserAnAdmin() == 1
if is_root and not self.allow_root:
self.log.critical("Running as root is forbidden. Use --allow-root to bypass.")

This comment has been minimized.

@takluyver

takluyver Feb 20, 2016
Member

I'd say something like "is not recommended" - it's not quite forbidden.

This comment has been minimized.

@vivi

vivi Feb 20, 2016
Author Contributor

Gotcha, will make the changes.

if os.geteuid() == 0:
self.log.critical("Running as root is not recommended. Use --allow-root to bypass.")
self.exit(1)
except AttributeError as e:

This comment has been minimized.

@takluyver

takluyver Feb 20, 2016
Member

I think it's probably OK here, but it's a good idea to put as little code as possible into a try block when you're catching NameError or AttributeError (any error, really, but especially these), because they can hide mistakes.

E.g. imagine if someone changed the logging level, but accidentally spelled it errorr. That's an attribute error - and the code will silently catch it and carry on as if nothing had happened, disabling the check completely.

@minrk minrk added this to the 5.0 milestone Feb 22, 2016
@minrk
Copy link
Member

@minrk minrk commented Feb 22, 2016

Thanks, @Secant, welcome to the project!

minrk added a commit that referenced this pull request Feb 22, 2016
refuse to run as root user
@minrk minrk merged commit b0fa952 into jupyter:master Feb 22, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Carreau
Copy link
Member

@Carreau Carreau commented Feb 22, 2016

Thanks for taking care of this ! Sorry I was mostly offline this Week-end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.