-
Notifications
You must be signed in to change notification settings - Fork 19
Add option to make SSL connections to the database #67
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
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
34bcf93 to
127ba60
Compare
rust/impls/Cargo.toml
Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add these with default-features = false to ensure we're not pulling in any unnecessary dependencies?
rust/impls/Cargo.toml
Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
rust/impls/src/postgres_store.rs
Outdated
| eprintln!("Connection error: {}", e); | ||
| } | ||
| }); | ||
| let client = if let Some(ca_file) = ca_file { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
certainly thank you
rust/impls/src/postgres_store.rs
Outdated
| eprintln!("Connection error: {}", e); | ||
| } | ||
| }); | ||
| let client = if let Some(ca_file) = ca_file { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this could be DRYed up with the code above?
tnull
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
rust/impls/src/postgres_store.rs
Outdated
|
|
||
| enum DbConnectionType { | ||
| Plain, | ||
| Tls(Option<String>), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
rust/impls/src/postgres_store.rs
Outdated
|
|
||
| impl PostgresBackendImpl { | ||
| /// Constructs a [`PostgresBackendImpl`] using `dsn` for PostgreSQL connection information. | ||
| impl PostgresBackendImplTls { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good, feel free to squash the fixups.
One question
rust/server/src/main.rs
Outdated
| ); | ||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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