From f4b08040fd3d3ac73d7c73dd0f41b38eefb8c6f3 Mon Sep 17 00:00:00 2001 From: Fulvio Date: Mon, 10 Jul 2023 17:34:41 +0200 Subject: [PATCH] Add verify server hostname to tls default (#17155) --- .changelog/17155.txt | 3 + agent/config/builder.go | 11 +- agent/config/runtime_test.go | 111 +++++++++++++++++- .../docs/agent/config/config-files.mdx | 15 +-- website/content/docs/agent/index.mdx | 3 - .../docs/security/security-models/core.mdx | 6 - 6 files changed, 126 insertions(+), 23 deletions(-) create mode 100644 .changelog/17155.txt diff --git a/.changelog/17155.txt b/.changelog/17155.txt new file mode 100644 index 0000000000..03cec33e99 --- /dev/null +++ b/.changelog/17155.txt @@ -0,0 +1,3 @@ +```release-note:improvement +config: Add new `tls.defaults.verify_server_hostname` configuration option. This specifies the default value for any interfaces that support the `verify_server_hostname` option. +``` diff --git a/agent/config/builder.go b/agent/config/builder.go index 6acd1b0039..98bac1711c 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -2653,10 +2653,10 @@ func (b *builder) buildTLSConfig(rt RuntimeConfig, t TLS) (tlsutil.Config, error return c, errors.New("verify_outgoing is not valid in the tls.grpc stanza") } - // Similarly, only the internal RPC configuration honors VerifyServerHostname + // Similarly, only the internal RPC and defaults configuration honor VerifyServerHostname // so we call it out here too. - if t.Defaults.VerifyServerHostname != nil || t.GRPC.VerifyServerHostname != nil || t.HTTPS.VerifyServerHostname != nil { - return c, errors.New("verify_server_hostname is only valid in the tls.internal_rpc stanza") + if t.GRPC.VerifyServerHostname != nil || t.HTTPS.VerifyServerHostname != nil { + return c, errors.New("verify_server_hostname is only valid in the tls.defaults and tls.internal_rpc stanzas") } // And UseAutoCert right now only applies to external gRPC interface. @@ -2706,8 +2706,11 @@ func (b *builder) buildTLSConfig(rt RuntimeConfig, t TLS) (tlsutil.Config, error } mapCommon("internal_rpc", t.InternalRPC, &c.InternalRPC) - c.InternalRPC.VerifyServerHostname = boolVal(t.InternalRPC.VerifyServerHostname) + c.InternalRPC.VerifyServerHostname = boolVal(t.Defaults.VerifyServerHostname) + if t.InternalRPC.VerifyServerHostname != nil { + c.InternalRPC.VerifyServerHostname = boolVal(t.InternalRPC.VerifyServerHostname) + } // Setting only verify_server_hostname is documented to imply verify_outgoing. // If it doesn't then we risk sending communication over plain TCP when we // documented it as forcing TLS for RPCs. Enforce this here rather than in diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index cc5451804d..b18a631624 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -2736,7 +2736,44 @@ func TestLoad_IntegrationWithFlags(t *testing.T) { } } `}, - expectedErr: "verify_server_hostname is only valid in the tls.internal_rpc stanza", + expected: func(rt *RuntimeConfig) { + rt.DataDir = dataDir + rt.TLS.InternalRPC.VerifyServerHostname = true + rt.TLS.InternalRPC.VerifyOutgoing = true + }, + }) + run(t, testCase{ + desc: "verify_server_hostname in the defaults stanza and internal_rpc", + args: []string{ + `-data-dir=` + dataDir, + }, + hcl: []string{` + tls { + defaults { + verify_server_hostname = false + }, + internal_rpc { + verify_server_hostname = true + } + } + `}, + json: []string{` + { + "tls": { + "defaults": { + "verify_server_hostname": false + }, + "internal_rpc": { + "verify_server_hostname": true + } + } + } + `}, + expected: func(rt *RuntimeConfig) { + rt.DataDir = dataDir + rt.TLS.InternalRPC.VerifyServerHostname = true + rt.TLS.InternalRPC.VerifyOutgoing = true + }, }) run(t, testCase{ desc: "verify_server_hostname in the grpc stanza", @@ -2759,7 +2796,7 @@ func TestLoad_IntegrationWithFlags(t *testing.T) { } } `}, - expectedErr: "verify_server_hostname is only valid in the tls.internal_rpc stanza", + expectedErr: "verify_server_hostname is only valid in the tls.defaults and tls.internal_rpc stanza", }) run(t, testCase{ desc: "verify_server_hostname in the https stanza", @@ -2782,7 +2819,7 @@ func TestLoad_IntegrationWithFlags(t *testing.T) { } } `}, - expectedErr: "verify_server_hostname is only valid in the tls.internal_rpc stanza", + expectedErr: "verify_server_hostname is only valid in the tls.defaults and tls.internal_rpc stanza", }) run(t, testCase{ desc: "translated keys", @@ -5723,6 +5760,74 @@ func TestLoad_IntegrationWithFlags(t *testing.T) { rt.TLS.InternalRPC.VerifyOutgoing = true }, }) + run(t, testCase{ + desc: "tls.defaults.verify_server_hostname implies tls.internal_rpc.verify_outgoing", + args: []string{ + `-data-dir=` + dataDir, + }, + json: []string{` + { + "tls": { + "defaults": { + "verify_server_hostname": true + } + } + } + `}, + hcl: []string{` + tls { + defaults { + verify_server_hostname = true + } + } + `}, + expected: func(rt *RuntimeConfig) { + rt.DataDir = dataDir + + rt.TLS.Domain = "consul." + rt.TLS.NodeName = "thehostname" + + rt.TLS.InternalRPC.VerifyServerHostname = true + rt.TLS.InternalRPC.VerifyOutgoing = true + }, + }) + run(t, testCase{ + desc: "tls.internal_rpc.verify_server_hostname overwrites tls.defaults.verify_server_hostname", + args: []string{ + `-data-dir=` + dataDir, + }, + json: []string{` + { + "tls": { + "defaults": { + "verify_server_hostname": false + }, + "internal_rpc": { + "verify_server_hostname": true + } + } + } + `}, + hcl: []string{` + tls { + defaults { + verify_server_hostname = false + }, + internal_rpc { + verify_server_hostname = true + } + } + `}, + expected: func(rt *RuntimeConfig) { + rt.DataDir = dataDir + + rt.TLS.Domain = "consul." + rt.TLS.NodeName = "thehostname" + + rt.TLS.InternalRPC.VerifyServerHostname = true + rt.TLS.InternalRPC.VerifyOutgoing = true + }, + }) run(t, testCase{ desc: "tls.grpc.use_auto_cert defaults to false", args: []string{ diff --git a/website/content/docs/agent/config/config-files.mdx b/website/content/docs/agent/config/config-files.mdx index 1b382341e4..8d46b63bd0 100644 --- a/website/content/docs/agent/config/config-files.mdx +++ b/website/content/docs/agent/config/config-files.mdx @@ -2094,6 +2094,12 @@ specially crafted certificate signed by the CA can be used to gain full access t * `TLSv1_2` (default) * `TLSv1_3` + - `verify_server_hostname` ((#tls_internal_rpc_verify_server_hostname)) When + set to true, Consul verifies the TLS certificate presented by the servers + match the hostname `server..`. By default this is false, + and Consul does not verify the hostname of the certificate, only that it + is signed by a trusted CA. + **WARNING: TLS 1.1 and lower are generally considered less secure and should not be used if possible.** @@ -2201,7 +2207,7 @@ specially crafted certificate signed by the CA can be used to gain full access t only way to enforce that no client can communicate with a server unencrypted is to also enable `verify_incoming` which requires client certificates too. - - `verify_server_hostname` ((#tls_internal_rpc_verify_server_hostname)) When + - `verify_server_hostname` Overrides [tls.defaults.verify_server_hostname](#tls_defaults_verify_server_hostname). When set to true, Consul verifies the TLS certificate presented by the servers match the hostname `server..`. By default this is false, and Consul does not verify the hostname of the certificate, only that it @@ -2285,9 +2291,6 @@ tls { ca_file = "/etc/pki/tls/certs/ca-bundle.crt" verify_incoming = true verify_outgoing = true - } - - internal_rpc { verify_server_hostname = true } } @@ -2316,9 +2319,7 @@ tls { "cert_file": "/etc/pki/tls/certs/my.crt", "ca_file": "/etc/pki/tls/certs/ca-bundle.crt", "verify_incoming": true, - "verify_outgoing": true - }, - "internal_rpc": { + "verify_outgoing": true, "verify_server_hostname": true } } diff --git a/website/content/docs/agent/index.mdx b/website/content/docs/agent/index.mdx index ec68e0a1ce..b5a06b39e6 100644 --- a/website/content/docs/agent/index.mdx +++ b/website/content/docs/agent/index.mdx @@ -276,9 +276,6 @@ tls { ca_file = "/consul/config/certs/consul-agent-ca.pem" cert_file = "/consul/config/certs/dc1-server-consul-0.pem" key_file = "/consul/config/certs/dc1-server-consul-0-key.pem" - } - - internal_rpc { verify_server_hostname = true } } diff --git a/website/content/docs/security/security-models/core.mdx b/website/content/docs/security/security-models/core.mdx index 92a5c1ac91..2b6bb0515d 100644 --- a/website/content/docs/security/security-models/core.mdx +++ b/website/content/docs/security/security-models/core.mdx @@ -128,9 +128,6 @@ environment and adapt these configurations accordingly. ca_file = "consul-agent-ca.pem" cert_file = "dc1-server-consul-0.pem" key_file = "dc1-server-consul-0-key.pem" - } - - internal_rpc { verify_server_hostname = true } } @@ -148,9 +145,6 @@ environment and adapt these configurations accordingly. verify_incoming = false verify_outgoing = true ca_file = "consul-agent-ca.pem" - } - - internal_rpc { verify_server_hostname = true } }