-
Notifications
You must be signed in to change notification settings - Fork 214
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
feat: Added column level access #135
feat: Added column level access #135
Conversation
dcf4646
to
ccc8b46
Compare
Thanks for this! Unfortunately in my testing it doesn't appear to correctly track state discrepancies, e.g. if I manually run |
Any reason this explicitly excludes system columns? For example, I've got a use case for managing access to |
Another issue I found: if columns are removed from the table and I remove them from the resource,
|
Hi @bismark |
ccc8b46
to
9b0e616
Compare
You're absolutely right. I've just pushed a commit that should fix this behaviour.
I don't see any reason why. I removed that bit.
The changes I just added should cover this scenario. |
@cyrilgdn could I get your input on this ? |
@kda-jt I triggered the tests and will take a look as soon as possible. Thanks for your work on that 👍 |
@cyrilgdn the linting test failed. So I fixed the issues and pushed a new commit. Should be fine now. |
@kda-jt I pulled your latest changes, but unfortunately it appears broken: the resources are always marked as required to be destroyed then recreated. it shows all columns as needing to be added, even after applying. |
It seems I cannot replicate the behaviour you're experiencing.
provider "postgresql" {
host = "localhost"
port = 5432
database = "postgres"
username = "postgres"
password = "useless_password"
sslmode = "disable"
superuser = false
}
resource "postgresql_role" "user_role" {
name = "user"
login = true
password = "user"
}
resource "postgresql_grant" "grant_col" {
database = "postgres"
role = "user"
schema = "public"
object_type = "column" # new object_type
objects = ["accounts"]
columns = ["username", "uselesscol"]
privileges = ["SELECT"]
}
@bismark Unless you can provide a thorough explanation of how things are broken, and a way of replicating that behaviour, I'm afraid I won't be able to do much. |
Yah, I'm not too sure. With the latest commit, no matter what I try, the resources are marked as needing replacement every time. Example:
First run:
Success:
Subsequent runs:
So for now I'm sticking with 9b0e616 since it works (despite the limitations I've described above). I suppose we'll hope for more testers! |
@bismark With the info provided, I can only think of the following scenario: You've created the column grant through Terraform, then "manually" revoked it. In this case, any subsequent run of Do you confirm that's the scenario you're having? |
Unfortunately no, the only thing I'm doing between the first |
Okay, crystal clear. I'm however not at all able to reproduce this behaviour. Could you please tell me:
|
@cyrilgdn |
@kda-jt @cyrilgdn it will be great if you merge it. I'm working on the RLS feature Tonkonozhenko@c78e3a6 |
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.
Thanks again, I'll trust the query thanks to the associated tests (and my manual tests) which is a bit hard to read 😅 . But otherwise, apart my comments, it seems ok
2da59f5
to
819cc90
Compare
I'm very interested in getting this functionality published and am willing to help out. Is there anything besides the RDS issue that remains outstanding? Is there a plan for working around the RDS superuser visibility limitation? |
SELECT table_name, column_name, array_agg(privilege_type) AS column_privileges | ||
FROM ( | ||
SELECT table_name, column_name, privilege_type | ||
FROM information_schema.column_privileges | ||
WHERE | ||
grantee = $1 | ||
AND | ||
table_schema = $2 | ||
AND | ||
table_name = $3 | ||
AND | ||
privilege_type = $6 | ||
EXCEPT | ||
SELECT pg_class.relname, pg_attribute.attname, privilege_type AS table_grant | ||
FROM pg_class | ||
JOIN pg_namespace ON pg_namespace.oid = pg_class.relnamespace | ||
LEFT JOIN ( | ||
SELECT acls.* | ||
FROM | ||
(SELECT relname, relnamespace, relkind, (aclexplode(relacl)).* FROM pg_class c) as acls | ||
WHERE grantee=$4 | ||
) privs | ||
USING (relname, relnamespace, relkind) | ||
LEFT JOIN pg_attribute ON pg_class.oid = pg_attribute.attrelid | ||
WHERE nspname = $2 AND relkind = $5 | ||
) | ||
AS col_privs_without_table_privs | ||
GROUP BY col_privs_without_table_privs.table_name, col_privs_without_table_privs.column_name, col_privs_without_table_privs.privilege_type | ||
ORDER BY col_privs_without_table_privs.column_name |
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.
@kda-jt I got curious, so I dug into this query a little bit. I think the visibility issues that were being discussed in this PR could be avoided by only querying the pg catalog tables. I'm not sure if there's a particular reason you chose to source the privileges from information_schema.column_privileges
and take a subtractive approach, so please correct me if I'm missing something here.
I've observed that column privileges are always visible in pg_attribute.attacl
, even if the querying user does not have read access to the table. I'm unable to find explicit confirmation of this observation in the postgres docs, but I've tested it on several postgres databases (in RDS, 11 and 13, both aurora and standard RDS) and it appears the behavior is consistent.
Additionally, pg_attribute.attacl
does not include entries for full-table grants, so using it as your source of truth also sidesteps the need to subtract table privilege grants.
With all that said, here's a query that I think faithfully produces the same output as your original, but only uses pg catalog tables:
SELECT relname AS table_name, attname AS column_name, array_agg(privilege_type) AS column_privileges
FROM (SELECT relname, attname, (aclexplode(attacl)).*
FROM pg_class
JOIN pg_namespace ON pg_class.relnamespace = pg_namespace.oid
JOIN pg_attribute ON pg_class.oid = attrelid
WHERE nspname = $2
AND relname = $3
AND relkind = $5)
AS col_privs
JOIN pg_roles ON pg_roles.oid = col_privs.grantee
WHERE rolname = '$1'
AND privilege_type = '$6'
GROUP BY col_privs.relname, col_privs.attname, col_privs.privilege_type
ORDER BY col_privs.attname
If you like, I can send this as PR to your fork to update this PR.
It's worth an extra note that, RDS aside, this will be important for anyone who wants to perform management of their database roles using a non-superuser account.
Checking in on this. With the fixes I've applied my company has been using this provider with RDS databases successfully for a couple months now. @kda-jt If you don't have time to devote to this effort, would you object to me opening a new PR from my fork that includes the fixes I've applied? Not trying to steal credit for your work, of course — you did the heavy lifting. I'd just like to move towards getting this merged. Otherwise, if you have time, my PR to your fork should include all the necessary updates. |
Hi @wilsonjackson Kudos for the work you put in fixing it 💪🏼 |
@wilsonjackson Hope this helps |
Co-authored-by: Cyril Gaudin <cyril.gaudin@gmail.com>
fd9991f
to
12214d4
Compare
Hi @cyrilgdn 👋🏼 What are your thoughts on this PR now? |
Looking forward to this! |
Salut @cyrilgdn 👋🏼 Have you had the chance of looking at this PR? |
@kda-jt do you have a working fork that can be found in any public registry perhaps I could use before this gets merged? |
@kda-jt Sorry for the radio silence, I did not have much time to devote to this project unfortunately. I still don't have much time but as it's a new feature and reviewed by @wilsonjackson, I can merge and release it after a quick look 👍 |
Thanks for this huge work and sorry again for the delay |
@cyrilgdn And thanks to @wilsonjackson for the thorough review! |
Fixes #321 Fix based on [doctolib's fork](doctolib@1caee37) version 1.19.0, with [PR 135](#135), the postgresql_grant resource gets re-created when there is a change. Replacing the resource is not a good idea because the "destroy/create" operations are completely separate. i.e. they are not atomic which means (given the example in the "Steps to Reproduce" section above) for a short moment between the 2 operations the public role loses access to the public schema. If for any reason Terraform fails midway or it gets interrupted, users will end up not being able to access the objects in the public schema. This is what happens in the PostgreSQL log: ``` 2023-07-11 14:50:05.989 UTC [1673] LOG: statement: BEGIN READ WRITE 2023-07-11 14:50:06.000 UTC [1673] LOG: statement: REVOKE ALL PRIVILEGES ON SCHEMA "public" FROM "public" 2023-07-11 14:50:06.001 UTC [1673] LOG: statement: COMMIT 2023-07-11 14:50:06.033 UTC [1675] LOG: statement: BEGIN READ WRITE 2023-07-11 14:50:06.043 UTC [1675] LOG: statement: REVOKE ALL PRIVILEGES ON SCHEMA "public" FROM "public" 2023-07-11 14:50:06.044 UTC [1675] LOG: statement: GRANT USAGE ON SCHEMA "public" TO "public" 2023-07-11 14:50:06.045 UTC [1675] LOG: statement: COMMIT ``` In our case we're only removing ForceNew from privileges, as this fixes our use case, but the overall solution allows every schema to be updated instead of recreated. Introduced a "getter" in order to fix [PR 135](#135) original issue that caused the introduction of "ForceNew". > Originally, the privileges argument did not force recreation of the resource. This was a problem because it meant that when changing the privileges in a grant resource, the update function would be triggered and would receive only the new configuration. So the revocation would not revoke the old permissions, but the new one, which is not very useful. .... I could not find a way to fetch the privilege stored in the state, & setting the argument to ForceNew solved this problem. So I did that. Fetching the privilege stored in state is the job of our new getter, this way we don't have to "ForceNew" everything. I think we might be able to keep a single "Create" function if we wanted, checking d.IsResourceNew() to decide if we should use the old one or new one, but the solution from doctolib seems robust enough.
This PR adds the ability to manage privileges on a per column basis.
General overview
This PR focusses on the
postgresql_grant
resources.It adds the value
column
as a newobject_type
, along with a new argumentcolumns
.It does not change the behaviour for other
object_types
in this resource.Example:
To simplify the code, when
object_type
iscolumn
, only oneprivileges
is allowed, and only one table is allowed in theobjects
argument.Important changes
Here are the changes to the code that were the most tricky for me to work out.
The
readRolePrivileges
SQL statementWe needed an SQL statement that could detect column level privileges, without those resulting from table level privileges.
We achieved this in the following way:
information_schema.column_privileges
table. Let's call this "A"Changing
privileges
in thepostgresql_grant
resource toForceNew
Originally, the
privileges
argument did not force recreation of the resource.This was a problem because it meant that when changing the
privileges
in agrant
resource, the update function would be triggered and would receive only the new configuration. So the revocation would not revoke the old permissions, but the new one, which is not very useful.Let's look at an example.
We want to change the privilege to
INSERT
The code would apply and work fine. However, when we'd look at the permissions in the PG DB, we could see that the
test_role
had bothINSERT
&UPDATE
on that column.This is because the revocation statement that ran was
REVOKE INSERT ON TABLE...
instead ofREVOKE UPDATE ON TABLE...
.I could not find a way to fetch the privilege stored in the state, & setting the argument to
ForceNew
solved this problem. So I did that.Changing revoking statements for
object_type
table
Previously, with
object_type
table
, any change on the table resource meant that a revocation statement would run and revoke all privileges on the table before granting new ones.This meant that, in the following configuration destroying
table_grant
would taint 'column_grant'To prevent this, I needed to modify the revocation statements for
tables
to make them more fine grained.Testing
This PR was tested thoroughly.
I added all the required test cases, and did a lot of manual testing with various scenarios to make sure everything worked as expected.
A few of the scenarios I tried:
For each scenario, I constantly checked in the database to see what permissions were effectively available, and reran
terraform plan
to see if theterraform refresh
were good.I tried these scenarios on various PostgreSQL versions running in a local Docker container :
Closing words
I'm not a Go developer & this is my first Terraform provider patch. I'm also not a PostgreSQL expert.
I tried my best however to make a high quality PR that would benefit my organisation & hopefully others as well.
@cyrilgdn I'm available to discuss this PR in a virtual meeting if you'd like.