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

TLS Support #107

Closed
wants to merge 1 commit into from
Closed

TLS Support #107

wants to merge 1 commit into from

Conversation

zonito
Copy link
Contributor

@zonito zonito commented Nov 10, 2021

Created another PR because of --signoff meshed up in #96

Signed-off-by: Love Sharma <zonito@protonmail.com>
@zonito
Copy link
Contributor Author

zonito commented Nov 10, 2021

@tagomoris All commits are signed off.

@zonito
Copy link
Contributor Author

zonito commented Nov 17, 2021

@tagomoris - Can we merge this?

@tagomoris
Copy link
Member

This change looks good to me. I want confirmation from other maintainers.
@fluent/golang-maintainers What do you think?

@fujimotos
Copy link
Member

fujimotos commented Nov 18, 2021

@tagomoris @zonito FWIW, I started testing this patch tonight.
I'll post more update tomorrow, so wait for me.

@fujimotos
Copy link
Member

I spent some time this evening testing this patch with Fluentd v1.14.2.
Attached below are the testing assets I crafted for the test.

TLDR I could confirm that fluent-logger-golang now can connect to
Fluentd via TLS with @zonito's patch.

fluentd.conf

<source>
  @type forward
  <transport tls>
    cert_path fluentd.crt
    private_key_path fluentd.key
    private_key_passphrase *********
  </transport>
</source>

<match debug.**>
  @type stdout
</match>

main.go

package main

import (
  "github.com/fluent/fluent-logger-golang/fluent"
)

func main() {
  logger, err := fluent.New(fluent.Config{FluentNetwork: "tls", TlsInsecureSkipVerify: true})
  if err != nil {
    return
  }
  defer logger.Close()
  var data = map[string]string{"foo":  "bar", "hoge": "hoge"}
  logger.Post("debug.log", data)
}

Result

2021-11-19 20:42:19.000000000 +0900 debug.log: {"foo":"bar","hoge":"hoge"}
2021-11-19 20:42:34.000000000 +0900 debug.log: {"foo":"bar","hoge":"hoge"}
2021-11-19 20:42:34.000000000 +0900 debug.log: {"foo":"bar","hoge":"hoge"}

@zonito
Copy link
Contributor Author

zonito commented Nov 19, 2021

@tagomoris @fujimotos Let's merge this.. then

@fujimotos
Copy link
Member

@tagomoris @zonito I'm basically okay with this patch, so I'll step ahead
and gonna merge this PR this weekend (probably on Sunday).

013a672

it looks to me that @zonito accidentally overwrote c16ce5d which is the
current master HEAD of this repo (committed by tagomoris 3 weeks ago).

I will merge this pr with some adjustment, so that it sits nicely on top of c16ce5d.

@zonito
Copy link
Contributor Author

zonito commented Nov 19, 2021

Thanks, @fujimotos, appreciated!

@fujimotos fujimotos self-assigned this Nov 19, 2021
@fujimotos
Copy link
Member

Merged via e5d6aa1.

@fujimotos fujimotos closed this Nov 20, 2021
This was referenced Nov 20, 2021
@fujimotos fujimotos mentioned this pull request Nov 20, 2021
Closed
@akerouanton
Copy link
Contributor

@fujimotos @zonito Although this PR allows using self-signed TLS certs, it doesn't offer proper way of securely doing TLS because there's no way to specify which CA cert is accepted. With this change, if an attacker can mitm/redirect fluentd's network stream, they could still generate their own self-signed cert and bypass TLS promises. IMHO it could give a false sense of security. It would be great to be able to specify which CA certs are accepted (on top of what's in system truststore).

@fujimotos
Copy link
Member

@akerouanton I created a ticket #112 to track that issue. I agree with you that it's better
if we provide cert options such as TLSCertPath.

With this change, if an attacker can mitm/redirect fluentd's network stream, they could still generate their own self-signed cert and bypass TLS promises

The current option is register the certificate (which you want to use) into your operating system. I believe this works, since Go's TLS library recognizes system certificates by default.

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