Allow cert chains#2700
Conversation
Otherwise it seems to default to true, which results in logging of
warning: verify_hostname requires hostname to be set
When running tests that use TLS without verifying the hostname.
Signed-off-by: Matthew Mussomele <matt@nefeli.io>
84f605a to
4cd9d6b
Compare
|
seems AppVeyor doesn't like |
|
unsure why the builds failed - the tests passed for me locally |
|
👍 this would help me too |
|
I will check this patch in this week.
This is travis-ci environment issue. It sometimes slow down and it causes unexpected test failure. |
| if verify_fqdn && fqdn && context.respond_to?(:verify_hostname=) | ||
| context.verify_hostname = true | ||
| else | ||
| context.verify_hostname = false |
There was a problem hiding this comment.
Why change this line to set false explicitly?
There was a problem hiding this comment.
see commit body of the first commit:
Otherwise it seems to default to true, which results in logging of
warning: verify_hostname requires hostname to be set
When running tests that use TLS without verifying the hostname.
If you leave it unset like the code did prior, then warning logs are printed. Perhaps the right thing to do is something along the lines of
if hostname verification set to true:
context.verify_hostname = true
else if hostname verification explicitly set to false:
context.verify_hostname = false
else
leave context.verify_hostname nil
?
| data('ack true' => true, | ||
| 'ack false' => false) | ||
| test 'TLS transport client cert chain' do |ack| | ||
| omit "TLS and 'ack false' always fails. Need to debug" if !ack |
There was a problem hiding this comment.
could you debug before merging this PR?
There was a problem hiding this comment.
This is an existing bug prior to my changes - see the test above.
There was a problem hiding this comment.
fluentd/test/plugin/test_out_forward.rb
Line 585 in 4cd9d6b
| omit "TLS and 'ack false' always fails. Need to debug" if !ack | |
| omit "TLS and 'ack false' always fails. Need to debug" if Fluent.windows? && !ack |
There was a problem hiding this comment.
corrected omit check
4cd9d6b to
5e05063
Compare
|
@repeatedly @ganmacs changes made, except for the |
|
Re-run test to check random test failures. |
Signed-off-by: Matthew Mussomele <matt@nefeli.io>
5e05063 to
38acd8d
Compare
|
sorry for the delay, have been very busy. Just updated the omit check to specify windows |
|
This was done by #2930 . |
Which issue(s) this PR fixes:
Fixes #2699
What this PR does / why we need it:
Adds support for certificate chains to Fluentd sockets. Needed for good practice around issuing certificates.
The added test fails if the new line:
is remove (effectively reverting to the old behavior of only loading the leaf certificate).
Docs Changes:
Unsure if needed.
Release Note: