Skip to content

out_http: adding support for intermediate certificates#3101

Closed
tyarimi wants to merge 2 commits into
fluent:masterfrom
ofek-public:master
Closed

out_http: adding support for intermediate certificates#3101
tyarimi wants to merge 2 commits into
fluent:masterfrom
ofek-public:master

Conversation

@tyarimi

@tyarimi tyarimi commented Aug 10, 2020

Copy link
Copy Markdown
Contributor

Signed-off-by: Tzach Yarimi tyarimi@salesforce.com

Which issue(s) this PR fixes:
Fixes #

What this PR does / why we need it:
Add support for sending a client certificate chain for mutual TLS authentication in the out_http plugin.

Docs Changes:

Release Note:

Signed-off-by: Tzach Yarimi <tyarimi@salesforce.com>
@repeatedly

Copy link
Copy Markdown
Member

@nurse This patch modifies Net::HTTP directly. Do you know this approach is safe or not?

@repeatedly repeatedly added the enhancement Feature request or improve operations label Aug 19, 2020
@tyarimi

tyarimi commented Oct 7, 2020

Copy link
Copy Markdown
Contributor Author

Hi folks. Any chance of merging this PR? We have been running it in production for some time now and it works fine.

@nurse

nurse commented Oct 7, 2020

Copy link
Copy Markdown
Contributor

Of course it's not safe. It should use public APIs.

@tyarimi

tyarimi commented Oct 7, 2020

Copy link
Copy Markdown
Contributor Author

@nurse extra_chain_cert was added in a later version of ruby. Is there any alternative?

@nurse

nurse commented Oct 7, 2020

Copy link
Copy Markdown
Contributor

Well, it should add more comment for example referring ruby/ruby@31af0da#diff-8c2ab8e0fb4f052e1d95ab6334e192c1
Also it should have a condition to avoid the monkey patch runs even if latest ruby.

Signed-off-by: Tzach Yarimi <tyarimi@salesforce.com>
@tyarimi

tyarimi commented Oct 7, 2020

Copy link
Copy Markdown
Contributor Author

@nurse I added the comment, but I'm not sure how to implement the conditional patching (not a ruby expert). Can you please help with that?

opt[:cert] = OpenSSL::X509::Certificate.new(File.read(@tls_client_cert_path))

bundle = File.read(@tls_client_cert_path)
bundle_certs = bundle.scan(/-----BEGIN CERTIFICATE-----(?:.|\n)+?-----END CERTIFICATE-----/)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This regexp seems to match also the middle of a line, is this intentional?

Comment on lines +26 to +31
class Net::HTTP
SSL_IVNAMES << :@extra_chain_cert unless SSL_IVNAMES.include?(:@extra_chain_cert)
SSL_ATTRIBUTES << :extra_chain_cert unless SSL_ATTRIBUTES.include?(:extra_chain_cert)

attr_accessor :extra_chain_cert
end

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With assuming SSL_ATTRIBUTES include :extra_chain_cert if SSL_IVNAMES include :@ extra_chain_cert, single condition can enclose the definitions.

Suggested change
class Net::HTTP
SSL_IVNAMES << :@extra_chain_cert unless SSL_IVNAMES.include?(:@extra_chain_cert)
SSL_ATTRIBUTES << :extra_chain_cert unless SSL_ATTRIBUTES.include?(:extra_chain_cert)
attr_accessor :extra_chain_cert
end
unless SSL_IVNAMES.include?(:@extra_chain_cert)
class Net::HTTP
SSL_IVNAMES << :@extra_chain_cert
SSL_ATTRIBUTES << :extra_chain_cert
attr_accessor :extra_chain_cert
end
end

@tyarimi

tyarimi commented Oct 8, 2020

Copy link
Copy Markdown
Contributor Author

Closing this PR in favor of #3146 (switching head branch)

@tyarimi tyarimi closed this Oct 8, 2020
repeatedly added a commit that referenced this pull request Oct 28, 2020
out_http: adding support for intermediate certificates (supersedes #3101)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Feature request or improve operations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants