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.

Sign up for GitHub

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

fix(inputs.x509): Multiple sources with non-overlapping DNS entries. #11613

Merged
merged 3 commits into from
Aug 5, 2022
Merged

fix(inputs.x509): Multiple sources with non-overlapping DNS entries. #11613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ae7d2f0
Add debugging for invalid certificates.
srebhan Aug 4, 2022
4aca0bd
Fix side-effect of reusing 'tlsCfg.ServerName'.
srebhan Aug 4, 2022
38bfc5b
Fix linter issue.
srebhan Aug 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 23 additions & 11 deletions plugins/inputs/x509_cert/x509_cert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//go:generate ../../../tools/readme_config_includer/generator
// Package x509_cert reports metrics from an SSL certificate.
//
//go:generate ../../../tools/readme_config_includer/generator
package x509_cert

import (
Expand Down Expand Up @@ -28,6 +29,7 @@ import (
)

// DO NOT REMOVE THE NEXT TWO LINES! This is required to embed the sampleConfig data.
//
//go:embed sample.conf
var sampleConfig string

Expand Down Expand Up @@ -143,14 +145,13 @@ func (c *X509Cert) getCert(u *url.URL, timeout time.Duration) ([]*x509.Certifica
if err != nil {
return nil, err
}
c.tlsCfg.ServerName = serverName

c.tlsCfg.InsecureSkipVerify = true
conn := tls.Client(ipConn, c.tlsCfg)
defer conn.Close()
downloadTLSCfg := c.tlsCfg.Clone()
downloadTLSCfg.ServerName = serverName
downloadTLSCfg.InsecureSkipVerify = true

// reset SNI between requests
defer func() { c.tlsCfg.ServerName = "" }()
conn := tls.Client(ipConn, downloadTLSCfg)
defer conn.Close()

hsErr := conn.Handshake()
if hsErr != nil {
Expand Down Expand Up @@ -196,15 +197,17 @@ func (c *X509Cert) getCert(u *url.URL, timeout time.Duration) ([]*x509.Certifica
if err != nil {
return nil, err
}
c.tlsCfg.ServerName = serverName
c.tlsCfg.InsecureSkipVerify = true

downloadTLSCfg := c.tlsCfg.Clone()
downloadTLSCfg.ServerName = serverName
downloadTLSCfg.InsecureSkipVerify = true

smtpConn, err := smtp.NewClient(ipConn, u.Host)
if err != nil {
return nil, err
}

err = smtpConn.Hello(c.tlsCfg.ServerName)
err = smtpConn.Hello(downloadTLSCfg.ServerName)
if err != nil {
return nil, err
}
Expand All @@ -221,7 +224,7 @@ func (c *X509Cert) getCert(u *url.URL, timeout time.Duration) ([]*x509.Certifica
return nil, fmt.Errorf("did not get 220 after STARTTLS: %s", err.Error())
}

tlsConn := tls.Client(ipConn, c.tlsCfg)
tlsConn := tls.Client(ipConn, downloadTLSCfg)
defer tlsConn.Close()

hsErr := tlsConn.Handshake()
Expand Down Expand Up @@ -363,6 +366,15 @@ func (c *X509Cert) Gather(acc telegraf.Accumulator) error {
tags["verification"] = "valid"
fields["verification_code"] = 0
} else {
c.Log.Debugf("Invalid certificate at index %2d!", i)
c.Log.Debugf(" cert DNS names: %v", cert.DNSNames)
c.Log.Debugf(" cert IP addresses: %v", cert.IPAddresses)
c.Log.Debugf(" opts.DNSName: %v", opts.DNSName)
c.Log.Debugf(" verify options: %v", opts)
c.Log.Debugf(" verify error: %v", err)
c.Log.Debugf(" location: %v", location)
c.Log.Debugf(" tlsCfg.ServerName: %v", c.tlsCfg.ServerName)
c.Log.Debugf(" ServerName: %v", c.ServerName)
Comment on lines +369 to +377
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this invalid cert info could be more useful as metric fields instead of console debug messages. I think if we log it like this, it might not be clear to the user what cert is invalid. They might think the invalid cert was found when any telegraf plugin connected to a service and not know it is from x509_cert.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we should expose these internal information as fields. There already is a verification_code field which contain the info. I just added this to be able to quickly look into "hey, telegraf says the cert is invalid but it is valid" issues. Should I remove the info then?

Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought, for a user to see these they'll have to turn on debug logging and the messages will start with "[inputs.x509_cert]" so they shouldn't confuse anyone.

When debugging it will be useful to have opts available.

tags["verification"] = "invalid"
fields["verification_code"] = 1
fields["verification_error"] = err.Error()
Expand Down
9 changes: 8 additions & 1 deletion plugins/inputs/x509_cert/x509_cert_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ func TestGatherRemoteIntegration(t *testing.T) {
sc := X509Cert{
Sources: []string{test.server},
Timeout: config.Duration(test.timeout),
Log: testutil.Logger{},
}
require.NoError(t, sc.Init())

Expand Down Expand Up @@ -165,6 +166,7 @@ func TestGatherLocal(t *testing.T) {

sc := X509Cert{
Sources: []string{f.Name()},
Log: testutil.Logger{},
}
require.NoError(t, sc.Init())

Expand Down Expand Up @@ -193,6 +195,7 @@ func TestTags(t *testing.T) {

sc := X509Cert{
Sources: []string{f.Name()},
Log: testutil.Logger{},
}
require.NoError(t, sc.Init())

Expand Down Expand Up @@ -242,6 +245,7 @@ func TestGatherExcludeRootCerts(t *testing.T) {
sc := X509Cert{
Sources: []string{f.Name()},
ExcludeRootCerts: true,
Log: testutil.Logger{},
}
require.NoError(t, sc.Init())

Expand Down Expand Up @@ -277,6 +281,7 @@ func TestGatherChain(t *testing.T) {

sc := X509Cert{
Sources: []string{f.Name()},
Log: testutil.Logger{},
}
require.NoError(t, sc.Init())

Expand Down Expand Up @@ -365,8 +370,8 @@ func TestGatherCertMustNotTimeoutIntegration(t *testing.T) {
duration := time.Duration(15) * time.Second
m := &X509Cert{
Sources: []string{"https://www.influxdata.com:443"},
Log: testutil.Logger{},
Timeout: config.Duration(duration),
Log: testutil.Logger{},
}
require.NoError(t, m.Init())

Expand All @@ -379,6 +384,7 @@ func TestGatherCertMustNotTimeoutIntegration(t *testing.T) {
func TestSourcesToURLs(t *testing.T) {
m := &X509Cert{
Sources: []string{"https://www.influxdata.com:443", "tcp://influxdata.com:443", "smtp://influxdata.com:25", "file:///dummy_test_path_file.pem", "/tmp/dummy_test_path_glob*.pem"},
Log: testutil.Logger{},
}
require.NoError(t, m.Init())

Expand Down Expand Up @@ -407,6 +413,7 @@ func TestServerName(t *testing.T) {
sc := &X509Cert{
ServerName: test.fromCfg,
ClientConfig: _tls.ClientConfig{ServerName: test.fromTLS},
Log: testutil.Logger{},
}
require.NoError(t, sc.Init())
u, err := url.Parse(test.url)
Expand Down