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

Add error log for invalid OCSP response #9674

Merged
merged 2 commits into from
May 10, 2023
Merged

Add error log for invalid OCSP response #9674

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7696c6a
Add error log for invalid OCSP response
maskit May 2, 2023
1df7130
Check times only if status is available
maskit May 3, 2023
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
6 changes: 4 additions & 2 deletions iocore/net/OCSPStapling.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -957,8 +957,10 @@ stapling_check_response(certinfo *cinf, TS_OCSP_RESPONSE *rsp)
if (!TS_OCSP_resp_find_status(bs, cinf->cid, &status, &reason, &rev, &thisupd, &nextupd)) {
// If ID not present just pass it back to client
Error("stapling_check_response: certificate ID not present in response for %s", cinf->certname);
} else {
TS_OCSP_check_validity(thisupd, nextupd, 300, -1);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, even if the response has a not-good status or we find it is invalid, we still cache it and send it to the client? Is that the right thing to do?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if we should cache it, but I think we should just tell the truth to the client to give the information that the response is invalid.

if (!TS_OCSP_check_validity(thisupd, nextupd, 300, -1)) {
Copy link
Contributor

@jpeach jpeach May 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If you failed to get the status, is it still OK to check the validity? I'm guessing that the extra checks only apply for status TS_OCSP_CERTSTATUS_GOOD, since otherwise, the response is already bad?

Copy link
Member Author

Choose a reason for hiding this comment

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

Clang analyzer pointed out a similar thing, and I just updated the code. I think we should check the times regardless of the status of the cert. Response that says the cert is revoked may have invalid times.

// The check is just for logging and pass the response back to client anyway
Error("stapling_check_response: status in response for %s is not valid already/yet", cinf->certname);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way (or need to) to propagate the actual error out?

Copy link
Member Author

Choose a reason for hiding this comment

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

To the client? Client should check the stapled response and get the same error (and it's up to the client whether they ignore the error).

}

switch (status) {
Expand Down