[pgpool-hackers: 3550] Re: [PATCH] Feature: Support for CRL (Certificate Revocation List)
Muhammad Usama
m.usama at gmail.com
Fri Mar 13 23:19:38 JST 2020
Hi Ishii-San
The patch is good now and does what it is intended for.
We can commit it to the master branch.
Thanks
Best regards
Muhammad Usama
On Wed, Mar 11, 2020 at 4:30 PM Tatsuo Ishii <ishii at sraoss.co.jp> wrote:
> 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
> >> >>>
> >> >>
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.sraoss.jp/pipermail/pgpool-hackers/attachments/20200313/418d803e/attachment-0001.html>
More information about the pgpool-hackers
mailing list