From 26609bc832b57eae21ccaa41c4e38d9f37f47fcc Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 2 May 2017 16:48:16 -0700 Subject: [PATCH 1/8] Extensively test verify_https_client behavior verify_https_client support added in #2587 --- command/agent/http_test.go | 124 +++++++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+) diff --git a/command/agent/http_test.go b/command/agent/http_test.go index de27c47c4..379bb06a2 100644 --- a/command/agent/http_test.go +++ b/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,125 @@ 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 { + t.Logf("error loading client certificate: %v", err) + return nil, err + } + return &c, nil + } + 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") From 731995e34b79cf42cb92819914cc3158c6d30b0c Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 2 May 2017 17:10:16 -0700 Subject: [PATCH 2/8] Document verify_https_client --- CHANGELOG.md | 2 ++ website/source/docs/agent/configuration/tls.html.md | 4 ++++ website/source/docs/agent/encryption.html.md | 12 +++++++++++- 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 228e7528c..c120c73bc 100644 --- a/CHANGELOG.md +++ b/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] diff --git a/website/source/docs/agent/configuration/tls.html.md b/website/source/docs/agent/configuration/tls.html.md index 80c3042d5..5534eef7a 100644 --- a/website/source/docs/agent/configuration/tls.html.md +++ b/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. diff --git a/website/source/docs/agent/encryption.html.md b/website/source/docs/agent/encryption.html.md index 99068eba7..f21ea8c6f 100644 --- a/website/source/docs/agent/encryption.html.md +++ b/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` From a2d453ad142b2e0edd5ead88b832c3d400faa3d6 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 2 May 2017 17:24:45 -0700 Subject: [PATCH 3/8] Fix error check for consul skip tls verify support --- command/agent/agent.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index 8080591e6..967740ce0 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -691,7 +691,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) } From 77d9b417c1a18320f4cd7606c47700fbac3f2199 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 2 May 2017 17:37:09 -0700 Subject: [PATCH 4/8] Skip https health check if verify_https_client is true --- command/agent/agent.go | 89 ++++++++++++++++------------------- command/agent/agent_test.go | 94 +++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 49 deletions(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index 967740ce0..d09ee4147 100644 --- a/command/agent/agent.go +++ b/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 { diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index 24d45ee8d..c1803d071 100644 --- a/command/agent/agent_test.go +++ b/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{}{} From c1d0de5bd2f2e8335d1d397f0bf3286b01239b31 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Wed, 3 May 2017 13:26:55 -0700 Subject: [PATCH 5/8] Don't reuse transport/client --- command/agent/http_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/command/agent/http_test.go b/command/agent/http_test.go index 379bb06a2..7dc43bbda 100644 --- a/command/agent/http_test.go +++ b/command/agent/http_test.go @@ -447,6 +447,8 @@ func TestHTTP_VerifyHTTPSClient(t *testing.T) { } 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) From e07235e883c0b8e89b4355121232640973801f53 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Wed, 3 May 2017 15:18:48 -0700 Subject: [PATCH 6/8] Adding logging for Travis --- command/agent/http_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/command/agent/http_test.go b/command/agent/http_test.go index 7dc43bbda..968c33802 100644 --- a/command/agent/http_test.go +++ b/command/agent/http_test.go @@ -440,9 +440,10 @@ func TestHTTP_VerifyHTTPSClient(t *testing.T) { // PASS: Requests that specify a valid hostname, CA cert, and client // certificate succeed. tlsConf.GetClientCertificate = func(*tls.CertificateRequestInfo) (*tls.Certificate, error) { + s.Agent.logger.Printf("Loading certificate") c, err := tls.LoadX509KeyPair(foocert, fookey) if err != nil { - t.Logf("error loading client certificate: %v", err) + s.Agent.logger.Printf("error loading client certificate: %v", err) return nil, err } return &c, nil From 5e46d0344851bb45a5ef3c8e40255628070e00bf Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Thu, 4 May 2017 17:04:59 -0700 Subject: [PATCH 7/8] Bump Travis to latest Go 1.8.x --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index d89e20c07..e6618a805 100644 --- a/.travis.yml +++ b/.travis.yml @@ -7,7 +7,7 @@ services: language: go go: - - 1.8 + - 1.8.x branches: only: From d1d34bf0196abbaee9923bc69d3cef7ce90ca24e Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Thu, 4 May 2017 17:35:54 -0700 Subject: [PATCH 8/8] Remove extra Travis logging --- command/agent/http_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/command/agent/http_test.go b/command/agent/http_test.go index 968c33802..90c4eb9bb 100644 --- a/command/agent/http_test.go +++ b/command/agent/http_test.go @@ -440,10 +440,8 @@ func TestHTTP_VerifyHTTPSClient(t *testing.T) { // PASS: Requests that specify a valid hostname, CA cert, and client // certificate succeed. tlsConf.GetClientCertificate = func(*tls.CertificateRequestInfo) (*tls.Certificate, error) { - s.Agent.logger.Printf("Loading certificate") c, err := tls.LoadX509KeyPair(foocert, fookey) if err != nil { - s.Agent.logger.Printf("error loading client certificate: %v", err) return nil, err } return &c, nil