-
Notifications
You must be signed in to change notification settings - Fork 16
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 TLS support for Pgx_async #108
Changes from 11 commits
c62474c
c7829b2
fda4530
b93623e
8d8d750
1fafbea
5fc8f7e
3334685
d4f426a
993def6
dd4f573
31291c1
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 |
---|---|---|
|
@@ -288,6 +288,7 @@ module Message_out = struct | |
| Describe_portal of portal (* DP *) | ||
| Startup_message of startup | ||
| Simple_query of query | ||
| SSLRequest | ||
[@@deriving sexp] | ||
|
||
let add_byte buf i = | ||
|
@@ -381,6 +382,10 @@ module Message_out = struct | |
add_byte msg 0; | ||
None, Buffer.contents msg | ||
| Simple_query q -> Some 'Q', str q | ||
| SSLRequest -> | ||
let msg = Buffer.create 8 in | ||
add_int32 msg 80877103l; | ||
None, Buffer.contents msg | ||
;; | ||
end | ||
|
||
|
@@ -526,7 +531,59 @@ module Make (Thread : Io) = struct | |
|
||
(*----- Connection. -----*) | ||
|
||
let attempt_tls_upgrade ?(ssl = `Auto) ({ ichan; chan; _ } as conn) = | ||
(* To initiate an SSL-encrypted connection, the frontend initially sends an SSLRequest message rather than a | ||
StartupMessage. The server then responds with a single byte containing S or N, indicating that it is willing | ||
or unwilling to perform SSL, respectively. The frontend might close the connection at this point if it is | ||
dissatisfied with the response. To continue after S, perform an SSL startup handshake (not described here, | ||
part of the SSL specification) with the server. If this is successful, continue with sending the usual | ||
StartupMessage. In this case the StartupMessage and all subsequent data will be SSL-encrypted. To continue | ||
after N, send the usual StartupMessage and proceed without encryption. | ||
See https://www.postgresql.org/docs/9.3/protocol-flow.html#AEN100021 *) | ||
match ssl with | ||
| `No -> return conn | ||
| (`Auto | `Always _) as ssl -> | ||
(match Io.upgrade_ssl with | ||
| `Not_supported -> | ||
(match ssl with | ||
| `Always _ -> | ||
failwith | ||
"TLS support is not compiled into this Pgx library but ~ssl was set to \ | ||
`Always" | ||
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. I'd like to make this impossible but I'm not sure how. 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. #suggestion: We can also enforce that the io wrappers for pgx should always pick an ssl option (async_ssl, tls or lwt_ssl), and get rid of this branch. I'd expect that eventually all io wrappers in the pgx repo will support encrypted connections, but if we don't want to support them in a particular backend, we can probably tweak the wrapper itself to not expose any ssl related options in its mli file |
||
| _ -> ()); | ||
debug | ||
"TLS-support is not compiled into this Pgx library, not attempting to upgrade" | ||
>>| fun () -> conn | ||
| `Supported upgrade_ssl -> | ||
debug "Request SSL upgrade from server" | ||
>>= fun () -> | ||
let msg = Message_out.SSLRequest in | ||
send_message conn msg | ||
>>= fun () -> | ||
flush chan | ||
>>= fun () -> | ||
input_char ichan | ||
>>= (function | ||
| 'S' -> | ||
debug "Server supports TLS, attempting to upgrade" | ||
>>= fun () -> | ||
let ssl_config = | ||
match ssl with | ||
| `Auto -> None | ||
| `Always ssl_config -> Some ssl_config | ||
Comment on lines
+572
to
+573
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. I'm not completely sure on this interace. My idea was that if people actually care about SSL, they probably want to send a config. |
||
in | ||
upgrade_ssl ?ssl_config ichan chan | ||
>>= fun (ichan, chan) -> return { conn with ichan; chan } | ||
| 'N' -> debug "Server does not support TLS, not upgrading" >>| fun () -> conn | ||
| c -> | ||
fail_msg | ||
"Got unexpected response '%c' from server after SSLRequest message. Response \ | ||
should always be 'S' or 'N'." | ||
c)) | ||
;; | ||
|
||
let connect | ||
?ssl | ||
?host | ||
?port | ||
?user | ||
|
@@ -600,6 +657,8 @@ module Make (Thread : Io) = struct | |
; prepared_num = Int64.of_int 0 | ||
} | ||
in | ||
attempt_tls_upgrade ?ssl conn | ||
>>= fun conn -> | ||
(* Send the StartUpMessage. NB. At present we do not support SSL. *) | ||
let msg = Message_out.Startup_message { Message_out.user; database } in | ||
(* Loop around here until the database gives a ReadyForQuery message. *) | ||
|
@@ -665,6 +724,7 @@ module Make (Thread : Io) = struct | |
;; | ||
|
||
let with_conn | ||
?ssl | ||
?host | ||
?port | ||
?user | ||
|
@@ -676,6 +736,7 @@ module Make (Thread : Io) = struct | |
f | ||
= | ||
connect | ||
?ssl | ||
?host | ||
?port | ||
?user | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,23 +1,14 @@ | ||
(** Async based Postgres client based on Pgx. *) | ||
open Async_kernel | ||
|
||
include Pgx.S with type 'a Io.t = 'a Deferred.t | ||
include | ||
Pgx.S | ||
with type 'a Io.t = 'a Deferred.t | ||
and type Io.ssl_config = Conduit_async.Ssl.config | ||
|
||
(* for testing purposes *) | ||
module Thread : Pgx.Io with type 'a t = 'a Deferred.t | ||
|
||
val with_conn | ||
: ?host:string | ||
-> ?port:int | ||
-> ?user:string | ||
-> ?password:string | ||
-> ?database:string | ||
-> ?unix_domain_socket_dir:string | ||
-> ?verbose:int | ||
-> ?max_message_length:int | ||
-> (t -> 'a Deferred.t) | ||
-> 'a Deferred.t | ||
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. This is unnecessary since it's exactly the same interface that we import above in |
||
|
||
(** Like [execute] but returns a pipe so you can operate on the results before they have all returned. | ||
Note that [execute_iter] and [execute_fold] can perform significantly better because they don't have | ||
as much overhead. *) | ||
|
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.
Now that the
async_ssl
dependency is mandatory, will there still be a scenario where attempting to setup a tls connection via conduit will fail because of missing support? I'm guessing with the current setup we might not need theNot_supported
branch at all?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 need the Not_supported branch because Pgx_lwt and Pgx_unix don't support TLS right now. Hopefully at some point we can get this supported for Pgx_lwt, but I doubt we'll ever support it for Pgx_unix.
I'm open to alternative ways of doing this though if there is one?