-
Notifications
You must be signed in to change notification settings - Fork 13
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
Proposal cylc authorisation #125
Conversation
Converting this to a draft as the config section in the proposal requires additional thought. |
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.
Looks really good so far @datamel, nice and clear. A few things to discuss at the next meeting, if not resolved by then.
…-admin into proposal-cylc-authorisation
I took another pass through this, still looks great 👍 The only thing I'm wondering about is the current read/write/execute model, where execute permission (scheduler start, stop) implies write permission (task mutations). Arguably that should be the other way around, or perhaps separate with no inheritance of one from the other. If you can start my workflow it will run exactly what I configured it to run. But if you can execute task mutations on my running workflow you can change what actually runs (potentially drastically). |
Yeah, need something like Where
As mentioned elsewhere, we can achieve fine grained authorisation via middleware. For example:
The reason we should use Graphene's inbuilt middleware, is because request object methods (like
At the middle level you could allow
I don't think we need to tether the grouping of our UIS authorisation to the system groupings (as the HPC/System admins may have their own grouping ideas), but allow for users to decide which other system users can do with their suites. At NIWA, I don't have write permission or are part of the same file system grouping as our operational role accounts (but have sudo access to assume their role). Obviously the config should be user write only (0644), and this should be checked when loading it in. It will essentially allow you to give full permission of your user and it's capabilities (as with
Once the CLI can go through the proxy => hub => UIS, we'll be covered 👍 Nice work! |
Thanks so much for the feedback @dwsutherland. I had investigated middleware and moved away from it initially, due to other things needing authorisation (not just graphql queries/mutations). Thanks for your example, I shall have another look. |
So far, we have the following options. Option A: Decorating handler actionsAdapt my current authorization PoC so that it parses the query body to get all operations in the graphql. I had originally identified this as something we would need to do (was aware that we could not rely on operation name alone):
I still feel strongly that the responsibility of authentication and authorization should sit with the handler layer and prevent unauthorised access as early as possible. We expose more code, the further down we check authorization, which is not great if we’re also being a bit hacky with said code to make it do auth elsewhere. Option B: MiddlewareAs @dwsutherland rightly suggests, we could place the auth in graphene middleware. Although we do not have immediate access to the authenticated user at this point.
For the graphql handler via the middleware list:
This worked fine, although we would still need to parse the graphql in the middleware to determine the operations. Option C: Decorating the resolver functionsAs per @oliver-sanders thoughts that we should avoid additional parsing of the query, which we need to do to discover operations in a query. I’m happy to give it a go for another PoC but there’s a few points for discussion first: Also worth noting, I have looked into whether we can parse the request at handler and then continue passing this through, ready parsed, but the graphene layer parses the request regardless. On a related point, if a user requests multiple mutations in one request, and they are authorised for e.g. 2 out of 3, do we want… |
I can see the temptation to parse the GraphQL document up front but I'll chuck in a few comments to show how this could be vulnerable. This approach depends a lot on our ability to replicate the internal workings of Graphene as closely as possible and to keep up with their internal implementation. If we lapse the approach might not be quite as iron-clad as we would like it to be. In parsing the document upfront we are trying to guess what endpoints that the request would hit rather than waiting for the server to tell us what endpoints are being hit. If we guess wrong then we are in trouble, here are some conditions that could break our guesses:
Then, without any protections further down the chain the entire authorisation layer could be sidestepped. In a conventional HTTP(s) server (like the Cylc 7 suite server) the server routes each request based on the URL, any auth is performed at the endpoint. In a GraphQL server the routing is based on the GraphQL document, the endpoints are the resolvers. I found an authorisation framework for GraphQL (2.8k⭐️) called graphql-shield, sadly it's Javascript (like all the GraphQL goodies) so we can't use it, however, worth noting they went down the middleware approach. |
I agree with @oliver-sanders on this. |
I agree with @oliver-sanders too, we should not be parsing the GraphQL document/object ourselves.. For all the stated reasons. However, we really need to be sure of what information we have and what we can do with GraphQL already, before ruling things out.. So just some clarifications on Option B:
Can't we just pass this in via info/context? You simply set fields on info.context.
No (sorry to be blunt). This information is all available (
There's all the information we need here. i.e. I think we need two levels:
Also:
Middleware is essentially a wrapper to all resolvers, including the default resolver (not defined in our code) which reads the data from |
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.
All looks good to me.
Just one little curveball that you may have already spotted, and which we were aware before we started on this. Commenting it here for the record and also as a reminder that we should add a note of some form to the documentation when we write it.
These site-configured limits can be hacked and overwritten by users.
There are at least two Cylc mechanisms by which the limits could be hacked from user-space:
- Setting
CYLC_SITE_CONF_PATH
, writing your own site config and injecting it:- In the spawner args (user config).
- In shell profile files (unless Cylc is deployed in a venv of some form with a wrapper script).
- On the cli (i.e. starting the UIS manually).
- Monkeypatching the UIS code from the config file.
- Because the config file is in Python and you can change anything in Python.
- We could switch to a JSON config file, but, Jupyter server supports Python config files and we can't turn that support off.
These sound like security bugs, but they are not. Preventing these mechanisms from being exploited would be pseudo security. There are plenty of non Cylc-specific mechanisms too:
- Monkeypatch the UIS code from the
PYTHONSTARTUP
file.- This applies to ALL Python programs!
- Wrap the Cylc UIS code to create your own uis command...
- ...and get the spawner to run this in the user config.
- ...and patch it by setting PYTHONPATH.
- I'm sure many OS level approaches.
On the surface this might sound bad, but it isn't because:
- The UIS runs in user space, users have control over this, there is nothing Cylc can do to stop them.
- Users can always give away full access to their accounts by sharing credentials (short of biometric security even 2FA can't prevent this).
- The only way to truly achieve this would involve running the entire UIS under a site-controlled privileged account.
So in the same way that "not giving away your user credentials" or "not using infrastructure for malicious purposes" are enforced as human policies "not hacking Cylc to fiddle the site-defined limits" is a human factor too.
|
||
We will need to consider the desired behaviour if a user appears twice with different defaults and limits set, this is probably most likely to occur when a user appears in either multiple groups or in a group and as a user. | ||
|
||
Proposal: all permissions are additive, if user appears elsewhere in config, the permission level should be taken as the greatest possible. Note, default in site config does not contribute to permissions, unless user does not appear in ui server owner's config. |
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.
That's sensible. If the higher level of permission appears in the config, then no one can rightfully complain about us granting it (even if lower levels are explicitly granted as well).
|
||
Some of the below are not currently available to users but including them here for consideration. This is also a fairly substantial list, which makes the case for read and admin pre-set access groups which would be easier for users and sites to configure, rather than defining a long list of mutations per user/group. | ||
|
||
As a springboard for discussion, defaults could be assigned as follows: |
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.
Looks to me like you've nailed it, as-is.
Excellent diagrams 🎉 |
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.
Nice proposal 👍 Just a handful of minor comments and suggestions.
Update docs/proposal-multi-user-approach.md Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com> Update proposal with READ/CONTROL decision
9d98014
to
9a675c0
Compare
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.
👍
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.
Several minor changes needed now ALL is not the same as READ + CONTROL
🎉 |
Proposal for UI-Server authorisation.
Proof of concept PR : cylc/cylc-uiserver#204