Skip to content
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

Implement pre/post hook queries [was: support setting/updating password for static roles] #565

Open
dani opened this issue Sep 12, 2023 · 15 comments
Labels

Comments

@dani
Copy link

dani commented Sep 12, 2023

ldap2pg allows creating static roles defined in the yaml config. While its main use is to create "group" roles (with NOLOGIN), it'd be really conveniant to be able to define password for roles with the LOGIN option, eg

rules:
  - roles:
      - comment: Parent role for all ldap2pg managed roles
        name: managed_roles
      - comment: Role for device-services
        name: device-services
        options: LOGIN
        password: SuperS3cr3tp@ssw0rd
        parents:
          - managed_roles

(or maybe a way to read it from env vars)

With that, ldap2pg could be a complete declarative role manager for postgres. It could even go one step further and be able to create databases too but hey, one RFE at a time ;-)

My use case is to simplify application initial setup in a container orchestrator (Nomad, but would be the same elswhere)

@bersace
Copy link
Member

bersace commented Sep 13, 2023

Hi @dani, thanks for reaching.

I'm not fond of handling secrets in ldap2pg, especially in ldap2pg.yml. This will ends up in logs and GitHub. ldap2pg logs every statements full.

However, I'm interested in feeding ldap2pg with other sources than LDAP like environment variables, vault, etc.

For the password itself, i'd prefer to not synchronize password but rather add a hook so that ldap2pg could be a single entrypoint for provisionning roles.

Something like:

# ldap2pg.yaml
rules:
-  ...

post:
- ALTER ROLE device-services SET PASSWORD 'SuperS3cr3tp@ssw0rd'

Or maybe, ldap2pg could set password hash instead of clear password. I still think that leaking a password hash is bad for security. Change my mind :-)

@dani
Copy link
Author

dani commented Sep 13, 2023

Indeed, logging password would be bad, so ldap2pg would have to handled this specially.

In my case, having passwords in the ldap2pg.yml is not an issue as I generate it on the fly (and could feed it with values from hashicorp vault) and just store it while it's running, in a private tmpfs. Setting hash would make it a bit harder to use, as you would have to adjust your hash computation depending on the target postgres.

Reading from env vars is probably the easiest, and most flexible way (using the more or less defacto standard of ${VAR_NAME} in the yaml file ?). I'd be fine with either my "password" proposal, or the "post" hook

@bersace
Copy link
Member

bersace commented Sep 13, 2023

Could you use envsubst to preprocess ldap2pg.yaml ?

@dani
Copy link
Author

dani commented Sep 13, 2023

I could, but that wouldn't bring me a lot of advantage for my use case (as my ldap2pg.yml is templated and already able to fetch values directly from vault).

@bersace
Copy link
Member

bersace commented Sep 13, 2023

I could, but that wouldn't bring me a lot of advantage for my use case (as my ldap2pg.yml is templated and already able to fetch values directly from vault).

What do you mean by templated ? What's rendering ldap2pg.yml ?

@dani
Copy link
Author

dani commented Sep 13, 2023

I have several layers of template engines in my setup ;-)
I use gomplate as a preprocessor engine to render my Nomad job files. This is where I merge various static configuration depending on my env (dev, qa, stage, prod), and create configuration files skeleton.
Then the rendered job is submitted to my Nomad cluster, which itself supports consul-template templating engine. This second step is where I fetch secrets from vault to produce the final ldap2pg.yml

@bersace
Copy link
Member

bersace commented Sep 13, 2023

ok, I'm gonna implement simple post queries. This will help for add some tests too.

I suggest the following syntax:

post:
- description: blah...
  sql: |
    ...

If SQL raises an error or returns false, ldap2pg will fails. This allows to write some tests with has_*_privilege like this:

post:
- description: Test role public can't create on public.
  sql: SELECT has_schema_privilege('public', 'public', 'CREATE');

But you could simply return void or NULL and use post as a simple hook.

What do you think of this ?

@dani
Copy link
Author

dani commented Sep 13, 2023

I think something like this could work for me :-)
While at it, could a similar "pre" hook be added to ? If the post ones could be used to set role passwords, pre ones could be used to create databases/schema before privileges gets applied

@bersace
Copy link
Member

bersace commented Sep 13, 2023

I think something like this could work for me :-) While at it, could a similar "pre" hook be added to ? If the post ones could be used to set role passwords, pre ones could be used to create databases/schema before privileges gets applied

Yep, good idea. This should help for a long standing issue : #171.

@bersace bersace changed the title Feat : support setting/updating password for static roles Implement pre/post queries [was: support setting/updating password for static roles] Sep 13, 2023
@bersace
Copy link
Member

bersace commented Sep 14, 2023

I have updates on the design.

What about including post and pre in rules like this :

rules:
- description: ...
  run: ALTER ROLE ....
- description: ...
  roles: ...

ldap2pg can then inject LDAP attributes in run values.

However, I don't know how to guarantee the order of queries generated.

Another option, directly related to #171 is to attache hook at role level :

rules:
- role:
    name: staticrole
    after_create:
    - ALTER ROLE 'staticrole' SET PASSWORD 'N0tS3cret';

I'm thinking of before_create, after_create, before_alter, after_alter and something like on_nochange. How could ldap2pg knows that you need to execute your ALTER command ? Maybe by defining an inspect query coupled with an apply query.

rules:
- role:
    name: toto
    on_condition:
    - if: SELECT true;
      then: ALTER ROLE ...

I'm thinking of implemented also before_grant, after_grant, before_revoke and after_revoke.

This configuration allows to determine precisely when the query will be executed.

What do you think of this ?

@dani
Copy link
Author

dani commented Sep 14, 2023

I have updates on the design.

What about including post and pre in rules like this :

rules:
- description: ...
  run: ALTER ROLE ....
- description: ...
  roles: ...

ldap2pg can then inject LDAP attributes in run values.

However, I don't know how to guarantee the order of queries generated.

Like you noticed, this prevents controling the ordering, so IMHO, not a good option

Another option, directly related to #171 is to attache hook at role level :

rules:
- role:
    name: staticrole
    after_create:
    - ALTER ROLE 'staticrole' SET PASSWORD 'N0tS3cret';

I'm thinking of before_create, after_create, before_alter, after_alter and something like on_nochange. How could ldap2pg knows that you need to execute your ALTER command ? Maybe by defining an inspect query coupled with an apply query.

While this would be ultra flexible, it's also a bit harder to use. If I want to be sure my role has the expected password, I have to add the "ALTER ROLE 'staticrole' SET PASSWORD 'N0tS3cret';" statement in all the available hooks (after_create, after_alter, on_no_change). Or maybe, another hook just named "after" (and respectivly "before"), which gets executed no matter what ?

@bersace
Copy link
Member

bersace commented Sep 14, 2023

While this would be ultra flexible, it's also a bit harder to use. If I want to be sure my role has the expected password, I have to add the "ALTER ROLE 'staticrole' SET PASSWORD 'N0tS3cret';" statement in all the available hooks (after_create, after_alter, on_no_change). Or maybe, another hook just named "after" (and respectivly "before"), which gets executed no matter what ?

Yep. I thought of an always event. Still you can define if and then to avoid reissuing useless ALTER.

rules:
- role:
    name: toto
    always:
      if: SELECT passwd != ${password_hash} FROM pg_shadow WHERE usename = 'toto';
      then: ALTER ROLE toto SET PASSWORD ${PASSWORD_HASH}

Default if is true, to always execute then. So you can write:

rules:
- role:
    name: toto
    always: ALTER ROLE ....

@bersace bersace changed the title Implement pre/post queries [was: support setting/updating password for static roles] Implement pre/post hook queries [was: support setting/updating password for static roles] May 10, 2024
@bersace
Copy link
Member

bersace commented May 14, 2024

Hi. I implemented before_create and after_create. However, I feel something wrong regarding always. Should this be simply a run statement ?

- role: alice
  run: ALTER ROLE alice SET PASSWORD ...

How are you actually injecting passwords in YAML ? Using a preprocessor ?

@dani
Copy link
Author

dani commented May 14, 2024

Yes, I'm using a preprocessor. In the Nomad orchestrator, you can submit just the YAML template with placeholders to dynamically fetch secrets from Hashicorp vault when rendered (you just have to store the resulting file in a tmpfs to be sure they are not written to disk)

@bersace
Copy link
Member

bersace commented May 21, 2024

Hi @dani, i added before/after hook to role creation. I'm still not confident on the meaning of always. I don't want to add password field because it defeat the purpose of LDAP creation and will confuse many users. I still think that a generic inspect/run stanza may help to adapt various situations. Still not sure if we should attach this to role.

- custom:
    inspect: SELECT true; -- true to run query, false to skip.
    query: ALTER ROLE ...
    # databases ?
    # start of sync ? end of sync ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants