<div dir="ltr"><div>Hi Ishii-San</div><div><br></div><div>The patch is good now and does what it is intended for.</div><div>We can commit it to the master branch.</div><div><br></div><div>Thanks</div><div>Best regards</div><div>Muhammad Usama</div><div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Mar 11, 2020 at 4:30 PM 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">Hi Umar,<br>
<br>
Now the regression test passed!<br>
Thank you for the update.<br>
<br>
Usama,<br>
<br>
Do you agree to commit this 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>
> Hi Ishii,<br>
> I guess I have to learn pgpool regression structure a bit more and It came<br>
> out not a good idea updating and existing test :-)<br>
> I was updating the 'ssl_crl_file' in pgpool.conf after calling './startall'<br>
> , which I suppose would not load the update configuration.<br>
> In latest patch I moved that update operation before './startall' and added<br>
> extra check to validate that pgpool configuration is correctly updated.<br>
> I hope this patch would fix the issue. Please find attached.<br>
> <br>
> Regards<br>
> Umar Hayat<br>
> <br>
> <br>
> <br>
> <br>
> On Wed, Mar 11, 2020 at 3:04 AM Tatsuo Ishii <<a href="mailto:ishii@sraoss.co.jp" target="_blank">ishii@sraoss.co.jp</a>> wrote:<br>
> <br>
>> 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<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>
>> 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<br>
>> 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<br>
>> 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<br>
>> 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>><br>
>> 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<br>
>> 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>
>> >>><br>
>> /home/t-ishii/work/Pgpool-II/current/pgpool2/src/test/regression/temp/installed<br>
>> >>> > *************************<br>
>> >>> > REGRESSION MODE : install<br>
>> >>> > PGPOOL-II :<br>
>> >>><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<br>
>> 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<br>
>> that:<br>
>> >>> >>>>> > "Relative paths are relative to the data<br>
>> >>> >>>>> > directory. This parameter can only be set in the<br>
>> 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>
>><br>
</blockquote></div></div>