<div dir="ltr">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 not configure CA certificates? <br>Meaning what would be the behavior of the system if someone specifies "ssl_crl_file" but leave out the ssl_ca_cert config?<br><br><br>2- In the patch you are loading the Certificate Revocation List after 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 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 sequence, we always load<br>the CA and other certificated before SSL_CTX_set_verify and SSL_CTX_set_client_CA_list functions.<br><br>So I guess the safe bet is to move the loading of Certificate Revocation List before<br>SSL_CTX_set_verify.<br><br>3- The comment /* OpenSSL 0.96 does not support X509_V_FLAG_CRL_CHECK */ mentions <br>OpenSSL version as 0.96, which is wrong and should be 0.9.6<br><br>4- In the test case you are only exporting PGSSLCERT and PGSSLKEY environment variables<br>that means the test case will try to pick up the root certificate from the default location<br>and on the systems where the root certificate would not be found there, the test case will fail.<br><br>So I think you need to modify the test case and use PGSSLROOTCERT environment variable to<br>explicitly specify the root certificate as well.<br><div><br></div>These comments are on top of Ishii-San's review comments.<div><span style="background-color:rgb(222,244,255);color:rgb(25,30,36);font-family:Consolas,Monaco,"Courier New",monospace;font-size:13.125px"><br></span></div>Thanks<br>Best Regards<br>Muhammad Usama<div><span style="background-color:rgb(222,244,255);color:rgb(25,30,36);font-family:Consolas,Monaco,"Courier New",monospace;font-size:13.125px"><br></span></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Mar 4, 2020 at 12:11 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>
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 (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 /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>
> 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>> 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 variable.<br>
>>>> 2. Updated test case '024.cert_auth' which verify valid CRL and 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>> wrote:<br>
>>>><br>
>>>>> > I just followed the description pattern used for other ssl 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 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 src/test/023.ssl_connection.<br>
>>>>> > I will try to generate CRL file for existing certification file 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>
</blockquote></div>