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

fix: Improve certificate revocation checking #2626

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions documentation/ssl-guide.html
Original file line number Diff line number Diff line change
Expand Up @@ -374,10 +374,21 @@ <h4>Fallback behavior when Openfire is the Client (S2S Connections)</h4>
<ol>
<li>Check OCSP stapled response (if available)</li>
<li>Attempt client-driven OCSP query if no stapled response is present</li>
<li>Check CRL (if OCSP is unavailable)</li>
<li>Fail the connection if all methods fail</li>
<li>Check CRL (if OCSP is unavailable, and CRL is available)</li>
<li>Allow the connection to succeed if the revocation status cannot be determined</li>
</ol>

<p>The system property <code>xmpp.socket.ssl.certificate.revocation.soft-fail</code> controls the behavior when
revocation status cannot be determined. By default, this property is set to <code>true</code>, which allows
the connection to succeed if the revocation status cannot be determined. If you want to enforce strict
revocation checking, you can set this property to <code>false</code>. When set to <code>false</code>, the
connection will fail if the revocation status cannot be determined.</p>

<p>By default, revocation checking only checks the leaf certificate in a chain. This avoids issues with chains
where the root certificate isn't included in the chain (e.g. Let's Encrypt) and its CRL distribution
point isn't accessible. If you want to enforce checking the entire chain, you can set the system
property <code>xmpp.socket.ssl.certificate.revocation.check-chain</code> to <code>true</code>.</p>

<h4>OCSP Stapling</h4>

<p>Openfire, when operating as a TLS server and presenting its own certificate, will attempt to staple OCSP
Expand Down
2 changes: 2 additions & 0 deletions i18n/src/main/resources/openfire_i18n.properties
Original file line number Diff line number Diff line change
Expand Up @@ -1236,6 +1236,8 @@ system_property.xmpp.auth.ssl.context_protocol=The TLS protocol to use for encry
system_property.xmpp.parser.buffer.size=Maximum size of an XMPP stanza. Larger stanzas will cause a connection to be closed.
system_property.xmpp.auth.ssl.enforce_sni=Controls if the server enforces the use of SNI (Server Name Indication) when clients connect using TLS.
system_property.xmpp.socket.ssl.active=Set to true to enable Direct TLS encrypted connections for clients, otherwise false
system_property.xmpp.socket.ssl.certificate.revocation.only-end-entity=Only verify revocation status of end-entity (leaf) certificates
system_property.xmpp.socket.ssl.certificate.revocation.soft-fail=Allow validation to continue if revocation information is unavailable
system_property.xmpp.socket.write-timeout-seconds=The write timeout time in seconds to handle stalled sessions and prevent DoS
system_property.xmpp.component.ssl.active=Set to true to enable Direct TLS encrypted connections for external components, otherwise false
system_property.xmpp.server.startup.retry.delay=Set to a positive value to allow a retry of a failed startup after the specified duration.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
import java.security.cert.Certificate;
import java.util.*;

import static org.jivesoftware.openfire.session.ConnectionSettings.Server.REVOCATION_CHECK_ONLY_END_ENTITY;
import static org.jivesoftware.openfire.session.ConnectionSettings.Server.REVOCATION_SOFT_FAIL;

/**
* A Trust Manager implementation that adds Openfire-proprietary functionality.
*
Expand Down Expand Up @@ -274,6 +277,30 @@ protected CertPath checkChainTrusted( CertSelector selector, X509Certificate...
pathBuilder = CertPathBuilder.getInstance( "PKIX" );
}

if (checkRevocation) {
// Configure revocation checking - using default OCSP preference (OCSP before CRL)
PKIXRevocationChecker revChecker = (PKIXRevocationChecker)pathBuilder.getRevocationChecker();

EnumSet<PKIXRevocationChecker.Option> options = EnumSet.noneOf(PKIXRevocationChecker.Option.class);

// Only verify revocation status of end-entity (leaf) certificates if configured
// This avoids issues with chains where the root certificate isn't included
// in the chain (e.g. Let's Encrypt) and its CRL distribution point isn't accessible
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems wrong.

The chain checked is presumably after path-building, so if the old Trust Anchor is not included in the peer chain, we'll have used our own copy anyway, surely?

(Exception being if DANE is in play, but then we possibly don't do CRL checking anyway, and Openfire doesn't yet do DANE).

Is there a better explanation of EE-only status checking?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Dave, your comment led me to realise I hadn't understood what was going on.

if (REVOCATION_CHECK_ONLY_END_ENTITY.getValue()) {
options.add(PKIXRevocationChecker.Option.ONLY_END_ENTITY);
}

// Allow validation to continue if revocation information is unavailable, if configured
// This prevents failures when OCSP/CRL servers are unreachable or
// when revocation information isn't available for some certificates
if (REVOCATION_SOFT_FAIL.getValue()) {
options.add(PKIXRevocationChecker.Option.SOFT_FAIL);
}

revChecker.setOptions(options);
parameters.addCertPathChecker(revChecker);
guusdk marked this conversation as resolved.
Show resolved Hide resolved
}

try
{
// Finally, construct (and implicitly validate) the certificate path.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,30 @@ public static final class Server {
public static final String TLS_CERTIFICATE_CHAIN_VERIFY = "xmpp.server.certificate.verify.chain";
public static final String TLS_ON_PLAIN_DETECTION_ALLOW_NONDIRECTTLS_FALLBACK = "xmpp.server.tls.on-plain-detection-allow-nondirecttls-fallback";

/**
* Only verify revocation status of end-entity (leaf) certificates.
*
* This avoids issues with chains where the root certificate isn't included
* in the chain (e.g. Let's Encrypt) and its CRL distribution point isn't accessible.
*/
public static final SystemProperty<Boolean> REVOCATION_CHECK_ONLY_END_ENTITY = SystemProperty.Builder.ofType(Boolean.class)
.setKey("xmpp.socket.ssl.certificate.revocation.only-end-entity")
.setDefaultValue(true)
.setDynamic(true)
.build();

/**
* Allow validation to continue if revocation information is unavailable.
*
* This prevents failures when OCSP/CRL servers are unreachable or when revocation
* information isn't available for some certificates.
*/
public static final SystemProperty<Boolean> REVOCATION_SOFT_FAIL = SystemProperty.Builder.ofType(Boolean.class)
.setKey("xmpp.socket.ssl.certificate.revocation.soft-fail")
.setDefaultValue(true)
.setDynamic(true)
.build();

public static final String COMPRESSION_SETTINGS = "xmpp.server.compression.policy";

public static final String PERMISSION_SETTINGS = "xmpp.server.permission";
Expand Down
Loading