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

Implement ocsp crl check method c #4545

Merged
merged 14 commits into from
Aug 17, 2023

Conversation

fmarco76
Copy link
Member

This is a porting of #4504 to master branch

The init order for OCSP is modified to allow CRL retrieval before
creating connection with DS or other services. Secure`connections will be
verified against the CRL.

Solve RHCS-4262
Add new field in CMS for a callback validation of certificate
instantiated by PKISocketFactory.

This is useful for OCSP where the OCSP protocol cannot be enabled and
the verification is done on CRLs.

Solve RHCS-4262
Add a new parameter to enable the crl check for OCSP connection when
acting as client. The new parameter is
`ocsp.store.ldapStore.checkSubsystemConnection` and its default value is
`false`. When set to `true` connection certificate are verified using
the crl stored in the LDAP.
When OCSP is acting as server certificate can be verified using CRL
internally stored.

To verify the certificates the `LDAPStore` has to be enabled with the
variable `ocsp.store.ldapStore.checkSubsystemConnection` and the
variable `auths.revocationChecking.enabled` both set to true.

Solve RHCS-4262
Socket callback moved to CMSEngine to avoid dependencies on global
variables.
The parameter `ocsp.store.ldapStore.checkSubsystemConnection` default
value has been modified to `true` so when LDAPStore is used certificates
are verified against the CRL.

Additionally, during the certificate verification the certificate signer
is verified with the CA certificate providing the CRL to be sure it is
the real issuer.
The option `ocsp.store.ldapStore.validateConnCertWithCRL` enables the
revocation verification of peer certificates using the CRL stored in the LDAP
shared with the CA.

When it is set to `true` (default value), the peer certificate of all the outcome connections from the OCSP subsystem are verified with the CRL.

If the option `auths.revocationChecking.enabled` is also set to `true` the peer certificate ot all the income connections to the OCSP subsystem are verified with the CRL.
Identification of CRL issuing point done by matching Authority Key
Identifier with Subject Key Identifier instead of DN matching.

This should make more reliable the check because not affected of
encoding or format changes in the DN.
Due to refactoring the engine object is not accessible using static
reference from outside the declaring package. Therefore the callback
reference have been stored globally in the `CMSEngine` class
@fmarco76 fmarco76 requested a review from edewata August 16, 2023 15:26
@fmarco76
Copy link
Member Author

@edewata The code has been evaluated in the original PR #4504. However, there has been a re-factory of the involved class requiring several additional changes.

In more detail, static reference to the engine have been removed so the way callback are stored and retrieved is different. Actually, I have to add a static reference in the CMSEngine class because I cannot find other viable options. Do you think it is OK with the current approach?

Copy link
Contributor

@edewata edewata left a comment

Choose a reason for hiding this comment

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

I have some minor comments about log messages. Feel free to update/merge.

About the static reference, I'm thinking we could add an SSLCertificateApprovalCallback field into PKISocketFactory, then the caller could initialize it like this:

PKISocketFactory factory = new PKISocketFactory();
factory.setApprovalCallback(engine.getApprovalCallback());

This will allow PKISocketFactory to be used on the client side too (without dependency on CMSEngine), but I'm not sure how much changes this will require, so let's handle this separately. The PR is fine as is.

@fmarco76 fmarco76 changed the title Implement ocspcrl check method c Implement ocsp crl check method c Aug 16, 2023
@fmarco76
Copy link
Member Author

This will allow PKISocketFactory to be used on the client side too (without dependency on CMSEngine), but I'm not sure how much changes this will require, so let's handle this separately. The PR is fine as is.

Yes, store the information in PKISocketFactory seems good. Not sure the amount of work needed but, since there are CI failures, I will verify if it is possible while I am fixing the remaining problems.

Add stack trace for error logs when they are generated from internal
error
@edewata
Copy link
Contributor

edewata commented Aug 17, 2023

Thanks for the update! Looks like all tests are passing now. I'll let you decide if you want to handle the static references here or separately.

@fmarco76
Copy link
Member Author

Thanks for the update! Looks like all tests are passing now. I'll let you decide if you want to handle the static references here or separately.

I fixed all the tests but the CRL check does not work as expected so I am investigating the reasons. We have to add some CI workflows to test the new configurations. In the meantime I have already moved the callback to the PKISocketFactory but not updated yet. Porting this to master is resulting much more complex than expected.

Moving the callback to `PKISocketFactory` there is no need to have store
it in a static variable. However, only OCSPEngine instances have a valid
value so no other instances are used.

The startup order has been fixed.
@sonarcloud
Copy link

sonarcloud bot commented Aug 17, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

0.0% 0.0% Coverage
8.1% 8.1% Duplication

@fmarco76
Copy link
Member Author

@edewata Thanks! It seems everything is working as expected. I will add new CI tests in a separate PR

@fmarco76 fmarco76 merged commit 05ff27e into dogtagpki:master Aug 17, 2023
151 checks passed
@fmarco76 fmarco76 deleted the ImplementOCSPCRLCheckMethodC branch August 17, 2023 17:08
fmarco76 added a commit to fmarco76/pki that referenced this pull request Aug 23, 2023
When LDAP store is used the OCSP can be configured to check certificate
using the stored CRL. This is implemented in PR dogtagpki#4545.
fmarco76 added a commit to fmarco76/pki that referenced this pull request Aug 24, 2023
When LDAP store is used the OCSP can be configured to check certificate
using the stored CRL. This is implemented in PR dogtagpki#4545.
fmarco76 added a commit that referenced this pull request Aug 24, 2023
When LDAP store is used the OCSP can be configured to check certificate
using the stored CRL. This is implemented in PR #4545.
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.

2 participants