Thread: Parsing errors in pg_hba.conf
In a number of places in pg_hba.conf, we don't actually log what goes wrong - instead we just goto a label that will log "invalid token \"%s\"". Is there any special reason for this, other than the fact that it was the easy way out? I think it would be reasonable to for example log "hostssl not supported on this platform" instead of that, when USE_SSL is not defined, etc. //Magnus
Magnus Hagander wrote: > In a number of places in pg_hba.conf, we don't actually log what goes > wrong - instead we just goto a label that will log "invalid token \"%s\"". > > Is there any special reason for this, other than the fact that it was > the easy way out? I think it would be reasonable to for example log > "hostssl not supported on this platform" instead of that, when USE_SSL > is not defined, etc. Without actually looking at what you're considering, I think it could be a security bug if you were to disclose all the details to the user. Perhaps the details can be passed to errdetail_log() to avoid this problem. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Magnus Hagander <magnus@hagander.net> writes:
> In a number of places in pg_hba.conf, we don't actually log what goes
> wrong - instead we just goto a label that will log "invalid token \"%s\"".
> Is there any special reason for this, other than the fact that it was
> the easy way out? I think it would be reasonable to for example log
> "hostssl not supported on this platform" instead of that, when USE_SSL
> is not defined, etc.
It would be seriously messy to try to make the parse-error code know
about things like that. Better would be to keep the GUC variable in
existence always and have an assign hook to throw the error.
regards, tom lane
Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> In a number of places in pg_hba.conf, we don't actually log what goes
>> wrong - instead we just goto a label that will log "invalid token \"%s\"".
>
>> Is there any special reason for this, other than the fact that it was
>> the easy way out? I think it would be reasonable to for example log
>> "hostssl not supported on this platform" instead of that, when USE_SSL
>> is not defined, etc.
>
> It would be seriously messy to try to make the parse-error code know
> about things like that. Better would be to keep the GUC variable in
> existence always and have an assign hook to throw the error.
Um, no, it wouldn't :-) At least that's my impression. We're only
talking a bout the pg_hba code. Today it basically looks lilke: if (token[4] == 's') /* "hostssl" */ {
#ifdef USE_SSL parsedline->conntype = ctHostSSL;
#else /* We don't accept this keyword at all if no SSL support */ goto hba_syntax;
#endif }
We could just replace the "goto" there with an ereport() and then a
"return false", because that's what hba_syntax does.
We have a total of 8 places that we do this instead of logging a
"complete error message".
(it used to be a bit more messy than this, which could also be the
reason why we didn't do it)
//Magnus
Alvaro Herrera wrote: > Magnus Hagander wrote: >> In a number of places in pg_hba.conf, we don't actually log what goes >> wrong - instead we just goto a label that will log "invalid token \"%s\"". >> >> Is there any special reason for this, other than the fact that it was >> the easy way out? I think it would be reasonable to for example log >> "hostssl not supported on this platform" instead of that, when USE_SSL >> is not defined, etc. > > Without actually looking at what you're considering, I think it could be > a security bug if you were to disclose all the details to the user. > Perhaps the details can be passed to errdetail_log() to avoid this > problem. This is during pg_hba parsing. E.g. at server start or config reload. It doesn't run in the context of a user connection. //Magnus
Magnus Hagander <magnus@hagander.net> writes:
> Tom Lane wrote:
>> It would be seriously messy to try to make the parse-error code know
>> about things like that. Better would be to keep the GUC variable in
>> existence always and have an assign hook to throw the error.
> Um, no, it wouldn't :-) At least that's my impression. We're only
> talking a bout the pg_hba code.
Oh, sorry, for some reason I read this as postgresql.conf parsing.
Too early in the morning, need more caffeine ;-)
regards, tom lane
Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> Tom Lane wrote:
>>> It would be seriously messy to try to make the parse-error code know
>>> about things like that. Better would be to keep the GUC variable in
>>> existence always and have an assign hook to throw the error.
>
>> Um, no, it wouldn't :-) At least that's my impression. We're only
>> talking a bout the pg_hba code.
>
> Oh, sorry, for some reason I read this as postgresql.conf parsing.
> Too early in the morning, need more caffeine ;-)
:-) THat's what I was guessing.
Anyway, here's what I intended:
//Magnus
*** a/src/backend/libpq/hba.c
--- b/src/backend/libpq/hba.c
***************
*** 637,644 **** parse_hba_line(List *line, int line_num, HbaLine *parsedline)
#ifdef USE_SSL
parsedline->conntype = ctHostSSL;
#else
! /* We don't accept this keyword at all if no SSL support */
! goto hba_syntax;
#endif
}
#ifdef USE_SSL
--- 637,649 ----
#ifdef USE_SSL
parsedline->conntype = ctHostSSL;
#else
! ereport(LOG,
! (errcode(ERRCODE_CONFIG_FILE_ERROR),
! errmsg("hostssl not supported on this platform"),
! errhint("compile with --enable-ssl to use SSL connections"),
! errcontext("line %d of configuration file \"%s\"",
! line_num, HbaFileName)));
! return false;
#endif
}
#ifdef USE_SSL
***************
*** 654,671 **** parse_hba_line(List *line, int line_num, HbaLine *parsedline)
}
} /* record type */
else
! goto hba_syntax;
/* Get the database. */
line_item = lnext(line_item);
if (!line_item)
! goto hba_syntax;
parsedline->database = pstrdup(lfirst(line_item));
/* Get the role. */
line_item = lnext(line_item);
if (!line_item)
! goto hba_syntax;
parsedline->role = pstrdup(lfirst(line_item));
if (parsedline->conntype != ctLocal)
--- 659,698 ----
}
} /* record type */
else
! {
! ereport(LOG,
! (errcode(ERRCODE_CONFIG_FILE_ERROR),
! errmsg("invalid connection type \"%s\"",
! token),
! errcontext("line %d of configuration file \"%s\"",
! line_num, HbaFileName)));
! return false;
! }
/* Get the database. */
line_item = lnext(line_item);
if (!line_item)
! {
! ereport(LOG,
! (errcode(ERRCODE_CONFIG_FILE_ERROR),
! errmsg("end-of-line before database specification"),
! errcontext("line %d of configuration file \"%s\"",
! line_num, HbaFileName)));
! return false;
! }
parsedline->database = pstrdup(lfirst(line_item));
/* Get the role. */
line_item = lnext(line_item);
if (!line_item)
! {
! ereport(LOG,
! (errcode(ERRCODE_CONFIG_FILE_ERROR),
! errmsg("end-of-line before role specification"),
! errcontext("line %d of configuration file \"%s\"",
! line_num, HbaFileName)));
! return false;
! }
parsedline->role = pstrdup(lfirst(line_item));
if (parsedline->conntype != ctLocal)
***************
*** 673,679 **** parse_hba_line(List *line, int line_num, HbaLine *parsedline)
/* Read the IP address field. (with or without CIDR netmask) */
line_item = lnext(line_item);
if (!line_item)
! goto hba_syntax;
token = pstrdup(lfirst(line_item));
/* Check if it has a CIDR suffix and if so isolate it */
--- 700,713 ----
/* Read the IP address field. (with or without CIDR netmask) */
line_item = lnext(line_item);
if (!line_item)
! {
! ereport(LOG,
! (errcode(ERRCODE_CONFIG_FILE_ERROR),
! errmsg("end-of-line before ip address specification"),
! errcontext("line %d of configuration file \"%s\"",
! line_num, HbaFileName)));
! return false;
! }
token = pstrdup(lfirst(line_item));
/* Check if it has a CIDR suffix and if so isolate it */
***************
*** 718,731 **** parse_hba_line(List *line, int line_num, HbaLine *parsedline)
{
if (pg_sockaddr_cidr_mask(&parsedline->mask, cidr_slash + 1,
parsedline->addr.ss_family) < 0)
! goto hba_syntax;
}
else
{
/* Read the mask field. */
line_item = lnext(line_item);
if (!line_item)
! goto hba_syntax;
token = lfirst(line_item);
ret = pg_getaddrinfo_all(token, NULL, &hints, &gai_result);
--- 752,780 ----
{
if (pg_sockaddr_cidr_mask(&parsedline->mask, cidr_slash + 1,
parsedline->addr.ss_family) < 0)
! {
! ereport(LOG,
! (errcode(ERRCODE_CONFIG_FILE_ERROR),
! errmsg("invalid CIDR mask in address \"%s\"",
! token),
! errcontext("line %d of configuration file \"%s\"",
! line_num, HbaFileName)));
! return false;
! }
}
else
{
/* Read the mask field. */
line_item = lnext(line_item);
if (!line_item)
! {
! ereport(LOG,
! (errcode(ERRCODE_CONFIG_FILE_ERROR),
! errmsg("end-of-line before netmask specification"),
! errcontext("line %d of configuration file \"%s\"",
! line_num, HbaFileName)));
! return false;
! }
token = lfirst(line_item);
ret = pg_getaddrinfo_all(token, NULL, &hints, &gai_result);
***************
*** 759,765 **** parse_hba_line(List *line, int line_num, HbaLine *parsedline)
/* Get the authentication method */
line_item = lnext(line_item);
if (!line_item)
! goto hba_syntax;
token = lfirst(line_item);
unsupauth = NULL;
--- 808,821 ----
/* Get the authentication method */
line_item = lnext(line_item);
if (!line_item)
! {
! ereport(LOG,
! (errcode(ERRCODE_CONFIG_FILE_ERROR),
! errmsg("end-of-line before authentication method"),
! errcontext("line %d of configuration file \"%s\"",
! line_num, HbaFileName)));
! return false;
! }
token = lfirst(line_item);
unsupauth = NULL;
***************
*** 937,959 **** parse_hba_line(List *line, int line_num, HbaLine *parsedline)
}
return true;
-
- hba_syntax:
- if (line_item)
- ereport(LOG,
- (errcode(ERRCODE_CONFIG_FILE_ERROR),
- errmsg("invalid token \"%s\"",
- (char *) lfirst(line_item)),
- errcontext("line %d of configuration file \"%s\"",
- line_num, HbaFileName)));
- else
- ereport(LOG,
- (errcode(ERRCODE_CONFIG_FILE_ERROR),
- errmsg("missing field at end of line"),
- errcontext("line %d of configuration file \"%s\"",
- line_num, HbaFileName)));
-
- return false;
}
--- 993,998 ----