Issue 8914 x509sni

[jjh74]

Required for all PRs:

• Updated associated README.md. (no need to update README)
• Wrote appropriate unit tests. (tests already exist)

resolves #8914
resolves #9278
resolves #9384

1. commit 448eac0 tries to ensure that fileglobs are not tried for remote uris (sources starting with tcp://, udp:// or https://). This is because strings.Index(source, ":\\") != 1 (https://github.com/influxdata/telegraf/blob/master/plugins/inputs/x509_cert/x509_cert.go#L70) matches almost all sources -> fileglobs are tried for remote sources.
2. commit 4e1e6f3 tries to ensure that c.tlsCfg.ServerName is reset between multiple certs/sources. Without this telegraf uses same SNI for multiple certs. Leading to telegraf requesting wrong certificates and failing validation because SNI/SAN doesn't match. (Longer discussion in: inputs.x509_cert unexpected behavior with SNI #8914)

[srebhan]

@jjh74 thanks for digging into this and submitting this PR. I added a comment and a question in the code, so it would be nice if you can comment on those.

However, taking a quick look at the code I think the issue arises from a conceptual weakness in the code. We reuse c.tlsCfg for multiple locations and are thus stateful in the code. IMO we should restructure the code such that we create one c.tlsCfg for each location in an Init() function making it an array. This way we can setup everything correctly there and do not need to clear fields in the loop over the locations. @jjh74 what is your opinion on this suggestion?

[akrantz01]

In addition to what @srebhan said, I think it'd be a good idea to add a unit test to make sure that URLs and file paths/globs are parsed correctly, so this doesn't come up again in the future. Also, please fix the issue raised by the linter.

[srebhan]

I have one remaining question regarding uriregexp in the code...

[jjh74]

@jjh74 thanks for digging into this and submitting this PR. I added a comment and a question in the code, so it would be nice if you can comment on those.

However, taking a quick look at the code I think the issue arises from a conceptual weakness in the code. We reuse c.tlsCfg for multiple locations and are thus stateful in the code. IMO we should restructure the code such that we create one c.tlsCfg for each location in an Init() function making it an array. This way we can setup everything correctly there and do not need to clear fields in the loop over the locations. @jjh74 what is your opinion on this suggestion?

I have no strong opinion one way or another.
Perhaps using c.tlsCfg for each location would make it easier to add other TLS configurations (like tls min version or cipher suites) ? (But writing configuration where you'll add lot's of parameters for each sources seems quite unreadable and it's probably easier to use multiple [[inputs.x509_cert]])

I think tests are missing a test case where you have more than one https/tcp sources and the test would check that correct SNI is used for each one.

[srebhan]

Currently we are using the user configuration and create one tlsCfg as a kind of template. However, this way we need to take care for resetting location-dependent properties of that template while we gather.
My suggestion is to use the user configuration and create multiple tlsCfg, one per location. We then initialize the location-dependent properties once (e.g. at Init()) and don't need to do anything during gather...

[srebhan]

@jjh74 seems like the windows test is failing (even for non-globbing tests)... :-( Maybe conversion of path-separators might be involved...

[aslgithub]

Is there any progress on this ? It seems to be the pro-offered fix for the fact that #6952 has fundamentally broken x509_cert for sources other than file ?
I've logged #9384

[jjh74]

@srebhan I just pushed a commit changing sourcesToURLs() so globpaths are tried only for file:// and / sources (ie. expect that globpath is not working on windows). (You can still use sources like file:///c:/WINDOWS/Temp/*.pem to try globpath on windows). Hopefully CI now builds/works on windows.

[srebhan]

Looks good to me. The CI error is unrelated (in plugins/inputs/tcp_listener). Thanks for caring @jjh74.

[srebhan]

!retry-failed

[jjh74]

@srebhan now should be rebased to current master

[srebhan]

@jjh74 seems like something went wrong with the rebase as you now have 139 files changed. 8-/

[jjh74]

@srebhan new attempt with current master. I hope it goes better this time.

[telegraf-tiger]

Looks like new artifacts were built from this PR. Get them here!

Artifact URLs

• darwin_amd64.tar.gz

• windows_amd64.zip

• windows_i386.zip

• linux_mipsel.tar.gz

• mipsel.deb

• static_linux_amd64.tar.gz

• armel.rpm

• linux_armel.tar.gz

• armel.deb

• aarch64.rpm

• linux_arm64.tar.gz

• arm64.deb

• ppc64le.rpm

• linux_ppc64le.tar.gz

• ppc64el.deb

• s390x.rpm

• linux_s390x.tar.gz

• s390x.deb

• x86_64.rpm

• freebsd_amd64.tar.gz

• linux_amd64.tar.gz

• amd64.deb

• armv6hl.rpm

• freebsd_armv7.tar.gz

• linux_armhf.tar.gz

• armhf.deb

• i386.rpm

• freebsd_i386.tar.gz

• linux_i386.tar.gz

• i386.deb

• linux_mips.tar.gz

• mips.deb

[sspaink]

Awesome! Thank you for fixing these issues and explaining everything so clearly.

[Nkmol]

Any ETA for a new release of this? I noticed this is essential for a multi-chain certificate solution. For example, the common Traefik LetsEncrypt setup uses a multichain setup that uses the server name to correctly pick the chain (LetsEncrypt vs Default Cert).

Thanks for these fixes btw :)

[reimda]

Any ETA for a new release of this?

The next scheduled patch release is July 7. We've been considering making an early patch release to fix a few regressions in 1.19.0, including this one, but we haven't settled on a date yet. It might be June 30.