Thread

Commits

Allow specifying CRL directory
- f5465fade908 14.0 landed
Introduce --with-ssl={openssl} as a configure option
- fe61df7f82aa 14.0 cited

Is it worth accepting multiple CRLs?

Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-07-31T08:39:11Z

Hello.

PostgreSQL server accepts only one CRL file. It is easy to expand
be_tls_init to accept a directory set in ssl_crl_file. But I'm not
sure CRL is actually even utilized in the field so that could ends
with just bloating the documentation.

Is it work doing?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Is it worth accepting multiple CRLs?

Henry B Hotz <hbhotz@oxy.edu> — 2020-07-31T12:53:53Z

A CA may issue a CRL infrequently, but issue a delta-CRL frequently. Does the logic support this properly?

Personal email. hbhotz@oxy.edu

> On Jul 31, 2020, at 1:39 AM, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> 
> Hello.
> 
> PostgreSQL server accepts only one CRL file. It is easy to expand
> be_tls_init to accept a directory set in ssl_crl_file. But I'm not
> sure CRL is actually even utilized in the field so that could ends
> with just bloating the documentation.
> 
> Is it work doing?
> 
> regards.
> 
> -- 
> Kyotaro Horiguchi
> NTT Open Source Software Center
> 
>

Re: Is it worth accepting multiple CRLs?

Stephen Frost <sfrost@snowman.net> — 2020-07-31T13:00:14Z

Greetings,

* Kyotaro Horiguchi (horikyota.ntt@gmail.com) wrote:
> PostgreSQL server accepts only one CRL file. It is easy to expand
> be_tls_init to accept a directory set in ssl_crl_file. But I'm not
> sure CRL is actually even utilized in the field so that could ends
> with just bloating the documentation.
> 
> Is it work doing?

Yes, CRLs are absolutely used in the field and having this would be
nice.

Thanks,

Stephen

Re: Is it worth accepting multiple CRLs?

Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-08-03T07:19:37Z

At Fri, 31 Jul 2020 05:53:53 -0700, Henry B Hotz <hbhotz@oxy.edu> wrote in 
> A CA may issue a CRL infrequently, but issue a delta-CRL frequently. Does the logic support this properly?

If you are talking about regsitering new revokations while server is
running, it checks newer CRLs upon each lookup according to the
documentation [1], so a new Delta-CRL can be added after server
start. If server restart is allowed, the CRL file specified by
ssl_crl_file can contain multiple CRLs by just concatenation.

[1]: https://www.openssl.org/docs/man1.1.1/man3/X509_LOOKUP_hash_dir.html

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Is it worth accepting multiple CRLs?

Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-08-03T07:20:40Z

At Fri, 31 Jul 2020 09:00:14 -0400, Stephen Frost <sfrost@snowman.net> wrote in 
> Greetings,
> 
> * Kyotaro Horiguchi (horikyota.ntt@gmail.com) wrote:
> > PostgreSQL server accepts only one CRL file. It is easy to expand
> > be_tls_init to accept a directory set in ssl_crl_file. But I'm not
> > sure CRL is actually even utilized in the field so that could ends
> > with just bloating the documentation.
> > 
> > Is it work doing?
> 
> Yes, CRLs are absolutely used in the field and having this would be
> nice.

Thanks for the opinion. I'll continue working on this.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Is it worth accepting multiple CRLs?

Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-08-03T09:17:56Z

Uggg.

At Mon, 03 Aug 2020 16:19:37 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> At Fri, 31 Jul 2020 05:53:53 -0700, Henry B Hotz <hbhotz@oxy.edu> wrote in 
> > A CA may issue a CRL infrequently, but issue a delta-CRL frequently. Does the logic support this properly?
> 
> If you are talking about regsitering new revokations while server is
> running, it checks newer CRLs upon each lookup according to the
> documentation [1], so a new Delta-CRL can be added after server
> start. If server restart is allowed, the CRL file specified by

I didin't know that ssl files are reloaded by SIGHUP (pg_ctl
reload). So the ssl_crl_file is also reloaded on server reload.

> ssl_crl_file can contain multiple CRLs by just concatenation.
> 
> [1]: https://www.openssl.org/docs/man1.1.1/man3/X509_LOOKUP_hash_dir.html

Still on-demand loading is the advantage of the hashed directory
method.  I'll continue working..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Is it worth accepting multiple CRLs?

Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-08-04T08:37:08Z

At Mon, 03 Aug 2020 16:20:40 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> Thanks for the opinion. I'll continue working on this.

This is it, but.. 

Looking closer I realized that certificates are verified in each
backend so CRL cache doesn't work at all for the hashed directory
method. Therefore, all CRL files relevant to a certificate to be
verfied are loaded every time a backend starts.

The only advantage of this is avoiding irrelevant CRLs from being
loaded in exchange of loading relevant CRLs at every session
start. Session startup gets slower by many delta CRLs from the same
CA.

Seems far from promising.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Is it worth accepting multiple CRLs?

Stephen Frost <sfrost@snowman.net> — 2020-08-15T17:18:22Z

Greetings,

* Kyotaro Horiguchi (horikyota.ntt@gmail.com) wrote:
> At Mon, 03 Aug 2020 16:20:40 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> > Thanks for the opinion. I'll continue working on this.
> 
> This is it, but.. 

Thanks!

> Looking closer I realized that certificates are verified in each
> backend so CRL cache doesn't work at all for the hashed directory
> method. Therefore, all CRL files relevant to a certificate to be
> verfied are loaded every time a backend starts.
> 
> The only advantage of this is avoiding irrelevant CRLs from being
> loaded in exchange of loading relevant CRLs at every session
> start. Session startup gets slower by many delta CRLs from the same
> CA.
> 
> Seems far from promising.

I agree that it's not ideal, but I don't know that this is a reason to
not move forward with this feature..?

We could certainly have a later patch which improves this in some way
(though exactly how isn't clear...  if we move the CRL loading into
postmaster then we'd have to load *all* of them, and then we'd still
need to check if they've changed since we loaded them, and presumably
have some way to signal the postmaster to update its set from time to
time..), but that can be a future effort.

I took a quick look through the patch and it seemed pretty straight
forward to me and a good improvement.

Would love to hear other thoughts.  I hope you'll submit this for the
September CF and ping me when you do and I'll see if I can get it
committed.

Thanks!

Stephen

Re: Is it worth accepting multiple CRLs?

Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-08-18T07:43:47Z

Hello.

At Sat, 15 Aug 2020 13:18:22 -0400, Stephen Frost <sfrost@snowman.net> wrote in 
> > Looking closer I realized that certificates are verified in each
> > backend so CRL cache doesn't work at all for the hashed directory
> > method. Therefore, all CRL files relevant to a certificate to be
> > verfied are loaded every time a backend starts.
> > 
> > The only advantage of this is avoiding irrelevant CRLs from being
> > loaded in exchange of loading relevant CRLs at every session
> > start. Session startup gets slower by many delta CRLs from the same
> > CA.
> > 
> > Seems far from promising.
> 
> I agree that it's not ideal, but I don't know that this is a reason to
> not move forward with this feature..?

Since one of the significant advantage of the directory method is
differential loading of new CRLs. But actually it has other advanges
like easier file handling and not needing server reload.

> We could certainly have a later patch which improves this in some way
> (though exactly how isn't clear...  if we move the CRL loading into
> postmaster then we'd have to load *all* of them, and then we'd still
> need to check if they've changed since we loaded them, and presumably
> have some way to signal the postmaster to update its set from time to
> time..), but that can be a future effort.
> 
> I took a quick look through the patch and it seemed pretty straight
> forward to me and a good improvement.
> 
> Would love to hear other thoughts.  I hope you'll submit this for the
> September CF and ping me when you do and I'll see if I can get it
> committed.

Thank you very much. I'll do that after some polishing.

A near-by discussion about OpenSSL3.0 conflicts with this but it's
easy to follow.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Is it worth accepting multiple CRLs?

Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-08-31T09:03:02Z

At Tue, 18 Aug 2020 16:43:47 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> Thank you very much. I'll do that after some polishing.
> 
> A near-by discussion about OpenSSL3.0 conflicts with this but it's
> easy to follow.

Rebased. Fixed bogus tests and strange tentative API change of
SSLServer.pm.  Corrected a (maybe) spelling mistake.  I'm going to
register this to the coming CF.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Is it worth accepting multiple CRLs?

Michael Paquier <michael@paquier.xyz> — 2020-09-17T03:06:03Z

On Mon, Aug 31, 2020 at 06:03:02PM +0900, Kyotaro Horiguchi wrote:
> Rebased. Fixed bogus tests and strange tentative API change of
> SSLServer.pm.  Corrected a (maybe) spelling mistake.  I'm going to
> register this to the coming CF.

Stephen, are you planning to look at that?  I know that you are not
registered as a reviewer, but you mentioned upthread that you may be
able to look at it.

The changes in libpq's backend/frontend are rather simple, but the
docs and the changes in the TAP tests require a careful lookup. 
--
Michael

Re: Is it worth accepting multiple CRLs?

Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-01-15T07:56:27Z

On 2020-08-31 11:03, Kyotaro Horiguchi wrote:
> At Tue, 18 Aug 2020 16:43:47 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
>> Thank you very much. I'll do that after some polishing.
>>
>> A near-by discussion about OpenSSL3.0 conflicts with this but it's
>> easy to follow.
> 
> Rebased. Fixed bogus tests and strange tentative API change of
> SSLServer.pm.  Corrected a (maybe) spelling mistake.  I'm going to
> register this to the coming CF.

Other systems that offer both a CRL file and a CRL directory usually 
specify those using two separate configuration settings.  Examples:

https://dev.mysql.com/doc/refman/5.7/en/server-system-variables.html#sysvar_ssl_crlpath
https://httpd.apache.org/docs/current/mod/mod_ssl.html#sslcarevocationpath

These are then presumably both passed to X509_STORE_load_locations(), 
which supports specifying a file and directory concurrently.

I think that would be a preferable approach.  In practical terms, it 
would allow a user to introduce the directory method gradually without 
having to convert the existing CRL file at the same time.

Re: Is it worth accepting multiple CRLs?

Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-01-19T00:17:34Z

At Fri, 15 Jan 2021 08:56:27 +0100, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote in 
> On 2020-08-31 11:03, Kyotaro Horiguchi wrote:
> > At Tue, 18 Aug 2020 16:43:47 +0900 (JST), Kyotaro Horiguchi
> > <horikyota.ntt@gmail.com> wrote in
> >> Thank you very much. I'll do that after some polishing.
> >>
> >> A near-by discussion about OpenSSL3.0 conflicts with this but it's
> >> easy to follow.
> > Rebased. Fixed bogus tests and strange tentative API change of
> > SSLServer.pm.  Corrected a (maybe) spelling mistake.  I'm going to
> > register this to the coming CF.
> 
> Other systems that offer both a CRL file and a CRL directory usually
> specify those using two separate configuration settings.  Examples:
> 
> https://dev.mysql.com/doc/refman/5.7/en/server-system-variables.html#sysvar_ssl_crlpath
> https://httpd.apache.org/docs/current/mod/mod_ssl.html#sslcarevocationpath
> 
> These are then presumably both passed to X509_STORE_load_locations(),
> which supports specifying a file and directory concurrently.
> 
> I think that would be a preferable approach.  In practical terms, it
> would allow a user to introduce the directory method gradually without
> having to convert the existing CRL file at the same time.

Thank you for the information. The only reason for sharing the same
variable for both file and directory is to avoid additional variable
only for this reason. I'll post a new version where new GUC
ssl_crl_path is added.

By the way we can do the same thing on CA file/dir, but I personally
think that the benefit from the specify-by-directory for CA files is
far less than CRL files. So I'm not going to do this for CA files for
now.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Is it worth accepting multiple CRLs?

Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-01-19T08:01:35Z

On 2021-01-19 01:17, Kyotaro Horiguchi wrote:
> Thank you for the information. The only reason for sharing the same
> variable for both file and directory is to avoid additional variable
> only for this reason. I'll post a new version where new GUC
> ssl_crl_path is added.

Okay, I look forward to that patch.

> By the way we can do the same thing on CA file/dir, but I personally
> think that the benefit from the specify-by-directory for CA files is
> far less than CRL files. So I'm not going to do this for CA files for
> now.

Yeah, that seems not so commonly used.

Re: Is it worth accepting multiple CRLs?

Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-01-19T08:32:00Z

At Tue, 19 Jan 2021 09:17:34 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> By the way we can do the same thing on CA file/dir, but I personally
> think that the benefit from the specify-by-directory for CA files is
> far less than CRL files. So I'm not going to do this for CA files for
> now.

This is it. A new guc ssl_crl_dir and connection option crldir are
added.

One problem raised upthread is the footprint for test is quite large
because all certificate and key files are replaced by this patch. I
think we can shrink the footprint by generating that files on-demand
but that needs openssl frontend to be installed on the development
environment.

If we agree that requirement, I'm going to go that direction.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Is it worth accepting multiple CRLs?

Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-01-30T21:20:19Z

On 2021-01-19 09:32, Kyotaro Horiguchi wrote:
> At Tue, 19 Jan 2021 09:17:34 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
>> By the way we can do the same thing on CA file/dir, but I personally
>> think that the benefit from the specify-by-directory for CA files is
>> far less than CRL files. So I'm not going to do this for CA files for
>> now.
> 
> This is it. A new guc ssl_crl_dir and connection option crldir are
> added.

This looks pretty good to me overall.

You need to update the expected result of the postgres_fdw test.

Also check your patch for whitespace errors with git diff --check or 
similar.

> One problem raised upthread is the footprint for test is quite large
> because all certificate and key files are replaced by this patch. I
> think we can shrink the footprint by generating that files on-demand
> but that needs openssl frontend to be installed on the development
> environment.

I don't understand why you need to recreate all these files.  All your 
patch should contain are the new *.r0 files that are computed from the 
existing *.crl files.  Nothing else should change, AIUI.

Some of the makefile rules for generating the CRL files need some 
refinement.  In

+ssl/root+server-crldir: ssl/server.crl
+   mkdir ssl/root+server-crldir
+   cp ssl/server.crl ssl/root+server-crldir/`openssl crl -hash -noout 
-in ssl/server.crl`.r0
+   cp ssl/root.crl ssl/root+server-crldir/`openssl crl -hash -noout -in 
ssl/root.crl`.r0
+ssl/root+client-crldir: ssl/client.crl
+   mkdir ssl/root+client-crldir
+   cp ssl/client.crl ssl/root+client-crldir/`openssl crl -hash -noout 
-in ssl/client.crl`.r0
+   cp ssl/root.crl ssl/root+client-crldir/`openssl crl -hash -noout -in 
ssl/root.crl`.r0

the rules should also have a dependency on ssl/root.crl in addition to 
ssl/server.crl.

By the way:

-   print $sslconf "ssl_crl_file='root+client.crl'\n";
+   print $sslconf "ssl_crl_file='$crlfile'\n" if (defined $crlfile);
+   print $sslconf "ssl_crl_dir='$crldir'\n" if (defined $crldir);

Trailing "if" doesn't need parentheses.

Re: Is it worth accepting multiple CRLs?

Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-02-01T02:42:32Z

At Sat, 30 Jan 2021 22:20:19 +0100, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote in 
> On 2021-01-19 09:32, Kyotaro Horiguchi wrote:
> > At Tue, 19 Jan 2021 09:17:34 +0900 (JST), Kyotaro Horiguchi
> > <horikyota.ntt@gmail.com> wrote in
> >> By the way we can do the same thing on CA file/dir, but I personally
> >> think that the benefit from the specify-by-directory for CA files is
> >> far less than CRL files. So I'm not going to do this for CA files for
> >> now.
> > This is it. A new guc ssl_crl_dir and connection option crldir are
> > added.
> 
> This looks pretty good to me overall.

Thanks!

> You need to update the expected result of the postgres_fdw test.

Oops. Fixed.

> Also check your patch for whitespace errors with git diff --check or
> similar.

Sorry for forgetting that. I found an extra new line in
be-secure-openssl.c and remved it.

> > One problem raised upthread is the footprint for test is quite large
> > because all certificate and key files are replaced by this patch. I
> > think we can shrink the footprint by generating that files on-demand
> > but that needs openssl frontend to be installed on the development
> > environment.
> 
> I don't understand why you need to recreate all these files.  All your
> patch should contain are the new *.r0 files that are computed from the
> existing *.crl files.  Nothing else should change, AIUI.

Ah. If I ran make with this patch, it complains of
ssl/root_ca-certindex lacking and I ran "make clean" to avoid the
complaint.  Instead, I created the additional crl directories by
manually executing the recipes of the additional rules.

v3:  41 files changed, 496 insertions(+), 255 deletions(-)
v4:  21 files changed, 258 insertions(+), 18 deletions(-)

I checked that 001_ssltests.pl succedds both with the preexisting ssl/
files and with the files created by "make sslfiles" after "make
sslfiles-clean".

> Some of the makefile rules for generating the CRL files need some
> refinement.  In
> 
> +ssl/root+server-crldir: ssl/server.crl
> +   mkdir ssl/root+server-crldir
> + cp ssl/server.crl ssl/root+server-crldir/`openssl crl -hash -noout
> -in ssl/server.crl`.r0
> + cp ssl/root.crl ssl/root+server-crldir/`openssl crl -hash -noout -in
> ssl/root.crl`.r0
> +ssl/root+client-crldir: ssl/client.crl
> +   mkdir ssl/root+client-crldir
> + cp ssl/client.crl ssl/root+client-crldir/`openssl crl -hash -noout
> -in ssl/client.crl`.r0
> + cp ssl/root.crl ssl/root+client-crldir/`openssl crl -hash -noout -in
> ssl/root.crl`.r0
> 
> the rules should also have a dependency on ssl/root.crl in addition to
> ssl/server.crl.

Right. Added.

> By the way:
> 
> -   print $sslconf "ssl_crl_file='root+client.crl'\n";
> +   print $sslconf "ssl_crl_file='$crlfile'\n" if (defined $crlfile);
> +   print $sslconf "ssl_crl_dir='$crldir'\n" if (defined $crldir);
> 
> Trailing "if" doesn't need parentheses.

I know. However I preferred to have them at the time, I don't have a
strong opinion about how it should be. Ripped off them.


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Is it worth accepting multiple CRLs?

Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-02-17T04:05:26Z

The commit fe61df7f82 shot down this.

This patch allows a new GUC ssl_crl_dir and a new libpq connection
option sslcrldir to specify CRL directory, which stores multiple files
that contains one CRL. With that method server loads only CRLs for the
CA of the certificate being validated.

Along with rebasing, the documentation is slightly reworded.

revocation list (CRL).  Certificates listed in this file, if it
         exists, will be rejected while attempting to authenticate the
-        server's certificate.  If both sslcrl and sslcrldir are not set,
-        this setting is assumed to be
+        server's certificate.  If neither sslcrl sslcrldir is set, this
+        setting is assumed to be
         <filename>~/.postgresql/root.crl</filename>. See

And added a line for the new variable in postgresql.conf.sample.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Is it worth accepting multiple CRLs?

Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-02-18T07:24:23Z

On 2021-02-17 05:05, Kyotaro Horiguchi wrote:
> The commit fe61df7f82 shot down this.
> 
> This patch allows a new GUC ssl_crl_dir and a new libpq connection
> option sslcrldir to specify CRL directory, which stores multiple files
> that contains one CRL. With that method server loads only CRLs for the
> CA of the certificate being validated.
> 
> Along with rebasing, the documentation is slightly reworded.

Committed this.

I changed the documentation a bit.  Instead of having a separate section 
describing the CRL options, I put that information directly into the 
libpq and GUC sections.  Some of the information, such as that the 
directory files are loaded on demand, isn't so obviously useful in the 
libpq case, so I found that a bit confusing.  Also, I got the impression 
that the hashed directory format is sort of internal to OpenSSL, and 
there are several versions of that format, so I didn't want to copy over 
the description of these internals.  Instead, I referred to the openssl 
rehash/c_rehash commands for information.  If we get support for 
non-OpenSSL providers, we'll probably have to revisit this.

Re: Is it worth accepting multiple CRLs?

Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-02-18T08:06:25Z

Thanks for committing this!

At Thu, 18 Feb 2021 08:24:23 +0100, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote in 
> On 2021-02-17 05:05, Kyotaro Horiguchi wrote:
> > The commit fe61df7f82 shot down this.
> > This patch allows a new GUC ssl_crl_dir and a new libpq connection
> > option sslcrldir to specify CRL directory, which stores multiple files
> > that contains one CRL. With that method server loads only CRLs for the
> > CA of the certificate being validated.
> > Along with rebasing, the documentation is slightly reworded.
> 
> Committed this.
> 
> I changed the documentation a bit.  Instead of having a separate
> section describing the CRL options, I put that information directly
> into the libpq and GUC sections.  Some of the information, such as
> that the directory files are loaded on demand, isn't so obviously
> useful in the libpq case, so I found that a bit confusing.  Also, I

Agreed.

> got the impression that the hashed directory format is sort of
> internal to OpenSSL, and there are several versions of that format, so
> I didn't want to copy over the description of these internals.
> Instead, I referred to the openssl rehash/c_rehash commands for
> information.  If we get support for non-OpenSSL providers, we'll
> probably have to revisit this.

Thanks.  I'm fine with that, either.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center