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

Check and adjust permissions before kernel spec is written #535

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
32 changes: 23 additions & 9 deletions ipykernel/kernelspec.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import shutil
import sys
import tempfile
import warnings

from jupyter_client.kernelspec import KernelSpecManager

Expand Down Expand Up @@ -61,35 +62,48 @@ def get_kernel_dict(extra_arguments=None):

def write_kernel_spec(path=None, overrides=None, extra_arguments=None):
"""Write a kernel spec directory to `path`

If `path` is not specified, a temporary directory is created.
If `overrides` is given, the kernelspec JSON is updated before writing.

if 'resources' is given, copies resources from non-standard location.

Copy link
Member

Choose a reason for hiding this comment

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

this should be removed, since resources were added, but then removed as a parameter to write_kernel_spec in this PR.

The path to the kernelspec is always returned.
"""
if path is None:
path = os.path.join(tempfile.mkdtemp(suffix='_kernels'), KERNEL_NAME)

# stage resources
shutil.copytree(RESOURCES, path)

# change permission if resources directory is not read-write able
if not os.access(path, os.W_OK | os.R_OK):
warnings.warn(
UserWarning("resources is not writable, adjusting permissions before creating kernel")
)
# changes permissions only for owner, do not touch group/other
os.chmod(path, os.stat(path).st_mode | 0o700)
for f in os.listdir(path):
file_path = os.path.join(path, f)
os.chmod(file_path, os.stat(file_path).st_mode | 0o600)

Copy link
Member

Choose a reason for hiding this comment

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

I believe the relevant piece of this code has already been merged in #593 (and unfortunately earlier in #377, which was overlooked). I think that we don't need the directory to have the execute bit set, only have it be writeable. We don't need all of the files to be writeable - since those are icons - they just need to be readable - which gets taken care of by copytree just above this section, since if they weren't readable from the RESOURCES directory, that would have failed.

I might have overlooked something subtle, though, but either way this would need to be rebased on master, as it conflicts with what ended up making it to ipykernel 5.5.0 in #593.

# write kernel.json
kernel_dict = get_kernel_dict(extra_arguments)

if overrides:
kernel_dict.update(overrides)
with open(pjoin(path, 'kernel.json'), 'w') as f:
json.dump(kernel_dict, f, indent=1)

return path


def install(kernel_spec_manager=None, user=False, kernel_name=KERNEL_NAME, display_name=None,
prefix=None, profile=None):
"""Install the IPython kernelspec for Jupyter

Parameters
----------

kernel_spec_manager: KernelSpecManager [optional]
A KernelSpecManager to use for installation.
If none provided, a default instance will be created.
Expand All @@ -108,7 +122,7 @@ def install(kernel_spec_manager=None, user=False, kernel_name=KERNEL_NAME, displ

Returns
-------

The path where the kernelspec was installed.
"""
if kernel_spec_manager is None:
Expand Down Expand Up @@ -143,12 +157,12 @@ def install(kernel_spec_manager=None, user=False, kernel_name=KERNEL_NAME, displ
class InstallIPythonKernelSpecApp(Application):
"""Dummy app wrapping argparse"""
name = 'ipython-kernel-install'

def initialize(self, argv=None):
if argv is None:
argv = sys.argv[1:]
self.argv = argv

def start(self):
import argparse
parser = argparse.ArgumentParser(prog=self.name,
Expand Down
31 changes: 31 additions & 0 deletions ipykernel/tests/test_kernelspec.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
except ImportError:
import mock # py2

import pytest

from jupyter_core.paths import jupyter_data_dir

from ipykernel.kernelspec import (
Expand Down Expand Up @@ -87,6 +89,35 @@ def test_write_kernel_spec_path():
assert_is_spec(path)
shutil.rmtree(path)

@pytest.mark.skipif(sys.platform == 'win32',
reason="does not run on windows")
def test_write_kernel_spec_permissions():
read_only_resources = os.path.join(tempfile.mkdtemp(), "_RESOURCES")
shutil.copytree(RESOURCES, read_only_resources)

# create copy of `RESOURCES` with no write permissions
os.chmod(read_only_resources, 0o500)
for f in os.listdir(read_only_resources):
os.chmod(os.path.join(read_only_resources, f), 0o400)

with mock.patch('ipykernel.kernelspec.RESOURCES', read_only_resources):
with pytest.warns(UserWarning):
path = write_kernel_spec()

assert_is_spec(path)

# ensure permissions actually restricted
# not sure how to run these as `RESOURCES` is directly imported from
# `ipykernel.kernelspec` so it can't be mocked
# print(RESOURCES, oct(os.stat(RESOURCES).st_mode & 0o777))
# assert os.stat(RESOURCES).st_mode & 0o777 == 0o500

# ensure permissions are not loosened too much, original permission was
# 0o500, so the 'fixed' one should be 0o700, still no rw for group/other
assert os.stat(path).st_mode & 0o77 == 0o00

shutil.rmtree(path)


def test_install_kernelspec():

Expand Down