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

Conversation

viv
Copy link
Collaborator

@viv viv commented Nov 28, 2024

Configure PKIXRevocationChecker to only check end-entity certificates and use soft-fail for unavailable revocation information.

This attempts to balance security and reliability:

  • Focusing revocation checks on leaf certificates, where revocation is most critical, avoiding issues with missing root certificate CRL distribution points
  • Preventing validation failures when OCSP/CRL servers are unreachable or when revocation information isn't available for some certificates
  • Avoiding common issues with intermediate/root certificate CRL checking

Configure PKIXRevocationChecker to only check end-entity certificates and
use soft-fail for unavailable revocation information.

This attempts to balance security and reliability:
- Focusing revocation checks on leaf certificates, where revocation is most critical, avoiding issues with missing root certificate CRL distribution points
- Preventing validation failures when OCSP/CRL servers are unreachable or when revocation information isn't available for some certificates
- Avoiding common issues with intermediate/root certificate CRL checking
@viv viv requested a review from guusdk November 28, 2024 12:59
@guusdk
Copy link
Member

guusdk commented Nov 28, 2024

Can we make both options configurable please? That way, if Openfire is operating in a closed, tightly controlled network, an administrator can reconfigure that balance between security and reliability.

I'm happy for this to be just a org.jivesoftware.util.SystemProperty without a set of new buttons/toggles on the admin console.

Allow administrators to configure revocation checking behaviour. If Openfire is operating in a closed, tightly controlled network, an administrator can reconfigure the balance between security and reliability.

// 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.

Configure Java and BouncyCastle to enable CRL Distribution Points (CRLDP) checking.

Openfire will now attempt to download CRLs from the URLs specified in the certificate's CRL Distribution Points extension, regardless of whether it's using BC or the Java built-in certificate validation.
Tightening up the default revocation checking config now that we can check revocation status of intermediate certificates via CRLDPs.
Copy link
Member

@guusdk guusdk left a comment

Choose a reason for hiding this comment

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

Changes look good to me. @viv did you leave this in 'draft' intentionally?

@viv
Copy link
Collaborator Author

viv commented Dec 2, 2024

No, I think I've covered everything.

@guusdk guusdk marked this pull request as ready for review December 2, 2024 20:31
@guusdk guusdk requested a review from dwd December 2, 2024 20:31
@guusdk
Copy link
Member

guusdk commented Dec 2, 2024

Ok, thanks, I've unmarked it as draft, and have poked @dwd for a re-review (but shall hit the glowing green button if he does not in a timely manner, where 'timely' shall be interpreted in a completely subjective manner that likely is influenced by my intake of caffeine in the morning).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants