pgcat icon indicating copy to clipboard operation
pgcat copied to clipboard

[BUG/FEATURE REQUEST] Using the system store of trusted CAs

Open WagnerPMC opened this issue 1 year ago • 6 comments

Severity

High

Description

When using self-signed certificate authorities, pgcat does not allow the connection to be established:

Client disconnected with error ClientAuthPassthroughError("AuthPassthroughError(\"Error trying to obtain password from auth_query, ignoring hash for user '**********'. Error: SocketError(\\\"Server TLS error: Custom { kind: InvalidData, error: InvalidCertificate(UnknownIssuer) }\\\")\")", ClientIdentifier { application_name: "pgcat", username: "**********", pool_name: "********" })

It crashes with an UnknownIssuer error.

After analyzing the PGCat source code, I came to the conclusion that PGCat only uses shared certificate authorities from Mozilla: https://github.com/postgresml/pgcat/blob/037d232fcdaed0b7af745ba86115a8263addd743/src/server.rs#L399-L413

In this case, any self-signed certificates will be rejected even if the generated certificate authorities reside in the OS trusted authorities. I believe that this is primarily a bug, and secondarily I want to suggest that you trust not only Mozilla's list of CAs, but also the OS's list of CAs.

How this can be implemented is not for me to decide, but I can suggest using the crate rustls/rustls-native-certs. Please accept this idea the sooner the better.

Steps to reproduce

  1. Protect PostgreSQL server with self-signed SQL certificate and set mandatory use of ssl in pg_hba.conf.
  2. Configure pgcat and configure it to validate the server certificate (general.verify_server_certificate = true)
  3. Try to establish a connection with PGCat.

WagnerPMC avatar Sep 22 '23 00:09 WagnerPMC

Your proposal is sound, especially considering how easy it looks like it is to use rustls-native-certs. I'll take a look. In the meantime, you can add this to your config to bypass this error and continue to use server TLS:

[general]

verify_server_certificate = false

P.S. if you'd like to try to implement your proposal, I'd welcome a pull request. Thanks!

levkk avatar Sep 22 '23 01:09 levkk

Your proposal is sound, especially considering how easy it looks like it is to use rustls-native-certs. I'll take a look. In the meantime, you can add this to your config to bypass this error and continue to use server TLS:

[general]

verify_server_certificate = false

P.S. if you'd like to try to implement your proposal, I'd welcome a pull request. Thanks!

Okay man. I'll see what I can do.

WagnerPMC avatar Sep 23 '23 15:09 WagnerPMC

For now, I decided to implement an additional setting in the general.trust_os_certificates configuration file that unambiguously, in my opinion, says what it does. It will default to FALSE, but if someone wants to, they can override that in the config files. I also considered the names trust_os_ca_certificates and trust_os_ca, but they seem ambiguous or redundant to me.

In this way the "breaking change" is not affected. So we can safely drop this feature into the main branch and release it in the next maintenance version of the current library.

So if a developer wants to verify the PostgreSQL server certificate via self-signed certificate, they first enable the general.verify_server_certificate setting and then general.trust_os_certificates.

In the next minor version, I would recommend changing the default from FALSE to TRUE. Therefore, even if the PR is accepted, I advise against closing this Issue.

Ideally, in the next major version, we should do away with built-in root certificates altogether and use operating system root certificates in their place.

So far, I'm busy working on a pull request.

WagnerPMC avatar Sep 23 '23 18:09 WagnerPMC

Your proposal is sound, especially considering how easy it looks like it is to use rustls-native-certs. I'll take a look. In the meantime, you can add this to your config to bypass this error and continue to use server TLS:

[general]

verify_server_certificate = false

P.S. if you'd like to try to implement your proposal, I'd welcome a pull request. Thanks!

I realized the feature I wrote you about. But now I notice that your pooler does not know how to work with verify-ca mode. It works in prefer or verify-full mode, but verify-ca mode is just not there. I think this is another huge flaw, which I will try to solve and send you a PR.

I think I implement verify-ca mode can be done through the verify_server_certificate setting. And to avoid breaking the "breaking change" rule, I'll make this setting accept not only true and false, but also the text value "only-ca", which is the same as verify-ca, but will only verify the issuer of the certificate. So when it is FALSE this setting will mean prefer, when it is TRUE it will mean verify-full, and when it is "only-ca" it will mean verify-ca.

That is, fundamentally nothing will change, only verify-ca mode will be added.

WagnerPMC avatar Sep 25 '23 21:09 WagnerPMC

The nice side-effect of this work is that it seems to get rid of webpki-roots - having that one around is/was a blocker for packaging pgcat for Debian/Ubuntu as that package got removed and is unlikely to come back: https://tracker.debian.org/news/1190836/removed-0200-1-from-unstable/

mbanck avatar Oct 12 '23 14:10 mbanck

The nice side-effect of this work is that it seems to get rid of webpki-roots - having that one around is/was a blocker for packaging pgcat for Debian/Ubuntu as that package got removed and is unlikely to come back: https://tracker.debian.org/news/1190836/removed-0200-1-from-unstable/

Oh, my. My PR is now ready to merge into a master branch. At the moment @levkk is in the process of verifying it

WagnerPMC avatar Oct 12 '23 17:10 WagnerPMC