pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedRemove one use of IDENT_USERNAME_MAX
6+ messages / 3 participants
[nested] [flat]
* Remove one use of IDENT_USERNAME_MAX
@ 2019-10-26 06:55 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 1 reply; 6+ messages in thread
From: Peter Eisentraut @ 2019-10-26 06:55 UTC (permalink / raw)
To: pgsql-hackers
It seems to me that using IDENT_USERNAME_MAX for peer authentication is
some kind of historical leftover and not really appropriate or useful,
so I propose the attached cleanup.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 1fed5a94b801471cb380e447b5f0b924b3819be6 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sat, 26 Oct 2019 08:48:28 +0200
Subject: [PATCH] Remove one use of IDENT_USERNAME_MAX
IDENT_USERNAME_MAX is the maximum length of the information returned
by an ident server, per RFC 1413. Using it as the buffer size in peer
authentication is inappropriate. It was done here because of the
historical relationship between peer and ident authentication. But
since it's also completely useless code-wise, remove it.
---
src/backend/libpq/auth.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 0cf65ba5de..b939e8b205 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -65,7 +65,7 @@ static int CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail);
* Ident authentication
*----------------------------------------------------------------
*/
-/* Max size of username ident server can return */
+/* Max size of username ident server can return (per RFC 1413) */
#define IDENT_USERNAME_MAX 512
/* Standard TCP port number for Ident service. Assigned by IANA */
@@ -73,6 +73,11 @@ static int CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail);
static int ident_inet(hbaPort *port);
+
+/*----------------------------------------------------------------
+ * Peer authentication
+ *----------------------------------------------------------------
+ */
#ifdef HAVE_UNIX_SOCKETS
static int auth_peer(hbaPort *port);
#endif
@@ -1979,7 +1984,6 @@ ident_inet(hbaPort *port)
static int
auth_peer(hbaPort *port)
{
- char ident_user[IDENT_USERNAME_MAX + 1];
uid_t uid;
gid_t gid;
struct passwd *pw;
@@ -2011,9 +2015,7 @@ auth_peer(hbaPort *port)
return STATUS_ERROR;
}
- strlcpy(ident_user, pw->pw_name, IDENT_USERNAME_MAX + 1);
-
- return check_usermap(port->hba->usermap, port->user_name, ident_user, false);
+ return check_usermap(port->hba->usermap, port->user_name, pw->pw_name, false);
}
#endif /* HAVE_UNIX_SOCKETS */
--
2.23.0
Attachments:
[text/plain] 0001-Remove-one-use-of-IDENT_USERNAME_MAX.patch (2.1K, ../../c798fba5-8b71-4f27-c78e-37714037ea31@2ndquadrant.com/2-0001-Remove-one-use-of-IDENT_USERNAME_MAX.patch)
download | inline diff:
From 1fed5a94b801471cb380e447b5f0b924b3819be6 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sat, 26 Oct 2019 08:48:28 +0200
Subject: [PATCH] Remove one use of IDENT_USERNAME_MAX
IDENT_USERNAME_MAX is the maximum length of the information returned
by an ident server, per RFC 1413. Using it as the buffer size in peer
authentication is inappropriate. It was done here because of the
historical relationship between peer and ident authentication. But
since it's also completely useless code-wise, remove it.
---
src/backend/libpq/auth.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 0cf65ba5de..b939e8b205 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -65,7 +65,7 @@ static int CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail);
* Ident authentication
*----------------------------------------------------------------
*/
-/* Max size of username ident server can return */
+/* Max size of username ident server can return (per RFC 1413) */
#define IDENT_USERNAME_MAX 512
/* Standard TCP port number for Ident service. Assigned by IANA */
@@ -73,6 +73,11 @@ static int CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail);
static int ident_inet(hbaPort *port);
+
+/*----------------------------------------------------------------
+ * Peer authentication
+ *----------------------------------------------------------------
+ */
#ifdef HAVE_UNIX_SOCKETS
static int auth_peer(hbaPort *port);
#endif
@@ -1979,7 +1984,6 @@ ident_inet(hbaPort *port)
static int
auth_peer(hbaPort *port)
{
- char ident_user[IDENT_USERNAME_MAX + 1];
uid_t uid;
gid_t gid;
struct passwd *pw;
@@ -2011,9 +2015,7 @@ auth_peer(hbaPort *port)
return STATUS_ERROR;
}
- strlcpy(ident_user, pw->pw_name, IDENT_USERNAME_MAX + 1);
-
- return check_usermap(port->hba->usermap, port->user_name, ident_user, false);
+ return check_usermap(port->hba->usermap, port->user_name, pw->pw_name, false);
}
#endif /* HAVE_UNIX_SOCKETS */
--
2.23.0
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Remove one use of IDENT_USERNAME_MAX
@ 2019-10-28 05:10 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 1 reply; 6+ messages in thread
From: Kyotaro Horiguchi @ 2019-10-28 05:10 UTC (permalink / raw)
To: peter.eisentraut@2ndquadrant.com; +Cc: pgsql-hackers
Hello.
At Sat, 26 Oct 2019 08:55:03 +0200, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in
> IDENT_USERNAME_MAX is the maximum length of the information returned
> by an ident server, per RFC 1413. Using it as the buffer size in peer
> authentication is inappropriate. It was done here because of the
> historical relationship between peer and ident authentication. But
> since it's also completely useless code-wise, remove it.
In think one of the reasons for the coding is the fact that *pw is
described to be placed in the static area, which can be overwritten by
succeeding calls to getpw*() functions. I think we can believe
check_usermap() never calls them but I suppose that some comments
needed..
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Remove one use of IDENT_USERNAME_MAX
@ 2019-10-28 13:45 Tom Lane <tgl@sss.pgh.pa.us>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2019-10-28 13:45 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: peter.eisentraut@2ndquadrant.com; pgsql-hackers
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Sat, 26 Oct 2019 08:55:03 +0200, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in
>> IDENT_USERNAME_MAX is the maximum length of the information returned
>> by an ident server, per RFC 1413. Using it as the buffer size in peer
>> authentication is inappropriate. It was done here because of the
>> historical relationship between peer and ident authentication. But
>> since it's also completely useless code-wise, remove it.
> In think one of the reasons for the coding is the fact that *pw is
> described to be placed in the static area, which can be overwritten by
> succeeding calls to getpw*() functions.
Good point ... so maybe pstrdup instead of using a fixed-size buffer?
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Remove one use of IDENT_USERNAME_MAX
@ 2019-10-29 07:10 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 6+ messages in thread
From: Peter Eisentraut @ 2019-10-29 07:10 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: pgsql-hackers
On 2019-10-28 14:45, Tom Lane wrote:
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
>> At Sat, 26 Oct 2019 08:55:03 +0200, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in
>>> IDENT_USERNAME_MAX is the maximum length of the information returned
>>> by an ident server, per RFC 1413. Using it as the buffer size in peer
>>> authentication is inappropriate. It was done here because of the
>>> historical relationship between peer and ident authentication. But
>>> since it's also completely useless code-wise, remove it.
>
>> In think one of the reasons for the coding is the fact that *pw is
>> described to be placed in the static area, which can be overwritten by
>> succeeding calls to getpw*() functions.
>
> Good point ... so maybe pstrdup instead of using a fixed-size buffer?
Maybe. Or we just decide that check_usermap() is not allowed to call
getpw*(). It's just a string-matching routine, so it doesn't have any
such business anyway.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Remove one use of IDENT_USERNAME_MAX
@ 2019-10-29 14:34 Tom Lane <tgl@sss.pgh.pa.us>
parent: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2019-10-29 14:34 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; +Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; pgsql-hackers
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2019-10-28 14:45, Tom Lane wrote:
>> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
>>> In think one of the reasons for the coding is the fact that *pw is
>>> described to be placed in the static area, which can be overwritten by
>>> succeeding calls to getpw*() functions.
>> Good point ... so maybe pstrdup instead of using a fixed-size buffer?
> Maybe. Or we just decide that check_usermap() is not allowed to call
> getpw*(). It's just a string-matching routine, so it doesn't have any
> such business anyway.
I'm okay with that as long as you add a comment describing this
assumption.
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Remove one use of IDENT_USERNAME_MAX
@ 2019-10-30 10:19 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 6+ messages in thread
From: Peter Eisentraut @ 2019-10-30 10:19 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; pgsql-hackers
On 2019-10-29 15:34, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> On 2019-10-28 14:45, Tom Lane wrote:
>>> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
>>>> In think one of the reasons for the coding is the fact that *pw is
>>>> described to be placed in the static area, which can be overwritten by
>>>> succeeding calls to getpw*() functions.
>
>>> Good point ... so maybe pstrdup instead of using a fixed-size buffer?
>
>> Maybe. Or we just decide that check_usermap() is not allowed to call
>> getpw*(). It's just a string-matching routine, so it doesn't have any
>> such business anyway.
>
> I'm okay with that as long as you add a comment describing this
> assumption.
Committed with a pstrdup(). That seemed more consistent with other code
in that file.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2019-10-30 10:19 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-10-26 06:55 Remove one use of IDENT_USERNAME_MAX Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2019-10-28 05:10 ` Kyotaro Horiguchi <horikyota.ntt@gmail.com>
2019-10-28 13:45 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-10-29 07:10 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2019-10-29 14:34 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-10-30 10:19 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox