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

Graphite output plugin: Enable secure connection #2602

Merged
merged 6 commits into from
Jun 13, 2017
Merged

Graphite output plugin: Enable secure connection #2602

merged 6 commits into from
Jun 13, 2017

Conversation

dheerajdwivedi
Copy link
Contributor

Feature Request

Proposal:

We are looking to use hosted solution for graphite.
Since the communication is over internet it needs to be secure.
Hence the support for tls over tcp is required.

Current behavior:

Insecure communication between telegraf client and server

Desired behavior:

Secure the communication between client and server

Use case:

Graphite hosted solutions where data flows over internet.
To avoid middleman attack, hence secure the auth credentials and metrics data between client and server

@danielnelson
Copy link
Contributor

#2600

@dheerajdwivedi
Copy link
Contributor Author

False failure can someone rebuild it please.

@nhaugo
Copy link
Contributor

nhaugo commented Mar 31, 2017

@dheerajdwivedi can you write some tests so we can support this long term.

Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

Current test situation is fine for this one.

@@ -34,6 +50,16 @@ var sampleConfig = `
template = "host.tags.measurement.field"
## timeout in seconds for the write connection to graphite
timeout = 2

## Enable secure tunnel
ssl_enabled = false
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need this option, enabling a secure connection can be based on the tlsConfig being nil or not.

# ssl_cert = "/etc/telegraf/cert.pem"
# ssl_key = "/etc/telegraf/key.pem"
## Use SSL but skip chain & host verification
# insecure_skip_verify = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent this section to 2 spaces

var conn net.Conn
if g.SSLEnabled {
// If ssl is enabled make secure tcp connection
conn, err = tls.Dial("tcp", server, g.tlsConfig)
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to use timeout with tls connections. Should be able to do this using tls.DialWithDialer.

I think its possible to just use a nil tls.Config to do a insecure connection, but not sure. This way you don't need the branch.

if err == nil {
// Append successful connections
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

@danielnelson danielnelson merged commit f0cbfe4 into influxdata:master Jun 13, 2017
@danielnelson
Copy link
Contributor

Thanks! Will be included in 1.4.0.

@michael-lohier
Copy link

Hello,

@danielnelson @dheerajdwivedi

The current configuration to establish an TLS/SSL connection between Telegraf and Graphite is made via the output plugin, setting these options in Telegraf:

  ## Optional TLS Config
  # tls_ca = "/etc/telegraf/ca.pem"
  # tls_cert = "/etc/telegraf/cert.pem"
  # tls_key = "/etc/telegraf/key.pem"
  ## Use TLS but skip chain & host verification
  # insecure_skip_verify = false

However, on Graphite side, it doesn't seem so clear how to set it up. I assume it is possible (or at least it was at the time you added this option). Could you please tell me how was the setup with Graphite you used to test the TLS/SSL connection between Telegraf and Graphite, at the time you added these options?

Thanks a lot

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.

4 participants