[pgpool-hackers: 3541] Re: [PATCH] Feature: Support for CRL (Certificate Revocation List)
Umar Hayat
m.umarkiani at gmail.com
Tue Mar 10 19:37:50 JST 2020
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/20200310/ace51884/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: crl_support_10march2020.diff
Type: application/octet-stream
Size: 14464 bytes
Desc: not available
URL: <http://www.sraoss.jp/pipermail/pgpool-hackers/attachments/20200310/ace51884/attachment-0001.obj>
More information about the pgpool-hackers
mailing list