Re: [PATCH] Log details for client certificate failures

Em qua., 5 de out. de 2022 às 09:19, Ranier Vilela escreveu: > Hi, > I think that 9d58bf > , > left a tiny oversight. > > guc_strdup uses strdup, so must be cleaned by free not pfree. > IMO, can be used once fre

Re: [PATCH] Log details for client certificate failures

Hi, I think that 9d58bf , left a tiny oversight. guc_strdup uses strdup, so must be cleaned by free not pfree. IMO, can be used once free call too. regards, Ranier Vilela simplifies_and_use_free_variable.patch

Re: [PATCH] Log details for client certificate failures

On Sat, Oct 1, 2022 at 7:53 PM Peter Eisentraut wrote: > > On 29.09.22 06:52, Masahiko Sawada wrote: > > While this seems a future-proof idea, I wonder if it might be overkill > > since we don't need to worry about accumulation of leaked memory in > > this case. Given that only check_cluter_name i

Re: [PATCH] Log details for client certificate failures

On 29.09.22 06:52, Masahiko Sawada wrote: While this seems a future-proof idea, I wonder if it might be overkill since we don't need to worry about accumulation of leaked memory in this case. Given that only check_cluter_name is the case where we found a small memory leak, I think it's adequate t

Re: [PATCH] Log details for client certificate failures

On Thu, Sep 29, 2022 at 1:43 AM Jacob Champion wrote: > > On Tue, Sep 27, 2022 at 6:14 PM Masahiko Sawada wrote: > > No. Since cluster_name is PGC_POSTMATER, we leak a little postmaster > > memory only once when starting up. application_name is PGC_USERSET but > > since we normally allocate memor

Re: [PATCH] Log details for client certificate failures

On Tue, Sep 27, 2022 at 6:14 PM Masahiko Sawada wrote: > No. Since cluster_name is PGC_POSTMATER, we leak a little postmaster > memory only once when starting up. application_name is PGC_USERSET but > since we normally allocate memory in PortalMemoryContext we eventually > can free it. Oh, I see;

Re: [PATCH] Log details for client certificate failures

On Wed, Sep 28, 2022 at 4:44 AM Jacob Champion wrote: > > On Tue, Sep 27, 2022 at 1:51 AM Masahiko Sawada wrote: > > I think we can fix it by the attached patch but I'd like to discuss > > whether it's worth fixing it. > > Whoops. So every time it's changed, we leak a little postmaster memory? N

Re: [PATCH] Log details for client certificate failures

On Tue, Sep 27, 2022 at 1:51 AM Masahiko Sawada wrote: > I think we can fix it by the attached patch but I'd like to discuss > whether it's worth fixing it. Whoops. So every time it's changed, we leak a little postmaster memory? Your patch looks good to me and I see no reason not to fix it. Tha

Re: [PATCH] Log details for client certificate failures

Hi, On Tue, Sep 13, 2022 at 11:11 PM Peter Eisentraut wrote: > > On 09.09.22 00:32, Jacob Champion wrote: > > On Thu, Jul 28, 2022 at 9:19 AM Jacob Champion > > wrote: > >> On Thu, Jul 21, 2022 at 4:29 PM Jacob Champion > >> wrote: > >>> v4 attempts to fix this by letting the check hooks pass

Re: [PATCH] Log details for client certificate failures

On Tue, Sep 13, 2022 at 7:11 AM Peter Eisentraut wrote: > This looks fine to me. Committed. Thanks! --Jacob

Re: [PATCH] Log details for client certificate failures

On 09.09.22 00:32, Jacob Champion wrote: On Thu, Jul 28, 2022 at 9:19 AM Jacob Champion wrote: On Thu, Jul 21, 2022 at 4:29 PM Jacob Champion wrote: v4 attempts to fix this by letting the check hooks pass MCXT_ALLOC_NO_OOM to pg_clean_ascii(). (It's ignored in the frontend, which just mallocs

Re: [PATCH] Log details for client certificate failures

On Thu, Jul 28, 2022 at 9:19 AM Jacob Champion wrote: > On Thu, Jul 21, 2022 at 4:29 PM Jacob Champion > wrote: > > v4 attempts to fix this by letting the check hooks pass > > MCXT_ALLOC_NO_OOM to pg_clean_ascii(). (It's ignored in the frontend, > > which just mallocs.) > > Ping -- should I add

Re: [PATCH] Log details for client certificate failures

On Thu, Jul 21, 2022 at 4:29 PM Jacob Champion wrote: > v4 attempts to fix this by letting the check hooks pass > MCXT_ALLOC_NO_OOM to pg_clean_ascii(). (It's ignored in the frontend, > which just mallocs.) Ping -- should I add an open item somewhere so this isn't lost? --Jacob

Re: [PATCH] Log details for client certificate failures

On Wed, Jul 20, 2022 at 3:42 PM Tom Lane wrote: > Jacob Champion writes: > > I'm currently hardcoding an elevel of ERROR on the new guc_strdup()s, > > because that seems to be a common case for the check hooks. > > Really? That's almost certainly NOT okay. As an example, if you > have a problem

Re: [PATCH] Log details for client certificate failures

Jacob Champion writes: > I'm currently hardcoding an elevel of ERROR on the new guc_strdup()s, > because that seems to be a common case for the check hooks. Really? That's almost certainly NOT okay. As an example, if you have a problem with a new value loaded from postgresql.conf during SIGHUP

Re: [PATCH] Log details for client certificate failures

On Wed, Jul 20, 2022 at 3:15 PM Tom Lane wrote: > guc_malloc's behavior varies depending on elevel. It's *not* > equivalent to palloc. Right, sorry -- a better way for me to ask the question: I'm currently hardcoding an elevel of ERROR on the new guc_strdup()s, because that seems to be a common

Re: [PATCH] Log details for client certificate failures

Jacob Champion writes: > The guc_strdup() approach really reduces the amount of code, so that's > what I did in v3. I'm not following why we need to return NULL on > failure, though -- both palloc() and guc_malloc() ERROR on failure, so > is it okay to keep those semantics the same? guc_malloc's

Re: [PATCH] Log details for client certificate failures

On Tue, Jul 19, 2022 at 3:38 PM Andres Freund wrote: > Or alternatively, perhaps we can just make pg_clean_ascii() return NULL > if allocation failed and then guc_strdup() the result in guc.c? The guc_strdup() approach really reduces the amount of code, so that's what I did in v3. I'm not followi

Re: [PATCH] Log details for client certificate failures

Hi, On 2022-07-19 15:08:38 -0700, Jacob Champion wrote: > v2 adds escaping to pg_clean_ascii(). My original attempt used > StringInfo allocation, but that didn't play well with guc_malloc(), so > I switched to a two-pass API where the caller allocates. Let me know > if I'm missing something obviou

Re: [PATCH] Log details for client certificate failures

On Tue, Jul 19, 2022 at 10:09 AM Andres Freund wrote: > On 2022-07-19 12:39:43 -0400, Tom Lane wrote: > > Having said that, I struggle to see why we are panicking about badly > > encoded log data from this source while blithely ignoring the problems > > posed by non-ASCII role names, database name

Re: [PATCH] Log details for client certificate failures

Hi, On 2022-07-19 12:39:43 -0400, Tom Lane wrote: > Having said that, I struggle to see why we are panicking about badly > encoded log data from this source while blithely ignoring the problems > posed by non-ASCII role names, database names, and tablespace names. I think we should fix these as w

Re: [PATCH] Log details for client certificate failures

Jacob Champion writes: > On 7/19/22 09:14, Andres Freund wrote: >> IMO this should replace the existing ascii escape function instead. > That will affect the existing behavior of application_name and > cluster_name; is that acceptable? I think Andres' point is exactly that these should all act a

Re: [PATCH] Log details for client certificate failures

[resending to list] On 7/19/22 09:14, Andres Freund wrote: > IMO this should replace the existing ascii escape function instead. That will affect the existing behavior of application_name and cluster_name; is that acceptable? --Jacob

Re: [PATCH] Log details for client certificate failures

Hi, On 2022-07-19 09:07:31 -0700, Jacob Champion wrote: > On Fri, Jul 15, 2022 at 4:45 PM Andres Freund wrote: > > On 2022-07-15 14:51:38 -0700, Jacob Champion wrote: > > > That seems much worse than escaping for this particular patch; if your > > > cert's Common Name is in (non-ASCII) UTF-8 then

Re: [PATCH] Log details for client certificate failures

On Fri, Jul 15, 2022 at 4:45 PM Andres Freund wrote: > On 2022-07-15 14:51:38 -0700, Jacob Champion wrote: > > That seems much worse than escaping for this particular patch; if your > > cert's Common Name is in (non-ASCII) UTF-8 then all you'll see is > > "CN=?" in the log lines that were

Re: [PATCH] Log details for client certificate failures

Hi, On 2022-07-15 14:51:38 -0700, Jacob Champion wrote: > > We already have pg_clean_ascii() and use it for application_name, fwiw. > > That seems much worse than escaping for this particular patch; if your > cert's Common Name is in (non-ASCII) UTF-8 then all you'll see is > "CN=?" in th

Re: [PATCH] Log details for client certificate failures

On 7/15/22 14:19, Andres Freund wrote: > On 2022-07-15 14:01:53 -0700, Jacob Champion wrote: >> On 7/15/22 13:35, Andres Freund wrote: >>> I don't know, but I don't think not caring at all is a good >>> option. Particularly for unauthenticated data I'd say that escaping >>> everything >>> but prin

Re: [PATCH] Log details for client certificate failures

Andres Freund writes: > We already have pg_clean_ascii() and use it for application_name, fwiw. Yeah, we should just use that. If anyone wants to upgrade the situation for non-ASCII data later, fixing it for all of these cases at once would be appropriate. regards, tom l

Re: [PATCH] Log details for client certificate failures

Hi, On 2022-07-15 14:01:53 -0700, Jacob Champion wrote: > On 7/15/22 13:35, Andres Freund wrote: > >> (And do we want to fix it now, regardless?) > > > > Yes. > > Cool. I can get on board with that. > > >> What guarantees are we supposed to be making for log encoding? > > > > I don't know, but

Re: [PATCH] Log details for client certificate failures

On 7/15/22 13:35, Andres Freund wrote: >> (And do we want to fix it now, regardless?) > > Yes. Cool. I can get on board with that. >> What guarantees are we supposed to be making for log encoding? > > I don't know, but I don't think not caring at all is a good > option. Particularly for unauthe

Re: [PATCH] Log details for client certificate failures

Hi, On 2022-07-15 13:20:59 -0700, Jacob Champion wrote: > On 7/15/22 12:11, Andres Freund wrote: > > This might have been discussed somewhere, but I'm worried about emitting > > unescaped data from pre-auth clients. What guarantees that subject / issuer > > name only contain printable ascii-chars?

Re: [PATCH] Log details for client certificate failures

On 7/15/22 12:11, Andres Freund wrote: > This might have been discussed somewhere, but I'm worried about emitting > unescaped data from pre-auth clients. What guarantees that subject / issuer > name only contain printable ascii-chars? Printing terminal control chars or > such would not be great, no

Re: [PATCH] Log details for client certificate failures

Hi, On 2022-07-15 09:46:40 -0700, Jacob Champion wrote: > On 7/15/22 09:34, Peter Eisentraut wrote: > > Committed like that. > > Thanks for all the reviews! This might have been discussed somewhere, but I'm worried about emitting unescaped data from pre-auth clients. What guarantees that subject

Re: [PATCH] Log details for client certificate failures

On 7/15/22 09:34, Peter Eisentraut wrote: > Committed like that. Thanks for all the reviews! --Jacob

Re: [PATCH] Log details for client certificate failures

On 14.07.22 23:09, Jacob Champion wrote: On Thu, Jul 14, 2022 at 1:12 PM Peter Eisentraut wrote: Concretely, I was thinking like the attached top-up patch. The other way can surely be made to work somehow, but this seems much simpler and with fewer questions about the details. Ah, seeing it

Re: [PATCH] Log details for client certificate failures

On Thu, Jul 14, 2022 at 1:12 PM Peter Eisentraut wrote: > Concretely, I was thinking like the attached top-up patch. > > The other way can surely be made to work somehow, but this seems much > simpler and with fewer questions about the details. Ah, seeing it side-by-side helps. That's much easier

Re: [PATCH] Log details for client certificate failures

On 13.07.22 01:06, Jacob Champion wrote: I had to read up on this "ex_data" API. Interesting. But I'm wondering a bit about how the life cycle of these objects is managed. What happens if the allocated error string is deallocated before its containing object? Or vice versa? Yeah, I'm curren

Re: [PATCH] Log details for client certificate failures

On Mon, Jul 11, 2022 at 6:09 AM Peter Eisentraut wrote: > I squashed those two together. I also adjusted the error message a bit > more for project style. (We can put both lines into detail.) Oh, okay. Log parsers don't have any issues with that? > I had to read up on this "ex_data" API. Inte

Re: [PATCH] Log details for client certificate failures

On Sat, Jul 9, 2022 at 6:49 AM Graham Leggett wrote: > Please don’t invent another format, or try and truncate the data. This is a > huge headache when troubleshooting. I hear you, and I agree that correlating these things across machines is something we should be making easier. I'm just not con

Re: [PATCH] Log details for client certificate failures

On 08.07.22 20:39, Jacob Champion wrote: I also added an optional 0002 that bubbles the error info up to the final ereport(ERROR), using errdetail() and errhint(). I can squash it into 0001 if you like it, or drop it if you don't. (This approach could be adapted to the client, too.) I squashed

Re: [PATCH] Log details for client certificate failures

On 01 Jul 2022, at 22:59, Jacob Champion wrote: >> I added this to httpd a while back: >> >> SSL_CLIENT_CERT_RFC4523_CEA >> >> It would be good to interoperate. > > What kind of interoperation did you have in mind? Are there existing > tools that want to scrape this information for observabili

Re: [PATCH] Log details for client certificate failures

On Thu, Jul 7, 2022 at 2:50 AM Peter Eisentraut wrote: > I looked into how you decode the serial number. I have found some code > elsewhere that passed the result of X509_get_serialNumber() directly to > ASN1_INTEGER_set(). But I guess a serial number of maximum length 20 > octets wouldn't fit i

Re: [PATCH] Log details for client certificate failures

On 05.07.22 18:34, Jacob Champion wrote: On Fri, Jul 1, 2022 at 1:51 PM Jacob Champion wrote: Sorry for the misunderstanding! v3 adds the Issuer to the logs as well. Resending v3; I messed up the certificate diff with my gitconfig. This patch looks pretty good to me. Some minor details: I

Re: [PATCH] Log details for client certificate failures

On Fri, Jul 1, 2022 at 1:51 PM Jacob Champion wrote: > Sorry for the misunderstanding! v3 adds the Issuer to the logs as well. Resending v3; I messed up the certificate diff with my gitconfig. --Jacob From 8d03e043cd86ec81dfb49a138e871c5ac110dc06 Mon Sep 17 00:00:00 2001 From: Jacob Champion Da

Re: [PATCH] Log details for client certificate failures

On Thu, Jun 30, 2022 at 2:54 AM Graham Leggett wrote: > > I added this to httpd a while back: > > SSL_CLIENT_CERT_RFC4523_CEA > > It would be good to interoperate. What kind of interoperation did you have in mind? Are there existing tools that want to scrape this information for observability? I

Re: [PATCH] Log details for client certificate failures

On Thu, Jun 30, 2022 at 2:43 AM Peter Eisentraut wrote: > > On 13.05.22 00:36, Jacob Champion wrote: > > v2 limits the maximum subject length and adds the serial number to the > > logs. > > I wrote that pg_stat_ssl uses the *issuer* plus serial number to > identify a certificate. What your patch

Re: [PATCH] Log details for client certificate failures

On 30 Jun 2022, at 10:43, Peter Eisentraut wrote: > I wrote that pg_stat_ssl uses the *issuer* plus serial number to identify a > certificate. What your patch shows is the subject and the serial number, > which isn't the same thing. Let's get that sorted out one way or the other. Quick obse

Re: [PATCH] Log details for client certificate failures

On 13.05.22 00:36, Jacob Champion wrote: On Thu, 2022-05-05 at 15:12 +, Jacob Champion wrote: On Wed, 2022-05-04 at 15:53 +0200, Peter Eisentraut wrote: In terms of aligning what is printed, I meant that pg_stat_ssl uses the issuer plus serial number to identify the certificate unambiguousl

Re: [PATCH] Log details for client certificate failures

On Thu, 2022-05-05 at 15:12 +, Jacob Champion wrote: > On Wed, 2022-05-04 at 15:53 +0200, Peter Eisentraut wrote: > > In terms of aligning what is printed, I meant that pg_stat_ssl uses the > > issuer plus serial number to identify the certificate unambiguously. > > Oh, that's a great idea. I'

Re: [PATCH] Log details for client certificate failures

On Wed, 2022-05-04 at 15:53 +0200, Peter Eisentraut wrote: > Just saying that cutting it off appears to be acceptable. A bit more > than 63 bytes should be okay for the log. Gotcha. > In terms of aligning what is printed, I meant that pg_stat_ssl uses the > issuer plus serial number to identify

Re: [PATCH] Log details for client certificate failures

On 04.05.22 01:05, Jacob Champion wrote: On Tue, 2022-05-03 at 21:06 +0200, Peter Eisentraut wrote: The information in pg_stat_ssl is limited to NAMEDATALEN (see struct PgBackendSSLStatus). It might make sense to align what your patch prints to identify certificates with what is shown in that

Re: [PATCH] Log details for client certificate failures

On Tue, 2022-05-03 at 21:06 +0200, Peter Eisentraut wrote: > The information in pg_stat_ssl is limited to NAMEDATALEN (see struct > PgBackendSSLStatus). > > It might make sense to align what your patch prints to identify > certificates with what is shown in that view. Sure, a max length should be

Re: [PATCH] Log details for client certificate failures

On 03.05.22 19:04, Jacob Champion wrote: One question/concern -- the Subject that's printed to the logs could be pretty big (OpenSSL limits the incoming certificate chain to 100K, by default), which introduces an avenue for intentional log spamming. Is there an existing convention for limiting th

[PATCH] Log details for client certificate failures

353c Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Mon, 29 Mar 2021 10:07:31 -0700 Subject: [PATCH] Log details for client certificate failures Currently, debugging client cert verification failures is mostly limited to looking at the TLS alert code on the client side. For simple deployments, som