-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Offer dialback when domain is not valid. #3042
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
base: main
Are you sure you want to change the base?
Conversation
This commit adds a configuration option, "xmpp.server.dialback.offer", that can be used to control if dialback stream feature should be offered. One usecase is where openfire is used as a gateway between XMPP domains ("trunking") - https://download.igniterealtime.org/openfire/docs/latest/documentation/trunking-guide.html In situations where dialback has to be enabled in the gateway, it also forces the server(s) behind the gateway to use dialback. Before this commit, the code did not add dialback unless the certificates are selfsigned. Which means that a session to a remote domain via the gateway will not be established both ways due to lack of dialback, when the certificates are signed by CAs and trust exists. With "xmpp.server.dialback.offer" set to "true", dialback will be added as a stream feature and sessions can be established in both directions.
- Adding a checkbox that enables/disables dialback - Adding a checkbox for offering dialback even if SASL fails - for openfire trunking - Add a warning if both peer certificate verification and dialback are disabled
I'm a bit confused about how these two checkboxes are intended to work together:
Aren’t these two options essentially opposites? For context: Openfire supports two authentication mechanisms for server-to-server communication, both of which are fairly standard among XMPP servers:
Before this PR, the admin console allowed explicit configuration of SASL EXTERNAL using the Disabled / Wanted / Needed radio buttons. There was no equivalent enable/disable option for Server Dialback, although this PR now adds one (which I think is good). I believe the following adjustments would make configuration both clearer and more robust:
The SASL EXTERNAL option "If attempting to validate a certificate fails, the connection is closed and not attempted via dialback authentication” should be reworded to reduce confusion. The current phrasing suggests that specifically Server Dialback is skipped when validation fails. Instead, the text should clarify the reasoning: If SASL EXTERNAL is attempted (because the peer presented a certificate) but fails (because the certificate was invalid), then, to avoid the risk of talking to a compromised peer, not any other (weaker) authentication mechanisms (like Server Dialback) should be attempted. However, if SASL EXTERNAL is enabled but cannot be used (because the peer did not offer a certificate), then it’s fine to fall back to another mechanism, if one is available. I believe this is a better label, instead of "If attempting to validate a cerificate fails, the connection is closed and not attempted via dialback authentication":
This last checkbox is likely the one causing confusion. In environments that use gatewaying or trunking (a fairly niche setup), running into 'invalid' certificates on the gateway are common, but they’re not necessarily a reason to skip other authentication methods like Server Dialback. In those cases, you’d want to uncheck that option. These all are, I think, good improvements - but I don't quite get yet how these would fix the problem that you were initially trying to address with this PR. Given that you're introducing an option that's basically the inverse of a preexisting option:
|
I have raised a new ticket for the suggested improvement on wording: https://igniterealtime.atlassian.net/browse/OF-3135 Another new ticket has been raised for improving the layout of these pages: https://igniterealtime.atlassian.net/browse/OF-3136 |
Let me join you in the confusion :-) So we have the StartTLS policy, which states if we need certs or not. This should be in my view grouped together with certificate validation (expiry, trust, revocation etc). Then we have authentication, with SASL and dialback. And you are right, the two checkboxes that you mention seem to address the same issue. However, during testing, the existing checkbox had no impact. Could possibly depend on which end that starts the connection. I have to do some digging there. Browsed through "XEP-0178: Best Practices for Use of SASL EXTERNAL with Certificates" After successful TLS negotiation: So, I think the first checkbox is trying to address one of the the "eithers" above when no match is found. Again maybe reword that checkbox to something about identifiers/doman name in certificate instead of mentioning certificate validation. Also, with this interpretation, I think this checkbox should take care of my trunking case. |
We seem to be getting on the same page. :) I think we both agree that things like STARTTLS policy, certificate validation, revocation etc mostly relate to the encryption configuration of a connection. SASL EXTERNAL and Server Dialback both relate to authentication Under OF-3136, I think this should be presented in a much clearer way. SASL is a framework that can have many different authentication mechanisms. For server-to-server connections, XMPP only defines EXTERNAL, which ties authentication to the identity that's provided by the peer in a certificate (see RFC6120 and the specifications that you've found. I agree that the preexisting checkbox should have addressed your issue. Perhaps we shouldn't be adding a feature, but fix a bug. |
Nice, these new pages makes the distinction between encryption and authentication much more clear! Back to the text in your new pages...I guess this was just a first preview on the new layout and that the text hasn't been finalized. Nevertheless, a few comments...
|
This reverts commit 0d56258.
- Instead of introducing a new property, the code now uses STRICT_CERTIFICATE_VALIDATION to determine if dialback feature should be offered. - Split up a few cases in the event handler for outbound connections, to support the above and give better guidance in troubleshooting by more precise logging.
The code now uses the old checkbox to determine if dialback should be offered. But there are multiple locations with code like "... if (dialbackOffered && (ServerDialback.isEnabled() || ServerDialback.isEnabledForSelfSigned()))" - without any checks that the presented cert is indeed selfsigned. |
Thanks for your feedback! That's exactly the type of thing that I was hoping for. I've been progressing a bit further, but I hit a bit of a snag: these pages are re-used for four types of connections:
The separation that we've been discussing here makes sense for server-to-server, but not so much for the others. They, for example, have most of their "authentication" settings on different places. For client-to-server connections, that's mostly on the "Registration & Login" page, for example. It's weird to have that page co-exist with an 'authentication' page under the connection settings. I am also not liking how these settings are duplicated for every port (plain and directtls). It is very flexible, but that could also be an annoyance: if you want to disable TLS 1.1 on the plaintext port for client-to-server, chances are that you also want that on the directtls port. To top off the confusion, some settings apply to both ports, even if we do show them separately. Hopefully we can find a better way to organize these pages, although I'm not immediately seeing how. |
Sounds a bit tricky indeed. Too much flexibility often leads to increased complexity. |
This commit adds a configuration option, "xmpp.server.dialback.offer", that can be used to control if dialback stream feature should be offered.
One usecase is where openfire is used as a gateway between XMPP domains ("trunking") - https://download.igniterealtime.org/openfire/docs/latest/documentation/trunking-guide.html
In situations where dialback has to be enabled in the gateway, it also forces the server(s) behind the gateway to use dialback. Before this commit, the code did not add dialback unless the certificates are selfsigned. Which means that a session to a remote domain via the gateway will not be established both ways due to lack of dialback, when the certificates are signed by CAs and trust exists.
With "xmpp.server.dialback.offer" set to "true", dialback will be added as a stream feature and sessions can be established in both directions.