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

security: Make inter-node certificate hostname verification optional #19265

Closed
bdarnell opened this issue Oct 15, 2017 · 3 comments
Closed

security: Make inter-node certificate hostname verification optional #19265

bdarnell opened this issue Oct 15, 2017 · 3 comments
Assignees
Labels
A-kv-client Relating to the KV client and the KV interface. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Milestone

Comments

@bdarnell
Copy link
Contributor

I think that allowing CA-only verification of inter-node TLS connections (ignoring hostnames) would be a useful "semi-secure" mode. This would be operationally easier than the current strict verification and I think it would still provide a useful level of security (unlike the "security theater" of using passwords on insecure pgwire connections while leaving the GRPC interface wide open).

The operational difficulties that would be avoided with this option are

  • It's difficult to know which hostnames/IPs to put in the certs. Our docs default to "use every name you can think of" and it can be difficult to know which names apply (security: certificate verification only uses short hostname #4434).
  • Many admins are comfortable with "shared secret" deployments in which the same password (or equivalent) gets copied to every node. An equivalent deployment of cockroach would use the same certificate on every node, but this is made complicated by the need to know all addresses of nodes that might use the cert (or use very broad wildcards).

The security risks are

  • MITM attacks become possible due to the lack of hostname revocation
  • Revoking individual certs is impossible; in the event of a compromise all nodes will need cert changes. (This is more of a theoretical concern since we don't yet support CRLs or OCSP)
  • The shared secret store is a potential point of compromise (in comparison to a CSR-based workflow in which keys never leave their nodes)

This mode will not be appropriate for all deployments, but I think we may be able to reduce the usage of --insecure mode if this option were available. A drawback of this change would be that it takes up documentation space and may also draw people away from the more secure (mitm-proof) modes.

More discussion in https://news.ycombinator.com/item?id=15460925. cc @mberhault

@mberhault mberhault self-assigned this Oct 15, 2017
@mberhault
Copy link
Contributor

I generally agree with this change as a mid-point between secure and insecure.

The main issue here is that to keep verifying the certificate chain but bypass the hostname verification, we'll need to plumb things at a lower level. eg: by using tls.Client directory rather than tls.Dial. Although even that may break at some point: https://groups.google.com/forum/#!topic/golang-nuts/4vnt7NdLvVU

@bdarnell bdarnell added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-kv-client Relating to the KV client and the KV interface. labels Apr 26, 2018
@toppk
Copy link

toppk commented Apr 30, 2018

If cockroach were to follow SNI, then there is no ambiguiity. as any hostname is labled with an explicate DNS or IP context for implementation.

@tbg tbg added this to the Later milestone Jul 22, 2018
@mberhault
Copy link
Contributor

Anything we do here would require the user of InsecureSkipVerify in the TLS config. This is too dangerous to use in secure mode. Skipping server name verification fits more with a shared-secret deployment mode as discussed in #16188

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-kv-client Relating to the KV client and the KV interface. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

No branches or pull requests

4 participants