Re: 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_SSL
if (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(at)gmail(dot)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/

Magnus Hagander <magnus(at)hagander(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Magnus Hagander <magnus(at)hagander(dot)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/