fix(inputs.x509): Multiple sources with non-overlapping DNS entries. #11613
Conversation
There was a problem hiding this comment.
Looks good to me. Thanks for doing this @srebhan
edit: just saw this is a draft, sorry 😰
srebhan
changed the title
srebhan
added
fix
|
Tested this on my end and it fixed my issue with 5 mail servers in one source file. Thanks so much @srebhan
|
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
fixed. |
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 👍 This pull request doesn't change the Telegraf binary size 📦 Click here to get additional PR build artifactsArtifact URLs |
|
@MyaLongmire can you maybe add your approval again so it is obvious that you are OK with the latest changes!? |
resolves #11623
This PR fixes an issue for
https,tcpandsmtpcertificate validation when using multiple sources with non-overlapping DNS entries in the certificates. That is if the DNS name of the first source is not valid in, at least one, other certificates.The error results from a stateful behavior of the
serverName()function, evaluating thec.tlsCfg.ServerNameentry and returning this with priority in combination with setting exactly this field (c.tlsCfg.ServerName) when getting the certificate viagetCert(). After getting the first source (e.g.server-A.org), the, previously empty,c.tlsCfg.ServerNamefield is set toserver-A.orgingetCert(). While thehttpsandtcpvariants of the function is emptying the field (leading to issues where the name was set by the user), thesmtpvariant leaves the field as is. Due to the fact thatgetCert()also usesserverName()to get the property, any further call togetCert()will not be able to overwrite the setting asc.tlsCfg.ServerNameis now set (toserver-A.org) and this is returned with priority. Therefore, from now on (after the first call togetCert()),serverName()will always return the name of the first entry insources!As the server name is also used to verify the certificate in
Gather(), the verification of all certificates runs against the name of the first entry insources, which will fail in case that name is not in the certificates of any of the other sources!The PR removes the state in
c.tlsCfgby cloning the configuration for certificate retrieval. This way, only user-set names are returned byserverName()and thus the verification works.