Please add this to commitfest to not lose track of it.
I took a look at v2 patch, here are some comments.
On Thu, Oct 29, 2020 at 11:01 AM vignesh C <vignes...@gmail.com> wrote:
>
> Stephen also shared his thoughts for the above changes, I have
> provided an updated patch for the same in the previous mail. Please
> have a look and let me know if you have any comments.
>
> 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)
>
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
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);
}
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 */
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
