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
Postgres Revocation Problems #1857
Comments
ISTM? |
Apologies for the acronym, updated. |
Oh. Huh. Never seen that before. Anyways, revocation SQL is on our roadmap. Someone enterprising might be able to get it in before us, but it's coming. |
I would have never though of it either but we are very careful about what users get what permissions and when I started working on implementing them in vault I started running into errors. These are really good references for future, I really had no idea the depth to which you could assign or revoke permissions until recently.
Thanks @jefferai |
That was to |
tl;dr: please implement this 👍 I'm currently working on integrating Vault and this is very much needed. Some background and perhaps pointers at usable work-aroundsThere is an idiosyncrasy of PostgreSQL that makes this change especially important to make the PostgreSQL secret backend actually usable for me: Roles with the In other words:
Now you can configure Vault like this:
Now, there are two drawbacks to that configuration:
Right now, the work-arounds are to:
|
Extra point of info, Vault's postgres functionality should not be primarily tested with a superuser role. A lot of behaviour is implicitly available as a superuser which Vault shouldn't rely on. Not only to be finicky about principle-of-least-privilege: using Postgres on RDS won't even give you a superuser role, only a not-quite-equivalent |
Nobody is arguing with any of these points, and we're aware of all of the benefits, but it requires someone to step up and do it if you want to see this done in a timeline faster than our own, as is being done with MySQL (#1914). If someone wants to code similar support in for Postgres and can do it by the end of Monday I will consider it for 0.6.2. You'd do well to note the comments I made in my review of #1914 though. |
@jefferai — Didn't mean to add pressure, just trying to contribute information for whoever is able to take the lead on implementation. We originally developed with Vault given a superuser and then had to backpedal (and relearn some things) because of RDS. For additional reference, how we're currently set up (Vault 0.6.1, PostgreSQL 9.5), which is working for us:
|
Just an aside: This is unnecessary/a noop. If |
That's good to know, thanks @jdelic! |
As of today, the revocation logic iterates through all the table schemas and revokes the Reading the statements one by one from, by say a role parameter Or, the statements in If Vault, without substituting any input, simply executes what is supplied via Therefore, we have decided to defer this issue for now, and seek inputs from the interested parties before we attempt a fix. |
As far as I can tell, that assumption with the way PostgreSQL credentials work will always be invalid unless Vault has superuser permissions or the issued credentials are read-only (which is how the current example in the Vault documentation works). Right now, that code is broken, at least on my setup. Roles never get dropped.
The way I see it, the only thing missing for the current code to work, is that Vault can't use its login credentials specified in As PostgreSQL credentials work (see also my previous comment),
As I understand the proposal in this ticket, this would be easy if Vault had a
A more complete example for some applications would be:
Iterating the above SQL for Caveat: Where I see |
@jdelic It doesn't seem that simple in all cases, looking at the current code. There are sequences, schemas, functions, etc. Users can be granted permissions on different schemas. I'd also be concerned, not knowing enough about setting a connection's role, whether there are times when that would fail based on the initial permissions. Maybe to put it another way, in your second example you say "a more complete example for some applications...". I'm concerned about whether a blank canvas for |
It seems to me that if the user knows the SQL statement to create the role, they should also be able to craft the SQL statement to revoke the role. I've found that in my setup (using AWS RDS) that none of my roles get revoked. My database schemas are fairly complex. Named schemas, not using the public schema at all, sequences, functions, triggers, tables, views etc are all being used. While vault has no problem creating a role, it cannot revoke it. :( |
@ekristen Generally speaking I agree with you, and that was my initial thought. What I wasn't sure about -- not knowing Postgres very well -- was why the existing logic works the way it does and whether simply being able to execute a set of statements would be enough. |
@jefferai I'm with you there. I'm relatively new to postgres permissions and its a PITA. For example if you grant usage and all privileges on all tables in a schema to a user and then a new table is added the user doesn't get access to that new table unless you specifically alter the default permissions for that user against that schema. I know that I have crafted a sql statement to create and revoke a role, this might be out of ignorance of the postgres permission system, there might be some really simple thing I'm missing. I think |
@jefferai lol, after I sent that last comment, your question popped up ... I could probably test it today. |
@ekristen I'm basically shoving in a last minute capability to do this in "beta" -- for now, it won't be on the web site docs, and won't be announced as a feature. But I do want to help you out if I can. Essentially, I'll be running our current acceptance tests to ensure I didn't break anything but probably will not be able to write new ones before release. If I can poke you and you can compile and test the code quickly, you can at least test it somewhere real before the tagging :-) |
I can compile, spin it up locally and throw my production configs into it and give it a test today. |
@jefferai I can't get the new code to work, unfortunately. I compiled post-merge
Any ideas? |
You have to set them both at the same time. |
@ekristen is correct. Try adding the |
@jdelic Vault endpoints used to follow the "pull all, update fields, push all" semantics. This is changing incrementally. In the documentation, you will notice some endpoints stating that it supports both |
I have now tried updating my configuration to set both at the same time:
However, the roles still don't get deleted. My PostgreSQL logs show this, though:
That's not my |
@jdelic so {
"sql": "CREATE ROLE \"{{name}}\" WITH LOGIN ENCRYPTED PASSWORD '{{password}}' VALID UNTIL '{{expiration}}' IN ROLE \"authserver\" INHERIT NOCREATEROLE NOCREATEDB NOSUPERUSER NOREPLICATION NOBYPASSRLS;",
"revocation_sql": "SET ROLE authserver; DROP ROLE IF EXISTS {{name}};"
} vault write db-authserver/roles/fullaccess @path/to/json/file/given/above.json |
@jdelic I built from source, are you sure there isn't another vault binary in your path? To be sure you could modify https://github.com/hashicorp/vault/blob/master/version/version_base.go#L7 in your local source that you build and change that value so that when you query for version you know you are using your built version? |
I think what's happening is a CDN cache problem. Trying to isolate. |
that's a good idea, thanks for the pointer. I am however 99.9% sure that I consistently used the correct binary, since I compared MD5s on them to rule that precise problem out. If @jefferai doesn't make any progress, I'll redo all my tests with a fresh compile with modified version number. Can you reproduce the above output? I think at this point, if a third person can replicate either results, that would be good data to have :) |
I honestly don't know how/what happened but when I built the new binaries they appear to have failed actually uploading to the CDN and I didn't see. Ugh. The binaries corresponding to the actual 0.6.2 tag should now be up...please try them. I'm not going to send out an announcement since the only difference is the postgres feature, which is unannounced and in beta :-) |
So I had to go to bed last night :), but I now continued testing. The good news: With the new binaries I can now replicate the behavior you've shown above and get the correct output.
The bad news: It still doesn't work correctly. My Vault debug logs now show
PostgreSQL logs:
Question: Does Vault execute the |
@jdelic I've verified this to work. I would hazard to guess you've got a SQL problem or a permissions problem with your connection string user. What happens if you run your SET ROLE authserver; DROP ROLE "root-69c3ea26-1f97-5836-8e27-e9b5c195fb7a"; |
Yes. |
Actually, let me follow up: Vault uses the same connection URL. But there is a connection pool under the hood, so Vault may not be using the actual literal same connection. |
Okay... multiple things to address in this comment :)
I spent the last 30 minutes debugging this and you were right, the problem ultimately was on my end. Reprovisioning the vagrant box and resetting the database fixed the permissions. I probably broke something during testing last night and changed the permissions around. @jefferai
If that specifically means that Vault might execute |
I actually don't have much insight as to the underlying semantics (because I haven't looked into lib/pq that deeply) but the Go SQL interface leaves the isolation factor up to the driver. My working assumption is that, because the statements are executed in a transaction, the same connection will be used to execute the entire transaction. |
@jefferai I guess you can close this behemoth of a ticket. Thank you for being so incredibly responsive (you too @ekristen!). I will push this into production now! 🍰 |
Cool. If that's not the case, I will find out when this goes into staging :). |
As long as the whole SQL (sql or revocation) string is being executed as a single query it should use the same connection. |
@ekristen It's not -- it's executed statement by statement (splitting on semicolons), but within a single transaction. I suppose that it could be executed as a single statement; it's just not how things are currently done. I took a look; internally to the Closing as requested. If someone is interested in running full sql/revocation_sql statements as a single query feel free to open up a PR :-) |
I agree, as long as it is the same transaction it should be good to go. |
@erydo Do you have any details on what/how you did to workaround this? I'm running into this bug a lot in our instance do to system architecture having multiple processes requesting logins at the same time. |
@thesoftwarejedi Yes, we actually did a couple things.
The above was accomplished by factoring those into PLPGSQL procedures that could be called directly from Vault's provisioning statement, which didn't allow PLPSQL itself. This meant that the actual Vault provisioning statement was something like: SELECT provision_ephemeral_role(
'foo-service', '{{name}}', '{{password}}', '{{expiration}}'); The CREATE FUNCTION provision_ephemeral_role(base_role_name VARCHAR, name VARCHAR, password VARCHAR, expiration VARCHAR)
RETURNS VOID
AS $$
BEGIN
PERFORM pg_advisory_xact_lock('x' || lpad(encode(digest(base_role_name, 'sha256'), 'hex'), 16, '0'))::BIT(64)::BIGINT);
EXECUTE FORMAT('CREATE ROLE %I WITH INHERIT LOGIN PASSWORD %L VALID UNTIL %L', name, password, expiration);
EXECUTE FORMAT('GRANT %I TO vault WITH ADMIN OPTION', name);
EXECUTE FORMAT('GRANT %I TO %I', base_role_name, name);
END
$$
LANGUAGE PLPGSQL; |
In addition to #1857 (comment): Regarding the use of large objects in this scenario, I got "permission denied for large object". For me, the following solution worked well so far:
By having this in the role config for vault, it is made sure that the correct owner (the persistent Postgres role) is not only set for migration scripts, but also when creating LOBs. |
If you are granting access to sequences and functions you have to revoke those too.
#699 only fixes schemas.
It seems to me that allowing for a revocation SQL query might be necessary?
Thoughts?
The text was updated successfully, but these errors were encountered: