Re: elog(FATAL)ing non-existent roles during client

Hi all,

I noticed that during client authentication by HBA, some times we will
necessarily determine whether or not a role exists. For example, password,
crypt and md5 auth methods call get_role_line() which tells the caller
whether the role exists. If it doesn't (or if the authentication fails due
to a password mismatch) we error out.

I wonder if we should check if the role exists for the other
authentication methods too? get_role_line() should be very cheap and it
would prevent unnecessary authentication work if we did it before
contacting, for example, the client ident server. Even with trust, it
would save work because otherwise we do not check if the user exists until
InitializeSessionUserId(), at which time we're set up our proc entry etc.

This might seem overly pessimistic, I know, but it seemed to me that a
malicious user on a local network might be able to tie up a system in
interesting ways by launching lots of connections without necessarily
knowing any usernames/passwords.

Thanks,

Gavin

Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> I wonder if we should check if the role exists for the other
> authentication methods too? get_role_line() should be very cheap and it
> would prevent unnecessary authentication work if we did it before
> contacting, for example, the client ident server. Even with trust, it
> would save work because otherwise we do not check if the user exists until
> InitializeSessionUserId(), at which time we're set up our proc entry etc.

This only saves work if the supplied ID is in fact invalid, which one
would surely think isn't the normal case; otherwise it costs more.

I could see doing this in the ident path, because contacting a remote
ident server is certainly expensive on both sides. I doubt it's a good
idea in the trust case.

regards, tom lane

On Thu, 30 Nov 2006, Tom Lane wrote:

> Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> > I wonder if we should check if the role exists for the other
> > authentication methods too? get_role_line() should be very cheap and it
> > would prevent unnecessary authentication work if we did it before
> > contacting, for example, the client ident server. Even with trust, it
> > would save work because otherwise we do not check if the user exists until
> > InitializeSessionUserId(), at which time we're set up our proc entry etc.
>
> This only saves work if the supplied ID is in fact invalid, which one
> would surely think isn't the normal case; otherwise it costs more.

Yes.

> I could see doing this in the ident path, because contacting a remote
> ident server is certainly expensive on both sides. I doubt it's a good
> idea in the trust case.

Agreed. How about Kerberos too, applying the same logic?

Gavin

On Tue, 5 Dec 2006, Gavin Sherry wrote:

> On Thu, 30 Nov 2006, Tom Lane wrote:
>
> > Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> > > I wonder if we should check if the role exists for the other
> > > authentication methods too? get_role_line() should be very cheap and it
> > > would prevent unnecessary authentication work if we did it before
> > > contacting, for example, the client ident server. Even with trust, it
> > > would save work because otherwise we do not check if the user exists until
> > > InitializeSessionUserId(), at which time we're set up our proc entry etc.
> >
> > This only saves work if the supplied ID is in fact invalid, which one
> > would surely think isn't the normal case; otherwise it costs more.
>
> Yes.
>
> > I could see doing this in the ident path, because contacting a remote
> > ident server is certainly expensive on both sides. I doubt it's a good
> > idea in the trust case.
>
> Agreed. How about Kerberos too, applying the same logic?

Attached is a patch check adds the checks.

Gavin

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---------------------------------------------------------------------------

Gavin Sherry wrote:
> On Tue, 5 Dec 2006, Gavin Sherry wrote:
>
> > On Thu, 30 Nov 2006, Tom Lane wrote:
> >
> > > Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> > > > I wonder if we should check if the role exists for the other
> > > > authentication methods too? get_role_line() should be very cheap and it
> > > > would prevent unnecessary authentication work if we did it before
> > > > contacting, for example, the client ident server. Even with trust, it
> > > > would save work because otherwise we do not check if the user exists until
> > > > InitializeSessionUserId(), at which time we're set up our proc entry etc.
> > >
> > > This only saves work if the supplied ID is in fact invalid, which one
> > > would surely think isn't the normal case; otherwise it costs more.
> >
> > Yes.
> >
> > > I could see doing this in the ident path, because contacting a remote
> > > ident server is certainly expensive on both sides. I doubt it's a good
> > > idea in the trust case.
> >
> > Agreed. How about Kerberos too, applying the same logic?
>
> Attached is a patch check adds the checks.
>
> Gavin
Content-Description:

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: In versions below 8.0, the planner will ignore your desire to
> choose an index scan if your joining column's datatypes do not
> match

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Patch applied. Thanks.

---------------------------------------------------------------------------

Gavin Sherry wrote:
> On Tue, 5 Dec 2006, Gavin Sherry wrote:
>
> > On Thu, 30 Nov 2006, Tom Lane wrote:
> >
> > > Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> > > > I wonder if we should check if the role exists for the other
> > > > authentication methods too? get_role_line() should be very cheap and it
> > > > would prevent unnecessary authentication work if we did it before
> > > > contacting, for example, the client ident server. Even with trust, it
> > > > would save work because otherwise we do not check if the user exists until
> > > > InitializeSessionUserId(), at which time we're set up our proc entry etc.
> > >
> > > This only saves work if the supplied ID is in fact invalid, which one
> > > would surely think isn't the normal case; otherwise it costs more.
> >
> > Yes.
> >
> > > I could see doing this in the ident path, because contacting a remote
> > > ident server is certainly expensive on both sides. I doubt it's a good
> > > idea in the trust case.
> >
> > Agreed. How about Kerberos too, applying the same logic?
>
> Attached is a patch check adds the checks.
>
> Gavin
Content-Description:

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: In versions below 8.0, the planner will ignore your desire to
> choose an index scan if your joining column's datatypes do not
> match

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +