Improvement of the TLS protocol

[WagnerPMC]

Reason for this PR

Issue #598 PR #605 (@levkk)

Description of Changes

1. Optional verify-ca mode to configure general.verify_server_certificate. It is equivalent to the sslmode=verify-ca mode
2. Rejection of embedded webpki certificates. OS root certificates are used instead.
3. Update root certificates on demand (SIGHUP or RELOAD)
4. By using OS certificates, the binary file size is reduced by about 750KB. (under question)

License Acceptance

By submitting this pull request, I confirm that my contribution is made under the terms of the PGCat license.

[WagnerPMC]

@levkk, Why is CI crashing on std::sync::OnceLock? This feature was stabilized in Rust 1.70.

[WagnerPMC]

@levkk , I think I found the cause. The Dockerfile.ci file uses Rust 1.67, so the compilation fails. Is there any reason why CI uses Rust 1.67? I could rewrite under once_cell::sync::OnceCell, but I don't see any reason to do so yet.

P.S. Think about it, it's already been 5 minor versions since rust 1.67 was released

[levkk]

We use once_cell elsewhere in the code, so it's not a bad idea to use it here too. We can move to OnceLock at some point later, for every usage of once_cell.

[WagnerPMC]

We use once_cell elsewhere in the code, so it's not a bad idea to use it here too. We can move to OnceLock at some point later, for every usage of once_cell.

All right, I'll modify it to fit the crate once_cell

[WagnerPMC]

We use once_cell elsewhere in the code, so it's not a bad idea to use it here too. We can move to OnceLock at some point later, for every usage of once_cell.

What do you think about my PR in general?

[levkk]

I have a couple thoughts, I'll review this PR today/tomorrow. It's the right direction through, so we're very close.

[WagnerPMC]

I have a couple thoughts, I'll review this PR today/tomorrow. It's the right direction through, so we're very close.

Okay, sir

I'm thinking of removing the re-initialization of root_store at this point: https://github.com/WagnerPMC/pgcat/blob/5519a4f70c2917c4491e00e50109d76ded5abf80/src/server.rs#L402

So far, I have found a solution that will bring performance improvement and auto-reload the system certificate configuration. But I'm not going to implement everything in one pull request. I can accept this one to start with, then I can create a second one for the next opportunity I wrote about above.

So I declare the PR to be final.

[WagnerPMC]

@levkk ,

Please review it now. I've also updated the PR header. A nice bonus: by using OS certificates, the binary file size is reduced by about 750KB.

For some reason CI crashes with an error:

error: package `clap_lex v0.5.1` cannot be built because it requires rustc 1.70.0 or newer, while the currently active rustc version is 1.67.1

@levkk, please check the dependencies

[WagnerPMC]

@levkk, I did everything you mentioned.

Currently, loading root certificates on PGCat startup will not occur unless the verify_server_certificate setting is configured. Root certificates will only be loaded in the following cases:

1. SIGHUP signal is given to the process control
2. The RELOAD command in the PGCat console was executed.
3. If server certificate validation was disabled before the configuration was automatically reload and enabled after.

I've removed the extra cloning, but we still have cloning happening in a single location: https://github.com/WagnerPMC/pgcat/blob/fc8998f0d16aaf53ec8a64685c10dd55a3b654b2/src/server.rs#L403

And unfortunately, there's no getting away from it. This is because rustls must borrow the RootCertStore. So we have no power to influence it here.

In support of this PR, I will say that constant cloning (in implicit form) has happened before: https://github.com/postgresml/pgcat/blob/a054b454d2566e1a90ef373c7a5360850f12484e/src/server.rs#L399-L412

This concludes my commentary. I have changed the PR header. @levkk, review it now 🙏

UPD. So far, this PR is not ready. I noticed that I did a certificate reload after restarting the pools, but it needs to be the other way around. I need some time to fix this.

[WagnerPMC]

I think PR is ready. @levkk , review it please

[levkk]

We need to figure out why this doesn't build in CI and then we can probably merge this as is. Without CI, we haven't tested this PR yet.

It looks like Cargo.lock updated several of the dependencies. Have you ran cargo update by any chance when you added the the new dependency? Ideally, you would just add it to Cargo.toml and run cargo build ensuring that no existing dependencies are updated.

[WagnerPMC]

We need to figure out why this doesn't build in CI and then we can probably merge this as is. Without CI, we haven't tested this PR yet.

It looks like Cargo.lock updated several of the dependencies. Have you ran cargo update by any chance when you added the the new dependency? Ideally, you would just add it to Cargo.toml and run cargo build ensuring that no existing dependencies are updated.

Yes, you're right. My mistake was that I did a cargo update. I reverted back to the previous versions of the dependencies, now CI runs successfully.

[WagnerPMC]

@levkk , Can you tell me why your Pull Request contains falling code?