Skip to content

factor-outbound-pg: Support SslMode::Require #3148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

endocrimes
Copy link
Contributor

When specifying SslMode::Require, postgres clients shouldn't validate TLS certificates:

https://www.postgresql.org/docs/current/libpq-ssl.html

require: I want my data to be encrypted, and I accept the overhead. I trust that the network will make sure I always connect to the server I want.

It's not a great security mode, but is a really useful feature when you can't easily install a self-signed CA on your host or use a public certificate for the database.

When specifying SslMode::Require, postgres clients shouldn't validate
TLS certificates:

https://www.postgresql.org/docs/current/libpq-ssl.html

> require: I want my data to be encrypted, and I accept the overhead. I trust that the network will make sure I always connect to the server I want.

It's not a great security mode, but is a really useful feature when you
can't easily install a self-signed CA on your host or use a public
certificate for the database.

Signed-off-by: Danielle Lancashire <[email protected]>
@endocrimes endocrimes requested a review from lann June 2, 2025 14:06
@endocrimes
Copy link
Contributor Author

Manually tested that this change works as expected

@lann
Copy link
Collaborator

lann commented Jun 2, 2025

This is tricky. It looks like the underlying library doesn't support even parsing sslmode=verify-ca: https://github.com/sfackler/rust-postgres/blob/e1cd6beef3a1530642a2abaf3584d6bd8ed6cd45/tokio-postgres/src/config.rs#L586-L589

If someone wanted to verify CA before this PR their only option was required, so this would be a regression for them.

@endocrimes
Copy link
Contributor Author

Ugh (I don't have rust tooling setup rn so didn't see cases, guessed at the name and hoped it'd compile).

It also looks like they're at least slightly opposed to adding verify-ca/verify-full as explicit options (sfackler/rust-postgres#988) - so I'm not sure what our path forward is.

@lann
Copy link
Collaborator

lann commented Jun 3, 2025

Looking at this note in https://www.postgresql.org/docs/17/libpq-ssl.html#LIBQ-SSL-CERTIFICATES

For backwards compatibility with earlier versions of PostgreSQL, if a root CA file exists, the behavior of sslmode=require will be the same as that of verify-ca, meaning the server certificate is validated against the CA.

We can pretend that Spin always has a root CA file (that happens to contain webpki roots), which at least rationalizes the current behavior with sslmode=require.

To allow insecure TLS we could (ab)use sslmode=allow in this PR. That is still very clearly wrong in terms of the documented behavior, but is a more defensible breaking change in that a) it is already broken (it never tries non-SSL) and b) if someone has explicitly chosen allow they should have very low expectations for the security of the connection. We would want to document all of this loudly in both Spin docs and probably the spin:postgres WIT.

If none of the above feels acceptable then we're probably left with a rev'ing the spin:postgres interface to a new major version to introduce new behavior, and even then I'd be slightly nervous about making it too easy to upgrade without noticing this change. Regardless of whether we apply the above hack we should consider making insecure tls config more explicit in a future revision of that interface.

@lann
Copy link
Collaborator

lann commented Jun 3, 2025

As a complete alternative, we could push this configuration into runtime config with the existing undocumented [[client_tls]] config. We'd need to add some e.g. insecure_skip_verify = true option and actually integrate with the postgres factor but neither of those would be particularly difficult.

This would fit my mental model of how Spin "ought to" work, but there are downsides:

  • runtime config in general doesn't have much adoption, so this wouldn't be very discoverable even with proper docs
  • for other runtimes like SpinKube we might need to do additional work to expose this config

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants