Re: Log message for GSS connection is missing once connection authorization is successful.

Thanks for the comments Bharath.

On Thu, Oct 29, 2020 at 12:15 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> 1. Instead of just "on/off" after GSS %s in the log message, wouldn't it be informative if we have authenticated and/or encrypted as suggested by Stephen?
>
> So the log message would look like this:
>
> if(be_gssapi_get_auth(port))
> replication connection authorized: user=bob application_name=foo GSS authenticated (principal=bar)
>
> if(be_gssapi_get_enc(port))
> replication connection authorized: user=bob application_name=foo GSS encrypted (principal=bar)
>
> if(be_gssapi_get_auth(port) && be_gssapi_get_enc(port))
> replication connection authorized: user=bob application_name=foo GSS authenticated and encrypted (principal=bar)
>
> +#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=%s GSS %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
>

This is handled in v3 patch posted at [1].

> 2. I think the log message preparation looks a bit clumsy with ternary operators and duplicate log message texts(I know that we already do this for SSL). Can we have the log message prepared using StringInfoData data structure/APIs and use just a single ereport? This way, that part of the code looks cleaner.
>
> Here's what I'm visualizing:
>
> if (Log_connections)
> {
> StringInfoData msg;
>
> if (am_walsender)
> append("replication connection authorized: user=%s");
> else
> append("connection authorized: user=%s database=%s");
>
> if (port->application_name)
> append("application_name=%s");
>
> #ifdef USE_SSL
> if (port->ssl_in_use)
> append("SSL enabled (protocol=%s, cipher=%s, bits=%d, compression=%s");
> #elif defined(ENABLE_GSS)
> blah,blah,blah
> #endif
>
> ereport (LOG, msg.data);
> }

This is handled in the v3 patch posted.

>
> 3. + if (be_gssapi_get_auth(port) || be_gssapi_get_princ(port))
>
> If be_gssapi_get_auth(port) returns false, I think there's no way that be_gssapi_get_princ(port) would return a non null value, see the comment. The function be_gssapi_get_princ() returns NULL if the auth is false, so the check if ( be_gssapi_get_princ(port)) would suffice.
>
> gss_name_t name; /* GSSAPI client name */
> char *princ; /* GSSAPI Principal used for auth, NULL if
> * GSSAPI auth was not used */
> bool auth; /* GSSAPI Authentication used */
> bool enc; /* GSSAPI encryption in use */
>

This is handled in the v3 patch posted.

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