Thread: Allow +group in pg_ident.conf
Over at [1] I speculated that it might be a good idea to allow +grouprole type user names in pg_ident.conf. The use case I have in mind is where the user authenticates to pgbouncer and then pgbouncer connects as the user using a client certificate. Without this mechanism that means that you need a mapping rule for each user in pg_ident.conf, which doesn't scale very well, but with this mechanism all you have to do is grant the specified role to users. So here's a small patch for that. Comments welcome. cheers andrew [1] https://postgr.es/m/6912eb9c-4905-badb-ad87-eeca8ace13e7@dunslane.net -- Andrew Dunstan EDB: https://www.enterprisedb.com
Attachment
On Mon, Jan 09, 2023 at 08:00:26AM -0500, Andrew Dunstan wrote:
> + If the <replaceable>database-username</replaceable> begins with a
> + <literal>+</literal> character, then the operating system user can login as
> + any user belonging to that role, similarly to how user names beginning with
> + <literal>+</literal> are treated in <literal>pg_hba.conf</literal>.
I would ѕuggest making it clear that this means role membership and not
privileges via INHERIT.
> - if (case_insensitive)
> + if (regexp_pgrole[0] == '+')
> + {
> + Oid roleid = get_role_oid(pg_role, true);
> + if (is_member(roleid, regexp_pgrole +1))
> + *found_p = true;
> + }
> + else if (case_insensitive)
It looks like the is_member() check will always be case-sensitive. Should
it respect the value of case_insensitive? If not, I think there should be
a brief comment explaining why.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On 2023-01-09 Mo 13:24, Nathan Bossart wrote:
> On Mon, Jan 09, 2023 at 08:00:26AM -0500, Andrew Dunstan wrote:
>> + If the <replaceable>database-username</replaceable> begins with a
>> + <literal>+</literal> character, then the operating system user can login as
>> + any user belonging to that role, similarly to how user names beginning with
>> + <literal>+</literal> are treated in <literal>pg_hba.conf</literal>.
> I would ѕuggest making it clear that this means role membership and not
> privileges via INHERIT.
I've adapted a sentence from the pg_hba.conf documentation so we stay
consistent.
>> - if (case_insensitive)
>> + if (regexp_pgrole[0] == '+')
>> + {
>> + Oid roleid = get_role_oid(pg_role, true);
>> + if (is_member(roleid, regexp_pgrole +1))
>> + *found_p = true;
>> + }
>> + else if (case_insensitive)
> It looks like the is_member() check will always be case-sensitive. Should
> it respect the value of case_insensitive? If not, I think there should be
> a brief comment explaining why.
It's not really relevant. We're not comparing role names here; rather we
look up two roles and then ask if one is a member of the other. I've
added a comment.
Thanks for the review (I take it you're generally in favor).
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Attachment
This seems very much related to my patch here: https://commitfest.postgresql.org/41/4081/ (yes, somehow the thread got split. I blame outlook) I'll try to review this one soonish.
On Mon, Jan 09, 2023 at 05:33:14PM -0500, Andrew Dunstan wrote:
> I've adapted a sentence from the pg_hba.conf documentation so we stay
> consistent.
+ <para>
+ If the <replaceable>database-username</replaceable> begins with a
+ <literal>+</literal> character, then the operating system user can login as
+ any user belonging to that role, similarly to how user names beginning with
+ <literal>+</literal> are treated in <literal>pg_hba.conf</literal>.
+ Thus, a <literal>+</literal> mark means <quote>match any of the roles that
+ are directly or indirectly members of this role</quote>, while a name
+ without a <literal>+</literal> mark matches only that specific role.
+ </para>
Should this also mention that the behavior is enforced even in cases
where we expect a case-sensitive match?
> It's not really relevant. We're not comparing role names here; rather we
> look up two roles and then ask if one is a member of the other. I've
> added a comment.
>
> Thanks for the review (I take it you're generally in favor).
- if (case_insensitive)
+ if (regexp_pgrole[0] == '+')
+ {
+ /*
+ * Since we're not comparing role names here, use of case
+ * insensitive matching doesn't really apply.
+ */
+ Oid roleid = get_role_oid(pg_role, true);
+ Assert(false);
+ if (is_member(roleid, regexp_pgrole +1))
+ *found_p = true;
+ }
+ else if (case_insensitive)
Hmm. As check_ident_usermap() is now coded, it means that the case of
a syntax substitution could be enforced to use a group with the user
name given by the client. For example, take this ident entry:
mymap /^(.*)@mydomain\.com$ \1
Then, if we attempt to access Postgres with "+testrole@mydomain.com",
we would get a substitution to "+testrole", which would be enforced to
check on is_member() with this expected role name. I am not sure what
should be the correct behavior here.
--
Michael
Attachment
Having looked closer now, I'm pretty sure you should base this patch on top of my patch: https://commitfest.postgresql.org/41/4081/ Mainly because you also need the token version of pg_role, which is one of the things my patch adds. > if (regexp_pgrole[0] == '+') For these lines you'll need to check if the original token was quoted. If it's quoted it shouldn't use the group behaviour, and instead compare the + character as part of the literal role. > if (is_member(roleid, regexp_pgrole +1)) > if (is_member(roleid, ++map_role)) You use these two checks to do the same, so it's best if they are written consistently. > if (regexp_pgrole[0] == '+') This check can be moved before the following line and do an early return (like I do for "all" in my patch). Since if the first character is a + we know that it's not \1 and thus we don't have to worry about getting the regex match. > if ((ofs = strstr(identLine->pg_role->string, "\\1")) != NULL)
On 2023-01-10 Tu 07:09, Jelte Fennema wrote: > Having looked closer now, I'm pretty sure you should base this patch > on top of my patch: https://commitfest.postgresql.org/41/4081/ > Mainly because you also need the token version of pg_role, which is > one of the things my patch adds. Ok, that sounds reasonable, but the cfbot doesn't like patches that depend on other patches in a different email. Maybe you can roll this up as an extra patch in your next version? It's pretty small. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Tue, Jan 10, 2023 at 09:42:19AM -0500, Andrew Dunstan wrote: > Ok, that sounds reasonable, but the cfbot doesn't like patches that > depend on other patches in a different email. Maybe you can roll this up > as an extra patch in your next version? It's pretty small. This can go two ways if both of you agree, by sending an updated patch on this thread based on the other one.. And actually, Jelte's patch has less C code than this thread's proposal, eventhough it lacks tests. -- Michael
Attachment
I'm working on a new patchset for my commitfest entry. I'll make sure to include a third patch for the +group support, and I'll include you (Andrew) in the thread when I send it. On Wed, 11 Jan 2023 at 02:14, Michael Paquier <michael@paquier.xyz> wrote: > > On Tue, Jan 10, 2023 at 09:42:19AM -0500, Andrew Dunstan wrote: > > Ok, that sounds reasonable, but the cfbot doesn't like patches that > > depend on other patches in a different email. Maybe you can roll this up > > as an extra patch in your next version? It's pretty small. > > This can go two ways if both of you agree, by sending an updated patch > on this thread based on the other one.. And actually, Jelte's patch > has less C code than this thread's proposal, eventhough it lacks > tests. > -- > Michael
On Wed, 11 Jan 2023 at 04:00, Jelte Fennema <me@jeltef.nl> wrote: > > I'm working on a new patchset for my commitfest entry. So I'll set it to "Waiting on Author" pending that new patchset... -- Gregory Stark As Commitfest Manager
On Wed, Mar 01, 2023 at 02:26:21PM -0500, Gregory Stark (as CFM) wrote: > So I'll set it to "Waiting on Author" pending that new patchset... There is still an entry as of https://commitfest.postgresql.org/42/4112/. Support for group detection in pg_ident.conf has been added in efb6f4a already, so I have switched this entry as committed. We were also aware of Andrew's proposal on the other commit, and it was much easier to just group everything together. -- Michael