Add verify server hostname to tls default by fulviodenza · Pull Request #17155 · hashicorp/consul

fulviodenza · 2023-04-26T17:57:55Z

Description

This PR solves Add verify_server_hostname to tls.defaults #17095
The PR improves the UX allowing the users to default the verify_server_hostname value avoiding anti-intuitive configurations such as

tls {
  defaults {
    key_file = "/etc/pki/tls/private/my.key"
    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_server_hostname = true
  }
}

now the internal_rpc is not needed anymore in this configuration and can be written as follows:

tls {
  defaults {
    key_file = "/etc/pki/tls/private/my.key"
    cert_file = "/etc/pki/tls/certs/my.crt"
    ca_file = "/etc/pki/tls/certs/ca-bundle.crt"
    verify_incoming = true
    verify_outgoing = true
    verify_server_hostname = true
  }
}

as @jkirschner-hashicorp suggested in #17095 (comment)

Testing & Reproduction steps

Unit test in runtime_test

Links

#17095

PR Checklist

updated test coverage
external facing docs updated
appropriate backport labels added
not a security concern

agent/config/builder.go

jkirschner-hashicorp · 2023-04-26T18:26:50Z

website/content/docs/agent/config/config-files.mdx

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

pglass

This looks great! Just some minor nits/comments.

.changelog/17155.txt

agent/config/builder.go

website/content/docs/agent/config/config-files.mdx

agent/config/builder.go

huikang

Shall we update the config doc at

Add verify_server_hostname to

consul/website/content/docs/agent/config/config-files.mdx

Line 2041 in 5eaeb7b

- `defaults` ((#tls_defaults)) Provides default settings that will be applied

Add the override note at

consul/website/content/docs/agent/config/config-files.mdx

Lines 2181 to 2185 in 5eaeb7b

    
           - `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.

huikang · 2023-04-26T23:33:22Z

agent/config/runtime_test.go

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

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

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

kisunji

huikang · 2023-04-28T17:51:47Z

website/content/docs/agent/config/config-files.mdx

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

you're right, thanks, I pushed the change

huikang · 2023-04-29T04:07:34Z

@fulviodenza the CI is failing due to a docker login. We are removing the step in another PR. Please rebase this one, once it's done.

fulviodenza · 2023-05-08T19:46:45Z

Hey @huikang I've seen the pr has been merged in main, shall we move forward with this?

jkirschner-hashicorp · 2023-05-12T20:19:59Z

Hi @fulviodenza,

It seems like some of the tests are still failing due to CI login failures for PRs submitted by contributors outside the HashiCorp org (nothing to do with the code you changed). It's something we'll need to look into.

johnlanda · 2023-05-23T15:59:38Z

agent/config/builder.go

Not sure what folks prefer for styling but we could get rid of the else block by setting the default first. IMO this is a bit easier to read.

c.InternalRPC.VerifyServerHostname = boolVal(t.Defaults.VerifyServerHostname) if t.InternalRPC.VerifyServerHostname != nil { c.InternalRPC.VerifyServerHostname = boolVal(t.InternalRPC.VerifyServerHostname) }

jkirschner-hashicorp · 2023-05-23T19:10:33Z

Hi @fulviodenza,

Once we've addressed the CI issue, we'll let you know. Until then, merging the latest changes from main into this PR won't change anything, unfortunately :(

Thank you for your patience!

david-yu · 2023-07-08T02:26:46Z

It looks like all tests passed. Should we go ahead and merge @pglass ?

fulviodenza requested a review from a team as a code owner April 26, 2023 17:57

github-actions bot added theme/config Relating to Consul Agent configuration, including reloading type/docs Documentation needs to be created/updated/clarified labels Apr 26, 2023

jkirschner-hashicorp reviewed Apr 26, 2023

View reviewed changes

agent/config/builder.go Outdated Show resolved Hide resolved

jkirschner-hashicorp reviewed Apr 26, 2023

View reviewed changes

jkirschner-hashicorp requested review from a team, kisunji and pglass and removed request for a team April 26, 2023 18:30

fulviodenza marked this pull request as draft April 26, 2023 19:27

fulviodenza marked this pull request as ready for review April 26, 2023 20:51

pglass reviewed Apr 26, 2023

View reviewed changes

pglass added the pr/no-backport label Apr 26, 2023

fulviodenza requested review from jkirschner-hashicorp and pglass April 26, 2023 22:08

pglass approved these changes Apr 26, 2023

View reviewed changes

huikang reviewed Apr 26, 2023

View reviewed changes

fulviodenza requested a review from huikang April 27, 2023 07:49

kisunji approved these changes Apr 28, 2023

View reviewed changes

huikang reviewed Apr 28, 2023

View reviewed changes

fulviodenza requested a review from huikang April 28, 2023 21:47

huikang approved these changes Apr 29, 2023

View reviewed changes

fulviodenza added 5 commits May 8, 2023 19:29

Support VerifyServerHostname on tls.default

d608f6c

Update docs

f2f28e1

Add test

e424fe8

Add testcase

7637f10

Add changelog

1be54d3

johnlanda reviewed May 23, 2023

View reviewed changes

fulviodenza requested a review from a team July 9, 2023 11:58

fulviodenza requested a review from a team as a code owner July 9, 2023 11:58

fulviodenza requested review from randyhdev and sarahethompson and removed request for a team July 9, 2023 11:58

fulviodenza added 2 commits July 9, 2023 14:03

Remove else block

c628acc

Merge branch 'main' into add-verify_server_hostname-to-tls-default

76281e2

pglass merged commit f4b0804 into hashicorp:main Jul 10, 2023

fulviodenza deleted the add-verify_server_hostname-to-tls-default branch July 10, 2023 15:40

david-yu mentioned this pull request Jul 10, 2023

Add verify_server_hostname to tls.defaults #17095

Closed

hc-github-team-consul-core added a commit that referenced this pull request Jul 21, 2023

	- `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.

Conversation

fulviodenza commented Apr 26, 2023

Description

Testing & Reproduction steps

Links

PR Checklist

Uh oh!

Uh oh!

jkirschner-hashicorp Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

pglass left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

huikang left a comment

Choose a reason for hiding this comment

Uh oh!

huikang Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huikang Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

fulviodenza Apr 28, 2023

Choose a reason for hiding this comment

Uh oh!

kisunji left a comment

Choose a reason for hiding this comment

Uh oh!

huikang Apr 28, 2023

Choose a reason for hiding this comment

Uh oh!

fulviodenza Apr 28, 2023

Choose a reason for hiding this comment

Uh oh!

huikang commented Apr 29, 2023

Uh oh!

fulviodenza commented May 8, 2023

Uh oh!

jkirschner-hashicorp commented May 12, 2023

Uh oh!

johnlanda May 23, 2023

Choose a reason for hiding this comment

Uh oh!

jkirschner-hashicorp commented May 23, 2023

Uh oh!

david-yu commented Jul 8, 2023

Uh oh!

huikang Apr 26, 2023 •

edited

Loading