Merge pull request #2608 from hashicorp/f-test-verify_https_client

Test verify_https_client behavior and skip Consul HTTPS health checks when enabled

.travis.yml

@@ -7,7 +7,7 @@ services:
 language: go
 
 go:
-  -  1.8
+  -  1.8.x
 
 branches:
   only:

CHANGELOG.md

@@ -4,6 +4,8 @@ IMPROVEMENTS:
  * core: Back-pressure when evaluations are nacked and ensure scheduling
    progress on evaluation failures [GH-2555]
  * core: Track multiple job versions and add a stopped state for jobs [GH-2566]
+ * api: Add `verify_https_client` to require certificates from HTTP clients
+   [GH-2587]
  * api/job: Ability to revert job to older versions [GH-2575]
  * client: Fingerprint all routable addresses on an interface including IPv6
    addresses [GH-2536]

command/agent/agent.go

@@ -351,43 +351,22 @@ func (a *Agent) setupServer() error {
 	a.server = server
 
 	// Consul check addresses default to bind but can be toggled to use advertise
-	httpCheckAddr := a.config.normalizedAddrs.HTTP
 	rpcCheckAddr := a.config.normalizedAddrs.RPC
 	serfCheckAddr := a.config.normalizedAddrs.Serf
 	if *a.config.Consul.ChecksUseAdvertise {
-		httpCheckAddr = a.config.AdvertiseAddrs.HTTP
 		rpcCheckAddr = a.config.AdvertiseAddrs.RPC
 		serfCheckAddr = a.config.AdvertiseAddrs.Serf
 	}
 
 	// Create the Nomad Server services for Consul
-	// TODO re-introduce HTTP/S checks when Consul 0.7.1 comes out
 	if *a.config.Consul.AutoAdvertise {
 		httpServ := &structs.Service{
 			Name:      a.config.Consul.ServerServiceName,
 			PortLabel: a.config.AdvertiseAddrs.HTTP,
 			Tags:      []string{consul.ServiceTagHTTP},
-			Checks: []*structs.ServiceCheck{
-				&structs.ServiceCheck{
-					Name:      "Nomad Server HTTP Check",
-					Type:      "http",
-					Path:      "/v1/status/peers",
-					Protocol:  "http",
-					Interval:  serverHttpCheckInterval,
-					Timeout:   serverHttpCheckTimeout,
-					PortLabel: httpCheckAddr,
-				},
-			},
 		}
-		if conf.TLSConfig.EnableHTTP {
-			if a.consulSupportsTLSSkipVerify {
-				httpServ.Checks[0].Protocol = "https"
-				httpServ.Checks[0].TLSSkipVerify = true
-			} else {
-				// No TLSSkipVerify support, don't register https check
-				a.logger.Printf("[WARN] agent: not registering Nomad HTTPS Health Check because it requires Consul>=0.7.2")
-				httpServ.Checks = []*structs.ServiceCheck{}
-			}
+		if check := a.agentHTTPCheck(); check != nil {
+			httpServ.Checks = []*structs.ServiceCheck{check}
 		}
 		rpcServ := &structs.Service{
 			Name:      a.config.Consul.ServerServiceName,
@@ -482,39 +461,15 @@ func (a *Agent) setupClient() error {
 	}
 	a.client = client
 
-	// Resolve the http check address
-	httpCheckAddr := a.config.normalizedAddrs.HTTP
-	if *a.config.Consul.ChecksUseAdvertise {
-		httpCheckAddr = a.config.AdvertiseAddrs.HTTP
-	}
-
 	// Create the Nomad Client  services for Consul
 	if *a.config.Consul.AutoAdvertise {
 		httpServ := &structs.Service{
 			Name:      a.config.Consul.ClientServiceName,
 			PortLabel: a.config.AdvertiseAddrs.HTTP,
 			Tags:      []string{consul.ServiceTagHTTP},
-			Checks: []*structs.ServiceCheck{
-				&structs.ServiceCheck{
-					Name:      "Nomad Client HTTP Check",
-					Type:      "http",
-					Path:      "/v1/agent/servers",
-					Protocol:  "http",
-					Interval:  clientHttpCheckInterval,
-					Timeout:   clientHttpCheckTimeout,
-					PortLabel: httpCheckAddr,
-				},
-			},
 		}
-		if conf.TLSConfig.EnableHTTP {
-			if a.consulSupportsTLSSkipVerify {
-				httpServ.Checks[0].Protocol = "https"
-				httpServ.Checks[0].TLSSkipVerify = true
-			} else {
-				// No TLSSkipVerify support, don't register https check
-				a.logger.Printf("[WARN] agent: not registering Nomad HTTPS Health Check because it requires Consul>=0.7.2")
-				httpServ.Checks = []*structs.ServiceCheck{}
-			}
+		if check := a.agentHTTPCheck(); check != nil {
+			httpServ.Checks = []*structs.ServiceCheck{check}
 		}
 		if err := a.consulService.RegisterAgent(consulRoleClient, []*structs.Service{httpServ}); err != nil {
 			return err
@@ -524,6 +479,42 @@ func (a *Agent) setupClient() error {
 	return nil
 }
 
+// agentHTTPCheck returns a health check for the agent's HTTP API if possible.
+// If no HTTP health check can be supported nil is returned.
+func (a *Agent) agentHTTPCheck() *structs.ServiceCheck {
+	// Resolve the http check address
+	httpCheckAddr := a.config.normalizedAddrs.HTTP
+	if *a.config.Consul.ChecksUseAdvertise {
+		httpCheckAddr = a.config.AdvertiseAddrs.HTTP
+	}
+	check := structs.ServiceCheck{
+		Name:      "Nomad Client HTTP Check",
+		Type:      "http",
+		Path:      "/v1/agent/servers",
+		Protocol:  "http",
+		Interval:  clientHttpCheckInterval,
+		Timeout:   clientHttpCheckTimeout,
+		PortLabel: httpCheckAddr,
+	}
+	if !a.config.TLSConfig.EnableHTTP {
+		// No HTTPS, return a plain http check
+		return &check
+	}
+	if !a.consulSupportsTLSSkipVerify {
+		a.logger.Printf("[WARN] agent: not registering Nomad HTTPS Health Check because it requires Consul>=0.7.2")
+		return nil
+	}
+	if a.config.TLSConfig.VerifyHTTPSClient {
+		a.logger.Printf("[WARN] agent: not registering Nomad HTTPS Health Check because verify_https_client enabled")
+		return nil
+	}
+
+	// HTTPS enabled; skip verification
+	check.Protocol = "https"
+	check.TLSSkipVerify = true
+	return &check
+}
+
 // reservePortsForClient reserves a range of ports for the client to use when
 // it creates various plugins for log collection, executors, drivers, etc
 func (a *Agent) reservePortsForClient(conf *clientconfig.Config) error {
@@ -691,7 +682,7 @@ func (a *Agent) setupConsul(consulConfig *config.ConsulConfig) error {
 	}
 
 	// Determine version for TLSSkipVerify
-	if self, err := client.Agent().Self(); err != nil {
+	if self, err := client.Agent().Self(); err == nil {
 		a.consulSupportsTLSSkipVerify = consulSupportsTLSSkipVerify(self)
 	}
 

command/agent/agent_test.go

@@ -4,12 +4,14 @@ import (
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
+	"log"
 	"net"
 	"os"
 	"strings"
 	"testing"
 	"time"
 
+	"github.com/hashicorp/nomad/helper"
 	"github.com/hashicorp/nomad/nomad"
 	sconfig "github.com/hashicorp/nomad/nomad/structs/config"
 )
@@ -360,6 +362,98 @@ func TestAgent_ClientConfig(t *testing.T) {
 	}
 }
 
+// TestAgent_HTTPCheck asserts Agent.agentHTTPCheck properly alters the HTTP
+// API health check depending on configuration.
+func TestAgent_HTTPCheck(t *testing.T) {
+	logger := log.New(ioutil.Discard, "", 0)
+	if testing.Verbose() {
+		logger = log.New(os.Stdout, "[TestAgent_HTTPCheck] ", log.Lshortfile)
+	}
+	agent := func() *Agent {
+		return &Agent{
+			logger: logger,
+			config: &Config{
+				AdvertiseAddrs:  &AdvertiseAddrs{HTTP: "advertise:4646"},
+				normalizedAddrs: &Addresses{HTTP: "normalized:4646"},
+				Consul: &sconfig.ConsulConfig{
+					ChecksUseAdvertise: helper.BoolToPtr(false),
+				},
+				TLSConfig: &sconfig.TLSConfig{EnableHTTP: false},
+			},
+		}
+	}
+
+	t.Run("Plain HTTP Check", func(t *testing.T) {
+		a := agent()
+		check := a.agentHTTPCheck()
+		if check == nil {
+			t.Fatalf("expected non-nil check")
+		}
+		if check.Type != "http" {
+			t.Errorf("expected http check not: %q", check.Type)
+		}
+		if expected := "/v1/agent/servers"; check.Path != expected {
+			t.Errorf("expected %q path not: %q", expected, check.Path)
+		}
+		if check.Protocol != "http" {
+			t.Errorf("expected http proto not: %q", check.Protocol)
+		}
+		if expected := a.config.normalizedAddrs.HTTP; check.PortLabel != expected {
+			t.Errorf("expected normalized addr not %q", check.PortLabel)
+		}
+	})
+
+	t.Run("Plain HTTP + ChecksUseAdvertise", func(t *testing.T) {
+		a := agent()
+		a.config.Consul.ChecksUseAdvertise = helper.BoolToPtr(true)
+		check := a.agentHTTPCheck()
+		if check == nil {
+			t.Fatalf("expected non-nil check")
+		}
+		if expected := a.config.AdvertiseAddrs.HTTP; check.PortLabel != expected {
+			t.Errorf("expected advertise addr not %q", check.PortLabel)
+		}
+	})
+
+	t.Run("HTTPS + consulSupportsTLSSkipVerify", func(t *testing.T) {
+		a := agent()
+		a.consulSupportsTLSSkipVerify = true
+		a.config.TLSConfig.EnableHTTP = true
+
+		check := a.agentHTTPCheck()
+		if check == nil {
+			t.Fatalf("expected non-nil check")
+		}
+		if !check.TLSSkipVerify {
+			t.Errorf("expected tls skip verify")
+		}
+		if check.Protocol != "https" {
+			t.Errorf("expected https not: %q", check.Protocol)
+		}
+	})
+
+	t.Run("HTTPS w/o TLSSkipVerify", func(t *testing.T) {
+		a := agent()
+		a.consulSupportsTLSSkipVerify = false
+		a.config.TLSConfig.EnableHTTP = true
+
+		if check := a.agentHTTPCheck(); check != nil {
+			t.Fatalf("expected nil check not: %#v", check)
+		}
+	})
+
+	t.Run("HTTPS + VerifyHTTPSClient", func(t *testing.T) {
+		a := agent()
+		a.consulSupportsTLSSkipVerify = true
+		a.config.TLSConfig.EnableHTTP = true
+		a.config.TLSConfig.VerifyHTTPSClient = true
+
+		if check := a.agentHTTPCheck(); check != nil {
+			t.Fatalf("expected nil check not: %#v", check)
+		}
+	})
+}
+
 func TestAgent_ConsulSupportsTLSSkipVerify(t *testing.T) {
 	assertSupport := func(expected bool, blob string) {
 		self := map[string]map[string]interface{}{}

command/agent/http_test.go

@@ -2,12 +2,16 @@ package agent
 
 import (
 	"bytes"
+	"crypto/tls"
+	"crypto/x509"
 	"encoding/json"
 	"fmt"
 	"io"
 	"io/ioutil"
+	"net"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"os"
 	"strconv"
 	"testing"
@@ -15,6 +19,7 @@ import (
 
 	"github.com/hashicorp/nomad/nomad/mock"
 	"github.com/hashicorp/nomad/nomad/structs"
+	"github.com/hashicorp/nomad/nomad/structs/config"
 	"github.com/hashicorp/nomad/testutil"
 )
 
@@ -337,6 +342,126 @@ func TestParseRegion(t *testing.T) {
 	}
 }
 
+// TestHTTP_VerifyHTTPSClient asserts that a client certificate signed by the
+// appropriate CA is required when VerifyHTTPSClient=true.
+func TestHTTP_VerifyHTTPSClient(t *testing.T) {
+	const (
+		cafile  = "../../helper/tlsutil/testdata/ca.pem"
+		foocert = "../../helper/tlsutil/testdata/nomad-foo.pem"
+		fookey  = "../../helper/tlsutil/testdata/nomad-foo-key.pem"
+	)
+	s := makeHTTPServer(t, func(c *Config) {
+		c.Region = "foo" // match the region on foocert
+		c.TLSConfig = &config.TLSConfig{
+			EnableHTTP:        true,
+			VerifyHTTPSClient: true,
+			CAFile:            cafile,
+			CertFile:          foocert,
+			KeyFile:           fookey,
+		}
+	})
+	defer s.Cleanup()
+
+	reqURL := fmt.Sprintf("https://%s/v1/agent/self", s.Agent.config.AdvertiseAddrs.HTTP)
+
+	// FAIL: Requests that expect 127.0.0.1 as the name should fail
+	resp, err := http.Get(reqURL)
+	if err == nil {
+		resp.Body.Close()
+		t.Fatalf("expected non-nil error but received: %v", resp.StatusCode)
+	}
+	urlErr, ok := err.(*url.Error)
+	if !ok {
+		t.Fatalf("expected a *url.Error but received: %T -> %v", err, err)
+	}
+	hostErr, ok := urlErr.Err.(x509.HostnameError)
+	if !ok {
+		t.Fatalf("expected a x509.HostnameError but received: %T -> %v", urlErr.Err, urlErr.Err)
+	}
+	if expected := "127.0.0.1"; hostErr.Host != expected {
+		t.Fatalf("expected hostname on error to be %q but found %q", expected, hostErr.Host)
+	}
+
+	// FAIL: Requests that specify a valid hostname but not the CA should
+	// fail
+	tlsConf := &tls.Config{
+		ServerName: "client.regionFoo.nomad",
+	}
+	transport := &http.Transport{TLSClientConfig: tlsConf}
+	client := &http.Client{Transport: transport}
+	req, err := http.NewRequest("GET", reqURL, nil)
+	if err != nil {
+		t.Fatalf("error creating request: %v", err)
+	}
+	resp, err = client.Do(req)
+	if err == nil {
+		resp.Body.Close()
+		t.Fatalf("expected non-nil error but received: %v", resp.StatusCode)
+	}
+	urlErr, ok = err.(*url.Error)
+	if !ok {
+		t.Fatalf("expected a *url.Error but received: %T -> %v", err, err)
+	}
+	_, ok = urlErr.Err.(x509.UnknownAuthorityError)
+	if !ok {
+		t.Fatalf("expected a x509.UnknownAuthorityError but received: %T -> %v", urlErr.Err, urlErr.Err)
+	}
+
+	// FAIL: Requests that specify a valid hostname and CA cert but lack a
+	// client certificate should fail
+	cacertBytes, err := ioutil.ReadFile(cafile)
+	if err != nil {
+		t.Fatalf("error reading cacert: %v", err)
+	}
+	tlsConf.RootCAs = x509.NewCertPool()
+	tlsConf.RootCAs.AppendCertsFromPEM(cacertBytes)
+	req, err = http.NewRequest("GET", reqURL, nil)
+	if err != nil {
+		t.Fatalf("error creating request: %v", err)
+	}
+	resp, err = client.Do(req)
+	if err == nil {
+		resp.Body.Close()
+		t.Fatalf("expected non-nil error but received: %v", resp.StatusCode)
+	}
+	urlErr, ok = err.(*url.Error)
+	if !ok {
+		t.Fatalf("expected a *url.Error but received: %T -> %v", err, err)
+	}
+	opErr, ok := urlErr.Err.(*net.OpError)
+	if !ok {
+		t.Fatalf("expected a *net.OpErr but received: %T -> %v", urlErr.Err, urlErr.Err)
+	}
+	const badCertificate = "tls: bad certificate" // from crypto/tls/alert.go:52 and RFC 5246 § A.3
+	if opErr.Err.Error() != badCertificate {
+		t.Fatalf("expected tls.alert bad_certificate but received: %q", opErr.Err.Error())
+	}
+
+	// PASS: Requests that specify a valid hostname, CA cert, and client
+	// certificate succeed.
+	tlsConf.GetClientCertificate = func(*tls.CertificateRequestInfo) (*tls.Certificate, error) {
+		c, err := tls.LoadX509KeyPair(foocert, fookey)
+		if err != nil {
+			return nil, err
+		}
+		return &c, nil
+	}
+	transport = &http.Transport{TLSClientConfig: tlsConf}
+	client = &http.Client{Transport: transport}
+	req, err = http.NewRequest("GET", reqURL, nil)
+	if err != nil {
+		t.Fatalf("error creating request: %v", err)
+	}
+	resp, err = client.Do(req)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	resp.Body.Close()
+	if resp.StatusCode != 200 {
+		t.Fatalf("expected 200 status code but got: %d", resp.StatusCode)
+	}
+}
+
 // assertIndex tests that X-Nomad-Index is set and non-zero
 func assertIndex(t *testing.T, resp *httptest.ResponseRecorder) {
 	header := resp.Header().Get("X-Nomad-Index")

website/source/docs/agent/configuration/tls.html.md

@@ -54,6 +54,10 @@ the [Agent's Gossip and RPC Encryption](/docs/agent/encryption.html).
   a Nomad client makes the client use TLS for making RPC requests to the Nomad
   servers.
 
+- `verify_https_client` `(bool: false)` - Specifies agents should require
+  client certificates for all incoming HTTPS requests. The client certificates
+  must be signed by the same CA as Nomad.
+
 - `verify_server_hostname` `(bool: false)` - Specifies if outgoing TLS
   connections should verify the server's hostname.
 

website/source/docs/agent/encryption.html.md

@@ -69,9 +69,19 @@ export NOMAD_CACERT=/path/to/ca.pem
 Run any command except `agent` with `-h` to see all environment variables and
 flags. For example: `nomad status -h`
 
-Since HTTPS currently does not validate client certificates you do not need to
+By default HTTPS does not validate client certificates, so you do not need to
 give the command line tool access to any private keys.
 
+### Network Isolation with TLS
+
+If you want to isolate Nomad agents on a network with TLS you need to enable
+both [`verify_https_client`][tls] and [`verify_server_hostname`][tls]. This
+will cause agents to require client certificates for all incoming HTTPS
+connections as well as verify proper names on all other certificates.
+
+Consul will not attempt to health check agents with `verify_https_client` set
+as it is unable to use client certificates.
+
 ## Encryption Examples
 
 ### TLS Configuration using `cfssl`