Re: Log message for GSS connection is missing once connection authorization is successful. - Mailing list pgsql-hackers

Thanks Stephen for your comments.

On Wed, Oct 28, 2020 at 9:44 PM Stephen Frost <sfrost@snowman.net> wrote:
>
> Greetings,
>
> * vignesh C (vignesh21@gmail.com) wrote:
> > Log message for GSS connection is missing once connection
> > authorization is successful. We have similar log messages for SSL
> > connections once the connection authorization is successful. This
> > message will help the user to identify the connection that was
> > selected from the logfile. I'm not sure if this log message was
> > intentionally left out due to some reason for GSS.
> > If the above analysis looks correct, then please find a patch that
> > adds log for gss connections.
> >
> > Thoughts?
>
> I agree with logging the principal and if GSS encryption is being used
> or not as part of the connection authorized message.  Not logging the
> principal isn't great and has been something I've wanted to fix for a
> while, so glad to see someone else is thinking about this.
>
> > From 95c906b9eaf1493ad10ac65d6cf7b27a7dd6acb9 Mon Sep 17 00:00:00 2001
> > From: Vignesh C <vignesh21@gmail.com>
> > Date: Wed, 28 Oct 2020 08:19:06 +0530
> > Subject: [PATCH v1] Log message for GSS connection is missing once connection
> >  authorization is successful.
> >
> > Log message for GSS connection is missing once connection authorization is
> > successful. We have similar log message for SSL connections once the connection
> > authorization is successful. This message will help the user to identify the
> > connection that was selected from the logfile.
> > ---
> >  src/backend/utils/init/postinit.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >
> > diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
> > index d4ab4c7..0fd38b7 100644
> > --- a/src/backend/utils/init/postinit.c
> > +++ b/src/backend/utils/init/postinit.c
> > @@ -267,6 +267,21 @@ PerformAuthentication(Port *port)
> >                                                                 be_tls_get_compression(port) ? _("on") :
_("off"))));
> >                       else
> >  #endif
> > +#ifdef ENABLE_GSS
> > +                     if (be_gssapi_get_enc(port))
>
> This is checking if GSS *encryption* is being used.
>
> > +                             ereport(LOG,
> > +                                             (port->application_name != NULL
> > +                                              ? errmsg("replication connection authorized: user=%s
application_name=%sGSS enabled (gssapi autorization=%s, principal=%s)",
 
> > +                                                               port->user_name,
> > +                                                               port->application_name,
> > +                                                               be_gssapi_get_auth(port) ? _("on") : _("off"),
> > +                                                               be_gssapi_get_princ(port))
> > +                                              : errmsg("replication connection authorized: user=%s GSS enabled
(gssapiautorization=%s, principal=%s)",
 
> > +                                                               port->user_name,
> > +                                                               be_gssapi_get_auth(port) ? _("on") : _("off"),
> > +                                                               be_gssapi_get_princ(port))));
>
> This is checking if GSS *authentication* was used.
>
> You can certainly have GSS authentication used without encryption, and
> you can (though I'm not sure how useful it really is) have GSS
> encryption with 'trust' authentication, so we should really break this
> out into their own sets of checks, which would look something like:
>
> if (be_gssapi_get_auth(port) || be_gssapi_get_princ(port))
>     connection authorized: GSS %s (principal=%s)
>
> With the first %s being: (authentication || encrypted || authenticated and encrypted)
>
> Or something along those lines, I would think.
>
> I don't think 'enabled' is a good term to use here.
>

I have made a v2 patch based on the changes you have suggested. The
patch for the same is attached.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com