Update Mutation not possible with column-level SELECT security

[pepijnverburg]

Hi there,

Recently we've been implementing a more sophisticated system to implement security layers on Postgres level. However, the plugin PgMutationUpdateDeletePlugin.js requires you to have rights on all columns as the RETURNING statement is set to *.

This causes a permission denied for relation ... error.

The following query seems to be the problem:
https://github.com/graphile/graphile-build/blob/a65cdc58bc181bdfc72cf05c75fb729737630606/packages/graphile-build-pg/src/plugins/PgMutationUpdateDeletePlugin.js#L121-L132

I would suggest to change it to returning a single column already present in the SET parameters. It looks like this query result is only used to count the affected rows. Possible suggestion (NOT tested):

sqlMutationQuery = sql.query`
    update ${sql.identifier(
      table.namespace.name,
      table.name
    )} set ${sql.join(
  sqlColumns.map(
    (col, i) => sql.fragment`${col} = ${sqlValues[i]}`
  ),
  ", "
)}
    where ${condition}
    returning ${sqlColumns[0]}`;

Using sqlColumns[0] is safe as there is a guard checking for its length right above it.

Let me know if this seems a suitable solution without any side-effects. PR is possible.

Note: DELETE probably causes problems in the same way.

Tested with:

• Postgres 10.4
• Postgraphile 4.0.0-rc.2
• NodeJS v8.9.3

[benjie]

Column-level select permissions are not well supported for many reasons like this and computed columns. I recommend using additional 1-to-1 tables at permissions boundaries, though I understand this can sometimes add complexity depending on your data schema/business logic.

Sadly the “returning *” is more complex than that; we take the returned value and feed it back into postgres to calculate the result of the query after the mutation - we need all fields to be able to cast it in PG. It turns out that querying after a mutation cannot reliably be achieved with common table expressions because you may have a stable function in the result (e.g. a custom query) that’s not explicitly dependent on the mutation result and yet is affected by it; in this case postgres sometimes call the stable function first, before the mutation, and hence the results are stale as they haven’t factored in the mutation. So we have to do it as two statements: mutate first, then query. But then it gets more complex: triggers can result in different data being output than was written to the table; the mutation might be a custom function; etc. If it were simpler we could just use the PK and re-select from the database, but there’s a lot of hidden complexity here that affects many applications.

I am not a fan of my “viaTemporaryTable” (misnomer) solution, but I am yet to find a better one. I’m considering deprecating the edge case support in favour of simplicity but compatibility and accuracy are very important to me.

I’ll think about this a bit harder; more people are using column-level grants than I anticipated!

[pepijnverburg]

Hi Benjie,

Thanks for the extensive answer. I was already hesitant for the change as I indeed noticed the viaTemporaryTable call, but didn't know what it was solving exactly. It would be great if this is possible in the future! For almost every project we are required to use RLS in combination with granting column-specific access. I'm not a big fan of introducing additional 1-on-1 tables due to complexity.

As another solution: how secure is the @omit smart comment? I prefer securing it on the lowest level, but if the API is not exposed and accessible, it is basically the same as providing my own middleware with these permission checks.

[benjie]

I have not done a security analysis of using @omit. If the field does not exist in the GraphQL schema then GraphQL guarantees that it cannot appear in the result. However there are other routes to potentially get the data; e.g. it might be leaked via errors?

I've bumped the priority of solving this up my ever-growing TODO list 👍 (By the way if you need a solution to this sooner rather than later then you can engage me as a contractor to implement the required changes. I'd also review a PR that made this an option (e.g. --select-column-rbac-workarounds); it'd probably want to work with computed column functions also as they currently pass the entire record and we'd need to add a way where they only pass the PK, or where they pass null for fields they're not allowed to access, or something like that.)

[pepijnverburg]

Hi Benjie,

Thanks for adding it to the TODO list and good to know that you are open to work as a contractor if the required changes are urgent! For now we are going to work with custom queries / functions to implement update functionality for these fields, as it is quite specific functionality (e.g. when updating the password, but disallowing SELECT as the password hash is then exposed). A bit more to maintain though when using this mechanism for many of the tables..

[benjie]

You could remove the password field from the User object via a plugin. I don't recommend this approach in general, but I think it would work for this. It would be up to you to determine if doing so was secure or not.

https://www.graphile.org/postgraphile/extending/#removing-things-from-the-schema

[mattbretl]

Sadly the “returning *” is more complex than that; we take the returned value and feed it back into postgres to calculate the result of the query after the mutation - we need all fields to be able to cast it in PG.

We implemented #296 by casting individual fields as text and wrapping them in an array (as opposed to casting the full record to text). Could we add a check for column-level select grants, and if any are found, run the mutation through the text[]-building approach instead of casting the full record to text?

[benjie]

I don't think that'll work, because we need to cast it back to the relevant type which means we need to specify all the fields. Omitting some fields that we cannot see (e.g. substituting them for null) would give different results when you use computed columns/etc on the result (e.g. if those computed columns are security definers that can see into the hidden columns).

I think the correct way to deal with this is to just throw our hands up and re-select the data from the DB after the mutation rather than using the mutation result. This will only work with the built in CRUD mutations (can't use it for procedures) but it should fix the issue for most people. Then we'd only require that the primary key(s) have read privileges.

[benjie]

If you're using column-level SELECT privileges (INSERT and UPDATE are unaffected) please get in touch ASAP*. Column-level SELECT grants cause a lot of woes for PostGraphile, so I want to understand peoples use cases and ensure there's sufficient evidence they're worth the trouble.

You can get me on our discord chat: http://discord.gg/graphile; DM me on twitter @benjie or email me benjie@gra??ile.org

Column-level SELECT privileges (get in touch):

GRANT SELECT (col1, col2) ON my_table TO graphql;

Table-level SELECT privileges (unaffected):

GRANT SELECT ON my_table TO graphql;

* ASAP: don't worry, I'm not breaking anything in the 4.x release line, but I'm starting to make a plan for 5.0 and column-level SELECT is complicating things.

[frankdugan3]

I think there is one huge value for column-level select privileges over one-to-one table solutions: If the security requirements change (a field moving to a more secure level, or vice-versa), that is a breaking API change for one-to-one security models. If column-level select privileges were better supported, I would much prefer to use them sparingly rather than splitting up the table.

[benjie]

I think they're quite an awkward pattern to have in your database in general - not being able to do select * or pass around full record objects is debilitating. But I get what you mean from a permissions POV. Personally I prefer smaller tables with well defined security boundaries, I would rather get a null for an entire row from GraphQL due to RLS than an error because the user has tried to request a column they're not allowed to query.

[ivawzh]

I think column-level permission setting is an essential feature for a general purpose framework. Database table modeling ideally should be done only with the focus of Domain Driven Design. It'd be awkwardly leaking boundary when we have extra 1:1 relationships because of security setting limitations.

And without column-level permission setting, I am not sure how we can implement a relationship based role permission setting. Do we have to create an extra table for every role? E.g.

users n:n roles
roles n:1 organisations
roles n:n projects
organisations n:n projects

| roles          	| project's name 	| project's address 	| project's wiki 	|
|----------------	|----------------	|-------------------	|--------------------	|
| owner          	| read, write    	| read, write       	| read, write        	|
| maintainer     	| read, write    	| read              	| read, write        	|
| customer       	| read           	| none              	| read, write        	|
| visitor        	| read           	| none              	| read               	|
| more roles...  	|                	|                   	|                    	|

[benjie]

Note this issue only relates to column-level SELECT permissions. Column-level UPDATE/INSERT is fine. So really your example comes down to not letting customer and visitor read the address value. I.e. they cannot do select * from projects if the projects table contains the address (ugh, select * should always be allowed!).

Personally, I'd put the address in a separate table (it is, IMO, a separate domain in Domain Driven Design). The GraphQL would look like:

{
  project {
    name
    address{
      street
      city
      etc
    }
  }
}

This looks entirely reasonable to me.

[pepijnverburg]

Quick update from our side: we've had several projects where we added the one-to-one relations between for example the user and associated password / address information. However, this data structure kept being a pain as most standard services would not support the one-to-one relation between user record and password without any significant modifications (=maintenance). I know it is a minor data structure adjustment, but you feel it everywhere throughout the stack.

Currently we override the GraphQL resolvers to not include these fields, but it feels like a missed opportunity not being fully compatible with RLS. We generate all RLS policies (including relational policies) based on simple JSON configurations and the SELECT permissions are skipped for now.

I think there are three major considerations:

1. Do we see RLS thrive in the future? I'm not aware to what extend the Postgres contributors see this as a fundamental feature and therefore gets frequent improvements.
2. @benjie Do do you feel like Postgraphile should be fully compatible with RLS? The high level of compatibility with many of the Postgres features (e.g. functions) is what is important for many and what distinguishes it from other solutions.
3. @benjie Do you think supporting this will haunt you a lot during future development after it is being implemented? I think this is for sure an important one, but hard to predict.

[benjie]

1. I can't see RLS going away, it was sufficiently demanded by people moving from Oracle to Postgres to require it, and attention has been given to its optimisation.
2. As best it can be given the constraints of GraphQL, yes.
3. I don't really know what you're referring to. I'm guessing you're taking about removing the RETURNING * - I'm planning to make this change in V5 anyway and use the PK to look up the record. This, however, won't work for custom mutations that return a "virtual" record (i.e. one that does not exist on disk, thus cannot be retrieved later), so there's complexity there w.r.t. backwards compatibility. We will be requiring that the primary key fields should be selectable though.

[pepijnverburg]

1. I agree, I like the developments in terms of optimisation in the recent releases.
2. Great!
3. Yes, I'm referring to the original problem of RETURNING *. Does this mean there will be an additional query? Where (1) the primary key is returned via e.g. RETURNING id and after that (2) the record is queried only using the requested fields from the GraphQL query? Sounds like a trade-off worth it.
4. With virtual records are you referring to results from for example functions? Isn't it possible to recognise this and switch back to RETURNING * in these situations?

[benjie]

3/ We already do two queries; the second one will just do slightly more now; and won't be as affected by dodgy triggers that return incorrect values 😉

4/ Yes, but handling via two different pathways is painful, and then it means people using column-level SELECT can't use functions for mutations. I think it's probably better to just assume that "virtual" records don't exist unless the function is tagged @virtual or similar, at which point we fall back to the old pattern. I'm not happy about it ;)

[pyramation]

Following. I'm also curious if anyone has made a plugin for this?

Definitely excited for v5 and postgraphile becoming more compatible with RLS :)

[pyramation]

Good news! @benjie helped me code a plugin today to solve this!

I've put the plugin here and example here

please send feedback for the code, naming conventions, etc, etc.

Happy to work with the community here. I've been wishing for this feature for a long time, so very happy to finally have it! I couldn't have done it without an extreme pair programming session with @benjie, so thank you thank you thank you 🙌🏻

hopefully we can soon say "there's a plugin for that™" :)

[benjie]

V5 tracks the columns that you access during planning and only requests these columns from the mutation. HOWEVER you can still have issues with computed column functions and the like. Nothing we can do about that, really.

[pepijnverburg]

That is awesome news! I really should get V5 up for a spin for some of those projects we used some granular RLS with. Thanks for the update and I'm sure you're happy to close an issue that was posted in 2018 ;)