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 path_prefix_from_file option, adding security by obscurity. This … #5541

Closed
wants to merge 2 commits into from

Conversation

joveh
Copy link

@joveh joveh commented Jan 31, 2022

…option is helpful on multi-user systems.

We noticed open tensorboards on our shared systems

  • Technical description of changes

  • Screenshots of UI changes

  • Detailed steps to verify changes work correctly (as executed by you)
    path_prefix_from_file is an alternativ to path_prefix, if both are given path_prefix will be used

  • Alternate designs / implementations considered
    It would be nice to have a config-file implementation with all options for tensorboard, mybe using json or yaml format

Copy link

@tz-rrze tz-rrze left a comment

Choose a reason for hiding this comment

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

A simple but really helpful addition to make TensorBoard usable on a shared multi-user system without severe security concerns! Solves a long standing security issue.

@tz-rrze
Copy link

tz-rrze commented Feb 6, 2022

@yoshipon: you correctly mentioned in #267 (comment) on a suggestion form @jdhayes that --path_prefix itself is still insecure on a multi-user system; I hope you like the new approach in this PR to read the hash from a file :-)

@ericdnielsen
Copy link
Contributor

We believe the comment #267 (comment) is still governing on changes/features requests along these lines.

@tz-rrze
Copy link

tz-rrze commented Feb 9, 2022

We believe the comment #267 (comment) is still governing on changes/features requests along these lines.

you cannot easily start NGiNX on a multi-user HPC system - can you? And even if you can it does not solve the problem at akk as NGINX and Tensorboard are still running on multiuser system exposing Tensorboard at least locally

@mahendrapaipuri
Copy link

We are facing a similar issue at our HPC center. This is already a good step forward to run TensorBoard on multi-user environment. Is it possible to use the approach of JupyterLab here by providing the path prefix as a token and user needs that token to access TensorBoard?

@jdhayes
Copy link

jdhayes commented Feb 10, 2022

Yes, I agree. HPC facilities cannot use NGINX, as the localhost is still exposed to all users on the node. Just a note, that if anyone was going to make it as a service, then NGINX would be a good option to add real authentication.

The Jupyter hash/token is a good method for HPC facilities to obscure the URL such that users do not accidentally intrude. Malicious users are less of an issue, but even they would have difficulty finding the address if a hash/token was added.

If reading from a file is not acceptable, could the --path_prefix take the value from an environment variables if not provided on the command line?

@joveh joveh mentioned this pull request Feb 16, 2022
@joveh
Copy link
Author

joveh commented Feb 16, 2022

Yes, I agree. HPC facilities cannot use NGINX, as the localhost is still exposed to all users on the node. Just a note, that if anyone was going to make it as a service, then NGINX would be a good option to add real authentication.

The Jupyter hash/token is a good method for HPC facilities to obscure the URL such that users do not accidentally intrude. Malicious users are less of an issue, but even they would have difficulty finding the address if a hash/token was added.

If reading from a file is not acceptable, could the --path_prefix take the value from an environment variables if not provided on the command line?

Please have a look at #5570

@joveh
Copy link
Author

joveh commented Feb 16, 2022

I close this request for now.
#5570 seems th be the superior solution to the security problem.
Feel free to reopen if you think this option is still needed/usefull

@joveh joveh closed this Feb 16, 2022
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.

5 participants