Fix Kapacitor Issue #942: HTTPS subscriptions don't work #7392
Conversation
|
@mglazer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nathanielc, @jsternberg and @orthogonous to be potential reviewers. |
mglazer
force-pushed
the
feature/https-subscriber
branch
3 times, most recently
from
ba08df5 to
d7318f5
Compare
There was a problem hiding this comment.
Thanks for the PR. If you look at the httpd Config you'll see we support providing both a certificate and a private key file. When you pass these into the tls package, Go will ignore an empty private key and assume that the certificate is a PEM-encoded bundle.
This way we support both a single PEM-encoded certificate bundle, or separate certificates and private key files.
Could you make this service behave in the same way?
|
Hey @e-dard thanks for looking. I'm not certain I understand you though. This feature doesn't need private keys to work, you just need the public key certificate to establish the trust relationship. By default, this was loading the CA Certs from where ever they lived on the current host (see: https://golang.org/src/crypto/x509/root_linux.go). I'm just supplying the configuration to load it from a different location. |
|
@mglazer ah cool. Sorry I hadn't had my coffee this morning and immediately assumed you were using these certs for server-side stuff 😄 @nathanielc can you take a look at the PR? |
| if c.CaCerts != "" && !fileExists(c.CaCerts) { | ||
| abspath, err := filepath.Abs(c.CaCerts) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
This error should probably return something like:
fmt.Errorf("ca-certs file %s does not exist", c.CaCerts)
that way the user gets a useful error relating to the miss configuration of the CaCerts, instead of an error about not being able to determine the abspath.
There was a problem hiding this comment.
Done. I'm actually not sure when .Abs() would fail. I originally didn't have this in here, but in order to work on Windows, I had to add it.
| return NewHTTP(u.String(), time.Duration(s.conf.HTTPTimeout)) | ||
| case "https": |
There was a problem hiding this comment.
Could we add a log line here if InsecureSkipVerify is true? This way we can warn the operator that an insecure setting has been used.
There was a problem hiding this comment.
Done. Let me know if the warning is in the correct format.
mglazer
force-pushed
the
feature/https-subscriber
branch
from
d7318f5 to
4101cc7
Compare
|
@mglazer Changes look good, just looks like there is a compilation error with the recent log message change. |
mglazer
force-pushed
the
feature/https-subscriber
branch
from
4101cc7 to
1feca06
Compare
|
My apologies. Fixed. |
|
@mglazer Thanks! |
Required for all non-trivial PRs