-
Notifications
You must be signed in to change notification settings - Fork 21
Disallow site auth limits in user configs. #751
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
base: master
Are you sure you want to change the base?
Conversation
|
|
||
|
|
||
| def inspect_user_config_files(config_paths): | ||
| not_allowed = ["site_authorization"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it make sense for this not_allowed list to be defined at the site config level by the administrators?
Because it's really a policy matter what the admins allow/disallow the user to do with access to themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually .. Strike this, reverse it ..
site_authorization should only be set at site level..
whereas user_authorization is set by the user, and bound by the site_authorization parameters..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't really need a list actually, I think there's only one relevant setting. The code all runs as the user, it's just that it doesn't make sense to have site_... settings in the user config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, this does not prevent a user from overriding the site configuration, so amounts to psudo security (making it look like something is secure when it isn't).
Making it look like this is secure / protected is worse for security than accepting that it isn't.
However, hacking the system is one thing, simply setting a valid Cylc config item is another - that's too easy, it almost makes the site limits pointless.
It's not pointless, the user has to explicitly overwrite the site policy, which is obviously not permitted by local policies. There are lots of policies which are upheld in this sort of manner, such as not giving your password to someone else, not mining bitcoin on corporate resource, not letting in hackers, etc.
We can always add more bells and whistles...
|
I don't buy that argument - it's as simple as this, really:
Disallowing it makes the system more secure in that it becomes much harder to violate site policy. (Yes, that's "more", not "absolutely" - but security always has caveats, even for (e.g.) SSH). Of course users can in principle hack around these kinds of restrictions, but very few could or would, and we can still strongly warn that site limits are in principle vulnerable. But simply disallowing it will prevent the vast majority if not all violations.
Somewhat beside the point, but you can be sure that these behaviours would not merely be left to site policy if technical restriction was feasible!
It is the The other sensible approach would be to not provide the site limit setting at all - that would certainly make it 100% clear that users have full control over their own auth settings. |
|
I understand your frustration, however, we investigated all of this at the time and determined that we we probably could not prevent users setting this value, even by legal means. For example, your changes here can be easily side-stepped by using the CLI argument: cylc gui --CylcUIServer.site_authorization={...}Or by using your root level jupyter config: # $HOME/jupyter_config.py
c.CylcUIServer.site_authorisation={...}Both of which are perfectly legal. For this and other reasons, we determined that there was no point in trying what you have attempted here, and why I discouraged you from trying! We do have our reasons! But that doesn't mean that the feature is without purpose, it still provides value to us, a soft limit is still better than no limit. It makes you think before you cross it. |
|
Good points @oliver-sanders - you've shown that a user config block as per this PR could still be avoided without outright hacking. For the record, we've also discussed offline:
Whichever we decide (I'll come back next week...) we should document the result very clearly, so we can always point to that if anyone complains (e.g.) that a site limit is not really a site limit... |
Our head HPC guy thinks it is a design flaw that the site authorization limits can be overridden or disabled by users just by setting the same item in their user config, and I kind of agree.
Admittedly, I had forgotten that we knowingly do nothing to prevent this on grounds that users have complete control over their own processes so could hack the system in several ways to get around any such restriction if they wanted to.
However, hacking the system is one thing, simply setting a valid Cylc config item is another - that's too easy, it almost makes the site limits pointless.
If it is easy for us to disallow use of
site_authorizationin user configs, then I can't think of a good reason not to do so. We can (and should) still document that users have complete control over their own processes and so could subvert this if they want to - but at least then that will require a deliberate hack, with considerably more knowledge and effort, not just normal use of a Cylc config item.Highjacking the Jupyter server config file loading mechanism is probably a step too far, but it seems we can use Python
importlibtools to inspect the content of the user config module without properly importing it tosys.modules.Currently on the PR, it works for
cylc gui.If agreed on the approach I can make it work for hub-based launch too.
Check List
CONTRIBUTING.mdand added my name as a Code Contributor.setup.cfg(andconda-environment.ymlif present).?.?.xbranch.