Verification options for TLS

[weargoggles]

This is my attempt at #2573

I don't know where to put the defaults?

[weargoggles]

I find it completely impossible to understand the output of the test suite for this failure. Can someone help me?

[weargoggles]

Hi @schmichael if you find some time can you help me understand why the test suite is failing for me, and what I might have got wrong?

[schmichael]

@weargoggles Thanks for the PR! You made my week easier. :)

The only failure I see is TestHTTP_Stream_Truncate which I think is just a flaky test. I'd ok merging with that failing.

The spirit of your PR is perfect and should work as intended, but we'd actually like to simplify the configuration a little bit:

Instead of exposing verify_{incoming,outgoing} only have a single configuration parameter: verify_https_client

That parameter should default to false and control VerifyIncoming on the HTTP server's TLS config.

When tls.rpc=true we shouldn't even allow people to skip Verify{Incoming,Outgoing}. With tls.http=true we still want to allow verifying client certificates to be optional, hence the new verify_https_client.

Sorry for changing the goal on you and thanks again for the PR!

[weargoggles]

@schmichael I've got a sort of secondary issue, which is that I have trouble health-checking nomad's HTTP API when HTTPS is enabled. My load balancer can't have a client certificate. So is there room for an optional HTTPS port, like Consul?

[schmichael]

Thanks! One slight change.

[schmichael]

This should still be hard-coded as true. If TLSConfig.EnableHTTP is set we don't want to even allow skipping server certificate verification.

[schmichael]

I've got a sort of secondary issue, which is that I have trouble health-checking nomad's HTTP API when HTTPS is enabled. My load balancer can't have a client certificate. So is there room for an optional HTTPS port, like Consul?

Unfortunately, I think we're going to wait until we rework our networking code (binding, advertisement, IP pools, etc) to support HTTP and HTTPS. Allowing both obviously negates the benefits of your change without external firewalling. Any chance your health checks could be switched to be simple TCP connect tests?

How are you planning on using a load balancer without client certificate support with Nomad? Was it just going to be for health checks / alerting?

You did remind me Consul's Nomad HTTP health checks will have this same issue, so we need to switch them to a tcp check if VerifyHTTPSClient==true: https://github.com/hashicorp/nomad/blob/master/command/agent/agent.go#L382-L383

I can make that change after merging this PR if you're not a Consul user.

[weargoggles]

@schmichael I'll explain a bit more clearly. My load balancer (AWS ELB) can run its health check with a different port and protocol to the one it uses as the backend. So:

• Consul is configured to have both HTTP and HTTPS ports
• The load balancer is just speaking TCP for the incoming connections
• the health check is HTTP, and the firewall is configured so that only the load balancer can access Consul's insecure HTTP port

I want to be able to do the same thing in Nomad; forward TCP connections (leaving TLS to Nomad) based on the health determined by a plain HTTP check.

I am using Consul to bootstrap and advertise. The configuration in the Consul health check you mention works perfectly for me, and it would work even if the 'skip verification' option was not set, because the certificates I'm using are valid. Perhaps that should be configurable; but I don't know that it can be controlled by verify_https_client because it's Consul being configured there.

Edit: I'll keep looking through the issue list for this, and open one if I can't find it - I don't think it's on topic for this PR.

[weargoggles]

@schmichael Do you consider your feedback addressed? My test failure doesn't look related to this change.

[schmichael]

Merged. Thanks @weargoggles!

Created #2606 to track the other feature.

[shilov]

This deserves its own issue but I'll mention it here in the interim.. I had Consul configured with a self-signed cert and Nomad was able to register services, but health checks would fail due to SSL verification. The verify_ssl flag was false, which is what must've allowed the service registration to succeed, but the health checks threw an error about x509 authority being unknown.

I'll try to make a separate issue about this later.

[weargoggles]

@shilov Consul supports TLSSkipVerify on HTTPS checks but Nomad doesn't yet expose this option in service stanzas.

[schmichael]

@shilov Consul supports TLSSkipVerify on HTTPS checks but Nomad doesn't yet expose this option in service stanzas.

Fixed in master!

[shilov]

Kudos! 👍

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.