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