Re: [HACKERS] Feature request: Logging SSL connections
On Fri, Jan 17, 2014 at 4:53 PM, Tom Lane t...@sss.pgh.pa.us wrote: Magnus Hagander mag...@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/
Re: [HACKERS] Feature request: Logging SSL connections
On Sun, Dec 8, 2013 at 10:27 AM, Marko Kreen mark...@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: https://commitfest.postgresql.org/action/patch_view?id=1324 Applied, thanks! -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Feature request: Logging SSL connections
Magnus Hagander mag...@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
Re: [HACKERS] Feature request: Logging SSL connections
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
Re: [HACKERS] Feature request: Logging SSL connections
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
--- 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);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Feature request: Logging SSL connections
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
Re: [HACKERS] Feature request: Logging SSL connections
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 /___/ /_/_/_/ /___/
---
--- 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);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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 /___/ /_/_/_/ /___/
-
--- postgresql-9.1.10/src/backend/utils/init/postinit.c 2013-10-08 05:13:47.0 +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);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Feature request: Logging SSL connections
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
Re: [HACKERS] Feature request: Logging SSL connections
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
