-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix postgresql hostaddr semantics #4102
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ mod ssl_mode; | |
| #[derive(Debug, Clone)] | ||
| pub struct PgConnectOptions { | ||
| pub(crate) host: String, | ||
| pub(crate) host_addr: Option<String>, | ||
| pub(crate) port: u16, | ||
| pub(crate) socket: Option<PathBuf>, | ||
| pub(crate) username: String, | ||
|
|
@@ -59,10 +60,9 @@ impl PgConnectOptions { | |
| .and_then(|v| v.parse().ok()) | ||
| .unwrap_or(5432); | ||
|
|
||
| let host = var("PGHOSTADDR") | ||
| .ok() | ||
| .or_else(|| var("PGHOST").ok()) | ||
| .unwrap_or_else(|| default_host(port)); | ||
| let host = var("PGHOST").ok().unwrap_or_else(|| default_host(port)); | ||
|
|
||
| let host_addr = var("PGHOSTADDR").ok(); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now not all strings are valid here, only numeric IPs are. But we are not doing any validation on the strings at all for all the sibling fields, so I did not start to do that here also. In fact there is not even an error path in this API. That I do not agree to the defaulting to ENVs in the first place was discussed in previous PRs, for the moment I consider the bugfix more important than improving tghe API so I just keep it "as wrong in my eyes as the siblings" while fixing host != hostaddr. |
||
|
|
||
| let username = var("PGUSER").ok().unwrap_or_else(whoami::username); | ||
|
|
||
|
|
@@ -71,6 +71,7 @@ impl PgConnectOptions { | |
| PgConnectOptions { | ||
| port, | ||
| host, | ||
| host_addr, | ||
| socket: None, | ||
| username, | ||
| password: var("PGPASSWORD").ok(), | ||
|
|
@@ -127,6 +128,33 @@ impl PgConnectOptions { | |
| self | ||
| } | ||
|
|
||
| /// Sets the host address to connect to. | ||
| /// | ||
| /// This is different to the host parameter as it overwrites DNS lookups for TCP/IP | ||
| /// communication. This is particuarly useful when the DB port has to be | ||
| /// proxied to localhost for security reasons. | ||
| /// | ||
| /// # Example | ||
| /// | ||
| /// ```rust | ||
| /// # use sqlx_postgres::PgConnectOptions; | ||
| /// let options = PgConnectOptions::new() | ||
| /// .host("example.com"); | ||
| /// | ||
| /// // Initially, host_addr should be None (unless PGHOSTADDR env var is set) | ||
| /// // For this test, we assume it's not set | ||
| /// assert_eq!(options.get_host_addr(), None); | ||
| /// | ||
| /// let options = options.host_addr("127.0.0.1"); | ||
| /// | ||
| /// // After setting, host_addr should contain the specified value | ||
| /// assert_eq!(options.get_host_addr(), Some("127.0.0.1")); | ||
| /// ``` | ||
| pub fn host_addr(mut self, host_addr: &str) -> Self { | ||
| self.host_addr = Some(host_addr.to_string()); | ||
| self | ||
| } | ||
|
|
||
| /// Sets the port to connect to at the server host. | ||
| /// | ||
| /// The default port for PostgreSQL is `5432`. | ||
|
|
@@ -477,6 +505,28 @@ impl PgConnectOptions { | |
| &self.host | ||
| } | ||
|
|
||
| /// Get the current host addr. | ||
| /// | ||
| /// # Example | ||
| /// | ||
| /// ```rust | ||
| /// # use sqlx_postgres::PgConnectOptions; | ||
| /// let options = PgConnectOptions::new() | ||
| /// .host("example.com"); | ||
| /// | ||
| /// // Initially, host_addr should be None (unless PGHOSTADDR env var is set) | ||
| /// // For this test, we assume it's not set | ||
| /// assert_eq!(options.get_host_addr(), None); | ||
| /// | ||
| /// let options = options.host_addr("127.0.0.1"); | ||
| /// | ||
| /// // After setting host_addr, it should return the configured value | ||
| /// assert_eq!(options.get_host_addr(), Some("127.0.0.1")); | ||
| /// ``` | ||
| pub fn get_host_addr(&self) -> Option<&str> { | ||
| self.host_addr.as_deref() | ||
| } | ||
|
|
||
| /// Get the server's port. | ||
| /// | ||
| /// # Example | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,5 +83,5 @@ This adds a required x509 v3 extension: | |
| Create a signed certificate using our CA key and the CSR: | ||
|
|
||
| ``` | ||
| openssl x509 -req -CA ca.crt -CAkey keys/ca.key -in server.csr -out server.crt -days 3650 -CAcreateserial | ||
| openssl x509 -req -CA ca.crt -CAkey keys/ca.key -in server.csr -out server.crt -days 3650 -CAcreateserial -copy_extensions copy | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OT: I consider it a terrible design descision to silently truncate extensions, instead they should have bailed "unacknowledged extension A, B, C present".
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. context: |
||
| ``` | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,12 @@ | ||
| -----BEGIN CERTIFICATE----- | ||
| MIIBnjCCAVCgAwIBAgIJALbHH0sRwKGPMAUGAytlcDBfMQswCQYDVQQGEwJ1czET | ||
| MBEGA1UECAwKY2FsaWZvcm5pYTEhMB8GA1UECgwYSW50ZXJuZXQgV2lkZ2l0cyBQ | ||
| dHkgTHRkMRgwFgYDVQQDDA9BdXN0aW4gQm9uYW5kZXIwHhcNMjUwNzAxMDUyMTU2 | ||
| WhcNMzUwNjI5MDUyMTU2WjBGMQswCQYDVQQGEwJ1czETMBEGA1UECAwKY2FsaWZv | ||
| cm5pYTEQMA4GA1UECgwHU1FMeC5yczEQMA4GA1UEAwwHc3FseC5yczAqMAUGAytl | ||
| cAMhAA33S2qsqpZssUcYrpleMXDj5/mhb56HPaO3CIIgY5c8o0IwQDAdBgNVHQ4E | ||
| FgQUPUpn95GHFuMe7+2pG5rbmJS55/wwHwYDVR0jBBgwFoAUCw2pVpGKz2xkIjbV | ||
| HYh0LnzdkW4wBQYDK2VwA0EAExEOza9IrSchoQs1NwPxfCdfXMHiXpsgMThDuig+ | ||
| 9hauW+b1KlBR3ZeW8AOIwazMhdstBFOhumaWPQ/wZNUkCg== | ||
| MIIBvTCCAW+gAwIBAgIULNqAltiOEP5feQrjIBXYA6YC0fUwBQYDK2VwMF8xCzAJ | ||
| BgNVBAYTAnVzMRMwEQYDVQQIDApjYWxpZm9ybmlhMSEwHwYDVQQKDBhJbnRlcm5l | ||
| dCBXaWRnaXRzIFB0eSBMdGQxGDAWBgNVBAMMD0F1c3RpbiBCb25hbmRlcjAeFw0y | ||
| NTExMTIyMTQ5NThaFw0zNTExMTAyMTQ5NThaMEYxCzAJBgNVBAYTAnVzMRMwEQYD | ||
| VQQIDApjYWxpZm9ybmlhMRAwDgYDVQQKDAdTUUx4LnJzMRAwDgYDVQQDDAdzcWx4 | ||
| LnJzMCowBQYDK2VwAyEADfdLaqyqlmyxRxiumV4xcOPn+aFvnoc9o7cIgiBjlzyj | ||
| VjBUMBIGA1UdEQQLMAmCB3NxbHgucnMwHQYDVR0OBBYEFD1KZ/eRhxbjHu/tqRua | ||
| 25iUuef8MB8GA1UdIwQYMBaAFAsNqVaRis9sZCI21R2IdC583ZFuMAUGAytlcANB | ||
| AKyosmZvuCIrWkvb4QN8k2Fwf09LICCNjh571XwNxp9eUEEwJOjl956o6SFxDlgK | ||
| Cr1llASvz5cPm6jUV2wlaQc= | ||
| -----END CERTIFICATE----- | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before: After: Note the subject alternative name is now present (was absent before)! |
||
Uh oh!
There was an error while loading. Please reload this page.
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've modified the integration tests to cover this behavior, they now:
sqlx.rsthat does not resolve to a postgresql instance on the public DNS.hostaddrto point it to127.0.0.1bypassing DNS lookupsverify-fullfromverify-ca.I use
sqlx.rsas hostname for this test as this is the hostname present in the test certificates.