fix: For cluster internal scopes also add variant without trailing dot #547

sbernauer · 2025-01-21T09:20:15Z

Description

Please add a description here. This will become the commit message of the merge request later.

Definition of Done Checklist

Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
Please make sure all these things are done and tick the boxes

Author

Give feedback

Changes are OpenShift compatible
CRD changes approved
CRD documentation for all fields, following the style guide.
Helm chart can be installed and deployed operator works
Integration tests passed (for non trivial changes)
Changes need to be "offline" compatible
Options

Reviewer

Give feedback

Code contains useful comments
Code contains useful logging statements
(Integration-)Test cases added
Documentation added or updated. Follows the style guide.
Changelog updated
Cargo.toml only contains references to git tags (not specific commits or branches)
Options

Acceptance

Give feedback

Feature Tracker has been updated
Proper release label has been added
Roadmap has been updated
Options

nightkr

We should fix the places that depend on the old value here, not just blindly add both.

nightkr · 2025-01-21T10:55:17Z

rust/operator-binary/src/backend/mod.rs

+        let mut cluster_domains = vec![cluster_domain.to_string()];
+        if let Some(cluster_domain_without_trailing_dot) = cluster_domain.strip_suffix('.') {
+            cluster_domains.push(cluster_domain_without_trailing_dot.to_owned());
+        }


In my testing, Kerberos wants consistency and TLS doesn't really care. Either should be helped by doing both.

Tried to respond in #547 (comment)

tests/templates/kuttl/cert-manager-tls/consumer.yaml

nightkr · 2025-01-21T10:58:54Z

rust/operator-binary/src/backend/kerberos_keytab.rs

 [domain_realm]
 cluster.local = {realm_name}
+cluster.local. = {realm_name}
 .cluster.local = {realm_name}
+.cluster.local. = {realm_name}


IME, this shouldn't be necessary at all (probably since we set the default realm before). But if we do keep it then we should read the actual cluster domain, not hard-code cluster.local specifically.

Good catch, this PR was mostly about trying out if we can fix the TLS cert problems we have.
Fixed it in 5781e66

sbernauer · 2025-01-21T15:13:30Z

General comment on why we are thinking of adding both (with and wo trailing dot) to scopes - both for TLS and Kerberos:

We don't know what users will be entering. Performance sensitive users might choose a trailing dot, users with problems might not be able to add a dot
The zookeeper ./scripts/run-tests --test smoke_zookeeper-3.9.2_use-server-tls-true_use-client-auth-tls-true_openshift-false tests failed with [ERROR] Could not establish secure connection using client certificates!
Everything was configured correctly with trailing dots. We could only get the zk client happy by adding a SAN without the dot
Let's imaging a 24.11 users first updates secret-op. Until he is able to update all other product operators everything can potentially break, because every talks cluster.local, but secret-op only hands out cluster.local.
Is there a downside of adding both? - At the first glance this PR seems like a "let's be better safe than sorry"

That being said this is a WIP, I would leave it totally up on @dervoeti and @maltesander to decide how to proceed, as they looked at the issue at the first place. I just happened to bump op-rs and run into a failing test

nightkr · 2025-01-21T16:17:34Z

We don't know what users will be entering. Performance sensitive users might choose a trailing dot, users with problems might not be able to add a dot

Wouldn't secret-operator know just as well as any other operator? Are you planning on supporting mixed environments?

We could only get the zk client happy by adding a SAN without the dot

curl was also happy IME too without the dot, so I guess this is one indication that TLS SANs should never have it.

Let's imaging a 24.11 users first updates secret-op. Until they are able to update all other product operators everything can potentially break, because every talks cluster.local, but secret-op only hands out cluster.local.

Migration is a fair concern, that's true. We should explicitly document those migration paths in the comments.

Is there a downside of adding both? - At the first glance this PR seems like a "let's be better safe than sorry"

We should know what credentials we're asking for, and why. Whether they need to be included when provisioning manually, and so on.

It's fine to add things to that list, it just shouldn't be something we do blindly.

dervoeti · 2025-01-21T16:36:17Z

I agree that, in general, we should prefer not adding the hostname without the dot if it's not really necessary / we can work around it.

I'm not sure what exactly the scenario was (@sbernauer and/or @maltesander did the research on this) but I think one reason was that Zookeeper does a reverse DNS lookup on the client IP and complains that the client cert is not valid for the returned hostname (without the trailing dot).

That would be a reason to add the alternative hostname to the SANs. Other ways to solve this are trying to fix this in Zookeeper or maybe explicitly not supporting Zookeeper mTLS if you use a cluster domain with a trailing dot. I'm fine with either solution, adding the alternative hostname to the SAN was probably just the easiest way make it work.

dervoeti · 2025-02-06T14:07:00Z

So, we have to make a decision. I can't really comment on the Kerberos related changes, but as far as I understand it, they are not strictly necessary but would make migration easier. In that case I would be fine with not merging these changes if they are controversial, an easy migration path is nice to have but I think it's okay if we don't have it.
Regarding the TLS related change, I can't see a practical negative effect on security if we add the non-FQDN DNS name to the SANs if a cert for a FQDN DNS name is requested. It would ease the migration path (at least in one direction) and, more importantly, it would fix the Zookeeper mTLS issue.
So I'd be in favor of merging at least the TLS related change.

But I'm also fine with not merging this at all and explicitly listing Zookeeper mTLS as "known not to work with FQDN cluster domains yet". In that case we probably still support many setups with FQDN cluster domains with 25.3, so it's better than before.

Opinions @nightkr @sbernauer @maltesander ?

nightkr · 2025-02-06T14:10:05Z

For TLS only the non-FQDN variant seems to matter at all, at least in my testing. We should only keep the non-FQDN variant there.

For Kerberos I'm not sure. I think the argument for having both makes sense, at least during the transitional period (though we should probably make sure we have both variants in both cases). Maybe an exception here would be if we can centralize this logic in listener-op, and have that be what decides the Flag Day™️.

dervoeti · 2025-02-06T15:10:28Z

For TLS only the non-FQDN variant seems to matter at all, at least in my testing. We should only keep the non-FQDN variant there.

I'll also do some tests with this later, if it works I'm fine with that solution as well.

maltesander · 2025-02-07T10:36:56Z

For TLS only the non-FQDN variant seems to matter at all, at least in my testing. We should only keep the non-FQDN variant there.

Yeah we definitly need the non-FQDN in there. That fixed most of the problems i had. IIRC zookeeper required the FQDN in the certificate.

I would punt on Kerberos as well. Main thing is to fix the certs?

dervoeti · 2025-02-07T10:46:21Z

I did some tests with Zookeeper yesterday, including mTLS tests with and without FQDN cluster domains, adding just the non-FQDN hostname to the SANs worked fine. Will do some more testing today with other products.
I only changed one line in secret-op: b5f3d51

dervoeti · 2025-02-14T10:39:28Z

@maltesander @nightkr @sbernauer I created a PR that only adds the non-FQDN variant to the SANs, works fine for me:
#564

fix: For cluster internal scopes, also add variant without trailing dot

32682f2

sbernauer mentioned this pull request Jan 21, 2025

Roll out stackable-operator 0.84.0/0.84.1 stackabletech/operator-rs#942

Closed

sbernauer changed the title ~~fix: For cluster internal scopes, also add variant without trailing dot~~ fix: For cluster internal scopes also add variant without trailing dot Jan 21, 2025

Add test

6ea489b

nightkr reviewed Jan 21, 2025

View reviewed changes

sbernauer added 3 commits January 21, 2025 15:13

Fix tests

d4f4ad7

fix: Use correct cluster domains in Kerberos config

5781e66

Revert docs and stuff

ef6e21c

sbernauer mentioned this pull request Feb 14, 2025

feat: only add non-FQDN variant of fully qualified domain names to SANs #564

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: For cluster internal scopes also add variant without trailing dot #547

fix: For cluster internal scopes also add variant without trailing dot #547

sbernauer commented Jan 21, 2025

Author

Reviewer

Acceptance

nightkr left a comment

nightkr Jan 21, 2025

sbernauer Jan 21, 2025

nightkr Jan 21, 2025

sbernauer Jan 21, 2025

sbernauer commented Jan 21, 2025

nightkr commented Jan 21, 2025

dervoeti commented Jan 21, 2025

dervoeti commented Feb 6, 2025

nightkr commented Feb 6, 2025 •

edited

Loading

dervoeti commented Feb 6, 2025

maltesander commented Feb 7, 2025

dervoeti commented Feb 7, 2025

dervoeti commented Feb 14, 2025

fix: For cluster internal scopes also add variant without trailing dot #547

Are you sure you want to change the base?

fix: For cluster internal scopes also add variant without trailing dot #547

Conversation

sbernauer commented Jan 21, 2025

Description

Definition of Done Checklist

Author

Reviewer

Acceptance

nightkr left a comment

Choose a reason for hiding this comment

nightkr Jan 21, 2025

Choose a reason for hiding this comment

sbernauer Jan 21, 2025

Choose a reason for hiding this comment

nightkr Jan 21, 2025

Choose a reason for hiding this comment

sbernauer Jan 21, 2025

Choose a reason for hiding this comment

sbernauer commented Jan 21, 2025

nightkr commented Jan 21, 2025

dervoeti commented Jan 21, 2025

dervoeti commented Feb 6, 2025

nightkr commented Feb 6, 2025 • edited Loading

dervoeti commented Feb 6, 2025

maltesander commented Feb 7, 2025

dervoeti commented Feb 7, 2025

dervoeti commented Feb 14, 2025

nightkr commented Feb 6, 2025 •

edited

Loading