feature request: TCP+SSL health check

[shantanugadgil]

Description of the Issue (and unexpected/desired result)

Would it be possible to add functionality for a TCP+SSL health check with optional SNI field?

Equivalent of the following OpenSSL command client command:
# openssl s_client -servername foo.example.com -connect zoo.example.com:443

The SSL server I am using doesn't like the connect-disconnect caused by the TCP health check, and prints error log messages. ☹️

For now, I am making do with a script based health check which is a wrapper over openssl commandline.

Reproduction steps

The SSL enabled server prints error log messages when the simple TCP health check occurs.

consul version for both Client and Server

Client: [Consul v1.0.6]
Server: [Consul v1.0.6]

Operating system and Environment details

CentOS 7.4+, Ubuntu 14.04, Ubuntu 16.04

Regards,
Shantanu

[shantanugadgil]

Hi,

Is this a fair ask? Or am I asking something inherently wrong?

Thanks and Regards,
Shantanu

[mkeeler]

@shantanugadgil Thanks for the idea. It certainly seems like a reasonable request. If you or another community member had the time and motivation to implement this then we would be happy to review a PR for the feature. Alternatively, if others would also like to see this implemented add a 👍 upvote reaction to the original issue here as that is one factor we use to prioritize work.

[shantanugadgil]

bump, poke .. 🐛 👉
😄

[shantanugadgil]

Some code I have cobbled together:
https://github.com/shantanugadgil/sslchecker

This serves my purpose as a standalone Go (static) based binary instead of having to include openssl inside the Docker container. Hopefully Consul would get this facility "builtin".

*** Last I checked, the Consul documentation doesn't highlight anywhere that script health checks attached to a Docker will execute inside the Docker.
(my job launcher being Nomad)

Regards,
Shantanu

[shantanugadgil]

@mkeeler any thoughts on the TCP+SSL checker from my repo?

[shantanugadgil]

any eyeballs 👀 on this (https://github.com/shantanugadgil/sslchecker) to integrate the idea into Consul?

@pearkes @johncowen @kyhavlov @i0rek

Thanks,
Shantanu

[banks]

@shantanugadgil it's not super clear to me which part of your example code you're proposing as a new feature?

I think the request is for a TLS check type that can send valid TLS with SNI extension instead of just opening the TCP conn then I think that's pretty easy to add. I'd consider just making it an option on the TCP check we have like TLS: true and a separate optional one for TLSServerName. Then you'd just modify

consul/agent/checks/check.go

Lines 512 to 523 in 9bb239a

	// check is invoked periodically to perform the TCP check
	func (c *CheckTCP) check() {
	conn, err := c.dialer.Dial(`tcp`, c.TCP)
	if err != nil {
	c.Logger.Printf("[WARN] agent: Check %q socket connection failed: %s", c.CheckID, err)
	c.Notify.UpdateCheck(c.CheckID, api.HealthCritical, err.Error())
	return
	}
	conn.Close()
	c.Logger.Printf("[DEBUG] agent: Check %q is passing", c.CheckID)
	c.Notify.UpdateCheck(c.CheckID, api.HealthPassing, fmt.Sprintf("TCP connect %s: Success", c.TCP))
	}

To optionally switch out to use tls.DialWithDialer or leave it as it is and then use tls.Client() to wrap the conn and do the handshake.

There are probably a few extras like sanity checking the config but seems pretty easy!

That said, it's not a super high priority for us given that it's easy to work around with a script check and only affects servers that happen to log errors for tcp conns that don't attempt to handshake so if you'd like to take a stab at a PR that would be great!

Thanks

[jsosulska]

This is still a valid ask. Currently, TCP checks don't use SSL, so this would be adding functionality as well as adding the SNI optional field.

[pgporada]

@banks and @mkeeler I've put up PR #18381 that should complete this issue.