Re: [PATCH] Patch to make pg_hba.conf handle virtualhost access control and samehost keyword

Hi
This patch againsts postgresql 7.1.2 allows you to control access based on the
virtual host address only (virtualhost access type), or both the remote
address and the local address (connection access type).

For example:

connection all 192.168.42.0 255.255.255.0 192.168.1.42 255.255.255.255 trust

This patch also allows keyword "samehost", similar to "sameuser" but for
hosts.

For example:

virtualhost sameuser samehost.sql.domain.com 255.255.255.255 trust

will prevent you from doing 1 entry per user, all you need is a
(local) dns entry for each host (user foo needs foo.sql.domain.com)
If the dns entry is not found, the line is dropped, so rejecting with
samehost is not a good idea for the moment.

Any comments are welcome.
Please not that I'm not on the list.

---
Damien Clermonte

Damien =?ISO-8859-1?Q?Clermont=E9?= <damien(dot)clermonte(at)free(dot)fr> writes:
> Any comments are welcome.

For one thing: a documentation patch is needed to go with this.

regards, tom lane

Hi

New try, with a (probably bad english) documentation patch this time :)

This patch againsts postgresql 7.1.2 allows you to control access based on the
virtual host address only (virtualhost access type), or both the remote
address and the local address (connection access type).

For example:

connection all 192.168.42.0 255.255.255.0 192.168.1.42 255.255.255.255 trust

This patch also allows keyword "samehost", similar to "sameuser" but for
hosts.

For example:

virtualhost sameuser samehost.sql.domain.com 255.255.255.255 trust

will prevent you from doing 1 entry per user, all you need is a
(local) dns entry for each host (user foo needs foo.sql.domain.com)
If the dns entry is not found, the line is dropped, so rejecting with
samehost is not a good idea for the moment.

Any comments are welcome.
Please not that I'm not on the list.

---
Damien Clermonte

Damien Clermonté writes:

> This patch againsts postgresql 7.1.2 allows you to control access based on the
> virtual host address only (virtualhost access type), or both the remote
> address and the local address (connection access type).
>
> For example:
>
> connection all 192.168.42.0 255.255.255.0 192.168.1.42 255.255.255.255 trust

I completely fail to understand what this does. What is the expression
that will be evaluated based on these four numbers?

--
Peter Eisentraut peter_e(at)gmx(dot)net http://funkturm.homeip.net/~peter

> Damien Clermont? writes:
>
> > This patch againsts postgresql 7.1.2 allows you to control access based on the
> > virtual host address only (virtualhost access type), or both the remote
> > address and the local address (connection access type).
> >
> > For example:
> >
> > connection all 192.168.42.0 255.255.255.0 192.168.1.42 255.255.255.255 trust
>
> I completely fail to understand what this does. What is the expression
> that will be evaluated based on these four numbers?

The killer for me is the added complexity to an already complex file,
pg_hba.conf.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

Bruce Momjian writes:

> The killer for me is the added complexity to an already complex file,
> pg_hba.conf.

I never figured pg_hba.conf was complex. It's one of the simplest
configuration files I've seen. What makes it look complex is that it
begins with 700 lines explaining it, obscuring the actual content.

--
Peter Eisentraut peter_e(at)gmx(dot)net http://funkturm.homeip.net/~peter

> Bruce Momjian writes:
>
> > The killer for me is the added complexity to an already complex file,
> > pg_hba.conf.
>
> I never figured pg_hba.conf was complex. It's one of the simplest
> configuration files I've seen. What makes it look complex is that it
> begins with 700 lines explaining it, obscuring the actual content.
>

Yes, I cleaned it up a bit for 7.2. I think the confusion is the
authentication options and the options that go with the authentication
options.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> I never figured pg_hba.conf was complex. It's one of the simplest
> configuration files I've seen. What makes it look complex is that it
> begins with 700 lines explaining it, obscuring the actual content.

Now now, it's only ~ 200 lines of comments. However...

Since pg_hba.conf is re-read on every connection, I've always thought
it was pretty bogus to bulk it up with that much internal documentation.
I've not tried to measure how much time it takes the postmaster to skip
over those 200 comment lines, but it can't be completely negligible.

I'd favor reducing the in-the-file docs to about one line saying "See
such-and-such-a-place in the documentation". Or a README. Or
something.

regards, tom lane

Tom Lane writes:

> Since pg_hba.conf is re-read on every connection, I've always thought
> it was pretty bogus to bulk it up with that much internal documentation.

Maybe it should be cached in memory and only be re-read on request
(SIGHUP). Parsing that file every time is undoubtedly a large fraction of
the total connection startup time.

--
Peter Eisentraut peter_e(at)gmx(dot)net http://funkturm.homeip.net/~peter

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Tom Lane writes:
>> Since pg_hba.conf is re-read on every connection, I've always thought
>> it was pretty bogus to bulk it up with that much internal documentation.

> Maybe it should be cached in memory and only be re-read on request
> (SIGHUP). Parsing that file every time is undoubtedly a large fraction of
> the total connection startup time.

Okay with me if someone wants to do it ... but that'd be a lot more work
than just moving the documentation ...

regards, tom lane

> > Maybe it should be cached in memory and only be re-read on request
> > (SIGHUP). Parsing that file every time is undoubtedly a large
> fraction of
> > the total connection startup time.
>
> Okay with me if someone wants to do it ... but that'd be a lot more work
> than just moving the documentation ...

Or cache the information and just do a file modification timestamp check on
each connection to see if it needs to be reread. (Best of both worlds??)

Chris

How about moving the documentation to a pg_hba.conf.README file?

People shouldn't be able to miss that very easily.

+ Justin

Christopher Kings-Lynne wrote:
>
> > > Maybe it should be cached in memory and only be re-read on request
> > > (SIGHUP). Parsing that file every time is undoubtedly a large
> > fraction of
> > > the total connection startup time.
> >
> > Okay with me if someone wants to do it ... but that'd be a lot more work
> > than just moving the documentation ...
>
> Or cache the information and just do a file modification timestamp check on
> each connection to see if it needs to be reread. (Best of both worlds??)
>
> Chris
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://www.postgresql.org/search.mpl

Tom Lane writes:

> Since pg_hba.conf is re-read on every connection, I've always thought
> it was pretty bogus to bulk it up with that much internal documentation.
> I've not tried to measure how much time it takes the postmaster to skip
> over those 200 comment lines, but it can't be completely negligible.

I've run a simplistic test for this. I've let psql start 10000 times
sequentially and timed the backend startup. All times are wall clock
(gettimeofday). The first checkpoint is after the accept(), the second
before the backend loop begins. The machine had a load average of 1.00 to
1.50 and wasn't running anything else besides "infrastructure".

default pg_hba.conf

count | min | max | avg | stddev
-------+----------+----------+--------------+---------------------
10000 | 0.024667 | 0.060208 | 0.0298723081 | 0.00746411804719077

pg_hba.conf, all comments removed

count | min | max | avg | stddev
-------+----------+---------+--------------------+---------------------
10000 | 0.022364 | 0.05946 | 0.0262477744000001 | 0.00570493964559965

So we're looking at a possible 12% win. I suggest we remove the comments
and direct the user to the Admin Guide.

Btw., in case someone wants to go optimizing, more than 75% of the backend
startup time is spent in InitPostgres():

count | min | max | avg | stddev
-------+---------+----------+--------------+---------------------
10000 | 0.01953 | 0.368216 | 0.0222271679 | 0.00629838985852663

--
Peter Eisentraut peter_e(at)gmx(dot)net http://funkturm.homeip.net/~peter

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> So we're looking at a possible 12% win.

Many thanks for doing this legwork.

The possible win from not having to read the file at all is probably
somewhat higher than that, but not vastly higher. Accordingly, I'd
say that pre-parsing the file is not worth the development time needed
to make it happen. However, moving the comments out is clearly worth
the (very small) amount of effort needed to make that happen. Any
objections?

> Btw., in case someone wants to go optimizing, more than 75% of the backend
> startup time is spent in InitPostgres():

No surprise, that's where all the initial database access happens. We'd
need to break it down more to learn anything useful, but I'd bet that
the initial loading of required catalog cache entries is a big chunk.

regards, tom lane

> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > So we're looking at a possible 12% win.
>
> Many thanks for doing this legwork.
>
> The possible win from not having to read the file at all is probably
> somewhat higher than that, but not vastly higher. Accordingly, I'd
> say that pre-parsing the file is not worth the development time needed
> to make it happen. However, moving the comments out is clearly worth
> the (very small) amount of effort needed to make that happen. Any
> objections?

Let me see if I can cache the contents. I hate to make things harder to
set us up, even if it is a small thing.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

> > > Maybe it should be cached in memory and only be re-read on request
> > > (SIGHUP). Parsing that file every time is undoubtedly a large
> > fraction of
> > > the total connection startup time.
> >
> > Okay with me if someone wants to do it ... but that'd be a lot more work
> > than just moving the documentation ...
>
> Or cache the information and just do a file modification timestamp check on
> each connection to see if it needs to be reread. (Best of both worlds??)

Rather than having to create data structures for the complex pg_hba.conf
format, I am going to do a quick-and-dirty and load the non-comment
lines into a List of strings and have the postmaster read that. It will
reload from the file on sighup, just like we do for postgresql.conf.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

I assume you are aware that 7.1.X postmaster can control which addresses
it accepts connections from with -h:

-h hostname
Specifies the TCP/IP hostname or address on which
the postmaster is to listen for connections from
client applications. Defaults to listening on all
configured addresses (including localhost).

My question is why virtualhosts are useful in the pg_hba.conf file?

> Hi
> This patch againsts postgresql 7.1.2 allows you to control access based on the
> virtual host address only (virtualhost access type), or both the remote
> address and the local address (connection access type).
>
> For example:
>
> connection all 192.168.42.0 255.255.255.0 192.168.1.42 255.255.255.255 trust
>
>
> This patch also allows keyword "samehost", similar to "sameuser" but for
> hosts.
>
> For example:
>
> virtualhost sameuser samehost.sql.domain.com 255.255.255.255 trust
>
> will prevent you from doing 1 entry per user, all you need is a
> (local) dns entry for each host (user foo needs foo.sql.domain.com)
> If the dns entry is not found, the line is dropped, so rejecting with
> samehost is not a good idea for the moment.
>
> Any comments are welcome.
> Please not that I'm not on the list.
>
> ---
> Damien Clermonte

> --- postgresql-7.1.2.orig/src/backend/libpq/hba.c Mon Jul 2 16:25:56 2001
> +++ postgresql-7.1.2/src/backend/libpq/hba.c Tue Jul 3 14:01:20 2001
> @@ -17,6 +17,7 @@
> #include <netinet/in.h>
> #include <arpa/inet.h>
> #include <unistd.h>
> +#include <netdb.h>
>
> #include "postgres.h"
>
> @@ -31,6 +32,9 @@
> #define IDENT_USERNAME_MAX 512
> /* Max size of username ident server can return */
>
> +#define MAX_HOSTNAME 1024
> + /* Max size of hostname */
> +
>
> /* Some standard C libraries, including GNU, have an isblank() function.
> Others, including Solaris, do not. So we have our own.
> @@ -256,8 +260,38 @@
>
> if (!inet_aton(buf, &file_ip_addr))
> {
> + if (!strncmp(buf, "samehost", 8)) /* samehost.somedomain */
> + {
> + struct hostent* he;
> + char host[MAX_HOSTNAME];
> +
> + strcpy(host, port->user);
> + if(strlen(buf) > 8)
> + {
> + strncat(host, buf + 8, MAX_HOSTNAME - 1 - strlen(host));
> + host[MAX_HOSTNAME - 1] = '\0';
> + }
> + he = gethostbyname(host);
> +
> + if(he != NULL)
> + {
> + file_ip_addr.s_addr = *(int*)he->h_addr;
> + }
> + else /* Error or Host not found */
> + {
> + read_through_eol(file);
> + snprintf(PQerrormsg, PQERRORMSG_LENGTH,
> + "process_hba_record: samehost '%s' not found in pg_hba.conf file\n", host);
> + fputs(PQerrormsg, stderr);
> + pqdebug("%s", PQerrormsg);
> + return;
> + }
> + }
> + else
> + {
> read_through_eol(file);
> goto syntax;
> + }
> }
>
> /* Read the mask field. */
> @@ -299,6 +333,301 @@
> (strcmp(db, "sameuser") != 0 || strcmp(port->database, port->user) != 0)) ||
> port->raddr.sa.sa_family != AF_INET ||
> ((file_ip_addr.s_addr ^ port->raddr.in.sin_addr.s_addr) & mask.s_addr) != 0x0000)
> + return;
> + }
> + else if (strcmp(buf, "virtualhost") == 0 || strcmp(buf, "virtualhostssl") == 0)
> + {
> + struct in_addr file_ip_addr,
> + mask;
> + bool discard = 0;/* Discard this entry */
> +
> +#ifdef USE_SSL
> + /* If SSL, then check that we are on SSL */
> + if (strcmp(buf, "virtualhostssl") == 0)
> + {
> + if (!port->ssl)
> + discard = 1;
> +
> + /* Placeholder to require specific SSL level, perhaps? */
> + /* Or a client certificate */
> +
> + /* Since we were on SSL, proceed as with normal 'host' mode */
> + }
> +#else
> + /* If not SSL, we don't support this */
> + if (strcmp(buf, "virtualhostssl") == 0)
> + goto syntax;
> +#endif
> +
> + /* Get the database. */
> +
> + next_token(file, db, sizeof(db));
> +
> + if (db[0] == '\0')
> + goto syntax;
> +
> + /* Read the IP address field. */
> +
> + next_token(file, buf, sizeof(buf));
> +
> + if (buf[0] == '\0')
> + goto syntax;
> +
> + /* Remember the IP address field and go get mask field. */
> +
> + if (!inet_aton(buf, &file_ip_addr))
> + {
> + if (!strncmp(buf, "samehost", 8)) /* samehost.somedomain */
> + {
> + struct hostent* he;
> + char host[MAX_HOSTNAME];
> +
> + strcpy(host, port->user);
> + if(strlen(buf) > 8)
> + {
> + strncat(host, buf + 8, MAX_HOSTNAME - 1 - strlen(host));
> + host[MAX_HOSTNAME - 1] = '\0';
> + }
> + he = gethostbyname(host);
> +
> + if(he != NULL)
> + {
> + file_ip_addr.s_addr = *(int*)he->h_addr;
> + }
> + else /* Error or Host not found */
> + {
> + read_through_eol(file);
> + snprintf(PQerrormsg, PQERRORMSG_LENGTH,
> + "process_hba_record: samehost '%s' not found in pg_hba.conf file\n", host);
> + fputs(PQerrormsg, stderr);
> + pqdebug("%s", PQerrormsg);
> + return;
> + }
> + }
> + else
> + {
> + read_through_eol(file);
> + goto syntax;
> + }
> + }
> +
> + /* Read the mask field. */
> +
> + next_token(file, buf, sizeof(buf));
> +
> + if (buf[0] == '\0')
> + goto syntax;
> +
> + if (!inet_aton(buf, &mask))
> + {
> + read_through_eol(file);
> + goto syntax;
> + }
> +
> + /*
> + * This is the record we're looking for. Read the rest of the
> + * info from it.
> + */
> +
> + read_hba_entry2(file, &port->auth_method, port->auth_arg, error_p);
> +
> + if (*error_p)
> + goto syntax;
> +
> + /*
> + * If told to discard earlier. Moved down here so we don't get
> + * "out of sync" with the file.
> + */
> + if (discard)
> + return;
> +
> + /*
> + * If this record isn't for our database, or this is the wrong
> + * sort of connection, ignore it.
> + */
> +
> + if ((strcmp(db, port->database) != 0 && strcmp(db, "all") != 0 &&
> + (strcmp(db, "sameuser") != 0 || strcmp(port->database, port->user) != 0)) ||
> + port->laddr.sa.sa_family != AF_INET ||
> + ((file_ip_addr.s_addr ^ port->laddr.in.sin_addr.s_addr) & mask.s_addr) != 0x0000)
> + return;
> + }
> + else if (strcmp(buf, "connection") == 0 || strcmp(buf, "connectionssl") == 0)
> + {
> + struct in_addr file_ip_raddr,
> + rmask;
> + struct in_addr file_ip_laddr,
> + lmask;
> + bool discard = 0;/* Discard this entry */
> +
> +#ifdef USE_SSL
> + /* If SSL, then check that we are on SSL */
> + if (strcmp(buf, "connectionssl") == 0)
> + {
> + if (!port->ssl)
> + discard = 1;
> +
> + /* Placeholder to require specific SSL level, perhaps? */
> + /* Or a client certificate */
> +
> + /* Since we were on SSL, proceed as with normal 'host' mode */
> + }
> +#else
> + /* If not SSL, we don't support this */
> + if (strcmp(buf, "connectionssl") == 0)
> + goto syntax;
> +#endif
> +
> + /* Get the database. */
> +
> + next_token(file, db, sizeof(db));
> +
> + if (db[0] == '\0')
> + goto syntax;
> +
> + /* Read the remote IP address field. */
> +
> + next_token(file, buf, sizeof(buf));
> +
> + if (buf[0] == '\0')
> + goto syntax;
> +
> + /* Remember the IP address field and go get mask field. */
> +
> + if (!inet_aton(buf, &file_ip_raddr))
> + {
> + if (!strncmp(buf, "samehost", 8)) /* samehost.somedomain */
> + {
> + struct hostent* he;
> + char host[MAX_HOSTNAME];
> +
> + strcpy(host, port->user);
> + if(strlen(buf) > 8)
> + {
> + strncat(host, buf + 8, MAX_HOSTNAME - 1 - strlen(host));
> + host[MAX_HOSTNAME - 1] = '\0';
> + }
> + he = gethostbyname(host);
> +
> + if(he != NULL)
> + {
> + file_ip_raddr.s_addr = *(int*)he->h_addr;
> + }
> + else /* Error or Host not found */
> + {
> + read_through_eol(file);
> + snprintf(PQerrormsg, PQERRORMSG_LENGTH,
> + "process_hba_record: samehost '%s' not found in pg_hba.conf file\n", host);
> + fputs(PQerrormsg, stderr);
> + pqdebug("%s", PQerrormsg);
> + return;
> + }
> + }
> + else
> + {
> + read_through_eol(file);
> + goto syntax;
> + }
> + }
> +
> + /* Read the remote mask field. */
> +
> + next_token(file, buf, sizeof(buf));
> +
> + if (buf[0] == '\0')
> + goto syntax;
> +
> + if (!inet_aton(buf, &rmask))
> + {
> + read_through_eol(file);
> + goto syntax;
> + }
> +
> + /* Read the local IP address field. */
> +
> + next_token(file, buf, sizeof(buf));
> +
> + if (buf[0] == '\0')
> + goto syntax;
> +
> + /* Remember the IP address field and go get mask field. */
> +
> + if (!inet_aton(buf, &file_ip_laddr))
> + {
> + if (!strncmp(buf, "samehost", 8)) /* samehost.somedomain */
> + {
> + struct hostent* he;
> + char host[MAX_HOSTNAME];
> +
> + strcpy(host, port->user);
> + if(strlen(buf) > 8)
> + {
> + strncat(host, buf + 8, MAX_HOSTNAME - 1 - strlen(host));
> + host[MAX_HOSTNAME - 1] = '\0';
> + }
> + he = gethostbyname(host);
> +
> + if(he != NULL)
> + {
> + file_ip_laddr.s_addr = *(int*)he->h_addr;
> + }
> + else /* Error or Host not found */
> + {
> + read_through_eol(file);
> + snprintf(PQerrormsg, PQERRORMSG_LENGTH,
> + "process_hba_record: samehost '%s' not found in pg_hba.conf file\n", host);
> + fputs(PQerrormsg, stderr);
> + pqdebug("%s", PQerrormsg);
> + return;
> + }
> + }
> + else
> + {
> + read_through_eol(file);
> + goto syntax;
> + }
> + }
> +
> + /* Read the source mask field. */
> +
> + next_token(file, buf, sizeof(buf));
> +
> + if (buf[0] == '\0')
> + goto syntax;
> +
> + if (!inet_aton(buf, &lmask))
> + {
> + read_through_eol(file);
> + goto syntax;
> + }
> +
> + /*
> + * This is the record we're looking for. Read the rest of the
> + * info from it.
> + */
> +
> + read_hba_entry2(file, &port->auth_method, port->auth_arg, error_p);
> +
> + if (*error_p)
> + goto syntax;
> +
> + /*
> + * If told to discard earlier. Moved down here so we don't get
> + * "out of sync" with the file.
> + */
> + if (discard)
> + return;
> +
> + /*
> + * If this record isn't for our database, or this is the wrong
> + * sort of connection, ignore it.
> + */
> +
> + if ((strcmp(db, port->database) != 0 && strcmp(db, "all") != 0 &&
> + (strcmp(db, "sameuser") != 0 || strcmp(port->database, port->user) != 0)) ||
> + port->laddr.sa.sa_family != AF_INET ||
> + ((file_ip_raddr.s_addr ^ port->raddr.in.sin_addr.s_addr) & rmask.s_addr) != 0x0000 ||
> + ((file_ip_laddr.s_addr ^ port->laddr.in.sin_addr.s_addr) & lmask.s_addr) != 0x0000)
> return;
> }
> else

>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Don't 'kill -9' the postmaster

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026