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

Broken TLS Quorum hostname verification #760

Open
nightkr opened this issue Jan 24, 2024 · 6 comments
Open

Broken TLS Quorum hostname verification #760

nightkr opened this issue Jan 24, 2024 · 6 comments
Assignees
Labels

Comments

@nightkr
Copy link
Member

nightkr commented Jan 24, 2024

Currently, enabling Quorum TLS will make the server validate SANs client certificates of connecting quorum peers against their reverse DNS address. This is less-than-helpful for two reasons:

  1. ZK pods' IP addresses will resolve to a hostname per service it participates in, only one of which is in the certificate SAN (zk-server-default-0.zk-server-default.default.svc.cluster.local is in SAN, 1-2-3-4.zk.default.svc.cluster.local is not in SAN).
  2. "This certificate matches the connecting peer" does not mean "this peer should be allowed to connect".

Instead, the ZK server should verify the SAN against the list of servers (servers.N in the config). A peer should be able to connect on the quorum port if and only if at least one SAN matches at least one of the listed servers.

Additionally, it would be nice to have a "disable client hostname verification" option that still leaves server hostname verification enabled.

Both of these would need to be implemented upstream.

@lfrancke
Copy link
Member

@nightkr nightkr self-assigned this May 24, 2024
@fhennig
Copy link
Member

fhennig commented May 27, 2024

Is there more to be refined? Are we going to do these upstream changes or is it enough to have reported the problem?

@nightkr
Copy link
Member Author

nightkr commented May 29, 2024

IMO it's fine to end up shipping a patch for now, but we should try to get it upstreamed.

@nightkr
Copy link
Member Author

nightkr commented May 29, 2024

The question here is.. do we just want to disable the mostly-useless old behaviour (verifying a strong identity (X509) against a weak one (IP/DNS)), or do we want to do the "correct" validation here (verifying that the strong identity (X509) matches who we expect to connect)?

@nightkr
Copy link
Member Author

nightkr commented May 30, 2024

Having looked through the codebase a bit more, authz seems like a somewhat bigger change.

For now, I'd prioritize disabling the DNS-based client hostname verification, and then break out authz into a separate issue.

@nightkr
Copy link
Member Author

nightkr commented May 30, 2024

Created #820 for the latter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Development: In Progress
Development

No branches or pull requests

4 participants