[pgpool-hackers: 3546] Re: [PATCH] Feature: Support for CRL (Certificate Revocation List)

Umar Hayat m.umarkiani at gmail.com
Wed Mar 11 17:55:31 JST 2020 

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
> >>>
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.sraoss.jp/pipermail/pgpool-hackers/attachments/20200311/89efb3ed/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: crl_support_11march2020.diff
Type: application/octet-stream
Size: 15140 bytes
Desc: not available
URL: <http://www.sraoss.jp/pipermail/pgpool-hackers/attachments/20200311/89efb3ed/attachment-0001.obj>

More information about the pgpool-hackers mailing list