Thanks for the comments Bharath. On Thu, Oct 29, 2020 at 12:15 PM Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.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