Fix sslsubject regression #159
Conversation
|
Hey all! Any thoughts on the above? |
|
Hey all -- friendly poke, would love to get your thoughts on this considering this component is currently broken and seems to have been that way since around the time of the Netty addition. |
|
Hey @jsvd -- sorry to ping you directly, but do you have an idea of when this could get looked at? |
There was a problem hiding this comment.
Thank you for the contribution @rwaweber, and thank you for your patience.
Unfortunately, as it stands, the PR breaks existing functionality - the plugin has two modes, server and client, which have quite different implementations. Reinstating the removed "dead" code and some minor refactoring should fix this, however.
|
Hey Rob! Thanks for the suggestions -- my bad on accidentally snipping the I'll reinstate the aforementioned methods to not break functionality and incorporate the other suggestions too. |
Attempt to follow the same general flow for tls principal extraction as the beats input plugin: https://github.com/logstash-plugins/logstash-input-beats/blob/5dd54594f65d32aad87d1dfd7b04d0c801216676/lib/logstash/inputs/beats/message_listener.rb#L125-L155 Extract's the ssl subject from inbound messages by: - adjust the interface to pass in the Netty ChannelHandlerContext instead of the Address name the InputHandler's ssl_handler - in the decoder, check if ssl_verify and ssl_enable are true and pull out the subjectname from the context Follow up after review: - Restore ssl-setup code needed for client-mode initialization - Swap conditionals to check for sslsubject extraction
Reducing the number of times we refer to the socket object so that most of the interaction lives in the `handle_socket` method. Remove redundant conditional in ssl_subject assignment, it's already assumed that we're in client mode at that point. Necessary to also fix up refderences in the DecoderImpl class as well, since the tcp plugin's decode_buffer method changed.
| @tcp.decode_buffer(@ip_address, @address, @port, @codec, | ||
| @proxy_address, @proxy_port, tbuf, nil) | ||
| if @tcp.ssl_enable && @tcp.ssl_verify | ||
| session = ctx.channel().pipeline().get("ssl-handler").engine().getSession() |
There was a problem hiding this comment.
this is a pretty long chain, and it is unclear why we stop here to bind a local variable session, which we use only once to continue the chain.
- are all methods in the chain documented to never return
nil? - should we extract this complexity to a helper method that has appropriate
rescueclauses?
There was a problem hiding this comment.
Hey @yaauie! I stopped there mostly as a means to follow the flow here: https://github.com/logstash-plugins/logstash-input-beats/blob/5dd54594f65d32aad87d1dfd7b04d0c801216676/lib/logstash/inputs/beats/message_listener.rb#L125-L155
I'm happy to add in the rescue that's covered here: https://github.com/logstash-plugins/logstash-input-beats/blob/5dd54594f65d32aad87d1dfd7b04d0c801216676/lib/logstash/inputs/beats/message_listener.rb#L132-L139
There was a problem hiding this comment.
are all methods in the chain documented to never return nil?
It seems that way, aside from the ChannelPipeline#get we invoke, though since we explicitly name our channelhandler here, we should be safe in that respect.
should we extract this complexity to a helper method that has appropriate rescue clauses?
I think thats a great idea, though I sort of think it might be worth tucking in somewhere like the logstash/base/inputs package, since we'd be repeating the same logic in this plugin, the beats input plugin, and potentially other plugins where netty is used and TLS metadata extraction is possible. We would need to be careful about formatting though, as different input plugins seem to serialize TLS metadata differently. (it gets cut to 3 fields in the beats plugin and one field here).
Also, as an update on my last message, looking more closely, I'm not sure that rescue clause I cite would really be adding much in terms of safety, but would be more for debug logging insight.
|
Hey all, friendly ping — happy to make some additional changes to get this feature fixed. |
|
Hey @acchen97 -- any chance you'd be able to lend a hand here? |
|
Hello, we are facing the same issue. Any reason why this is not getting merged ? |
Alters the TCP#decode_buffer signature to accept the _ssl subject_ instead of expecting a ruby socket, so that it can be interoperable between the ruby-based client mode and the netty-powered server mode. In server mode, the SSL subject is extracted _once_ when initializing the connection IFF SSL is enabled and verification is turned on. Co-authored-by: Will Weber <[email protected]> Closes: logstash-plugins#159
Alters the TCP#decode_buffer signature to accept the _ssl subject_ instead of expecting a ruby socket, so that it can be interoperable between the ruby-based client mode and the netty-powered server mode. In server mode, the SSL subject is extracted _once_ when initializing the connection IFF SSL is enabled and verification is turned on. Co-authored-by: Will Weber <[email protected]> Closes: logstash-plugins#159
Alters the TCP#decode_buffer signature to accept the _ssl subject_ instead of expecting a ruby socket, so that it can be interoperable between the ruby-based client mode and the netty-powered server mode. In server mode, the SSL subject is extracted _once_ when initializing the connection IFF SSL is enabled and verification is turned on. Co-authored-by: Will Weber <[email protected]> Closes: logstash-plugins#159
* Fix SSL Subject regression in server mode Alters the TCP#decode_buffer signature to accept the _ssl subject_ instead of expecting a ruby socket, so that it can be interoperable between the ruby-based client mode and the netty-powered server mode. In server mode, the SSL subject is extracted _once_ when initializing the connection IFF SSL is enabled and verification is turned on. Co-authored-by: Will Weber <[email protected]> Closes: #159 * include docker jdk17 env Co-authored-by: Will Weber <[email protected]>
- Removes some (what looks like to me) dead code for TLS setupNope! The code was used in client mode, totally misread it.Attempt to follow the same general flow for tls principal extraction as the beats input plugin:
https://github.com/logstash-plugins/logstash-input-beats/blob/5dd54594f65d32aad87d1dfd7b04d0c801216676/lib/logstash/inputs/beats/message_listener.rb#L125-L155
Extract the ssl subject from inbound messages by:
adjust the interface to pass in the Netty ChannelHandlerContext instead of an Address object. I think that this opens up the possibility for being able to eventually work in other metadata into TLS-ified objects
name the InputHandler's ssl_handler to more reliably retrieve it from the available netty Channel Pipelines.
in the decoder, check if ssl_verify and ssl_enable are true and pull out the subjectname from the context
Should help close #143