Subject: Re: Is it worth accepting multiple CRLs?
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
References: <20200831.180302.1762036380359974457.horikyota.ntt@gmail.com>
 <591fd449-f475-c2f1-0606-b2a47887c23f@enterprisedb.com>
 <20210119.091734.2050908166228453830.horikyota.ntt@gmail.com>
 <20210119.173200.1687041891704633506.horikyota.ntt@gmail.com>
Cc: sfrost@snowman.net, pgsql-hackers@postgresql.org
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Organization: EnterpriseDB
Message-ID: <516cf19c-840b-3db6-3320-4145d49c24d8@enterprisedb.com>
Date: Sat, 30 Jan 2021 22:20:19 +0100
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:52.0)
 Gecko/20100101 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20210119.173200.1687041891704633506.horikyota.ntt@gmail.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2021-01-19 09:32, Kyotaro Horiguchi wrote:
> At Tue, 19 Jan 2021 09:17:34 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
>> By the way we can do the same thing on CA file/dir, but I personally
>> think that the benefit from the specify-by-directory for CA files is
>> far less than CRL files. So I'm not going to do this for CA files for
>> now.
> 
> This is it. A new guc ssl_crl_dir and connection option crldir are
> added.

This looks pretty good to me overall.

You need to update the expected result of the postgres_fdw test.

Also check your patch for whitespace errors with git diff --check or 
similar.

> One problem raised upthread is the footprint for test is quite large
> because all certificate and key files are replaced by this patch. I
> think we can shrink the footprint by generating that files on-demand
> but that needs openssl frontend to be installed on the development
> environment.

I don't understand why you need to recreate all these files.  All your 
patch should contain are the new *.r0 files that are computed from the 
existing *.crl files.  Nothing else should change, AIUI.

Some of the makefile rules for generating the CRL files need some 
refinement.  In

+ssl/root+server-crldir: ssl/server.crl
+   mkdir ssl/root+server-crldir
+   cp ssl/server.crl ssl/root+server-crldir/`openssl crl -hash -noout 
-in ssl/server.crl`.r0
+   cp ssl/root.crl ssl/root+server-crldir/`openssl crl -hash -noout -in 
ssl/root.crl`.r0
+ssl/root+client-crldir: ssl/client.crl
+   mkdir ssl/root+client-crldir
+   cp ssl/client.crl ssl/root+client-crldir/`openssl crl -hash -noout 
-in ssl/client.crl`.r0
+   cp ssl/root.crl ssl/root+client-crldir/`openssl crl -hash -noout -in 
ssl/root.crl`.r0

the rules should also have a dependency on ssl/root.crl in addition to 
ssl/server.crl.

By the way:

-   print $sslconf "ssl_crl_file='root+client.crl'\n";
+   print $sslconf "ssl_crl_file='$crlfile'\n" if (defined $crlfile);
+   print $sslconf "ssl_crl_dir='$crldir'\n" if (defined $crldir);

Trailing "if" doesn't need parentheses.