Обсуждение: 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

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

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

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

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