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.

Sign up for GitHub

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

Jump to bottom

Add additional tests for OCSP #4547

Merged
merged 2 commits into from
Aug 24, 2023
Merged

Add additional tests for OCSP #4547

merged 2 commits into from
Aug 24, 2023

Conversation

fmarco76
Copy link
Member

Added tests for non existing certificate and non managed CA.

Additionally, fixed a condition which was not working properly.

@fmarco76 fmarco76 added the WIP Work In Progress label Aug 21, 2023
@fmarco76 fmarco76 force-pushed the OCSPExtraChecks branch 3 times, most recently from 40d02f2 to b1c875c Compare August 22, 2023 09:05
@fmarco76
Add additional tests for OCSP
3a0c755 
Added tests for non existing certificate and non managed CA.

Additionally, fixed a condition which was not working properly.
@sonarcloud
Copy link

sonarcloud bot commented Aug 23, 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 0 Code Smells

0.0% 0.0% Coverage
1.9% 1.9% Duplication

@fmarco76
Copy link
Member Author

Add CI for #4545 and #4524

@fmarco76 fmarco76 removed the WIP Work In Progress label Aug 23, 2023
ckelleyRH
ckelleyRH approved these changes Aug 23, 2023
View reviewed changes
Copy link
Contributor

@ckelleyRH ckelleyRH left a comment

Choose a reason for hiding this comment

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

Nice! I like how clean things are with Ansible.

edewata
edewata reviewed Aug 23, 2023
View reviewed changes
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.

Nice use of ansible! Please see my comments/questions below.

.github/workflows/ocsp-basic-test.yml
@@ -246,6 +246,104 @@ jobs:
echo good > expected
diff expected actual

- name: Check OCSP responder with non existing cert
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understood correctly this means that if a cert doesn't exist in the CA, the OCSP subsystem wil respond with a "good" status, but the CA's built-in OCSP responder will respond with an "unknown" status. Is this correct? Is it possible to document somewhere (e.g. in wiki) the expected value for each case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is correct. The OCSP has only the list of revoked certificate and if it is not revoked it has no information. For the original specification Good is the best answer in this case. However, in the update specification (rfc6960) they accept the original behaviour but if possible the status should be revoked or unknown.
I have created this page and linked to OCSPClient page.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fmarco76 Thanks!
@ladycfu @06shalini FYI

tests/ansible/ocsp/tasks/certificate_self_validation_with_crl.yml Outdated Show resolved Hide resolved
tests/ansible/ocsp_admin.crt Outdated Show resolved Hide resolved
tests/ansible/ocsp/tasks/certificate_self_validation_with_crl.yml
- name: Restart OCSP 2
community.docker.docker_container_exec:
container: "{{ ocsp_container }}"
command: pki-server restart --wait
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC the server has the ability to auto recover from broken DS connection, so it might not be necessary to restart the server, but it's fine to leave this in.

@fmarco76
Add CI test for OCSP self crl check
b3a6d00 
When LDAP store is used the OCSP can be configured to check certificate
using the stored CRL. This is implemented in PR dogtagpki#4545.
edewata
edewata approved these changes Aug 24, 2023
View reviewed changes
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.

Thanks for the update! LGTM.

@fmarco76
Copy link
Member Author

@edewata @ckelleyRH Thanks!

@fmarco76 fmarco76 merged commit a426aa5 into dogtagpki:master Aug 24, 2023
149 checks passed
@edewata
Copy link
Contributor

edewata commented Aug 24, 2023

Sorry, looks like ca_signing.crt was still included. Feel free to remove that if it's not needed.

@fmarco76
Copy link
Member Author

Sorry, looks like ca_signing.crt was still included. Feel free to remove that if it's not needed.

I miss it! I have removed it from master now. Very good catch, thanks!

@fmarco76 fmarco76 deleted the OCSPExtraChecks branch August 24, 2023 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckelleyRH ckelleyRH ckelleyRH approved these changes

@edewata edewata edewata approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fmarco76 @edewata @ckelleyRH