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

TCP TLS support #28

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

Stexxen
Copy link

@Stexxen Stexxen commented Nov 8, 2017

Adds TCP TLS Support via tcp_tls.rb

With the current implementation of gelf-rb, there are 2 issues

  1. All SSL Errors are eaten, so a user cannot see any problems in the logs.
  2. 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 graylog-labs/gelf-rb#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 #26

…epted to allow Exceptions to be seen in the logstash logs

Has no effect on prior versions of gelf-rb
@Stexxen
Copy link
Author

Stexxen commented Nov 8, 2017

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

@jordansissel
Copy link
Contributor

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

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

Stexxen commented Nov 8, 2017

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

Stexxen commented Nov 8, 2017

Looks like the the V6.x JRuby 9.x builds have never successfully completed.
Started failing at this commit 0521c5b , when Jruby 9 was added

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

Added docs for the TLS option hash and additionally the protocol option as it seemed to be missing.
@Stexxen
Copy link
Author

Stexxen commented Nov 8, 2017

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

@Stexxen
Copy link
Author

Stexxen commented Nov 20, 2017

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

@jordansissel
Copy link
Contributor

jordansissel commented Nov 20, 2017

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

immunda commented Oct 3, 2018

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

@victornet
Copy link

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

@juan-vg
Copy link

juan-vg commented Oct 30, 2020

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 👍

Why is this project so abandoned??

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.

Feature request: TLS support
5 participants