Fix S2S dialback not offered in post-TLS features when self-signed certs accepted#3291
Open
milne1282 wants to merge 4 commits into
Open
Fix S2S dialback not offered in post-TLS features when self-signed certs accepted#3291milne1282 wants to merge 4 commits into
milne1282 wants to merge 4 commits into
Conversation
Member
|
Oh, interesting. Thanks for this @milne1282 ! Did you happen to check if other connection types suffer from a similar issue, triggered by the same type of divergence after the prefixed properties were introduced? |
Author
|
Hey @guusdk I have not checked other properties as I was focused on an implementation topology that this affected and have not made a more general search |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR migrates TLS certificate validation from a global property-based system to a connection-listener-based approach, deprecating legacy configurations in ChangesTLS Configuration Migration to Connection-Listener Model
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Member
|
@milne1282 I've raised a ticket for this change in https://igniterealtime.atlassian.net/browse/OF-3259 I've also added a couple of commits to this PR, that addresses similar issues with certificate-verification related properties. I'd love your feedback on those. Please refer to the individual commit messages for details. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xmppserver/src/main/java/org/jivesoftware/openfire/spi/ConnectionListener.java (1)
747-762:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBug:
oldValueretrieves from wrong method.Line 749 calls
verifyCertificateValidity()but should callacceptSelfSignedCertificates(). This causes the comparison on line 754 to be incorrect, and the log messages will report wrong old/new values for the self-signed certificate acceptance setting.🐛 Proposed fix
public void setAcceptSelfSignedCertificates( boolean accept ) { - final boolean oldValue = verifyCertificateValidity(); + final boolean oldValue = acceptSelfSignedCertificates(); // Always set the property explicitly even if it appears the equal to the old value (the old value might be a fallback value). JiveGlobals.setProperty( type.getPrefix() + "certificate.accept-selfsigned", Boolean.toString( accept ) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xmppserver/src/main/java/org/jivesoftware/openfire/spi/ConnectionListener.java` around lines 747 - 762, The setter setAcceptSelfSignedCertificates currently reads the old value from verifyCertificateValidity(), which is wrong; change it to call acceptSelfSignedCertificates() to obtain the actual previous setting, so the equality check and subsequent Log.debug messages report the correct old/new state; keep the JiveGlobals.setProperty call and restart() behavior the same but ensure comparisons and log formatting use acceptSelfSignedCertificates() as the source of oldValue.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/spi/ConnectionListener.java`:
- Around line 747-762: The setter setAcceptSelfSignedCertificates currently
reads the old value from verifyCertificateValidity(), which is wrong; change it
to call acceptSelfSignedCertificates() to obtain the actual previous setting, so
the equality check and subsequent Log.debug messages report the correct old/new
state; keep the JiveGlobals.setProperty call and restart() behavior the same but
ensure comparisons and log formatting use acceptSelfSignedCertificates() as the
source of oldValue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d3b6635-cdbf-4a10-a5c4-d6f937da46eb
📒 Files selected for processing (7)
xmppserver/src/main/java/org/jivesoftware/openfire/net/ClientTrustManager.javaxmppserver/src/main/java/org/jivesoftware/openfire/net/ServerStanzaHandler.javaxmppserver/src/main/java/org/jivesoftware/openfire/net/ServerTrustManager.javaxmppserver/src/main/java/org/jivesoftware/openfire/net/TLSStreamHandler.javaxmppserver/src/main/java/org/jivesoftware/openfire/server/ServerDialback.javaxmppserver/src/main/java/org/jivesoftware/openfire/session/ConnectionSettings.javaxmppserver/src/main/java/org/jivesoftware/openfire/spi/ConnectionListener.java
Member
This code review finding is something that is already being fixed in a different PR: #3308 |
…rts accepted ServerDialback.isEnabledForSelfSigned() reads the legacy property xmpp.server.certificate.accept-selfsigned (TLS_ACCEPT_SELFSIGNED_CERTS), but ConnectionListener.setAcceptSelfSignedCertificates() — which backs the Admin Console UI checkbox — writes to the per-connection-type prefixed property xmpp.socket.ssl.certificate.accept-selfsigned. These diverged when per-type prefixed properties were introduced but the readers in ServerDialback were not updated. The consequence is that after a successful TLS handshake with a self-signed peer certificate, LocalIncomingServerSession.getAvailableStreamFeatures() calls isEnabledForSelfSigned(), gets false, and omits <db:dialback> from the post-TLS <stream:features>. The connecting server then finds no usable authentication mechanism and closes the connection. This causes all encrypted S2S connections to fail silently when the only option is dialback, even if the operator has enabled self-signed certificate acceptance via the UI. Fix: check both property names in isEnabledForSelfSigned(), preserving full backwards compatibility with installations that have the legacy property set. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…y configuration In OF-946 refactor, connection configuration introduced per-type prefixed properties. However, not all usages of the legacy properties were migrated. As a result, some functionality still depends on the old properties while others use the new ones (sometimes even within related features) leading to inconsistent and incompatible behavior (e.g., server-to-server uses new properties, while Dialback relies on legacy ones). This commit deprecates the legacy `xmpp.server.certificate.accept-selfsigned` and xmpp.client.certificate.accept-selfsigned` properties in favor of `ConnectionListener#acceptSelfSignedCertificates`. Changes: - Add fallback: the new configuration uses the legacy property if it is explicitly set. - Migrate all remaining usages from the legacy property to the new configuration. This ensures consistent behavior across all connection types and prevents configuration mismatches.
…s and unify configuration In OF-946 refactor, connection configuration introduced per-type prefixed properties. However, not all usages of the legacy properties were migrated. As a result, some functionality still depends on the old properties while others use the new ones (sometimes even within related features) leading to inconsistent and incompatible behavior (e.g., server-to-server uses new properties, while Dialback relies on legacy ones). This commit deprecates the legacy `xmpp.server.certificate.verify.validity` and `xmpp.client.certificate.verify.validity` properties in favor of `ConnectionListener#verifyCertificateValidity`. Changes: - Add fallback: the new configuration uses the legacy property if it is explicitly set. - Migrate all remaining usages from the legacy property to the new configuration. This ensures consistent behavior across all connection types and prevents configuration mismatches.
…rties for removal `org.jivesoftware.openfire.net.ClientTrustManager` has been replaced by `org.jivesoftware.openfire.keystore.OpenfireX509TrustManager` at least a decade ago. It is unlikely that it's still being used. These properties are used _exclusively_ by this old implementation. As that implementation isn't in use any more, these properties are effectively unused. - `xmpp.client.certificate.verify` - `xmpp.client.certificate.crl` - `xmpp.client.certificate.verify.chain` - `xmpp.client.certificate.verify.root` This commit explicitly marks the `ClientTrustManager` as being deprecated, for removal in Openfire 5.2.0. I don't think there's much value in retaining the properties, as their functionality seems questionably specific. Apparently, it hasn't been missed in the last decade. There properties above are specific to client connectivity. For server connectivity, things are slightly different. There also exists `org.jivesoftware.openfire.net.ServerTrustManager` which also goes unused. It is unlikely that it's still begin used. This implementation, unlike its client-counterpart, does not (directly) use similar properties. This commit explicitly marks the `ServerTrustManager` as being deprecated, for removal in Openfire 5.2.0. The properties mentioned above have server-equivalents: - `xmpp.server.certificate.verify` - `xmpp.server.certificate.verify.chain` - `xmpp.server.certificate.verify.root` (`xmpp.server.certificate.crl` is not defined) Unlike the client properties, these server properties are used in other bits of code. `xmpp.server.certificate.verify` (as constant `org.jivesoftware.openfire.session.ConnectionSettings.Server#TLS_CERTIFICATE_VERIFY`) - appears to be used in `org.jivesoftware.openfire.net.SASLAuthentication` to assert if, for every server-to-server SASL authentication mechanism applied to incoming server-to-server connections, the peer's certificates should be validated. There's just one SASL mechanism for incoming server-to-server connections, which is EXTERNAL. Its implementation seems to unconditionally verify certificates in `org.jivesoftware.openfire.sasl.ExternalServerSaslServer` - the SASL mechanisms that we offer to server peers (in `org.jivesoftware.openfire.net.SASLAuthentication`) only offers EXTERNAL when the peer's certificates are unconditionally validated. In this path, the property doesn't seem to be useful. - is used in `org.jivesoftware.openfire.net.ServerStanzaHandler` to populate a variable that is not used. In this path, the property does not seem to be useful either. - is used in `org.jivesoftware.openfire.net.TLSStreamHandler` to configure a TLS Engine, specifically to check if a peer's certificate is wanted, or needed. `TLSStreamHandler` itself is used by `org.jivesoftware.openfire.server.ServerDialback` and `org.jivesoftware.openfire.net.SocketConnection` (which is documented to be used only for Server Dialback). Server Dialback is a mechanism used when SASL EXTERNAL (which is based on TLS certifiate validation) is unavailable. It seems unlikely that low-level configuration of TLS certificates is required (or even desired) for Server Dialback. In all, `xmpp.server.certificate.verify` seems to be largely unused. `xmpp.server.certificate.verify.chain` (as constant `org.jivesoftware.openfire.session.ConnectionSettings.Server#TLS_CERTIFICATE_CHAIN_VERIFY`) - is used in `org.jivesoftware.openfire.net.ServerStanzaHandler` to populate a variable that is not used. In this path, the property does not seem to be useful. - is used in `org.jivesoftware.openfire.net.TLSStreamHandler` to configure a TLS Engine, specifically to check if a peer's certificate is wanted, or needed. TLSStreamHandler itself is used by `org.jivesoftware.openfire.server.ServerDialback` and `org.jivesoftware.openfire.net.SocketConnection` (which is documented to be used only for Server Dialback). Server Dialback is a mechanism used when SASL EXTERNAL (which is based on TLS certifiate validation) is unavailable. It seems unlikely that low-level configuration of TLS certificates is required (or even desired) for Server Dialback. In all, `xmpp.server.certificate.verify.chain` seems to be largely unused. `xmpp.server.certificate.verify.root` (as constant `org.jivesoftware.openfire.session.ConnectionSettings.Server#TLS_CERTIFICATE_ROOT_VERIFY`) - Only defined as a constant, but the constant is not used. I believe all of the above is reason to phase out all of these properties: - `xmpp.client.certificate.verify` - `xmpp.client.certificate.crl` - `xmpp.client.certificate.verify.chain` - `xmpp.client.certificate.verify.root` - `xmpp.server.certificate.verify` - `xmpp.server.certificate.verify.chain` - `xmpp.server.certificate.verify.root` Unlike the properties `xmpp.server.certificate.verify.validity`, `xmpp.client.certificate.verify.validity`, `xmpp.server.certificate.accept-selfsigned` and xmpp.client.certificate.accept-selfsigned` that have been deprecated in earlier commits, but have been replaced by `ConnectionListener` functionality, I see little value in introducing new functionality in `ConnectionListener` to match the properties above. In this commit, I have marked the constants that define the properties above as being deprecated, for removal in Openfire 5.2.0. Properties for which no constant is defined are used in code that is already marked for removal, so they'll disappear automatically.
guusdk
force-pushed
the
fix/s2s-selfsigned-dialback-property
branch
from
0bbf1e2 to
d750017
Compare
Member
|
I've rebased this to the latest HEAD of the Main branch, which has some improvements to the CI workflow. Hopefully, tests are more stable now. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ServerDialback.isEnabledForSelfSigned() reads the legacy property xmpp.server.certificate.accept-selfsigned (TLS_ACCEPT_SELFSIGNED_CERTS), but ConnectionListener.setAcceptSelfSignedCertificates() — which backs the Admin Console UI checkbox — writes to the per-connection-type prefixed property xmpp.socket.ssl.certificate.accept-selfsigned. These diverged when per-type prefixed properties were introduced but the readers in ServerDialback were not updated.
The consequence is that after a successful TLS handshake with a self-signed peer certificate, LocalIncomingServerSession.getAvailableStreamFeatures() calls isEnabledForSelfSigned(), gets false, and omits db:dialback from the post-TLS stream:features. The connecting server then finds no usable authentication mechanism and closes the connection. This causes all encrypted S2S connections to fail silently when the only option is dialback, even if the operator has enabled self-signed certificate acceptance via the UI.
Fix: check both property names in isEnabledForSelfSigned(), preserving full backwards compatibility with installations that have the legacy property set.