Greetings Don! * Don Seiler (d...@seiler.us) wrote: > On Tue, Aug 7, 2018 at 12:32 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > > Don Seiler <d...@seiler.us> writes: > > > > > 1. We want to make a generic, central ascii-lobotomizing function similar > > > to check_application_name that we can re-use there and for other checks > > (eg > > > user name). > > > 2. Change check_application_name to call this function (or just call this > > > function instead of check_application_name()?) > > > > check_application_name's API is dictated by the GUC check-hook interface, > > and doesn't really make sense for this other use. So the first part of > > that, not the second. > > > > > 3. Call this function when storing the value in the port struct. > > > > I'm not sure where exactly is the most sensible place to call it, > > but trying to minimize the number of places that know about this > > kluge seems like a good principle. > > OK I created a new function called clean_ascii() in common/string.c. I call > this from my new logic in postmaster.c as well as replacing the logic in > guc.c's check_application_name() and check_cluster_name().
Since we're putting it into common/string.c (which seems pretty reasonable to me, at least), I went ahead and changed it to be 'pg_clean_ascii'. I didn't see any other obvious cases where we could use this function (though typecmds.c does have an interesting ASCII check for type categories..). Otherwise, I added some comments, added application_name to the replication 'connection authorized' messages (seems like we really should be consistent across all of them...), ran it through pgindent, and updated a variable name or two here and there. > I've been fighting my own confusion with git and rebasing and fighting the > same conflicts over and over and over, but this patch should be what I > want. If anyone has time to review my git process, I would appreciate it. I > must be doing something wrong to have these same conflicts every time I > rebase (or I completely misunderstand what it actually does). I'd be happy to chat about it sometime, of course, just have to find time when we both have a free moment. :) Attached is the updated patch. If you get a chance to look over it again and make sure it looks good to you, that'd be great. I did a bit of testing of it myself but wouldn't complain if someone else wanted to also. One thing I noticed while testing is that our 'disconnection' message still emits 'database=' for replication connections even though the 'connection authorized' message doesn't (presumably because we realized it's a bit silly to do so when we say 'replication connection'...). Seems like it'd be nice to have the log_connection / log_disconnection messages have some consistency about them but that's really a different discussion from this. Thanks! Stephen
From 7151ee89e1663a762f928f33ad4023e70257ef9e Mon Sep 17 00:00:00 2001 From: Stephen Frost <sfr...@snowman.net> Date: Mon, 24 Sep 2018 15:59:50 -0400 Subject: [PATCH] Add application_name to connection authorized msg The connection authorized message has quite a bit of useful information in it, but didn't include the application_name (when provided), so let's add that as it can be very useful. Note that at the point where we're emitting the connection authorized message, we haven't processed GUCs, so it's not possible to get this by using log_line_prefix (which pulls from the GUC). There's also something to be said for having this included in the connection authorized message and then not needing to repeat it for every line, as having it in log_line_prefix would do. The GUC cleans the application name to pure-ascii, so do that here too, but pull out the logic for cleaning up a string into its own function in common and re-use it from those places, and check_cluster_name which was doing the same thing. Author: Don Seiler <d...@seiler.us> Discussion: https://postgr.es/m/CAHJZqBB_Pxv8HRfoh%2BAB4KxSQQuPVvtYCzMg7woNR3r7dfmopw%40mail.gmail.com --- src/backend/postmaster/postmaster.c | 16 +++++++++ src/backend/utils/init/postinit.c | 54 ++++++++++++++++++++--------- src/backend/utils/misc/guc.c | 17 ++------- src/common/string.c | 19 ++++++++++ src/include/common/string.h | 1 + src/include/libpq/libpq-be.h | 7 ++++ 6 files changed, 84 insertions(+), 30 deletions(-) diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 305ff36258..41de140ae0 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -99,6 +99,7 @@ #include "catalog/pg_control.h" #include "common/file_perm.h" #include "common/ip.h" +#include "common/string.h" #include "lib/ilist.h" #include "libpq/auth.h" #include "libpq/libpq.h" @@ -2096,6 +2097,21 @@ retry1: pstrdup(nameptr)); port->guc_options = lappend(port->guc_options, pstrdup(valptr)); + + /* + * Copy application_name to port if we come across it. This + * is done so we can log the application_name in the + * connection authorization message. Note that the GUC would + * be used but we haven't gone through GUC setup yet. + */ + if (strcmp(nameptr, "application_name") == 0) + { + char *tmp_app_name = pstrdup(valptr); + + pg_clean_ascii(tmp_app_name); + + port->application_name = tmp_app_name; + } } offset = valoffset + strlen(valptr) + 1; } diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index 5ef6315d20..4f1d2a0d28 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -249,34 +249,56 @@ PerformAuthentication(Port *port) #ifdef USE_SSL if (port->ssl_in_use) ereport(LOG, - (errmsg("replication connection authorized: user=%s SSL enabled (protocol=%s, cipher=%s, bits=%d, compression=%s)", - port->user_name, - be_tls_get_version(port), - be_tls_get_cipher(port), - be_tls_get_cipher_bits(port), - be_tls_get_compression(port) ? _("on") : _("off")))); + (port->application_name != NULL + ? errmsg("replication connection authorized: user=%s application_name=%s SSL enabled (protocol=%s, cipher=%s, bits=%d, compression=%s)", + port->user_name, + port->application_name, + be_tls_get_version(port), + be_tls_get_cipher(port), + be_tls_get_cipher_bits(port), + be_tls_get_compression(port) ? _("on") : _("off")) + : errmsg("replication connection authorized: user=%s SSL enabled (protocol=%s, cipher=%s, bits=%d, compression=%s)", + port->user_name, + be_tls_get_version(port), + be_tls_get_cipher(port), + be_tls_get_cipher_bits(port), + be_tls_get_compression(port) ? _("on") : _("off")))); else #endif ereport(LOG, - (errmsg("replication connection authorized: user=%s", - port->user_name))); + (port->application_name != NULL + ? errmsg("replication connection authorized: user=%s application_name=%s", + port->user_name, + port->application_name) + : errmsg("replication connection authorized: user=%s", + port->user_name))); } else { #ifdef USE_SSL if (port->ssl_in_use) ereport(LOG, - (errmsg("connection authorized: user=%s database=%s SSL enabled (protocol=%s, cipher=%s, bits=%d, compression=%s)", - port->user_name, port->database_name, - be_tls_get_version(port), - be_tls_get_cipher(port), - be_tls_get_cipher_bits(port), - be_tls_get_compression(port) ? _("on") : _("off")))); + (port->application_name != NULL + ? errmsg("connection authorized: user=%s database=%s application_name=%s SSL enabled (protocol=%s, cipher=%s, bits=%d, compression=%s)", + port->user_name, port->database_name, port->application_name, + be_tls_get_version(port), + be_tls_get_cipher(port), + be_tls_get_cipher_bits(port), + be_tls_get_compression(port) ? _("on") : _("off")) + : errmsg("connection authorized: user=%s database=%s SSL enabled (protocol=%s, cipher=%s, bits=%d, compression=%s)", + port->user_name, port->database_name, + be_tls_get_version(port), + be_tls_get_cipher(port), + be_tls_get_cipher_bits(port), + be_tls_get_compression(port) ? _("on") : _("off")))); else #endif ereport(LOG, - (errmsg("connection authorized: user=%s database=%s", - port->user_name, port->database_name))); + (port->application_name != NULL + ? errmsg("connection authorized: user=%s database=%s application_name=%s", + port->user_name, port->database_name, port->application_name) + : errmsg("connection authorized: user=%s database=%s", + port->user_name, port->database_name))); } } diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index e9f542cfed..f4c8ac0605 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -41,6 +41,7 @@ #include "commands/vacuum.h" #include "commands/variable.h" #include "commands/trigger.h" +#include "common/string.h" #include "funcapi.h" #include "jit/jit.h" #include "libpq/auth.h" @@ -10754,13 +10755,7 @@ static bool check_application_name(char **newval, void **extra, GucSource source) { /* Only allow clean ASCII chars in the application name */ - char *p; - - for (p = *newval; *p; p++) - { - if (*p < 32 || *p > 126) - *p = '?'; - } + pg_clean_ascii(*newval); return true; } @@ -10776,13 +10771,7 @@ static bool check_cluster_name(char **newval, void **extra, GucSource source) { /* Only allow clean ASCII chars in the cluster name */ - char *p; - - for (p = *newval; *p; p++) - { - if (*p < 32 || *p > 126) - *p = '?'; - } + pg_clean_ascii(*newval); return true; } diff --git a/src/common/string.c b/src/common/string.c index 3260d37a84..ea31675544 100644 --- a/src/common/string.c +++ b/src/common/string.c @@ -56,3 +56,22 @@ strtoint(const char *pg_restrict str, char **pg_restrict endptr, int base) errno = ERANGE; return (int) val; } + + +/* + * pg_clean_ascii -- Replace any non-ASCII chars with a '?' char + * + * Modifies the string passed in which must be NULL-terminated. + */ +void +pg_clean_ascii(char *str) +{ + /* Only allow clean ASCII chars in the string */ + char *p; + + for (p = str; *p != '\0'; p++) + { + if (*p < 32 || *p > 126) + *p = '?'; + } +} diff --git a/src/include/common/string.h b/src/include/common/string.h index 78a450192e..7c3594557a 100644 --- a/src/include/common/string.h +++ b/src/include/common/string.h @@ -13,5 +13,6 @@ extern bool pg_str_endswith(const char *str, const char *end); extern int strtoint(const char *pg_restrict str, char **pg_restrict endptr, int base); +extern void pg_clean_ascii(char *str); #endif /* COMMON_STRING_H */ diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h index ef5528c897..eb8bba4ed8 100644 --- a/src/include/libpq/libpq-be.h +++ b/src/include/libpq/libpq-be.h @@ -138,6 +138,13 @@ typedef struct Port char *cmdline_options; List *guc_options; + /* + * The startup packet application name, only used here for the "connection + * authorized" log message. We shouldn't use this post-startup, instead + * the GUC should be used as application can change it afterward. + */ + char *application_name; + /* * Information that needs to be held during the authentication cycle. */ -- 2.17.1
signature.asc
Description: PGP signature
