feat: implement support for SSL connection #24

thibaultcha · 2016-02-26T03:58:59Z

Hi,

A stab at SSL connections (#9).

Implements client-server SSL connection. We assume to be in an ngx_lua
environment, where 'tcpsock:sslhandshake()' is available. ngx_lua only
supports server authentication via 'lua_ssl_trusted_certificate'.

In plain Lua/JIT, we rely on LuaSec (the same way we rely on LuaSocket
already) and a fallback method is provided which follows the ngx_lua
method signature, plus accept LuaSec options. LuaSec allows for server
and client authentication.

An option is provided to require that the server supports SSL,
otherwise, aborts the connection, as suggested in section 43.2.9 of the
protocol flow.

New options (defaults):

{
  ssl = false,
  ssl_verify = false,
  ssl_required = false,
  cafile = nil, -- LuaSec
  cert = nil, -- LuaSec
  key = nil -- LuaSec
}

From those options, only ssl is included as a class attribute for simplicity
(is ssl is false, none of those options make sense), not sure if they should
all be there or not.

Caveats:

One change had to be done: the LuaSocket proxy metatable does not
cache its original methods anymore. If it does, the old socket is
retained from previous calls to 'send()' or 'receive()', and since
LuaSec closes the socket when wrapping it, further calls do not
succeed anymore.
Not easily testable in CI and test suite. I added a simple test,
but ssl_required and other options are harder to test on Travis. I
tested all options manually with Lua and ngx_lua, on a server that
does not accept non-SSL connections.

Usage:

local pgmoon = require "pgmoon"
local pg = pgmoon.new {
  ssl = true,
  ssl_verify = true,
  ssl_required = true,
  cafile = "/path/to/ca.pem"
  -- cert
  -- key
}

assert(pg:connect())

pg:disconnect()

Let me know!

thibaultcha · 2016-02-26T04:06:53Z

(waiting on API validation before updating the docs)

thibaultcha · 2016-03-08T21:11:35Z

Hey @leafo, did you get any chance to take a look at this? If you have any feedback I would make the necessary changes. I hope not to be pushy! Thanks :)

leafo · 2016-03-08T23:49:05Z

Didn't get a chance to look yet, was sick last week. Hope to do so soon, thanks.

ahmadnassri · 2016-03-22T21:39:10Z

hey @leafo, hope you're feeling better, let us know if there's anything we can help with on our end ;)

leafo · 2016-03-28T23:39:19Z

sorry for the delay! (I had a confernece and got sick again...) will check this out ASAP

thibaultcha · 2016-03-28T23:47:57Z

Sure, no problem!

I feel you, I too got sick twice this winter 😡 😷

leafo · 2016-04-13T03:57:15Z

pgmoon/init.moon


  connect: =>
    @sock = socket.new!
    ok, err = @sock\connect @host, @port
    return nil, err unless ok

    if @sock\getreusedtimes! == 0
+      if @ssl
+        success, err = @send_ssl_message!
+        return nil, err unless success


when ssl_required is false and ssl is true, then send_ssl_message returns nil with no error message when the ssl connection can't be established. Probably not what you intended?

Yes we should return true! I added this just before submitting the patching without testing it, totally guilty.

We also probably want to disconnect too if the server does not support SSL connections:

elseif @ssl_required @disconnect! nil, "the server does not support SSL connections"

Additionally, we are missing the handling of ErrorMessage responses:

The frontend should also be prepared to handle an ErrorMessage response to SSLRequest from the server. This would only occur if the server predates the addition of SSL support to PostgreSQL. In this case the connection must be closed, but the frontend might choose to open a fresh connection and proceed without requesting SSL.

Implements client-server SSL connection. We assume to be in an ngx_lua environment, where 'tcpsock:sslhandshake()' is available. ngx_lua only supports server authentication via 'lua_ssl_trusted_certificate'. In plain Lua/JIT, we rely on LuaSec (the same way we rely on LuaSocket already) and a fallback method is provided which follows the ngx_lua method signature, plus accept LuaSec options. LuaSec allows for server and client authentication. An option is provided to require that the server supports SSL, otherwise, aborts the connection. New options (defaults): ``` { ssl = false, ssl_verify = false, ssl_required = false, cafile = nil, -- LuaSec cert = nil, -- LuaSec, key = nil -- LuaSec } ``` Caveats: - One change had to be done: the LuaSocket proxy metatable does not cache its original methods anymore. If it does, the old socket is retained from previous calls to 'send()' or 'receive()', and since LuaSec closes the socket when wrapping it, further calls do not succeed anymore. - Not easily testable in CI and test suite. I added a simple test, but `ssl_required` and other options are harder to test on Travis. I tested all options manually with Lua and ngx_lua, on a server that does not accept non-SSL connections.

thibaultcha · 2016-04-13T06:53:45Z

I just updated the patch, thanks for the review.

Changes:

Restore memoization of socket's closures. I figured the safest is to set every attached function to nil except those we override in proxy_mt. There were other ways but I think this is the cleanest/safest.
Handle ErrorMessage in response of an SSLRequest. As per the spec, we disconnect on such.
Handle the edge-case with ssl true and ssl_required false.
Fix a stack overflow that could occur in the proxied sslhandshake when the handshake failed and we tried to access @sock which was nil.
Fix the SSL test + added a new one for ssl_required. We expect a server with ssl = off for those.

When/if you're ok with this, let me know and I'll push some documentation!

forkfork · 2016-05-16T04:23:33Z

@thibaultcha I'm happy to write some docs if it helps to progress this PR - let me know if you want a hand.

subnetmarco · 2016-06-03T17:54:00Z

Any update on this PR?

aduwadi · 2016-06-24T14:27:06Z

any update on this PR?

thibaultcha · 2016-06-24T22:14:46Z

Happy to wrap this up if @leafo has any more suggestions. If everything looks good code-wise, I'll quickly provide the documentation for this!

leafo · 2016-06-29T03:39:05Z

sorry for being MIA for so long, I got caught up with a bunch of work and have been neglecting my opensource projects.

I just tested it out and it appears to be working fine.

I wrote a basic test suite for the luasocket implementation:

https://github.com/leafo/pgmoon/blob/leafo/ssl/spec/postgres.sh
https://github.com/leafo/pgmoon/blob/leafo/ssl/spec/pgmoon_ssl_spec.moon

Everything looks good:

https://travis-ci.org/leafo/pgmoon/jobs/140978776

Only strange thing I noticed is when the socket is closed postgres prints this to the log

LOG:  could not receive data from client: Connection reset by peer

And google tells me this: https://devcenter.heroku.com/articles/postgres-logs-errors#log-unexpected-eof-on-client-connection

I'm not sure if it's worth trying to figure out, I didn't see anything obvious that might have caused it.

Anyway, I'm planning on merging everything in unless you want to look into the log message. If you want to add some docs to the readme too that would be great.

thibaultcha · 2016-06-29T19:23:44Z

I just pushed a commit which adds some documentation about SSL connections. Trying to keep it as simple as possible yet still mention the semi-optional LuaSec dependency.

thibaultcha · 2016-06-29T19:25:26Z

About the improperly-closed connection issue, I really have no clue what could be causing this and I'm not sure if this could come from LuaSec either... Nothing changed in disconnect() and we are properly using LuaSec's close()...

leafo · 2016-07-13T03:36:44Z

Documentation changed merged, new version deployed to luarocks.org: https://luarocks.org/modules/leafo/pgmoon

Thanks for the patch, once again, sorry about being so slow. Hope it didn't interrupt anything.

thibaultcha mentioned this pull request Mar 2, 2016

PostgreSQL support Kong/kong#331

Closed

leafo reviewed Apr 13, 2016
View reviewed changes

thibaultcha force-pushed the feature/ssl branch from bfb2e73 to aa5c21c Compare April 13, 2016 06:45

thibaultcha mentioned this pull request Apr 19, 2016

release: 0.8.0 Kong/docs.konghq.com#208

Merged

11 tasks

mars mentioned this pull request May 13, 2016

Update to 0.8.1 heroku/heroku-buildpack-kong#7

Closed

thibaultcha mentioned this pull request Jun 10, 2016

Allow connecting to Postgres database via SSL Kong/kong#1309

Closed

docs(readme) add SSL connection section and LuaSec mentions

9493506

leafo closed this Jul 13, 2016

thibaultcha mentioned this pull request Nov 7, 2016

fix(ssl) use proper sslhandshake signature in ngx_lua #41

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement support for SSL connection #24

feat: implement support for SSL connection #24

thibaultcha commented Feb 26, 2016

thibaultcha commented Feb 26, 2016

thibaultcha commented Mar 8, 2016

leafo commented Mar 8, 2016

ahmadnassri commented Mar 22, 2016

leafo commented Mar 28, 2016

thibaultcha commented Mar 28, 2016

leafo Apr 13, 2016

thibaultcha Apr 13, 2016

thibaultcha Apr 13, 2016

thibaultcha commented Apr 13, 2016

forkfork commented May 16, 2016

subnetmarco commented Jun 3, 2016

aduwadi commented Jun 24, 2016

thibaultcha commented Jun 24, 2016

leafo commented Jun 29, 2016 •

edited

Loading

thibaultcha commented Jun 29, 2016

thibaultcha commented Jun 29, 2016 •

edited

Loading

leafo commented Jul 13, 2016 •

edited

Loading

feat: implement support for SSL connection #24

feat: implement support for SSL connection #24

Conversation

thibaultcha commented Feb 26, 2016

thibaultcha commented Feb 26, 2016

thibaultcha commented Mar 8, 2016

leafo commented Mar 8, 2016

ahmadnassri commented Mar 22, 2016

leafo commented Mar 28, 2016

thibaultcha commented Mar 28, 2016

leafo Apr 13, 2016

Choose a reason for hiding this comment

thibaultcha Apr 13, 2016

Choose a reason for hiding this comment

thibaultcha Apr 13, 2016

Choose a reason for hiding this comment

thibaultcha commented Apr 13, 2016

forkfork commented May 16, 2016

subnetmarco commented Jun 3, 2016

aduwadi commented Jun 24, 2016

thibaultcha commented Jun 24, 2016

leafo commented Jun 29, 2016 • edited Loading

thibaultcha commented Jun 29, 2016

thibaultcha commented Jun 29, 2016 • edited Loading

leafo commented Jul 13, 2016 • edited Loading

leafo commented Jun 29, 2016 •

edited

Loading

thibaultcha commented Jun 29, 2016 •

edited

Loading

leafo commented Jul 13, 2016 •

edited

Loading