-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add unix domain socket support for Postgres #253
Changes from all commits
10a9aa8
aceae4b
b775cbe
b0ca149
2e5b576
4f0410b
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 |
---|---|---|
|
@@ -110,9 +110,13 @@ pub struct PgConnection { | |
|
||
// https://www.postgresql.org/docs/12/protocol-flow.html#id-1.10.5.7.3 | ||
async fn startup(stream: &mut PgStream, url: &Url) -> crate::Result<BackendKeyData> { | ||
// Defaults to postgres@.../postgres | ||
let username = url.username().unwrap_or(Cow::Borrowed("postgres")); | ||
let database = url.database().unwrap_or("postgres"); | ||
// Defaults to $USER@.../$USER | ||
// and falls back to postgres@.../postgres | ||
let username = url | ||
.username() | ||
.or_else(|| std::env::var("USER").map(Cow::Owned).ok()) | ||
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. Perhaps https://crates.io/crates/whoami is a better idea to be more resilient? |
||
.unwrap_or(Cow::Borrowed("postgres")); | ||
let database = url.database().unwrap_or(&username); | ||
|
||
// See this doc for more runtime parameters | ||
// https://www.postgresql.org/docs/12/runtime-config-client.html | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,7 +23,27 @@ pub struct PgStream { | |
|
||
impl PgStream { | ||
pub(super) async fn new(url: &Url) -> crate::Result<Self> { | ||
let stream = MaybeTlsStream::connect(&url, 5432).await?; | ||
let host = url.host(); | ||
let port = url.port(5432); | ||
#[cfg(unix)] | ||
let stream = { | ||
let host = host | ||
.map(|host| { | ||
percent_encoding::percent_decode_str(host) | ||
.decode_utf8() | ||
.expect("percent-encoded hostname contained non-UTF-8 bytes") | ||
}) | ||
.or_else(|| url.param("host")) | ||
.unwrap_or("/var/run/postgresql".into()); | ||
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. By default we may want to iterate potential locations for the socket and not just hard-code this one.
|
||
if host.starts_with("/") { | ||
let path = format!("{}/.s.PGSQL.{}", host, port); | ||
MaybeTlsStream::connect_uds(&path).await? | ||
} else { | ||
MaybeTlsStream::connect(&host, port).await? | ||
} | ||
}; | ||
#[cfg(not(unix))] | ||
let stream = MaybeTlsStream::connect(host.unwrap_or("localhost"), port).await?; | ||
|
||
Ok(Self { | ||
notifications: None, | ||
|
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 don't like how this in
MaybeTlsStream
. I think it conflates what the type here is doing.I'd rather something like:
A separate
Socket
(name?) type that is internally an enumeration betweenUnixStream
andTcpStream
.This
MaybeTlsStream
is then generalized around anS
type parameter.Thoughts?
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 don't see much reason to parameterize
MaybeTlsStream
if the type is always going to beSocket
.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.
We could call it
MaybeUdsStream
and reverse the relationship so thatMaybeUdsStream
is an enumeration betweenUnixStream
andMaybeTlsStream
.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 guess before we get ahead of ourselves here.. is TLS over UDS even a reasonable thing? If it is, we probably need to at least support that with the type sandwhich.
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.
The wording in the documentation doesn't suggest TLS support at all over the domain socket: https://www.postgresql.org/docs/12/ssl-tcp.html
It doesn't really make sense anyways to encrypt over the UDS because if someone can sniff the traffic on that socket then they can probably also just inspect your process' memory or Postgres' memory/files for juicy secrets (since they'd have to be running on the same machine with elevated privileges already).