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.defaults #17095

Closed
blmhemu opened this issue Apr 24, 2023 · 5 comments
Closed

Add verify_server_hostname to tls.defaults #17095

blmhemu opened this issue Apr 24, 2023 · 5 comments

Comments

@blmhemu
Copy link

blmhemu commented Apr 24, 2023

Feature Description

When configuring consul, it is not immediately obvious that we need to fiddle with verify_server_hostname which is present only under tls.internal_rpc. Adding it to tls.defaults makes all the verify_* config be grouped in the defaults section.

Use Case(s)

Ease of use / config / discoverability.

@huikang
Copy link
Collaborator

huikang commented Apr 24, 2023

@blmhemu , thanks reporting this. I think the proposal makes sense, given other verify_* can override the values in tls.defaults
Screen Shot 2023-04-24 at 6 59 05 PM

But I will leave this to our PM to decide @jkirschner-hashicorp

@jkirschner-hashicorp
Copy link
Contributor

@huikang : I agree this change would simplify the UX.

Currently, the standard configuration looks something like this:

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
  }
}

With the change, there would be no need for the internal_rpc stanza:

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
  }
}

One could argue that it's strange to set a default that only applies to one interface (internal RPC) rather than all 3 (internal RPC, gRPC, HTTPS). However, we already have precedence for defaults that only apply to a subset of the interfaces: verify_outgoing is seemingly not used in gRPC, but can still be set in tls.defaults.

@fulviodenza
Copy link
Contributor

fulviodenza commented Apr 25, 2023

I think the change makes sense, if needed I can work on it.
@jkirschner-hashicorp @huikang

@david-yu
Copy link
Contributor

The #17155 PR was merged and is scheduled to be released in 1.17.0

@david-yu
Copy link
Contributor

Will go ahead and close this issue. Thanks @fulviodenza for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants