Skip to content
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 HTTPS support #18

Closed
wants to merge 5 commits into from
Closed

Conversation

LewisJEllis
Copy link
Contributor

@LewisJEllis LewisJEllis commented Apr 12, 2017

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
Copy link
Contributor

ye commented Apr 13, 2017

@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
Copy link
Contributor

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
Copy link

guanlan commented Apr 18, 2017

@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 openresty/lua-nginx-module#178

@LewisJEllis
Copy link
Contributor Author

@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
Copy link

guanlan commented Apr 19, 2017

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

@jdesgats Would you mind review this?

Copy link
Contributor

@jdesgats jdesgats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting this PR! From a quick review it looks good except for a few nitpicks. I'll test that a bit more as soon as possible.


-- wrap_tls: Wraps a connected socket with TLS
function _M.wrap_tls(self, sock)
if ngx then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check can be done at module loading: you can define a signle :wrap_tls() method differently depending on the available TLS implementation.

-- <li><span class="parameter">tags</span> extra tags to include on all reported errors</li>
-- <li><span class="parameter">logger</span></li>
-- <li><span class="parameter">verify_ssl</span> boolean of whether to perform SSL certificate verification</li>
-- <li><span class="parameter">cacert</span> path to CA certificate bundle file (defaults to ./data/cacert.pem)</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a note pointing out it is for LuaSocket only?

@jdesgats
Copy link
Contributor

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 jdesgats mentioned this pull request May 12, 2017
@jdesgats
Copy link
Contributor

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
Copy link
Contributor

This PR has been superseded by #21

@jdesgats jdesgats closed this May 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants