Skip to content

OCSP Error Return #7028

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

Merged
merged 1 commit into from
Dec 5, 2023
Merged

Conversation

ejohnstown
Copy link
Contributor

Description

Modify the check of the OCSP response and return better error codes.

  1. In CheckOcspResponse(), remove the existing check for UNKNOWN certificate status. Given the values of ret and ocsp->error, unknown won't get checked.
  2. Separated checks for UKNOWN and REJECTED for logging purposes. Return that as an error.
  3. Anything else should be a failure.

Fixes ZD 17027.

Testing

I set up an OCSP responder with a set of certificates and demo CA I have set up. I created a new certificate to use with the example server.

openssl ocsp -port 12345 -ndays 1000 -index demoCA/index.txt -rsigner demoCA/cacert.pem -rkey demoCA/private/cakey.pem -CA demoCA/cacert.pem
./examples/server/server -d -A ./ca/demoCA/cacert.pem -c ./ca/slack.pem -k ./ca/key-ecc.pem
./examples/client/client -A ./ca/demoCA/cacert.pem -o

I first ran the OCSP responder. I generated a new server certificate, slack.pem, and connected to the server with the client. The responder didn't know about the certificate yet and returned unknown. I restarted the responder and connected again getting a good status. I revoked the certificate and restarted the responder, client returned the revoked error.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

1. In CheckOcspResponse(), remove the existing check for UNKNOWN
   certificate status. Given the values of ret and ocsp->error, unknown
   won't get checked.
2. Separated checks for UKNOWN and REJECTED for logging purposes. Return
   that as an error.
3. Anything else should be a failure.
@bandi13
Copy link
Contributor

bandi13 commented Dec 4, 2023

retest this please

Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Looks good. Over to PR cap.

Copy link
Contributor

@JacobBarthelmeh JacobBarthelmeh left a comment

Choose a reason for hiding this comment

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

Needs a follow up regression test case added. Code change looks good though, thanks!

@JacobBarthelmeh JacobBarthelmeh merged commit 4c85a5a into wolfSSL:master Dec 5, 2023
@ejohnstown ejohnstown deleted the ocsp-err-ret branch December 26, 2023 16:43
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.

5 participants