Feature request: Logging SSL connections
Hello,
we were really missing the information in our log files if (and which
of) our users are using SSL during their connections.
The attached patch is a very simple solution to this problem - it just
tests if the ssl pointer in Port is null. If no, it adds "SSL" to the
logfile, otherwise it adds "NOSSL".
Or have I been missing an existing way to reach the same?
Regards,
Andreas
--
-------------------------------------------------------------
____ ______ ____
Dr. Andreas Kunert / __/ / / / __/
HU-Berlin, ZE Rechenzentrum (CMS) / /_ / / / / __\\
www.hu-berlin.de/~kunert /___/ /_/_/_/ /___/
-------------------------------------------------------------
Attachments:
log.patchtext/x-patch; name=log.patchDownload
--- postgresql-9.1.10/src/backend/utils/init/postinit.c 2013-10-08 05:13:47.000000000 +0200
+++ /usr/postgres/sources/postgresql-9.1.10/src/backend/utils/init/postinit.c 2013-12-05 14:21:53.180148568 +0100
@@ -225,9 +225,22 @@
(errmsg("replication connection authorized: user=%s",
port->user_name)));
else
+#ifdef USE_SSL
+ if (port->ssl > 0) {
+ ereport(LOG,
+ (errmsg("connection authorized: user=%s database=%s SSL",
+ port->user_name, port->database_name)));
+ }
+ else {
+ ereport(LOG,
+ (errmsg("connection authorized: user=%s database=%s NOSSL",
+ port->user_name, port->database_name)));
+ }
+#else
ereport(LOG,
(errmsg("connection authorized: user=%s database=%s",
port->user_name, port->database_name)));
+#endif
}
set_ps_display("startup", false);
On 12/5/13, 8:53 AM, Dr. Andreas Kunert wrote:
we were really missing the information in our log files if (and which
of) our users are using SSL during their connections.The attached patch is a very simple solution to this problem - it just
tests if the ssl pointer in Port is null. If no, it adds "SSL" to the
logfile, otherwise it adds "NOSSL".
That seems useful. Do we need more information, like whether a client
certificate was presented, or what ciphers were used?
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Dec 05, 2013 at 09:43:31AM -0500, Peter Eisentraut wrote:
On 12/5/13, 8:53 AM, Dr. Andreas Kunert wrote:
we were really missing the information in our log files if (and which
of) our users are using SSL during their connections.The attached patch is a very simple solution to this problem - it just
tests if the ssl pointer in Port is null. If no, it adds "SSL" to the
logfile, otherwise it adds "NOSSL".That seems useful. Do we need more information, like whether a client
certificate was presented, or what ciphers were used?
Yes, please show ciphersuite and TLS version too. Andreas, you can use my
recent \conninfo patch as template:
https://github.com/markokr/postgres/commit/7d1b27ac74643abd15007cc4ec0b56ba92b39d90
Also, please show the SSL level also for walsender connections. It's
quite important to know whether they are using SSL or not.
But I think the 'bits' output is unnecessary, as it's cipher strength
is known by ciphersuite. Perhaps it can be removed from \conninfo too.
--
marko
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
That seems useful. Do we need more information, like whether a client
certificate was presented, or what ciphers were used?Yes, please show ciphersuite and TLS version too. Andreas, you can use my
recent \conninfo patch as template:https://github.com/markokr/postgres/commit/7d1b27ac74643abd15007cc4ec0b56ba92b39d90
Also, please show the SSL level also for walsender connections. It's
quite important to know whether they are using SSL or not.But I think the 'bits' output is unnecessary, as it's cipher strength
is known by ciphersuite. Perhaps it can be removed from \conninfo too.
A new patch is attached. I added the ciphersuite and TLS version like
shown in your template (minus the 'bits' output). I also added the SSL
information for walsender connections, but due to a missing test setup I
cannot test that part.
Anything else missing?
--
Andreas
Attachments:
log.patchtext/x-patch; name=log.patchDownload
--- postinit.c.orig 2013-12-06 10:26:47.773341120 +0100
+++ postinit.c 2013-12-06 10:37:30.185894650 +0100
@@ -220,6 +220,26 @@
if (Log_connections)
{
+#ifdef USE_SSL
+ if (am_walsender)
+ if (port->ssl > 0)
+ ereport(LOG,
+ (errmsg("replication connection authorized: user=%s SSL(protocol: %s, cipher: %s)",
+ port->user_name, SSL_get_version(port->ssl), SSL_get_cipher(port->ssl))));
+ else
+ ereport(LOG,
+ (errmsg("replication connection authorized: user=%s",
+ port->user_name)));
+ else
+ if (port->ssl > 0)
+ ereport(LOG,
+ (errmsg("connection authorized: user=%s database=%s SSL(protocol: %s, cipher: %s)",
+ port->user_name, port->database_name, SSL_get_version(port->ssl), SSL_get_cipher(port->ssl))));
+ else
+ ereport(LOG,
+ (errmsg("connection authorized: user=%s database=%s",
+ port->user_name, port->database_name)));
+#else
if (am_walsender)
ereport(LOG,
(errmsg("replication connection authorized: user=%s",
@@ -228,6 +248,7 @@
ereport(LOG,
(errmsg("connection authorized: user=%s database=%s",
port->user_name, port->database_name)));
+#endif
}
set_ps_display("startup", false);
On Fri, Dec 06, 2013 at 11:43:55AM +0100, Dr. Andreas Kunert wrote:
That seems useful. Do we need more information, like whether a client
certificate was presented, or what ciphers were used?Yes, please show ciphersuite and TLS version too. Andreas, you can use my
recent \conninfo patch as template:https://github.com/markokr/postgres/commit/7d1b27ac74643abd15007cc4ec0b56ba92b39d90
Also, please show the SSL level also for walsender connections. It's
quite important to know whether they are using SSL or not.But I think the 'bits' output is unnecessary, as it's cipher strength
is known by ciphersuite. Perhaps it can be removed from \conninfo too.A new patch is attached. I added the ciphersuite and TLS version
like shown in your template (minus the 'bits' output). I also added
the SSL information for walsender connections, but due to a missing
test setup I cannot test that part.Anything else missing?
Functionally it's fine now, but I see few style problems:
- "if (port->ssl > 0)" is wrong, ->ssl is pointer. So use just
"if (port->ssl)".
- Deeper indentation would look nicer with braces.
- There are some duplicated message, could you restructure it so that
each message exists only once.
Something like this perhaps:
#if USE_SSL
if (port->ssl)
{
if (walsender) ..
else ..
}
else
#endif
...
--
marko
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Anything else missing?
Functionally it's fine now, but I see few style problems:
- "if (port->ssl > 0)" is wrong, ->ssl is pointer. So use just
"if (port->ssl)".
- Deeper indentation would look nicer with braces.
- There are some duplicated message, could you restructure it so that
each message exists only once.
New version is attached. I could add braces before and after the
ereport()-calls too, but then I need two more #ifdefs to catch the
closing braces.
--
---------------------------------------------------------------------------
____ ______ ____
Dr. Andreas Kunert / __/ / / / __/
HU-Berlin, ZE Rechenzentrum (CMS) / /_ / / / / __\\
www.hu-berlin.de/~kunert /___/ /_/_/_/ /___/
---------------------------------------------------------------------------
Attachments:
log.patchtext/x-patch; name=log.patchDownload
--- postinit.c.orig 2013-12-06 14:42:14.827832432 +0100
+++ postinit.c 2013-12-06 14:42:25.171842695 +0100
@@ -221,13 +221,31 @@
if (Log_connections)
{
if (am_walsender)
- ereport(LOG,
- (errmsg("replication connection authorized: user=%s",
- port->user_name)));
+ {
+#ifdef USE_SSL
+ if (port->ssl)
+ ereport(LOG,
+ (errmsg("replication connection authorized: user=%s SSL(protocol: %s, cipher: %s)",
+ port->user_name, SSL_get_version(port->ssl), SSL_get_cipher(port->ssl))));
+ else
+#endif
+ ereport(LOG,
+ (errmsg("replication connection authorized: user=%s",
+ port->user_name)));
+ }
else
- ereport(LOG,
- (errmsg("connection authorized: user=%s database=%s",
- port->user_name, port->database_name)));
+ {
+#ifdef USE_SSL
+ if (port->ssl)
+ ereport(LOG,
+ (errmsg("connection authorized: user=%s database=%s SSL(protocol: %s, cipher: %s)",
+ port->user_name, port->database_name, SSL_get_version(port->ssl), SSL_get_cipher(port->ssl))));
+ else
+#endif
+ ereport(LOG,
+ (errmsg("connection authorized: user=%s database=%s",
+ port->user_name, port->database_name)));
+ }
}
set_ps_display("startup", false);
On Fri, Dec 06, 2013 at 02:53:27PM +0100, Dr. Andreas Kunert wrote:
Anything else missing?
Functionally it's fine now, but I see few style problems:
- "if (port->ssl > 0)" is wrong, ->ssl is pointer. So use just
"if (port->ssl)".
- Deeper indentation would look nicer with braces.
- There are some duplicated message, could you restructure it so that
each message exists only once.New version is attached. I could add braces before and after the
ereport()-calls too, but then I need two more #ifdefs to catch the
closing braces.
Thank you. Looks good now. I added it to next commitfest:
https://commitfest.postgresql.org/action/patch_view?id=1324
--
marko
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Dec 8, 2013 at 10:27 AM, Marko Kreen <markokr@gmail.com> wrote:
On Fri, Dec 06, 2013 at 02:53:27PM +0100, Dr. Andreas Kunert wrote:
Anything else missing?
Functionally it's fine now, but I see few style problems:
- "if (port->ssl > 0)" is wrong, ->ssl is pointer. So use just
"if (port->ssl)".
- Deeper indentation would look nicer with braces.
- There are some duplicated message, could you restructure it so that
each message exists only once.New version is attached. I could add braces before and after the
ereport()-calls too, but then I need two more #ifdefs to catch the
closing braces.Thank you. Looks good now. I added it to next commitfest:
Applied, thanks!
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
Applied, thanks!
Minor bikeshedding: the messages would read better, to my eye, as
"user=%s database=%s SSL enabled (protocol=%s, cipher=%s)"
Putting "enabled" where it is requires extra mental gymnastics on
the part of the reader. And why the random change between "="
in one part of the string and ": " in the new part? You could take
that last point a bit further and make it
"user=%s database=%s SSL=enabled (protocol=%s, cipher=%s)"
but I'm not sure if that's an improvement.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jan 17, 2014 at 4:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
Applied, thanks!
Minor bikeshedding: the messages would read better, to my eye, as
"user=%s database=%s SSL enabled (protocol=%s, cipher=%s)"
Putting "enabled" where it is requires extra mental gymnastics on
the part of the reader. And why the random change between "="
in one part of the string and ": " in the new part? You could take
that last point a bit further and make it
Makes sense.
"user=%s database=%s SSL=enabled (protocol=%s, cipher=%s)"
but I'm not sure if that's an improvement.
I don't think it is, so I've applied the first suggestion.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/