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

Closed
nightkr opened this issue Jan 24, 2024 · 8 comments
Closed

Broken TLS Quorum hostname verification #760

nightkr opened this issue Jan 24, 2024 · 8 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 moved this to Refinement: In Progress in Stackable Engineering May 24, 2024
@nightkr nightkr self-assigned this May 24, 2024
@fhennig
Copy link
Contributor

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.

@nightkr nightkr moved this from Refinement: In Progress to Development: In Progress in Stackable Engineering May 30, 2024
@nightkr
Copy link
Member Author

nightkr commented Jun 19, 2024

Turns out, enabling "FIPS mode" on ZK 3.8.2+ already "solves" this, and it is on by default on 3.9.0+.

I've submitted a PR for a more specific toggle as apache/zookeeper#2173. Moving this to track as we're now waiting for upstream.

@nightkr
Copy link
Member Author

nightkr commented Jun 26, 2024

Closing since FIPS mode is available in all our supported versions, and on by default in all non-deprecated versions.

Moved the followup work into #829 instead.

@nightkr nightkr closed this as completed Jun 26, 2024
@lfrancke lfrancke moved this from Development: Track to Done in Stackable Engineering Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

No branches or pull requests

4 participants