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

fluent-plugin: Add client certificate verification #1189

Merged
merged 3 commits into from
Nov 5, 2019

Conversation

putrasattvika
Copy link
Contributor

What this PR does / why we need it:
Allow the fluentd to push logs to Loki servers located behind a reverse proxy with client certificate verification.

Checklist

  • Documentation added
  • Tests updated

@CLAassistant
Copy link

CLAassistant commented Oct 22, 2019

CLA assistant check
All committers have signed the CLA.

@@ -67,6 +74,13 @@ def configure(conf)

@label_keys = @label_keys.split(/\s*,\s*/) if @label_keys
@remove_keys = @remove_keys.split(',').map(&:strip) if @remove_keys

@cert = OpenSSL::X509::Certificate.new(File.read(@cert)) if @cert
@key = OpenSSL::PKey::RSA.new(File.read(key)) if @key
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance to use OpenSSL::PKey.read?

ref: https://ruby-doc.org/stdlib-2.4.0/libdoc/openssl/rdoc/OpenSSL/PKey.html#method-c-read

This method should be more flexible to handle private key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be possible, I'll update this in the next commit


if !@cert.nil? && !@key.nil?
opts = opts.merge(
verify_mode: OpenSSL::SSL::VERIFY_PEER,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we handle VERIFY_NONE case for self-signed sertificates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we're using self-signed certificates, wouldn't it be better to pass ca_cert to verify the server's self-signed cert?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, you are right.
Please leave it as-is until VERIFY_NONE use case is found.

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

@cyriltovena cyriltovena merged commit cb4f5b4 into grafana:master Nov 5, 2019
@tarokkk
Copy link
Contributor

tarokkk commented Nov 8, 2019

Did you tried this PR out? I immediately got

configure': Unsupported private key type NilClass (RuntimeError)

without cert config.

edit: I added a fix here #1242

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.

None yet

5 participants