Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert

Thanks for clarifying your reasoning. I now agree that ssrootcert=system
is now the best option.

> > 2. Should we allow the same approach with ssl_ca_file on the server side, for client cert validation?
>
> I don't know enough about this use case to implement it safely. We'd
> have to make sure the HBA entry is checking the hostname (so that we
> do the reverse DNS dance), and I guess we'd need to introduce a new
> clientcert verify-* mode? Also, it seems like server operators are
> more likely to know exactly which roots they need, at least compared
> to clients. I agree the feature is useful, but I'm not excited about
> attaching it to this patchset.

The main thing would be to have ssl_ca_file=system check against
the certs from the system CA store. And probably we would want
to disallow clientcert=verify-ca when ssl_ca_file is set to system.
Other than that I don't think anything is necessary. I definitely agree
that this patch should not be blocked on this. But it seems simple
enough to implement and imho it would be a bit confusing if ssl_ca_file
does not support the "system" value, but sslrootcert does.

I also took a closer look at the code, and the only comment I have is:

> appendPQExpBuffer(&conn->errorMessage,

These calls can all be replaced by the recently added libpq_append_conn_error

Finally I tested this against a Postgres server I created on Azure and
the new value works as expected. The only thing that I think would be
good to change is the error message when sslmode=verify-full, and
sslrootcert is not provided, but ~/.postgresql/root.crt is also not available.
I think it would be good for the error to mention sslrootcert=system

> psql: error: connection to server at "xxx.postgres.database.azure.com" (123.456.789.123), port 5432 failed: root certificate file "/home/jelte/.postgresql/root.crt" does not exist
> Either provide the file or change sslmode to disable server certificate verification.

Changing that last line to something like (maybe removing the part
about disabling server certificate verification):
> Either provide the file using the sslrootcert parameter, or use sslrootert=system to use the OS certificate store, or change sslmode to disable server certificate verification.

On Fri, 6 Jan 2023 at 19:20, Jacob Champion <jchampion(at)timescale(dot)com> wrote:
>
> On Fri, Jan 6, 2023 at 2:18 AM Jelte Fennema <postgres(at)jeltef(dot)nl> wrote:
> >
> > Huge +1 from me. On Azure we're already using public CAs to sign certificates for our managed postgres offerings[1][2]. Right now, our customers have to go to the hassle of downloading a specific root cert or finding their OS default location. Neither of these allow us to give users a simple copy-pastable connection string that uses secure settings. This would change this and make it much easier for our customers to use secure connections to their database.
>
> Thanks! Timescale Cloud is in the same boat.
>
> > I have two main questions:
> > 1. From the rest of the thread it's not entirely clear to me why this patch goes for the sslrootcert=system approach, instead of changing what sslrootcert='' means when using verify-full. Like Tom Lane suggested, we could change it to try ~/.postgresql/root.crt and if that doesn't exist make it try the system store, instead of erroring out like it does now when ~/.postgresql/root.crt doesn't exist.
>
> I mentioned it briefly upthread, but to expand: For something this
> critical to security, I don't like solutions that don't say exactly
> what they do. What does the following connection string mean?
>
> $ psql 'host=example.org sslmode=verify-full'
>
> If it sometimes means to use root.crt (if one exists) and sometimes to
> use the system store, then
> 1) it's hard to audit the actual behavior without knowing the state of
> the filesystem, and
> 2) if I want to connect to a server using the system store, and *only*
> the system store, then I have to make sure that the default root.crt
> never exists. But what if other software on my system relies on it?
>
> It also provides a bigger target for exploit chains, because I can
> remove somebody's root.crt file and their connections may try to
> continue with an effectively weaker verification level instead of
> erroring out. I realize that for many people this is a nonissue ("if
> you can delete the root cert, you can probably do much worse") but IME
> arbitrary file deletion vulnerabilities are treated with less concern
> than arbitrary file writes.
>
> By contrast,
>
> $ psql 'host=example.org sslrootcert=system sslmode=verify-full'
>
> has a clear meaning. We'll never use a root.crt.
>
> (A downside of reusing sslrootcert like this is the cross-version
> hazard. The connection string 'host=example.org sslrootcert=system'
> means something strong with this patchset, but something very weak to
> libpq 15 and prior. So clients should probably continue to specify
> sslmode=verify-full explicitly for the foreseeable future.)
>
> > This approach seems nicer to me, as it doesn't require introducing another special keyword.
>
> Maybe I'm being overly aspirational, but one upside to that special
> keyword is that it's a clear signal that the user wants to use the
> public CA model. So we have the opportunity to remove footguns
> aggressively when we see this mode. In the future we may have further
> opportunities to strengthen sslrootcert=system (OCSP and/or
> must-staple support?) that would be harder to roll out by default if
> we're just trying to guess what the user wants.
>
> > It would also remove the need for the changing of defaults depending on the value of sslrootcert.
>
> Agreed. Personally I think the benefit of 0002 outweighs its cost, but
> maybe there's a better way to implement it.
>
> > 2. Should we allow the same approach with ssl_ca_file on the server side, for client cert validation?
>
> I don't know enough about this use case to implement it safely. We'd
> have to make sure the HBA entry is checking the hostname (so that we
> do the reverse DNS dance), and I guess we'd need to introduce a new
> clientcert verify-* mode? Also, it seems like server operators are
> more likely to know exactly which roots they need, at least compared
> to clients. I agree the feature is useful, but I'm not excited about
> attaching it to this patchset.
>
> --Jacob