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

On Thu, Oct 29, 2020 at 7:26 PM Stephen Frost <sfrost@snowman.net> wrote:
>
> Greetings,
>
> * vignesh C (vignesh21@gmail.com) wrote:
> > I have made a v2 patch based on the changes you have suggested. The
> > patch for the same is attached.
>
> > From b067cf823750f200102be0a0cad9a26a08e29a92 Mon Sep 17 00:00:00 2001
> > From: Vignesh C <vignesh21@gmail.com>
> > Date: Wed, 28 Oct 2020 08:19:06 +0530
> > Subject: [PATCH v2] 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.
>
> Just to be clear- it's not that the message is 'missing', it's just not
> providing the (certainly useful) information about how the connection
> was authorized.  The commit message should make it clear that what we're
> doing here is improving the connection authorization message for GSS
> authenticated or encrypted connections.
>

I have updated the commit message accordingly.

> > diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
> > index d4ab4c7..7980e92 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_auth(port) || be_gssapi_get_princ(port))
> > +                             ereport(LOG,
> > +                                             (port->application_name != NULL
> > +                                              ? errmsg("replication connection authorized: user=%s
application_name=%sGSS %s (principal=%s)",
 
> > +                                                               port->user_name,
> > +                                                               port->application_name,
> > +                                                               be_gssapi_get_auth(port) || be_gssapi_get_enc(port)
?_("on") : _("off"),
 
> > +                                                               be_gssapi_get_princ(port))
> > +                                              : errmsg("replication connection authorized: user=%s GSS %s
(principal=%s)",
> > +                                                               port->user_name,
> > +                                                               be_gssapi_get_auth(port) || be_gssapi_get_enc(port)
?_("on") : _("off"),
 
> > +                                                               be_gssapi_get_princ(port))));
> > +                     else
> > +#endif
>
> No, this isn't what I was suggesting.  "on" and "off" really isn't
> communicating the details about the GSS-using connection.  What I
> suggested before was something like:
>
> errmsg("replication connection authorized: user=%s application_name=%s GSS %s (principal=%s)",
>         port->user_name,
>         port->application_name,
>         (be_gssapi_get_auth(port) && be_gssapi_get_enc(port)) ?  "authenticated and encrypted" :
be_gssapi_get_auth(port)?  "authenticated" : "encrypted",
 
>         be_gssapi_get_princ(port))
>
> Though I'll admit that perhaps there's something better which could be
> done here- but just 'on/off' certainly isn't that.  Another option might
> be:
>
> errmsg("replication connection authorized: user=%s application_name=%s GSS authenticated: %s, encrypted: %s,
principal:%s",
 
>         port->user_name,
>         port->application_name,
>         be_gssapi_get_auth(port) ? "yes" : "no",
>         be_gssapi_get_enc(port) ? "yes" : "no",
>         be_gssapi_get_princ(port))
>

I like the above method that you suggested, I have changed it based on
the above.

> Also, it would be good to see if there's a way to add to the tests we
> have for GSSAPI authentication/encryption to show that we hit each of
> the possible cases and check that we get the correct messages in the log
> as a result.
>

I have added the log validation to the existing tests that are present
for authentication.

Attached v3 patch has the change for the same.

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