Re: Reporting hba lines

When debugging strange and complex pg_hba lines, it can often be quite
useful to know which line is matching a particular connection that
failed for some reason. Because more often than not, it's actually not
using the line in pg_hba.conf that's expected.

The easiest way to do this is to emit an errdetail for the login
failure, per this patch.

Question is - is that leaking information to the client that we
shouldn't be leaking?

And if it is, what would be the preferred way to deal with it? We
could put that as a detail to basically every single error message
coming out of the auth system, but that seems like a bad idea. Or we
could make a separate ereport(LOG) before send it to the client,
perhaps?

Thoughts?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Le mercredi 27 juin 2012 14:54:15, Magnus Hagander a écrit :
> When debugging strange and complex pg_hba lines, it can often be quite
> useful to know which line is matching a particular connection that
> failed for some reason. Because more often than not, it's actually not
> using the line in pg_hba.conf that's expected.
>
> The easiest way to do this is to emit an errdetail for the login
> failure, per this patch.
>
> Question is - is that leaking information to the client that we
> shouldn't be leaking?

I fear it is exactly the problem. Too bad because the feature is interesting.

> And if it is, what would be the preferred way to deal with it? We
> could put that as a detail to basically every single error message
> coming out of the auth system, but that seems like a bad idea. Or we
> could make a separate ereport(LOG) before send it to the client,
> perhaps?

The problem is also that 1/ you're not sure how you did connect exactly 2/
you're not connecting like you wanted to do. (my case with ipv4 vs ipv6 just
below). Providing more information on what we receive from client and tell it
to the client sounds good, no ?

> Thoughts?

I just consume some moment before fixing an ipv6 vs ipv4 login failure. The
matched line from pg_hba .conf would have been very useful.

Maybe it is enough to report what happens on the connection: from which host,
TCP/socket (and which one, given that we should have multiple option here
soon), dbname, user, .... inspecting pg_hba.conf will remain difficult if map is
used or some other +group option.

--
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> When debugging strange and complex pg_hba lines, it can often be quite
> useful to know which line is matching a particular connection that
> failed for some reason. Because more often than not, it's actually not
> using the line in pg_hba.conf that's expected.

> The easiest way to do this is to emit an errdetail for the login
> failure, per this patch.

> Question is - is that leaking information to the client that we
> shouldn't be leaking?

Yes.

> And if it is, what would be the preferred way to deal with it?

Report to the postmaster log only. errdetail_log should do.

BTW, are you sure that auth_failed is only called in cases where
an hba line has already been identified? Even if true today,
it seems fairly risky to assume that.

regards, tom lane

On Wed, Jun 27, 2012 at 3:55 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> When debugging strange and complex pg_hba lines, it can often be quite
>> useful to know which line is matching a particular connection that
>> failed for some reason. Because more often than not, it's actually not
>> using the line in pg_hba.conf that's expected.
>
>> The easiest way to do this is to emit an errdetail for the login
>> failure, per this patch.
>
>> Question is - is that leaking information to the client that we
>> shouldn't be leaking?
>
> Yes.
>
>> And if it is, what would be the preferred way to deal with it?
>
> Report to the postmaster log only.  errdetail_log should do.

Oh, I wasn't aware we had that :) You learn something new every day.

> BTW, are you sure that auth_failed is only called in cases where
> an hba line has already been identified?  Even if true today,
> it seems fairly risky to assume that.

It is true today, but yes, it might be safe to guard against it with
something like this?

I also fixed the error message to follow the guidelines better - I think :)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Wed, Jun 27, 2012 at 3:55 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> BTW, are you sure that auth_failed is only called in cases where
>> an hba line has already been identified? Even if true today,
>> it seems fairly risky to assume that.

> It is true today, but yes, it might be safe to guard against it with
> something like this?

FWIW, the usual approach for conditionally emitting bits of an ereport
is more like

ereport(FATAL,
(errcode(errcode_return),
errmsg(errstr, port->user_name),
port->hba ? errdetail_log("Connection matched pg_hba.conf line %d", port->hba->linenumber) : 0));

but that's just a nitpick. A bigger issue is that I'm not convinced
that a line number will be tremendously helpful: it's easy to miscount
lines, and a line number will certainly not be helpful in the frequent
cases where people are modifying the wrong hba file. Can we show
the source text of the hba line?

regards, tom lane

On Wed, Jun 27, 2012 at 4:14 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> On Wed, Jun 27, 2012 at 3:55 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> BTW, are you sure that auth_failed is only called in cases where
>>> an hba line has already been identified?  Even if true today,
>>> it seems fairly risky to assume that.
>
>> It is true today, but yes, it might be safe to guard against it with
>> something like this?
>
> FWIW, the usual approach for conditionally emitting bits of an ereport
> is more like
>
>        ereport(FATAL,
>                (errcode(errcode_return),
>                 errmsg(errstr, port->user_name),
>                 port->hba ? errdetail_log("Connection matched pg_hba.conf line %d", port->hba->linenumber) : 0));

Hmm. Ok. So it treats a 0/NULL there as a way to ignore it. I tried
something with the NULL inside the errdetail, which obviously failed.

> but that's just a nitpick.  A bigger issue is that I'm not convinced
> that a line number will be tremendously helpful: it's easy to miscount
> lines, and a line number will certainly not be helpful in the frequent

Editors will help you count the lines, no? :-)

> cases where people are modifying the wrong hba file.  Can we show
> the source text of the hba line?

We don't currently keep the full source text around - but we certainly
could do that if we wanted to.

I'm not sure how much it helps - usually, you're going to end up on a
line that's completely irrelevant if you get the wrong hba file (e.g.
a comment or a line that's not even in the file at all due to size).
Maybe we should just include the *name* of the HBA file in the error
message?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Wed, Jun 27, 2012 at 4:14 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> cases where people are modifying the wrong hba file. Can we show
>> the source text of the hba line?

> We don't currently keep the full source text around - but we certainly
> could do that if we wanted to.

If we're concerned about providing a message like this, I think it'd be
well worthwhile. We presumably would only store the active lines not
comment lines, so the extra memory space would be negligible in just
about any real use-case.

> I'm not sure how much it helps - usually, you're going to end up on a
> line that's completely irrelevant if you get the wrong hba file (e.g.
> a comment or a line that's not even in the file at all due to size).

Only if you count accurately, and aren't fooled into miscounting by the
expectation that you must arrive on a non-comment line. I don't buy the
"the editor can do it for you" argument either, at least not since
noticing that recent versions of emacs don't count lines the way I do.

regards, tom lane

On Wed, Jun 27, 2012 at 4:27 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> On Wed, Jun 27, 2012 at 4:14 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> cases where people are modifying the wrong hba file.  Can we show
>>> the source text of the hba line?
>
>> We don't currently keep the full source text around - but we certainly
>> could do that if we wanted to.
>
> If we're concerned about providing a message like this, I think it'd be
> well worthwhile.  We presumably would only store the active lines not
> comment lines, so the extra memory space would be negligible in just
> about any real use-case.

Turned out to be a bit more work than I thought, since the current
parser reads pg_hba byte by byte, and not line by line. So I had to
change that. See attached, seems reasonable?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Turned out to be a bit more work than I thought, since the current
> parser reads pg_hba byte by byte, and not line by line. So I had to
> change that. See attached, seems reasonable?

A couple of comments:

* In some places you have "if ((c = *(*lineptr)++) != '\0')" and in other
places just "if ((c = *(*lineptr)++))". This should be consistent (and
personally I prefer the first way).

* I'm not sure that this conversion is right:

! if (c != EOF)
! ungetc(c, fp);
---
! if (c != '\0')
! (*lineptr)--;

In the file case, it's impossible to push back EOF, and unnecessary
since another getc will produce EOF again anyway. In the string case,
though, I think you might need to decrement the lineptr unconditionally,
else next call will run off the end of the string no?

* This bit seems a bit ugly, and not Windows-aware either:

! /* We don't store the trailing newline */
! if (rawline[strlen(rawline)-1] == '\n')
! rawline[strlen(rawline)-1] = '\0';
!

It might be better to strip trailing \n and \r from the line immediately
upon read, rather than here.

regards, tom lane

On Fri, Jun 29, 2012 at 4:39 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Turned out to be a bit more work than I thought, since the current
>> parser reads pg_hba byte by byte, and not line by line. So I had to
>> change that. See attached, seems reasonable?
>
> A couple of comments:
>
> * In some places you have "if ((c = *(*lineptr)++) != '\0')" and in other
> places just "if ((c = *(*lineptr)++))". This should be consistent (and
> personally I prefer the first way).
>
> * I'm not sure that this conversion is right:
>
> ! if (c != EOF)
> ! ungetc(c, fp);
> ---
> ! if (c != '\0')
> ! (*lineptr)--;
>
> In the file case, it's impossible to push back EOF, and unnecessary
> since another getc will produce EOF again anyway. In the string case,
> though, I think you might need to decrement the lineptr unconditionally,
> else next call will run off the end of the string no?
>
> * This bit seems a bit ugly, and not Windows-aware either:
>
> ! /* We don't store the trailing newline */
> ! if (rawline[strlen(rawline)-1] == '\n')
> ! rawline[strlen(rawline)-1] = '\0';
> !
>
> It might be better to strip trailing \n and \r from the line immediately
> upon read, rather than here.

I re-found this branch that I had completely forgotten about, when
cleaning up my reposiroty. oops.

Attached is an updated version of the patch, per the comments from Tom
and rebased on top of the current master. Since it's been a long time
ago, and some code churn in the area, another round of review is
probably a good thing...

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On 5 January 2013 16:58, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> Attached is an updated version of the patch, per the comments from Tom
> and rebased on top of the current master. Since it's been a long time
> ago, and some code churn in the area, another round of review is
> probably a good thing...
>

I took a look at this patch, and it seems to be in pretty good shape.
It applies cleanly to head, and seems to work as advertised/discussed.
I have a couple of comments on the code...

In next_token(), in the case of an overlong token, this change looks wrong:

/* Discard remainder of line */
! while ((c = getc(fp)) != EOF && c != '\n')
! ;
break;
}

--- 188,195 ----
errmsg("authentication file token too long, skipping: \"%s\"",
start_buf)));
/* Discard remainder of line */
! while ((c = (*(*lineptr)++)) != '\0' && c != '\n')
! (*lineptr)++;
break;

It appears to be incrementing lineptr twice per loop iteration, so it
risks missing the EOL/EOF and running off the end of the buffer.

Nitpicking, at the end of the loop you have:

! c = (**lineptr);
! (*lineptr)++;

perhaps for consistency with the preceding code, that should be "c =
(*(*lineptr)++)". Personally, I'd also get rid of the outer sets of
brackets in each of these expressions and just write "c =
*(*lineptr)++", since I don't think they add anything.

Finally, the comment block for tokenize_file() needs updating, now
that it returns three lists.

Regards,
Dean

On Sun, Jan 20, 2013 at 5:56 PM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> On 5 January 2013 16:58, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> Attached is an updated version of the patch, per the comments from Tom
>> and rebased on top of the current master. Since it's been a long time
>> ago, and some code churn in the area, another round of review is
>> probably a good thing...
>>
>
> I took a look at this patch, and it seems to be in pretty good shape.
> It applies cleanly to head, and seems to work as advertised/discussed.
> I have a couple of comments on the code...
>
>
> In next_token(), in the case of an overlong token, this change looks wrong:
>
> /* Discard remainder of line */
> ! while ((c = getc(fp)) != EOF && c != '\n')
> ! ;
> break;
> }
>
> --- 188,195 ----
> errmsg("authentication file token too long, skipping: \"%s\"",
> start_buf)));
> /* Discard remainder of line */
> ! while ((c = (*(*lineptr)++)) != '\0' && c != '\n')
> ! (*lineptr)++;
> break;
>
> It appears to be incrementing lineptr twice per loop iteration, so it
> risks missing the EOL/EOF and running off the end of the buffer.
>
>
> Nitpicking, at the end of the loop you have:
>
> ! c = (**lineptr);
> ! (*lineptr)++;
>
> perhaps for consistency with the preceding code, that should be "c =
> (*(*lineptr)++)". Personally, I'd also get rid of the outer sets of
> brackets in each of these expressions and just write "c =
> *(*lineptr)++", since I don't think they add anything.
>
>
> Finally, the comment block for tokenize_file() needs updating, now
> that it returns three lists.

Thanks for the review - I've updated the patch per your comments
(minus the changing of the outer set of brackets - kept that the way
it was for consistency, but that can always be changed later if people
prefer that way), and will push it as it now stands.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/