TCP TLS support - Githubissues

Stexxen commented 6 years ago

Adds TCP TLS Support via tcp_tls.rb With the current implementation of gelf-rb, there are 2 issues

All SSL Errors are eaten, so a user cannot see any problems in the logs.
The Default Ciphers are incorrect, so a default TLS config always fail (invisibly due to problem 1)

With the current implementation of gelf-rb and this PR, a working test can be achieved with bin/logstash -e 'input { stdin { } } output {gelf {tls => {**all_ciphers => true** no_verify => true} protocol => "TCP" host => "localhost" port=> xxxxxx}}' This allows all ciphers, including some insecure ones

I have submitted a pull request to gelf-rb https://github.com/graylog-labs/gelf-rb/pull/68 That fixes both of the above problems and limits the usable Ciphers to ones not cryptographically broken yet trying to keep broad compatibility.

Once that PR has been accepted and that implementation is in use, the following cmd can be used bin/logstash -e 'input { stdin { } } output {gelf {tls => {no_verify => true} protocol => "TCP" host => "localhost" port=> xxxxx}}' and SSL Errors will also now bubble up into the logstash logs.

This will also fix https://github.com/logstash-plugins/logstash-output-gelf/issues/26

Stexxen commented 6 years ago

The CI failure is the problem with v6, same problem as master

jordansissel commented 6 years ago

The failure on 6.x and master is relevant to this PR:

  1) LogStash::Outputs::Gelf#send sends the generated event to gelf
     Failure/Error: next if value == nil

     NoMethodError:
       undefined method `time' for nil:NilClass
     # /home/travis/build/logstash/logstash-core/lib/logstash/timestamp.rb:16:in `<=>'
     # ./lib/logstash/outputs/gelf.rb:170:in `block in receive'
     # ./lib/logstash/outputs/gelf.rb:169:in `receive'
     # ./spec/outputs/gelf_spec.rb:32:in `block in (root)'
     # /home/travis/.rvm/gems/jruby-9.1.13.0/gems/rspec-wait-0.0.9/lib/rspec/wait.rb:46:in `block in (root)'

I haven't looked at the code, but this is a legitimate failure. Very likely unrelated to this PR, but still concerning.

jordansissel commented 6 years ago

Thanks for your work on this plugin; some comments:

The documentation for this plugin lives in docs/index.asciidoc and it would be ever so lovely if the documentation for this change accompanied the PR.
The comment above the config setting references a URL for users to learn what may go in the tls setting. It would be easier for users to have this information in the same page they are already reading.
I am not sure a blanket tls setting is ideal, though I have considered the idea before. We'll need some validation around this, if you can, to notify the user of an incorrect or invalid setting. Like, tls => { "foo" => "bar" } is probably invalid and we should notify the user of some action to take to remediate.

Stexxen commented 6 years ago

Yep docs, gotcha 👍 I'll get that sorted.

In regard to the blanket tls I would disagree, this method decouples this plugin from the versioning of the downstream gelf_rb, so if, at some point someone added other options to it, such as the ability to configure the ciphers used, this plugin could immediately take advantage of that.

You point about helping users is a good one, and is the reasoning behind my gelf_rb PR and its new ability to float the SSL exceptions up to the logstash logs, but I think any direct tls config validation should be done in gelf_rb.

Stexxen commented 6 years ago

Looks like the the V6.x JRuby 9.x builds have never successfully completed. Started failing at this commit https://github.com/logstash-plugins/logstash-output-gelf/commit/0521c5b47bcf53a1bbf3a7f5d7ff4881f523c4d9 , when Jruby 9 was added

     NoMethodError:
       undefined method `time' for nil:NilClass
     # /home/travis/build/logstash/logstash-core/lib/logstash/timestamp.rb:13:in `<=>'

Stexxen commented 6 years ago

Docs updated. I've added the protocol option as it was missing (mentioned here) Also can that issue be closed now?

Stexxen commented 6 years ago

Hi, Is there anything more I should to do this PR to make it more likely to get merged?

jordansissel commented 6 years ago

any direct tls config validation

I understand your goal. My concern is knowledge burden on users because each plugin's SSL settings have different names. We're gradually trying to consolidate all SSL/TLS settings to be the same names across all plugins, and this introduces a new and different way to represent TLS settings. It also requires users visit two pages in order to learn how to configure TLS.

My preference would use the same tls/ssl setting names that, for example, the beats input uses.

immunda commented 5 years ago

Any progress on this? Has the SSL naming been standardised now?

victornet commented 5 years ago

Hi - is there any progress on this? It would be nice to ship logs via encrypted tcp.

juan-vg commented 3 years ago

I have checked this by manually editing my output-gelf plugin and applying the changes.

It works! Graylog2 is finally receiving logs via GELF-TCP & TLS :+1:

Why is this project so abandoned??

logstash-plugins / logstash-output-gelf

TCP TLS support #28