Subject: Re: [PATCH] Include application_name in "connection authorized" log message

From: Don Seiler <[email-masked]>
Date: Tue, 7 Aug 2018 08:31:38 -0500

On Mon, Jul 30, 2018 at 5:20 AM, Peter Eisentraut <
[email-masked]> wrote:

> On 13/07/2018 20:20, Don Seiler wrote:
> > See attached for latest revision.
>
> This doesn't compile with SSL enabled because there is a comma missing.
>

Hmm I'll check this out tonight. Sorry I wasn't able to get to this until
now.

> This implementation doesn't run the application_name through
> check_application_name(), so it could end up logging application_name
> values that are otherwise not acceptable.  I'm not sure of the best way
> to fix that.

Is the concern that any user can set their client's application name value
to any string they want? Is there a reason we can't call
check_application_name() before setting it in the Port struct in
postmaster.c?

Don.

-- 
Don Seiler
www.seiler.us

---

From: Stephen Frost <[email-masked]>
Date: Tue, 7 Aug 2018 09:46:08 -0400

Greetings,

> Hmm I'll check this out tonight. Sorry I wasn't able to get to this until

Thanks.

> check_application_name() before setting it in the Port struct in

I've not looked very closely, but I don't think it's necessairly a big
deal to print out the application name provided by the client system
into the log before we run check_application_name(), as long as there
isn't any risk that printing it out or passing it around without
performing that check will cause incorrect operation or such.

I'm guessing it's not easy because check_application_name() isn't easily
available from where we're wanting to print it out or earlier, but,
again, haven't looked closely.  If you aren't sure or run into issues,
feel free to ping me on slack and I'll be happy to help.

Thanks!

Stephen

---

From: Tom Lane <[email-masked]>
Date: Tue, 07 Aug 2018 09:59:09 -0400

> performing that check will cause incorrect operation or such.

I think the issue is exactly that putting a malformed appname into the
postmaster log could confuse log-reading apps (eg by causing encoding
problems).  Moreover, if you don't check it then the appname recorded
by log_connections would not match appearances for the same session
later in the log, which puts the entire use-case for this patch into
question.  So no, this concern must not be dismissed.

However ... I've not looked at the patch, but I thought the idea was to
allow assignment of that GUC to occur before the log_connections log entry
is emitted, so that it'd show up in the entry's log_line_prefix.  Wouldn't
check_application_name happen automatically at that point?

			regards, tom lane

---

From: Don Seiler <[email-masked]>
Date: Tue, 7 Aug 2018 09:09:13 -0500

> > Hmm I'll check this out tonight. Sorry I wasn't able to get to this until

Attaching a new patch fixing the missing comma. Sorry for missing this,
obviously my previous test builds didn't have SSL enabled. Mea culpa.

I'll send a new one if we require a change for the check_application_name()
concern. I need the git rebase/autosquash practice anyway.

Thanks!
Don.

-- 
Don Seiler
www.seiler.us

---

From: Stephen Frost <[email-masked]>
Date: Tue, 7 Aug 2018 10:20:03 -0400

Greetings,

* Tom Lane ([email-masked]) wrote:
> problems).  

Evidently, I need to go look at this, since it seems like this would be
the exact same risk with "user", which is part of why I wasn't terribly
concerned about it here.  Considering the concern raised here, if we're
serious about that issue, then I wonder if we have an existing issue.

> Moreover, if you don't check it then the appname recorded

If the call to check_application_name() fails then I had been expecting
the connection to fail.  If we continue to let the connection go on then
we've already got an issue as someone might pass in an application name
that isn't being set to the GUC and isn't being therefore used in the
existing log_line_prefix where it should be.

> check_application_name happen automatically at that point?

We log that message quite early and it certainly didn't look trivial to
set up the GUC to be already in place at that point, so the plan was to
simply spit out what gets passed in (as we were doing for "user", if I'm
remembering that code correctly...).

Thanks!

Stephen

---

From: Stephen Frost <[email-masked]>
Date: Tue, 7 Aug 2018 10:22:34 -0400

Greetings Don,

> obviously my previous test builds didn't have SSL enabled. Mea culpa.

No worries, it happens. :)  Thanks for working on a new patch.

> concern. I need the git rebase/autosquash practice anyway.

Sounds like maybe we should be looking at if the concerns raised by Tom
might be applicable to the other strings being written out by that log
message, and if not, why not, and can we do whatever we're doing for
those for application_name as well.

Thanks!

Stephen

---

From: Tom Lane <[email-masked]>
Date: Tue, 07 Aug 2018 10:42:51 -0400

> existing log_line_prefix where it should be.

No, check_application_name doesn't reject funny names, it just silently
modifies them in-place.

> remembering that code correctly...).

Hm.  Well, the code isn't exactly complicated, you could duplicate it.
Or maybe better refactor to allow it to be called from $wherever.
Looks like check_cluster_name, for one, could also share use of
an ascii-lobotomizing subroutine.

But having said that, I don't exactly see why you couldn't force it
with an ultimately-redundant SetConfigOption call to put the value
in place before the ereport happens.  The GUC machinery is surely
functional before we do authorization.

			regards, tom lane

---

From: Stephen Frost <[email-masked]>
Date: Tue, 7 Aug 2018 10:57:59 -0400

Greetings Tom,

> No, check_application_name doesn't reject funny names, it just silently

Blargh, that doesn't seem particularly good to me, but I guess no one
has been comlaining about it either.

> an ascii-lobotomizing subroutine.

Yeah, I'd be alright with either of those approaches.

> functional before we do authorization.

If that's the approach you think makes the most sense, I wouldn't object
to it.  I will point out that we'd end up with the application name in
the log line if it's also included in log_line_prefix, but that's what
happens with "user" anyway, isn't it?, so that doesn't seem to be a big
deal.  I do think it's still good to have appplication_name explicitly
in the log message for users who want to just log application_name on
connection and not have it on every single log line.

Thanks!

Stephen

---

From: Tom Lane <[email-masked]>
Date: Tue, 07 Aug 2018 12:29:15 -0400

> connection and not have it on every single log line.

Well, if you're going to insist on that part, it's probably not worth
making the application_name GUC have inconsistent behavior.

			regards, tom lane

---

From: Don Seiler <[email-masked]>
Date: Tue, 7 Aug 2018 11:57:11 -0500

> making the application_name GUC have inconsistent behavior.

OK so just to make sure I understand:

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()?)
3. Call this function when storing the value in the port struct.

Please let me know if I'm missing/misunderstanding anything.

Don.

-- 
Don Seiler
www.seiler.us

---

From: Tom Lane <[email-masked]>
Date: Tue, 07 Aug 2018 13:32:42 -0400

> 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.

			regards, tom lane