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

Add ability to allow insecure writes #182

Merged
merged 2 commits into from Feb 10, 2020

Conversation

kevin-bates
Copy link
Member

Per a previous discussion, this change enables users to bypass the file permissions enforcement within secure_write(). This has become an issue when secure files reside on filesystems that automatically promote file permissions to o777 and prevents changes to those settings. In such cases, users can prevent enforcement by setting environment variable JUPYTER_ALLOW_INSECURE_WRITES to true (default = false).

I debated on the polarity of the env variable name. I liked JUPYTER_ENFORCE_SECURE_WRITES but felt having a variable that defaults to True was a bit odd. Also, I felt that users setting something to skip security by having to set it to True was a more explicit action.

In looking at testing this change, I couldn't find a way to get a file created such that it would exhibit the behavior of one of these filesystems. Since the change is trivial, I figured this could be a visual inspection kind of thing.

Per a previous discussion, this change enables users to bypass
the file permissions enforcement within `secure_write()`.  This
has become an issue when secure files reside on filesystems that
automatically promote file permissions to o777 and prevents
changes to those settings.  In such cases, users can prevent
enforcement by setting environment variable `JUPYTER_ALLOW_INSECURE_WRITES`
to true (default = false).
Copy link
Contributor

@MSeal MSeal left a comment

Choose a reason for hiding this comment

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

+1 for overall. Do we need to do anything similar for the nt path? I suppose we could extend the env variable usage there if we encounter cases for it in the future. I added a suggestion for printing a warning so it's visible to admins that the check isn't being made.

@@ -428,7 +432,7 @@ def secure_write(fname, binary=False):
if os.name != 'nt':
# Enforce that the file got the requested permissions before writing
file_mode = get_file_mode(fname)
if 0o0600 != file_mode:
if 0o0600 != file_mode and not allow_insecure_writes:
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking maybe we should print a warning?

Suggested change
if 0o0600 != file_mode and not allow_insecure_writes:
if allow_insecure_writes:
warnings.warn("Insecure write mode enabled for `jupyter_core.paths.secure_write` function")
elif 0o0600 != file_mode:

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about this (although I would have just logged the warning in the 'insecure' case, rather than unconditionally) but felt it would occur (essentially) at every kernel startup request on applicable systems and could be lost in the general log messages (particularly when DEBUG is enabled, which I often recommend). However, I see that warnings.warn() just logs once, but again, that could be "lost in the noise".

I do like the idea of informing the admin that this setting is in effect, more or less confirming their intention. I wonder if placing this warning message in JupyterApp.initialize(), or equivalent, might be more useful. That way it's logged before any "violation" takes place. We'd probably want a bold "WARNING" and not rely on the logging level indicator for something like this. The downside would be for those apps that use jupyter_core but bypass JupyterApp.

This message would be something like...
WARNING: Insecure writes have been enabled via environment variable 'JUPYTER_ALLOW_INSECURE_WRITES'! If this is not intended, remove the variable or set its value to 'False'.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be fine with that. Having the warning just-in-time is fine too imo. Just-in-time also catches when an application sets the environment after it loads before it runs something but that's probably not an edge case worth solving for on purpose. Up to you how you want to address it, i'd be good with merging with what's been described.

jupyter_core/paths.py Show resolved Hide resolved
jupyter_core/paths.py Show resolved Hide resolved
@kevin-bates
Copy link
Member Author

Thanks @MSeal. I went with messaging on two fronts, at app startup and first violation. Because not all users of secure_write may derive from JupyterApp, I felt the JIT warning was also necessary. Here's what a user starting Notebook will see if they've enabled the env variable:

$ jupyter notebook
WARNING: Insecure writes have been enabled via environment variable 'JUPYTER_ALLOW_INSECURE_WRITES'! If this is not intended, remove the variable or set its value to 'False'.
[I 09:08:32.470 NotebookApp] Serving notebooks from local directory: /Users/kbates/repos/oss/jupyter/jupyter_core
[I 09:08:32.470 NotebookApp] The Jupyter Notebook is running at:
[I 09:08:32.470 NotebookApp] http://localhost:8888/?token=5eb58a5d49303ac15a8b36d318d25a75edab360202072f0a
[I 09:08:32.470 NotebookApp]  or http://127.0.0.1:8888/?token=5eb58a5d49303ac15a8b36d318d25a75edab360202072f0a
[I 09:08:32.470 NotebookApp] Use Control-C to stop this server and shut down all kernels (twice to skip confirmation).
[I 09:08:32.470 NotebookApp] Welcome to Project Jupyter! Explore the various tools available and their corresponding documentation. If you are interested in contributing to the platform, please visit the communityresources section at https://jupyter.org/community.html.
[C 09:08:32.478 NotebookApp] 

I found I needed a custom format method so that the warning's location isn't prefixed to the message and I found that the output would also include the actual statement (which seems strange).

Since the same warning message is logged only once, applications deriving from JupyterApp won't see the JIT message, while applications not derived from JupyterApp will see it on first occurrence - and not after. If we wanted this to be more "in your face" we can switch to using logger.warn instead - then all violations will be posted.

Copy link
Contributor

@MSeal MSeal left a comment

Choose a reason for hiding this comment

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

LGTM!

@MSeal MSeal merged commit c980a2b into jupyter:master Feb 10, 2020
@MSeal
Copy link
Contributor

MSeal commented Feb 10, 2020

Do you want me to get a release going?

@kevin-bates
Copy link
Member Author

That would be awesome - thank you.

@kevin-bates kevin-bates deleted the allow-insecure-writes branch February 10, 2020 19:13
@MSeal
Copy link
Contributor

MSeal commented Feb 11, 2020

Released in 4.6.2

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

Successfully merging this pull request may close these issues.

None yet

2 participants