Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add verify server hostname to tls default #17155

Merged
merged 7 commits into from
Jul 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/17155.txt
Original file line number Diff line number Diff line change
@@ -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.
```
11 changes: 7 additions & 4 deletions agent/config/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
111 changes: 108 additions & 3 deletions agent/config/runtime_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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
},
})
Copy link
Collaborator

@huikang huikang Apr 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we add a test case testing that tls.internal_rpc.verify_server_hostname overrides tls.defaults.verify_server_hostname?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fulviodenza , I am not sure if this test case has been added. Could you please confirm this? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, sorry, didn't push a change. Now should have been added

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{
Expand Down
15 changes: 8 additions & 7 deletions website/content/docs/agent/config/config-files.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -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.<datacenter>.<domain>`. By default this is false,
and Consul does not verify the hostname of the certificate, only that it
is signed by a trusted CA.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to add Overrides [tls.defaults.verify_server_hostname](#tls_defaults_verify_server_hostname). at line 2187.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, thanks, I pushed the change

**WARNING: TLS 1.1 and lower are generally considered less secure and
should not be used if possible.**

Expand Down Expand Up @@ -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.<datacenter>.<domain>`. By default this is false,
and Consul does not verify the hostname of the certificate, only that it
Expand Down Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the diligence in updating these config examples in the docs :)

},
"internal_rpc": {
"verify_outgoing": true,
fulviodenza marked this conversation as resolved.
Show resolved Hide resolved
"verify_server_hostname": true
}
}
Expand Down
3 changes: 0 additions & 3 deletions website/content/docs/agent/index.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down
6 changes: 0 additions & 6 deletions website/content/docs/security/security-models/core.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand All @@ -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
}
}
Expand Down