<div dir="ltr">Hi Ishii,<div>I guess I have to learn pgpool regression structure a bit more and It came out not a good idea updating and existing test :-)</div><div>I was updating the 'ssl_crl_file' in pgpool.conf after calling './startall' , which I suppose would not load the update configuration. <br></div><div>In latest patch I moved that update operation before './startall' and added extra check to validate that pgpool configuration is correctly updated.</div><div>I hope this patch would fix the issue. Please find attached.</div><div><br></div><div>Regards</div><div>Umar Hayat</div><div><br></div><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Mar 11, 2020 at 3:04 AM Tatsuo Ishii <<a href="mailto:ishii@sraoss.co.jp">ishii@sraoss.co.jp</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Unfortunately the regression test failed again.<br>
<br>
$ ./regress.sh 024<br>
creating pgpool-II temporary installation ...<br>
moving pgpool_setup to temporary installation path ...<br>
moving watchdog_setup to temporary installation path ...<br>
using pgpool-II at /home/t-ishii/work/Pgpool-II/current/pgpool2/src/test/regression/temp/installed<br>
*************************<br>
REGRESSION MODE : install<br>
PGPOOL-II : /home/t-ishii/work/Pgpool-II/current/pgpool2/src/test/regression/temp/installed<br>
PostgreSQL bin : /usr/local/pgsql/bin<br>
PostgreSQL Major version : 12<br>
pgbench : /usr/local/pgsql/bin/pgbench<br>
PostgreSQL jdbc : /usr/local/pgsql/share/postgresql-9.2-1003.jdbc4.jar<br>
*************************<br>
testing 024.cert_auth...failed.<br>
out of 1 ok:0 failed:1 timeout:0<br>
<br>
regression log attached.<br>
<br>
> Thanks Usama for feedback. updated patch with suggested changed attached.<br>
> Please see comments inline.<br>
> <br>
> Ishii,<br>
> IMO, tests were failing because some certificates generation and signing<br>
> was using global configuration. Previously, I only provided<br>
> minimum configuration for CRL generation.<br>
> Now I provided minimum custom configuration for Cert generation and Signing<br>
> too. I hope now that last tests should be passing now.<br>
> <br>
> Regards<br>
> Umar Hayat<br>
> <br>
> <br>
> On Wed, Mar 4, 2020 at 7:19 PM Muhammad Usama <<a href="mailto:m.usama@gmail.com" target="_blank">m.usama@gmail.com</a>> wrote:<br>
> <br>
>> Hi<br>
>><br>
>> I have looked into the patch and here are my two cents.<br>
>><br>
>> 1- Does the "Certificate Revocation List (CRL)" have any effect if we do<br>
>> not configure CA certificates?<br>
>> Meaning what would be the behavior of the system if someone specifies<br>
>> "ssl_crl_file" but leave out the ssl_ca_cert config?<br>
>><br>
>> If CA is not configure then certification validation will fail while<br>
> validating certification.<br>
> <br>
>><br>
>> 2- In the patch you are loading the Certificate Revocation List after<br>
>> configuring<br>
>> SSL library to ask for client certificates<br>
>> i.e after calling SSL_CTX_set_verify and SSL_CTX_set_client_CA_list<br>
>> functions<br>
>><br>
>> Although I am not able to find any related SSL documentation on the<br>
>> subject and not sure if it can cause any problem. But in the normal<br>
>> sequence, we always load<br>
>> the CA and other certificated before SSL_CTX_set_verify and<br>
>> SSL_CTX_set_client_CA_list functions.<br>
>><br>
>> So I guess the safe bet is to move the loading of Certificate Revocation<br>
>> List before<br>
>> SSL_CTX_set_verify.<br>
>><br>
>> I wasn't sure either that would matter as we are just registering event at<br>
> that state, but as suggest i reorder the code as you suggested ( as<br>
> Postgres does too )<br>
> <br>
>> 3- The comment /* OpenSSL 0.96 does not support X509_V_FLAG_CRL_CHECK */<br>
>> mentions<br>
>> OpenSSL version as 0.96, which is wrong and should be 0.9.6<br>
>><br>
>> Comment fixed.<br>
> <br>
>> 4- In the test case you are only exporting PGSSLCERT and PGSSLKEY<br>
>> environment variables<br>
>> that means the test case will try to pick up the root certificate from the<br>
>> default location<br>
>> and on the systems where the root certificate would not be found there,<br>
>> the test case will fail.<br>
>><br>
>> So I think you need to modify the test case and use PGSSLROOTCERT<br>
>> environment variable to<br>
>> explicitly specify the root certificate as well.<br>
>><br>
>> PGSSLROOTCERT defined.<br>
> <br>
>> These comments are on top of Ishii-San's review comments.<br>
>><br>
>> Thanks<br>
>> Best Regards<br>
>> Muhammad Usama<br>
>><br>
>><br>
>> On Wed, Mar 4, 2020 at 12:11 PM Tatsuo Ishii <<a href="mailto:ishii@sraoss.co.jp" target="_blank">ishii@sraoss.co.jp</a>> wrote:<br>
>><br>
>>> Hi Umar,<br>
>>><br>
>>> Any update on this?<br>
>>><br>
>>> From: Tatsuo Ishii <<a href="mailto:ishii@sraoss.co.jp" target="_blank">ishii@sraoss.co.jp</a>><br>
>>> Subject: [pgpool-hackers: 3523] Re: [PATCH] Feature: Support for CRL<br>
>>> (Certificate Revocation List)<br>
>>> Date: Fri, 28 Feb 2020 14:27:55 +0900 (JST)<br>
>>> Message-ID: <<a href="mailto:20200228.142755.205379439290164558.t-ishii@sraoss.co.jp" target="_blank">20200228.142755.205379439290164558.t-ishii@sraoss.co.jp</a>><br>
>>><br>
>>> > Here are comments on your patch.<br>
>>> ><br>
>>> > - There are some extra trailing spaces.<br>
>>> ><br>
>>> > $ git apply ~/crl_support_with_testcase.diff<br>
>>> > /home/t-ishii/crl_support_with_testcase.diff:42: trailing whitespace.<br>
>>> > Specifies the name of the file containing the SSL server<br>
>>> > /home/t-ishii/crl_support_with_testcase.diff:43: trailing whitespace.<br>
>>> > certificate revocation list (CRL). The default is empty,<br>
>>> > /home/t-ishii/crl_support_with_testcase.diff:199: new blank line at EOF.<br>
>>> > +<br>
>>> > warning: 3 lines add whitespace errors.<br>
>>> ><br>
>>> > - The pached source code compililes without any error.<br>
>>> ><br>
>>> > - the regression test (024.cert_auth) failed.<br>
>>> ><br>
>>> > ./regress.sh 024<br>
>>> > creating pgpool-II temporary installation ...<br>
>>> > moving pgpool_setup to temporary installation path ...<br>
>>> > moving watchdog_setup to temporary installation path ...<br>
>>> > using pgpool-II at<br>
>>> /home/t-ishii/work/Pgpool-II/current/pgpool2/src/test/regression/temp/installed<br>
>>> > *************************<br>
>>> > REGRESSION MODE : install<br>
>>> > PGPOOL-II :<br>
>>> /home/t-ishii/work/Pgpool-II/current/pgpool2/src/test/regression/temp/installed<br>
>>> > PostgreSQL bin : /usr/local/pgsql/bin<br>
>>> > PostgreSQL Major version : 12<br>
>>> > pgbench : /usr/local/pgsql/bin/pgbench<br>
>>> > PostgreSQL jdbc :<br>
>>> /usr/local/pgsql/share/postgresql-9.2-1003.jdbc4.jar<br>
>>> > *************************<br>
>>> > testing 024.cert_auth...failed.<br>
>>> > out of 1 ok:0 failed:1 timeout:0<br>
>>> ><br>
>>> > This is Ubuntu 18.04.4 LTS.<br>
>>> ><br>
>>> > $ openssl version<br>
>>> > OpenSSL 1.1.1 11 Sep 2018<br>
>>> ><br>
>>> > Please find attached log file for the<br>
>>> > regression test.<br>
>>> ><br>
>>> > Best regards,<br>
>>> > --<br>
>>> > Tatsuo Ishii<br>
>>> > SRA OSS, Inc. Japan<br>
>>> > English: <a href="http://www.sraoss.co.jp/index_en.php" rel="noreferrer" target="_blank">http://www.sraoss.co.jp/index_en.php</a><br>
>>> > Japanese:<a href="http://www.sraoss.co.jp" rel="noreferrer" target="_blank">http://www.sraoss.co.jp</a><br>
>>> ><br>
>>> >> Hi Umar,<br>
>>> >><br>
>>> >> I seemed to miss your last email. I will take care your patch<br>
>>> >> tomorrow morning.<br>
>>> >><br>
>>> >> Best regards,<br>
>>> >> --<br>
>>> >> Tatsuo Ishii<br>
>>> >> SRA OSS, Inc. Japan<br>
>>> >> English: <a href="http://www.sraoss.co.jp/index_en.php" rel="noreferrer" target="_blank">http://www.sraoss.co.jp/index_en.php</a><br>
>>> >> Japanese:<a href="http://www.sraoss.co.jp" rel="noreferrer" target="_blank">http://www.sraoss.co.jp</a><br>
>>> >><br>
>>> >>> Hi Tatsuo,<br>
>>> >>> Any update for last patch?<br>
>>> >>> I will be sending more patches in the same area of SSL ( for few other<br>
>>> >>> features ) and the those patches might create conflict on merge.<br>
>>> >>><br>
>>> >>> Regards,<br>
>>> >>> Umar Hayat<br>
>>> >>> Principal Software Engineer<br>
>>> >>> EnterpriseDB: <a href="https://www.enterprisedb.com" rel="noreferrer" target="_blank">https://www.enterprisedb.com</a><br>
>>> >>><br>
>>> >>> On Wed, Feb 19, 2020 at 1:39 PM Umar Hayat <<a href="mailto:m.umarkiani@gmail.com" target="_blank">m.umarkiani@gmail.com</a>><br>
>>> wrote:<br>
>>> >>><br>
>>> >>>> Hi Tatsuo,<br>
>>> >>>> Please find the attached updated patch with following changes:<br>
>>> >>>> 1. Updated the description of '*ssl_crl_file'* configuration<br>
>>> variable.<br>
>>> >>>> 2. Updated test case '024.cert_auth' which verify valid CRL and<br>
>>> invalid<br>
>>> >>>> CRL ( CRL with revocation entry )<br>
>>> >>>><br>
>>> >>>> Regards,<br>
>>> >>>> Umar Hayat<br>
>>> >>>><br>
>>> >>>><br>
>>> >>>> On Thu, Feb 13, 2020 at 3:43 AM Tatsuo Ishii <<a href="mailto:ishii@sraoss.co.jp" target="_blank">ishii@sraoss.co.jp</a>><br>
>>> wrote:<br>
>>> >>>><br>
>>> >>>>> > I just followed the description pattern used for other ssl<br>
>>> variables. We<br>
>>> >>>>> > can use PostgreSQL doc if we remove following two line from that:<br>
>>> >>>>> > "Relative paths are relative to the data<br>
>>> >>>>> > directory. This parameter can only be set in the postgresql.conf<br>
>>> file<br>
>>> >>>>> > or on the server command line.<br>
>>> >>>>> > "<br>
>>> >>>>><br>
>>> >>>>> Sounds good to me.<br>
>>> >>>>><br>
>>> >>>>> > - It would be nice to include regression test patch. See<br>
>>> >>>>> >> src/test/023.ssl_connection for an example.<br>
>>> >>>>> >><br>
>>> >>>>> ><br>
>>> >>>>> > Sure, I will create and send test patch in<br>
>>> src/test/023.ssl_connection.<br>
>>> >>>>> > I will try to generate CRL file for existing certification file<br>
>>> in this<br>
>>> >>>>> > this test. If thats not possible, then I have to generate new<br>
>>> >>>>> certification<br>
>>> >>>>> > and CRL file.<br>
>>> >>>>><br>
>>> >>>>> Thank you. Looking forward to the new patch.<br>
>>> >>>>><br>
>>> >>>>> Best regards,<br>
>>> >>>>> --<br>
>>> >>>>> Tatsuo Ishii<br>
>>> >>>>> SRA OSS, Inc. Japan<br>
>>> >>>>> English: <a href="http://www.sraoss.co.jp/index_en.php" rel="noreferrer" target="_blank">http://www.sraoss.co.jp/index_en.php</a><br>
>>> >>>>> Japanese:<a href="http://www.sraoss.co.jp" rel="noreferrer" target="_blank">http://www.sraoss.co.jp</a><br>
>>> >>>>><br>
>>> >>>><br>
>>> >> _______________________________________________<br>
>>> >> pgpool-hackers mailing list<br>
>>> >> <a href="mailto:pgpool-hackers@pgpool.net" target="_blank">pgpool-hackers@pgpool.net</a><br>
>>> >> <a href="http://www.pgpool.net/mailman/listinfo/pgpool-hackers" rel="noreferrer" target="_blank">http://www.pgpool.net/mailman/listinfo/pgpool-hackers</a><br>
>>> _______________________________________________<br>
>>> pgpool-hackers mailing list<br>
>>> <a href="mailto:pgpool-hackers@pgpool.net" target="_blank">pgpool-hackers@pgpool.net</a><br>
>>> <a href="http://www.pgpool.net/mailman/listinfo/pgpool-hackers" rel="noreferrer" target="_blank">http://www.pgpool.net/mailman/listinfo/pgpool-hackers</a><br>
>>><br>
>><br>
</blockquote></div>