Add option to make SSL connections to the database#67
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
34bcf93 to
127ba60
Compare
| bb8-postgres = "0.7" | ||
| bytes = "1.4.0" | ||
| tokio = { version = "1.38.0", default-features = false } | ||
| openssl = "0.10.75" |
There was a problem hiding this comment.
Can we add these with default-features = false to ensure we're not pulling in any unnecessary dependencies?
| bb8-postgres = "0.7" | ||
| bytes = "1.4.0" | ||
| tokio = { version = "1.38.0", default-features = false } | ||
| openssl = "0.10.75" |
There was a problem hiding this comment.
Rather than always using openssl, should we rather use native-tls to account for different TLS backends on different platforms? https://github.com/sfackler/rust-native-tls
| eprintln!("Connection error: {}", e); | ||
| } | ||
| }); | ||
| let client = if let Some(ca_file) = ca_file { |
There was a problem hiding this comment.
Do we really always require a certificate file for the service? Can't we 'usually' just use PKI?
There was a problem hiding this comment.
certainly thank you
| eprintln!("Connection error: {}", e); | ||
| } | ||
| }); | ||
| let client = if let Some(ca_file) = ca_file { |
There was a problem hiding this comment.
Seems like this could be DRYed up with the code above?
tnull
left a comment
There was a problem hiding this comment.
Mostly looks good I think, but the copied code makes it unnecessarily hard to review, IMO. Could we try to DRY it up a bit more?
As follow-ups we also might want to consider to enable certificate pinning and TOFU as best practices. Maybe worth opening issues for these?
|
|
||
| enum DbConnectionType { | ||
| Plain, | ||
| Tls(Option<String>), |
There was a problem hiding this comment.
I do wonder if it makes sense to read the file once rather than in different places, and have the Tls variant hold a override_cert: Option<Certificate> or similar?
|
|
||
| impl PostgresBackendImpl { | ||
| /// Constructs a [`PostgresBackendImpl`] using `dsn` for PostgreSQL connection information. | ||
| impl PostgresBackendImplTls { |
There was a problem hiding this comment.
Hmm, not the biggest fan of duplicating the init logic. Can we maybe move everything that's not related to TLS to shared helper methods?
Thanks for the nudge, I think things look better now take a look
Done |
tnull
left a comment
There was a problem hiding this comment.
Mostly looks good, feel free to squash the fixups.
One question
| ); | ||
| let store: Arc<dyn KvStore> = if let Some(tls_config) = postgresql_config.tls { | ||
| let additional_certificate = tls_config.ca_file.map(|file| { | ||
| let cert = std::fs::read(&file).unwrap(); |
There was a problem hiding this comment.
Should we print an error message and error out via std::process::exit(-1); rather than just unwraping here and elsewhere?
In particular, this might happen frequently if the user gives a wrong path or the file is not readable.
Also add the option to specify an additional root certificate for the client to trust.
|
Thanks squashed with the following diff: diff --git a/rust/server/src/main.rs b/rust/server/src/main.rs
index 2b36df6..38fdccd 100644
--- a/rust/server/src/main.rs
+++ b/rust/server/src/main.rs
@@ -73,14 +73,40 @@ fn main() {
let db_name = postgresql_config.database;
let store: Arc<dyn KvStore> = if let Some(tls_config) = postgresql_config.tls {
let additional_certificate = tls_config.ca_file.map(|file| {
- let cert = std::fs::read(&file).unwrap();
- Certificate::from_pem(&cert).unwrap()
+ let certificate = match std::fs::read(&file) {
+ Ok(cert) => cert,
+ Err(e) => {
+ println!("Failed to read certificate file: {}", e);
+ std::process::exit(-1);
+ },
+ };
+ match Certificate::from_pem(&certificate) {
+ Ok(cert) => cert,
+ Err(e) => {
+ println!("Failed to parse certificate file: {}", e);
+ std::process::exit(-1);
+ },
+ }
});
- Arc::new(
- PostgresTlsBackend::new(&endpoint, &db_name, additional_certificate).await.unwrap(),
- )
+ let postgres_tls_backend =
+ match PostgresTlsBackend::new(&endpoint, &db_name, additional_certificate).await {
+ Ok(backend) => backend,
+ Err(e) => {
+ println!("Failed to start postgres tls backend: {}", e);
+ std::process::exit(-1);
+ },
+ };
+ Arc::new(postgres_tls_backend)
} else {
- Arc::new(PostgresPlaintextBackend::new(&endpoint, &db_name).await.unwrap())
+ let postgres_plaintext_backend =
+ match PostgresPlaintextBackend::new(&endpoint, &db_name).await {
+ Ok(backend) => backend,
+ Err(e) => {
+ println!("Failed to start postgres plaintext backend: {}", e);
+ std::process::exit(-1);
+ },
+ };
+ Arc::new(postgres_plaintext_backend)
};
println!("Connected to PostgreSQL backend with DSN: {}/{}", endpoint, db_name);
let rest_svc_listener = |
Fixes #56