Add error log for invalid OCSP response #9674
Conversation
|
@jpeach Thanks for pointing out the unchecked return value. |
iocore/net/OCSPStapling.cc
Outdated
| } else { | ||
| TS_OCSP_check_validity(thisupd, nextupd, 300, -1); | ||
| } | ||
| if (!TS_OCSP_check_validity(thisupd, nextupd, 300, -1)) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
iocore/net/OCSPStapling.cc
Outdated
| } | ||
| if (!TS_OCSP_check_validity(thisupd, nextupd, 300, -1)) { | ||
| // 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); |
There was a problem hiding this comment.
Is there any way (or need to) to propagate the actual error out?
There was a problem hiding this comment.
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).
iocore/net/OCSPStapling.cc
Outdated
| @@ -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); | |||
| } | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
* asf/master: (33 commits) Add error log for invalid OCSP response (apache#9674) Add new settings to specify TLS versions to use (apache#9667) Remove flask from tests/Pipfile (apache#9688) Doc: Add example of --enable-lto build option with LLVM (apache#9654) Added Zhengxi to the asf contributors (apache#9685) Don't build native QUIC implementation (apache#9670) Stabilize autest tls_hooks17 (apache#9671) Cleanup: remove ts::buffer from HostDB. (apache#9677) Fix leak in MultiTextMod in ControlBase. (apache#9675) Cleanup: remove TsBuffer.h from MIME.cc (apache#9661) Cleanup: remove ts::Buffer from ControlBase. (apache#9664) setup pre-commit hook at cmake generation time (apache#9669) Updated parent retry attempt logic (apache#9620) Try to do less work in hot function HttpHookState::getNext (apache#9660) cmake build, fixed warning using older openssl APIs (apache#9648) rename attempts to retry_attempts (apache#9655) OCSP: Fix unitialized variable error. (apache#9662) Add support for CONNECT method on HTTP/2 connection (apache#9616) Remove deprecated debug output functions from 10 source files. (apache#9657) Remove deprecated debug output functions from 14 source files. (apache#9658) ...
No description provided.