search

cloudflare / raven-lua

A Lua interface to Sentry
BSD 3-Clause "New" or "Revised" License
118 stars 52 forks source link

Add HTTPS support #18

Closed LewisJEllis closed 7 years ago

LewisJEllis commented 7 years ago

This takes #4 and adds some options around certificate verification and associated docs.

@mattrobenolt mentioned in #4 that "Someone who knew more about Lua [said] that this was a bad implementation", but I am not sure what that might be referring to. After studying the source of this module and becoming familiar with luasec and the nginx lua module docs (esp around the cosocket API and its handshake stuff) I wouldn't do it significantly differently from how Matt did in his original PR.

The one alternative possibility is for the nginx case, we could do something along the lines of a location /proxy { block in the nginx conf and use proxy_pass etc, but I'd rather not require modifications to nginx conf just to make raven work, and that pattern hasn't been necessary previously. I'm not an expert in the nginx lua module though, just an occasional user, so it's entirely possible that there is an alternative approach I need to take.

@ye @dndx @calio @agentzh - I want to make Lua become an officially supported platform in Sentry, so please let me know anything I need to do to push this forward, or any other reservations or feedback you might have.

ye commented 7 years ago

@LewisJEllis good stuff! I am not an expert either but I knew @mattrobenolt was too modest :) I can be of help in testing this PR if that's necessary; otherwise I am going to refer to the other contributors you mentioned in the PR for further and thorough code reviews/comments etc.

PS: thanks for giving it another shot!

mattrobenolt commented 7 years ago

I literally had no idea, and was basing on someone saying something vague in an IRC channel that it wasn't great. But beyond that, I don't think it's a big deal and we should just move forward.

guanlan commented 7 years ago

@LewisJEllis Thanks for the contribution, can you make sure the luasec library you are using is based on ngx_lua cosocket? Otherwise it might causing significant performance slowdown.

See discussion https://groups.google.com/forum/#!topic/openresty-en/p6Wnz_wDqsc and https://github.com/openresty/lua-nginx-module/issues/178

LewisJEllis commented 7 years ago

@guanlan if ngx is present it already uses the cosocket tcpsock:sslhandshake method instead of luasec; note the ngx_wrap_tls method here and the branch check here.

guanlan commented 7 years ago

@LewisJEllis Thanks for the explanation, that make sense. Is that possible to provide some tests for your changes?

@jdesgats Would you mind review this?

jdesgats commented 7 years ago

I made the fixes I suggested earlier in my fork. Feel free to pull them in this PR: https://github.com/jdesgats/raven-lua/commits/https

jdesgats commented 7 years ago

Actually I missed that you bundled the entire Mozilla public certificate bundle. This is not a good way to handle this IMHO, as it requires a lot of maintenance and could quickly cause security issues.

Unless luasec provides a portable way to get system certificates, I don't think this is the responsibility of this library to implement one. Moreover luasec is not the primary backend of this library.

jdesgats commented 7 years ago

This PR has been superseded by #21