Обсуждение: Proposal: SET ROLE hook
In many environments there is a policy requiring users to login using unprivileged accounts, and then escalate their privileges if and when they require it. In PostgreSQL this could be done by granting the superuser role to an unprivileged user with noinherit, and then making the superuser role nologin. Something like: 8<------------- psql -U postgres create user joe noinherit; grant postgres to joe; alter user postgres nologin; \q psql -U joe -- do stuff *not requiring* escalated privs set role postgres; -- do stuff *requiring* escalated privs reset role; 8<------------- One of the problems with this is we would ideally like to know whenever joe escalates himself to postgres. Right now that is not really possible without doing untenable things such as logging every statement. In order to address this issue, I am proposing a new SET ROLE hook. The attached patch (role-esc-hook.diff) is I believe all we need. Then extension authors could implement logging of privilege escalation. A proof of concept extension patch is also attached. That one is not meant to be applied, just illustrates one potential use of the hook. I just smashed it on top of passwordcheck for the sake of convenience. With both patches applied, the following scenario: 8<------------------------ psql -U joe postgres psql (9.6devel) Type "help" for help. postgres=> set role postgres; SET postgres=# select rolname, rolpassword from pg_authid; rolname | rolpassword ----------+------------- joe | postgres | (2 rows) postgres=# set log_statement = none; SET postgres=# reset role; RESET 8<------------------------ Generates the following in the log: 8<------------------------ LOG: Role joe transitioning to Superuser Role postgres STATEMENT: set role postgres; LOG: statement: select rolname, rolpassword from pg_authid; LOG: statement: set log_statement = none; LOG: Superuser Role postgres transitioning to Role joe STATEMENT: reset role; 8<------------------------ Note that we cannot prevent joe from executing set log_statement = none; but we at least get the evidence in the log and can ask joe why he felt the need to do that. We could also set up alerts based on the logged events, etc. This particular hook will not capture role changes due to SECURITY DEFINER functions, but I think that is not only ok but preferred. Escalation during a SECURITY DEFINER function is a preplanned sanctioned event, unlike an ad hoc unconstrained role change to superuser. And given the demo patch, we could see any SECURITY DEFINER function created by the superuser when it gets created in the log (which again is subject to auditing, alerts, etc.) Ultimately I would also like to see a general hook available which would fire for all changes to GUC settings, but I think this one is still very useful as it is highly targeted. Comments? Thanks, Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Вложения
On 2015-10-16 09:20:26 -0700, Joe Conway wrote: > One of the problems with this is we would ideally like to know whenever > joe escalates himself to postgres. Right now that is not really possible > without doing untenable things such as logging every statement. Alternatively you can just have a elevate_user() function that does the logging and escalating? That seems like the same amount of code and it'd work with released versions of postgres? Sure, that has some disandvantages over your approach, but for the presented use case with humans needing to escalate I don't see any. Greetings, Andres Freund
On 10/16/2015 09:28 AM, Andres Freund wrote: > Alternatively you can just have a elevate_user() function that does the > logging and escalating? That seems like the same amount of code and it'd > work with released versions of postgres? > > Sure, that has some disadvantages over your approach, but for the > presented use case with humans needing to escalate I don't see any. Hmmm, do you mean essentially skip the "GRANT postgres to joe" and use a SECURITY DEFINER C function that does the set role to postgres under the covers with "GRANT EXECUTE on FUNCTION elevate_user() to joe"? Being able to use something like that on existing versions would be very nice, but it feels kind of grotty. Or maybe you mean something else? Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Joe Conway <mail@joeconway.com> writes: > In many environments there is a policy requiring users to login using > unprivileged accounts, and then escalate their privileges if and when > they require it. In PostgreSQL this could be done by granting the > superuser role to an unprivileged user with noinherit, and then making > the superuser role nologin. Something like: > > 8<------------- > psql -U postgres > create user joe noinherit; > grant postgres to joe; > alter user postgres nologin; A bit off-topic, but I've often had to create an intermediate role with no-inherit between the gifted role to permit accessing it without the grantee losing ability to inherit some other(s). To wit; We have an owner role for a given DB who is supposed to own most/all objects. We might want 1 or more other roles/users to be able to run as owner guy to create objects... We want to enforce that objects so created are automatically owned by the correct owner role. create role owner; create role owner_role no inherit in role owner; create role read_write; create role jerry in role read_write, owner_role; I can by default run as the read_write guy and mangle data to my little heart's content (the super-dev that I am) but if a new table is needs creation I must do set role owner. This insures that the table is owned by owner and not jerry afterward. Often wondered why not; grant owner to jerry no inherit; FWIW > psql -U joe > -- do stuff *not requiring* escalated privs > set role postgres; > -- do stuff *requiring* escalated privs > reset role; > 8<------------- > > One of the problems with this is we would ideally like to know whenever > joe escalates himself to postgres. Right now that is not really possible > without doing untenable things such as logging every statement. > > In order to address this issue, I am proposing a new SET ROLE hook. The > attached patch (role-esc-hook.diff) is I believe all we need. Then > extension authors could implement logging of privilege escalation. > > A proof of concept extension patch is also attached. That one is not > meant to be applied, just illustrates one potential use of the hook. I > just smashed it on top of passwordcheck for the sake of convenience. > > With both patches applied, the following scenario: > 8<------------------------ > psql -U joe postgres > psql (9.6devel) > Type "help" for help. > > postgres=> set role postgres; > SET > postgres=# select rolname, rolpassword from pg_authid; > rolname | rolpassword > ----------+------------- > joe | > postgres | > (2 rows) > > postgres=# set log_statement = none; > SET > postgres=# reset role; > RESET > 8<------------------------ > > Generates the following in the log: > > 8<------------------------ > LOG: Role joe transitioning to Superuser Role postgres > STATEMENT: set role postgres; > LOG: statement: select rolname, rolpassword from pg_authid; > LOG: statement: set log_statement = none; > LOG: Superuser Role postgres transitioning to Role joe > STATEMENT: reset role; > 8<------------------------ > > Note that we cannot prevent joe from executing > set log_statement = none; > but we at least get the evidence in the log and can ask joe why he felt > the need to do that. We could also set up alerts based on the logged > events, etc. > > This particular hook will not capture role changes due to SECURITY > DEFINER functions, but I think that is not only ok but preferred. > Escalation during a SECURITY DEFINER function is a preplanned sanctioned > event, unlike an ad hoc unconstrained role change to superuser. And > given the demo patch, we could see any SECURITY DEFINER function created > by the superuser when it gets created in the log (which again is subject > to auditing, alerts, etc.) > > Ultimately I would also like to see a general hook available which would > fire for all changes to GUC settings, but I think this one is still very > useful as it is highly targeted. > > Comments? > > Thanks, > > Joe -- Jerry Sievers Postgres DBA/Development Consulting e: postgres.consulting@comcast.net p: 312.241.7800
On 2015-10-16 10:30:20 -0700, Joe Conway wrote: > On 10/16/2015 09:28 AM, Andres Freund wrote: > > Alternatively you can just have a elevate_user() function that does the > > logging and escalating? That seems like the same amount of code and it'd > > work with released versions of postgres? > > > > Sure, that has some disadvantages over your approach, but for the > > presented use case with humans needing to escalate I don't see any. > > Hmmm, do you mean essentially skip the "GRANT postgres to joe" and use a > SECURITY DEFINER C function that does the set role to postgres under the > covers with "GRANT EXECUTE on FUNCTION elevate_user() to joe"? Yes. > Being able to use something like that on existing versions would be > very nice, but it feels kind of grotty. Hm. To me it doesn't feel too bad - security definer functions are there to allow to do things that users would normally not be allowed to do... Greetings, Andres Freund
On 10/16/15 12:51 PM, Andres Freund wrote: >> >Hmmm, do you mean essentially skip the "GRANT postgres to joe" and use a >> >SECURITY DEFINER C function that does the set role to postgres under the >> >covers with "GRANT EXECUTE on FUNCTION elevate_user() to joe"? > Yes. > >> >Being able to use something like that on existing versions would be >> >very nice, but it feels kind of grotty. > Hm. To me it doesn't feel too bad - security definer functions are there > to allow to do things that users would normally not be allowed to do... Tons of environments can't use C functions, and ignoring that it still feels ugly. How much harder would it be to add SET ROLE to event triggers? (Could be done in conjunction with the hook; they serve different purposes.) -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
Hi
2015-10-16 18:20 GMT+02:00 Joe Conway <mail@joeconway.com>:
In many environments there is a policy requiring users to login using
unprivileged accounts, and then escalate their privileges if and when
they require it. In PostgreSQL this could be done by granting the
superuser role to an unprivileged user with noinherit, and then making
the superuser role nologin. Something like:
8<-------------
psql -U postgres
create user joe noinherit;
grant postgres to joe;
alter user postgres nologin;
\q
psql -U joe
-- do stuff *not requiring* escalated privs
set role postgres;
-- do stuff *requiring* escalated privs
reset role;
8<-------------
One of the problems with this is we would ideally like to know whenever
joe escalates himself to postgres. Right now that is not really possible
without doing untenable things such as logging every statement.
In order to address this issue, I am proposing a new SET ROLE hook. The
attached patch (role-esc-hook.diff) is I believe all we need. Then
extension authors could implement logging of privilege escalation.
A proof of concept extension patch is also attached. That one is not
meant to be applied, just illustrates one potential use of the hook. I
just smashed it on top of passwordcheck for the sake of convenience.
With both patches applied, the following scenario:
8<------------------------
psql -U joe postgres
psql (9.6devel)
Type "help" for help.
postgres=> set role postgres;
SET
postgres=# select rolname, rolpassword from pg_authid;
rolname | rolpassword
----------+-------------
joe |
postgres |
(2 rows)
postgres=# set log_statement = none;
SET
postgres=# reset role;
RESET
8<------------------------
Generates the following in the log:
8<------------------------
LOG: Role joe transitioning to Superuser Role postgres
STATEMENT: set role postgres;
LOG: statement: select rolname, rolpassword from pg_authid;
LOG: statement: set log_statement = none;
LOG: Superuser Role postgres transitioning to Role joe
STATEMENT: reset role;
8<------------------------
Note that we cannot prevent joe from executing
set log_statement = none;
but we at least get the evidence in the log and can ask joe why he felt
the need to do that. We could also set up alerts based on the logged
events, etc.
This particular hook will not capture role changes due to SECURITY
DEFINER functions, but I think that is not only ok but preferred.
Escalation during a SECURITY DEFINER function is a preplanned sanctioned
event, unlike an ad hoc unconstrained role change to superuser. And
given the demo patch, we could see any SECURITY DEFINER function created
by the superuser when it gets created in the log (which again is subject
to auditing, alerts, etc.)
Ultimately I would also like to see a general hook available which would
fire for all changes to GUC settings, but I think this one is still very
useful as it is highly targeted.
Comments?
I read discussion in this thread and I am thinking so creating hook is the most simple and workable solution.
Personally, I am not sure if missing support SECURE DEFINER function is ok. When you do some secure audit, probably any role change can be interesting. This situation can be distinguished by another hook parameter.
Support of event triggers can be other topic, nice to have it but not necessary in first moment.
Regards
Pavel
Thanks,
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Ultimately I would also like to see a general hook available which would
fire for all changes to GUC settings, but I think this one is still very
useful as it is highly targeted.
Comments?I read discussion in this thread and I am thinking so creating hook is the most simple and workable solution.Personally, I am not sure if missing support SECURE DEFINER function is ok. When you do some secure audit, probably any role change can be interesting. This situation can be distinguished by another hook parameter.
I though more about this topic, and my request was +/- premature optimization. It can be solved simply in future by different hook when it be requested. This should not be blocker.
Regards
Pavel
Support of event triggers can be other topic, nice to have it but not necessary in first moment.RegardsPavel
Thanks,
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Hi
I did a review of this patch.On 01/06/2016 02:39 AM, Pavel Stehule wrote: > I did a review of this patch. > > 1. the proposal is clean and there are not any objection against it. I > checked a implementation, and it does exactly same what was proposed. > > 2. This hook has impact only on SET role to XXX statement, what isn't > used in our critical path, so there are not any performance impact > > 3. I was able to apply patch cleanly without any problems, warnings. > > 4. All regress tests was passed > > 5. there are not tests and documentation, but it is usual for any hook > > I have not any objection > > I'll mark this patch as ready for committer Thanks for the review! For what it's worth, I did look at Andres' idea and in fact created an extension available here: https://github.com/pgaudit/set_user I also looked at implementing equivalent functionality with the ProcessUtility_hook. That appears feasible but would involve a fair amount of code duplication from the backend. Compared to both of these alternatives, I still feel that the specific SET ROLE hook is cleanest and best path forward. If there are no other comments or concerns, I will commit this in a day or two. Thanks, Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Joe Conway <mail@joeconway.com> writes:
> Compared to both of these alternatives, I still feel that the specific
> SET ROLE hook is cleanest and best path forward. If there are no other
> comments or concerns, I will commit this in a day or two.
While I don't think there's any great harm in inventing such a hook, I'm
not sure it's going to be all that useful where placed. GUC assign_hooks
basically cannot risk throwing errors, which enormously restricts what can
safely be done inside the proposed hook: it would be unwise to do catalog
accesses, for example. (Which means I think the example usage is broken;
in fact, it's already broken by your note that the code has to be able to
execute in a failed transaction.)
I think a design that was actually somewhat robust would require two
hooks, one at check_role and one at assign_role, wherein the first one
would do any potentially-failing work and package all required info into
a blob that could be passed through to the assign hook.
regards, tom lane
On 01/06/2016 10:36 AM, Tom Lane wrote: > I think a design that was actually somewhat robust would require two > hooks, one at check_role and one at assign_role, wherein the first one > would do any potentially-failing work and package all required info into > a blob that could be passed through to the assign hook. Fair enough -- will rework the patch and example code. Thanks! Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Joe Conway wrote: > On 01/06/2016 10:36 AM, Tom Lane wrote: > > I think a design that was actually somewhat robust would require two > > hooks, one at check_role and one at assign_role, wherein the first one > > would do any potentially-failing work and package all required info into > > a blob that could be passed through to the assign hook. > > Fair enough -- will rework the patch and example code. Returned with feedback. Thanks. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 01/07/2016 09:08 AM, Joe Conway wrote: > On 01/06/2016 10:36 AM, Tom Lane wrote: >> I think a design that was actually somewhat robust would require two >> hooks, one at check_role and one at assign_role, wherein the first one >> would do any potentially-failing work and package all required info into >> a blob that could be passed through to the assign hook. Attached. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Вложения
Hi
2016-02-29 2:40 GMT+01:00 Joe Conway <mail@joeconway.com>:
On 01/07/2016 09:08 AM, Joe Conway wrote:
> On 01/06/2016 10:36 AM, Tom Lane wrote:
>> I think a design that was actually somewhat robust would require two
>> hooks, one at check_role and one at assign_role, wherein the first one
>> would do any potentially-failing work and package all required info into
>> a blob that could be passed through to the assign hook.
Attached.
These patches are pretty trivial, and I can confirm so all regress tests are passed.
I see following issues:
1. Missing the possibility to pass custom data from SetRoleCheck_hook to SetRoleAssign_hook. Tom mentioned it in his comment.
2. Missing little bit more comments and an explanation why and when to use these hooks.
Regards
Pavel
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Вложения
On 03/01/2016 02:09 AM, Pavel Stehule wrote: > 2016-02-29 2:40 GMT+01:00 Joe Conway <mail@joeconway.com > <mailto:mail@joeconway.com>>: > > On 01/07/2016 09:08 AM, Joe Conway wrote: > > On 01/06/2016 10:36 AM, Tom Lane wrote: > >> I think a design that was actually somewhat robust would require two > >> hooks, one at check_role and one at assign_role, wherein the first one > >> would do any potentially-failing work and package all required info into > >> a blob that could be passed through to the assign hook. > I see following issues: > > 1. Missing the possibility to pass custom data from SetRoleCheck_hook to > SetRoleAssign_hook. Tom mentioned it in his comment. You can pass the data between the two plugin hook functions in your extension itself, so I see no need to try to pass custom data through the backend. Do any of the other hooks even do that? I think the main point was to have two hooks. The potentially-failing work could be done during the check_role() hook and the collected info could be used during the assign_role() hook. This works fine with the patch as-is. > 2. Missing little bit more comments and an explanation why and when to > use these hooks. Doesn't look all that different from existing user hooks to me, but sure, I'll add a bit more to the comments. The thing I wish we had was a place in the docs where we list all the user plugin hooks. But as far as I know that doesn't exist (please correct me if I'm wrong) and I am not volunteering to create it just for the sake of this patch ;-) Thanks for the review! Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
2016-03-01 16:52 GMT+01:00 Joe Conway <mail@joeconway.com>:
On 03/01/2016 02:09 AM, Pavel Stehule wrote:
> 2016-02-29 2:40 GMT+01:00 Joe Conway <mail@joeconway.com
> <mailto:mail@joeconway.com>>:
>
> On 01/07/2016 09:08 AM, Joe Conway wrote:
> > On 01/06/2016 10:36 AM, Tom Lane wrote:
> >> I think a design that was actually somewhat robust would require two
> >> hooks, one at check_role and one at assign_role, wherein the first one
> >> would do any potentially-failing work and package all required info into
> >> a blob that could be passed through to the assign hook.
> I see following issues:
>
> 1. Missing the possibility to pass custom data from SetRoleCheck_hook to
> SetRoleAssign_hook. Tom mentioned it in his comment.
You can pass the data between the two plugin hook functions in your
extension itself, so I see no need to try to pass custom data through
the backend. Do any of the other hooks even do that?
I don't know about it, but these hooks are specific. is it ensured a order of calls of these hooks?
I think the main point was to have two hooks. The potentially-failing
work could be done during the check_role() hook and the collected info
could be used during the assign_role() hook. This works fine with the
patch as-is.
> 2. Missing little bit more comments and an explanation why and when to
> use these hooks.
Doesn't look all that different from existing user hooks to me, but
sure, I'll add a bit more to the comments.
I would to see more lines about just corner cases. Can/cannot to raise a exception there, can/cannot to access system catalogue there. I have negative experience with missing these corner cases with log hook :).
some like "I think the main point was to have two hooks. The potentially-failing
work could be done during the check_role() hook and the collected info
could be used during the assign_role() hook."
work could be done during the check_role() hook and the collected info
could be used during the assign_role() hook."
The thing I wish we had was a place in the docs where we list all the
user plugin hooks. But as far as I know that doesn't exist (please
correct me if I'm wrong) and I am not volunteering to create it just for
the sake of this patch ;-)
This doc can be nice, but it is out of scope of this patch, and I don't require it :)
Regards
Pavel
Thanks for the review!
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
On 03/01/2016 08:18 AM, Pavel Stehule wrote: > 2016-03-01 16:52 GMT+01:00 Joe Conway: > On 03/01/2016 02:09 AM, Pavel Stehule wrote: > > > On 01/06/2016 10:36 AM, Tom Lane wrote: > > >> I think a design that was actually somewhat robust would require two > > >> hooks, one at check_role and one at assign_role, wherein the first one > > >> would do any potentially-failing work and package all required info into > > >> a blob that could be passed through to the assign hook. > > > I see following issues: > > > > 1. Missing the possibility to pass custom data from SetRoleCheck_hook to > > SetRoleAssign_hook. Tom mentioned it in his comment. > > You can pass the data between the two plugin hook functions in your > extension itself, so I see no need to try to pass custom data through > the backend. Do any of the other hooks even do that? > > I don't know about it, but these hooks are specific. is it ensured a > order of calls of these hooks? > > 2. Missing little bit more comments and an explanation why and when to > > use these hooks. > > Doesn't look all that different from existing user hooks to me, but > sure, I'll add a bit more to the comments. > some like "I think the main point was to have two hooks. The > potentially-failing > work could be done during the check_role() hook and the collected info > could be used during the assign_role() hook." Ok, I added a comment similar to that at the check_role() function hook call site. Updated patch attached. I still don't see any point in trying to pass data back from the hooks as the extension can maintain that state just fine, although it looks like it would be pretty trivial to do using a new void* member added to role_auth_extra. Tom (or anyone else), any comment on that? I do however find myself wishing I could pass the action down from set_config_option() to at least the assign_role() hook, but that seems more invasive than I'd like. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Вложения
On 6 March 2016 at 04:49, Joe Conway <mail@joeconway.com> wrote:
I still don't see any point in trying to pass data back from the hooks
as the extension can maintain that state just fine, although it looks
like it would be pretty trivial to do using a new void* member added to
role_auth_extra. Tom (or anyone else), any comment on that?
I think it's possibly worth doing, but since auth is simple and linear I'm not overly bothered. It'd hardly be the first place Pg relied on passing information between functions using globals.
If you expect to daisy-chain hooks then a private-data member isn't too helpful, since a prior hook in the call chain may have set it already and you don't know what's there or how to manage it. So overall, I'm -1 for a private_data arg, I don't think it gains us anything in lifetime management, code clarity etc and it makes daisy-chaining way more complex.
I don't see what the point of SetRoleAssign_hook is, since it returns void and doesn't have out parameters. If it's expected to takes some action, what is it? If it's meant to modify myextra->roleid and myextra->is_superuser prior to their use by SetCurrentRoleId they should be passed pointer-indirected so they can be modified by the hook.
I'd like to see parameter names specified in the hook type definition.
It needs tests added, along with a note somewhere on usage of the hook that mentions the usual pattern for using it, possibly in the test/example. Something like:
static SetRoleCheck_hook_type previous_SetRoleCheck_hook = NULL;
void my_check_role(Oid sessionuserid, Oid wanted_roleid, Oid issuper)
{
if (previous_SetRoleCheck_hook)
previous_SetRoleCheck_hook(sessionuserid, wanted_roleid, issuper);
/* do my stuff here */
}
_PG_init()
{
if (SetRoleCheck_hook)
previous_SetRoleCheck_hook = SetRoleCheck_hook;
SetRoleCheck_hook = my_check_role;
}
Also behaviour of each hook should be more completely specified. Can it elog(ERROR)? What happens if it does? What can it safely change and what not? Are there restrictions to what it can do in terms of access to syscache/relcache/heap etc?
Why do you pass GetSessionUserId() to the hook given that the caller can look it up directly? Just a convenience?
Joe Conway <mail@joeconway.com> writes:
> I still don't see any point in trying to pass data back from the hooks
> as the extension can maintain that state just fine, although it looks
> like it would be pretty trivial to do using a new void* member added to
> role_auth_extra. Tom (or anyone else), any comment on that?
The point of packaging GUC-related state into a blob that guc.c
knows about is that then the right things will happen when guc.c handles
something like a SET LOCAL, GUC reversion at subtransaction rollback,
SET clauses attached to functions, yadda yadda. Are you sure your
extension can, or wants to, track all those possibilities for itself?
I remember thinking that we probably would need to extend role_auth_extra
to make this work, so I have no objection if you're finding that that's
actually the case. Might need to think about how more than one hook
could include state into the blob.
(Note: I've not actually read this version of your patch, although
I could make time for that next week sometime.)
regards, tom lane
2016-03-05 21:49 GMT+01:00 Joe Conway <mail@joeconway.com>:
On 03/01/2016 08:18 AM, Pavel Stehule wrote:
> 2016-03-01 16:52 GMT+01:00 Joe Conway:
> On 03/01/2016 02:09 AM, Pavel Stehule wrote:
> > > On 01/06/2016 10:36 AM, Tom Lane wrote:
> > >> I think a design that was actually somewhat robust would require two
> > >> hooks, one at check_role and one at assign_role, wherein the first one
> > >> would do any potentially-failing work and package all required info into
> > >> a blob that could be passed through to the assign hook.
>
> > I see following issues:
> >
> > 1. Missing the possibility to pass custom data from SetRoleCheck_hook to
> > SetRoleAssign_hook. Tom mentioned it in his comment.
>
> You can pass the data between the two plugin hook functions in your
> extension itself, so I see no need to try to pass custom data through
> the backend. Do any of the other hooks even do that?
>
> I don't know about it, but these hooks are specific. is it ensured a
> order of calls of these hooks?
> > 2. Missing little bit more comments and an explanation why and when to
> > use these hooks.
>
> Doesn't look all that different from existing user hooks to me, but
> sure, I'll add a bit more to the comments.
> some like "I think the main point was to have two hooks. The
> potentially-failing
> work could be done during the check_role() hook and the collected info
> could be used during the assign_role() hook."
Ok, I added a comment similar to that at the check_role() function hook
call site. Updated patch attached.
the comments are good enough now
I still don't see any point in trying to pass data back from the hooks
as the extension can maintain that state just fine, although it looks
like it would be pretty trivial to do using a new void* member added to
role_auth_extra. Tom (or anyone else), any comment on that?
see Tom's comment, I share his opinion.
I do however find myself wishing I could pass the action down from
set_config_option() to at least the assign_role() hook, but that seems
more invasive than I'd like.
describe this use case, please.
In this case, I don't afraid of some possible API changes in future. This API is "invisible" for common user. So it should not to handle all possible use cases.
For me, the possibility to control private data is in category better to have, the extension can be safer, smaller, but probably 75% of potential customer will be happy from current state of this patch.
On the other hand, if we can do some simple changes (few or little bit more than few lines), why don't do it now?
Pavel
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
On 3/6/16 1:17 AM, Pavel Stehule wrote: > 2016-03-05 21:49 GMT+01:00 Joe Conway <mail@joeconway.com > <mailto:mail@joeconway.com>>: > > I still don't see any point in trying to pass data back from the hooks > as the extension can maintain that state just fine, although it looks > like it would be pretty trivial to do using a new void* member added to > role_auth_extra. Tom (or anyone else), any comment on that? > > see Tom's comment, I share his opinion. > > I do however find myself wishing I could pass the action down from > set_config_option() to at least the assign_role() hook, but that seems > more invasive than I'd like. > > describe this use case, please. Joe, it looks there are some unresolved questions from Pavel and Craig on this thread and probably a new patch is required. Any idea when you can get to that? Thanks, -- -David david@pgmasters.net
On 03/16/2016 08:28 AM, David Steele wrote: > On 3/6/16 1:17 AM, Pavel Stehule wrote: > >> 2016-03-05 21:49 GMT+01:00 Joe Conway <mail@joeconway.com >> <mailto:mail@joeconway.com>>: >> >> I still don't see any point in trying to pass data back from the hooks >> as the extension can maintain that state just fine, although it looks >> like it would be pretty trivial to do using a new void* member added to >> role_auth_extra. Tom (or anyone else), any comment on that? >> >> see Tom's comment, I share his opinion. >> >> I do however find myself wishing I could pass the action down from >> set_config_option() to at least the assign_role() hook, but that seems >> more invasive than I'd like. >> >> describe this use case, please. > > Joe, it looks there are some unresolved questions from Pavel and Craig > on this thread and probably a new patch is required. Any idea when you > can get to that? I won't likely have time for a day or two, but will definitely get back to this worst case this coming weekend. Thanks, Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development