Обсуждение: 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 /___/ /_/_/_/ /___/
-------------------------------------------------------------
Вложения
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?
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
>> 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
Вложения
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_SSLif (port->ssl){ if (walsender) .. else ..}else #endif... -- marko
>> 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 /___/ /_/_/_/ /___/
---------------------------------------------------------------------------
Вложения
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
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:Thank you. Looks good now. I added it to next commitfest:
> >>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.
https://commitfest.postgresql.org/action/patch_view?id=1324
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
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/