Use FetchSM for OCSP HTTP requests #9591
Conversation
|
[approve ci fedora] |
iocore/net/OCSPStapling.cc
Outdated
| Error("failed to connect to OCSP server; host=%s path=%s", host, path); | ||
| // Content-Type, Content-Length, Request Body | ||
| if (use_post) { | ||
| httpreq.set_body("application/ocsp-request", ASN1_ITEM_rptr(OCSP_REQUEST), (const ASN1_VALUE *)req); |
There was a problem hiding this comment.
You are ignoring the return value of this function. Should set_body() return void, and call set_error() for errors?
There was a problem hiding this comment.
Good catch. I added error handling.
| memset(&sin, 0, sizeof(sin)); | ||
| sin.sin_family = AF_INET; | ||
| sin.sin_addr.s_addr = inet_addr("127.0.0.1"); | ||
| sin.sin_port = 65535; |
There was a problem hiding this comment.
What process is listening on this port?
There was a problem hiding this comment.
Nothing. I don't think the address and port are actually used for something.
| return resp; | ||
| } | ||
| } | ||
| return resp; |
There was a problem hiding this comment.
Probably also be good to put the value of p in the trace output for bad responses.
There was a problem hiding this comment.
Now error log has first 5 bytes of a response if ATS fails to parse it.
| BIO_free_all(cbio); | ||
| if (resp) { | ||
| return resp; | ||
| } |
There was a problem hiding this comment.
It seems to me that the array of char that is returned by `httpreq.get_response_body() is leaked. I think you need to change this code to:
// Parse the response
int len;
const unsigned char *res = httpreq.get_response_body(&len);
const unsigned char *p = res;
resp = reinterpret_cast<OCSP_RESPONSE *>(ASN1_item_d2i(nullptr, &p, len, ASN1_ITEM_rptr(OCSP_RESPONSE)));
if (resp) {
if ((res + len) == p) {
free(res);
return resp;
}
const unsigned char *garbage = res + len;
int glen = p - garbage;
if (glen <= 0) {
garbage = "OPENSSL ERROR";
glen = strlen(garbage);
}
Error("garbage at the end of response from OCSP server; uri=%s len=%d garbage=%.*s", uri, len, garbage, glen);
free(res);
return nullptr;
}
There was a problem hiding this comment.
Fixed the memory leak.
Why do we need to check (res + len) == p ?
There was a problem hiding this comment.
To check for garbage at the end of the response.
There was a problem hiding this comment.
OpenSSL's implementation (OSSL_HTTP_REQ_CTX_nbio_d2i) doesn't seem to check it. The check could be too strict and might find unexpected incompatibility. I don't want to have it on this PR.
|
How did you test this? Can you add a regression test? |
|
The test I did is completely manual. Setup a test web server that serves ocsp_response.der used in tls_ocsp autest, setup ATS with certs for the autest, then launch ATS. I checked ssl_ocsp debug log. It would be great if we could have an autest for this, but I don't think we can have it because the URL for OCSP responder (http://localhost:8888) is written in the cert. I guess autest can open a port with a fixed number but I don't think that's something we want to do. Also, I'm testing the code on several of our prod servers. Although the memory leak exist, everything looks fine so far. |
|
[approve ci] |
* asf/master: (40 commits) Change remap filter behavior to match ip_allow.yaml (apache#9631) Cleanup: Get rid of dead code from Cache (apache#9621) Replace obsolete Debug() macro with Dbg() in SocksProxy.cc. (apache#9613) Updates for the new go-httpbin v2.6.0 release. (apache#9633) Fix debian symbol not found for test_HttpTransact (apache#9617) add traffic_ctl to cmake (apache#9628) Fix Proxy Protocol outbound (apache#9632) DOC: Fix variable name `proxy.config.exec_thread.autoconfig.enabled`. (apache#9629) traffic_ctl: metric monitor. Handle SIGINT to drop collected stats. (apache#9570) traffic_ctl: plugin msg command, print out the response from server. (apache#9610) Doc: document IP allow filter for remap. (apache#9626) Cleanup: Rename d with vol (apache#9619) Ensure a reason phrase when sending an HTTP/1 response (apache#9615) Cmake plugins and install things (apache#9597) quic: Fix session cleanup assert. (apache#9622) Enables switching SSL certificates on QUIC with QUICHE (apache#9347) Use FetchSM for OCSP HTTP requests (apache#9591) Make a couple of the threads configs correct (apache#9604) Change submit_and_wait to take ink_hrtime. Fix test_AIO for io_uring. (apache#9555) Update build_h3_tools for mac (apache#9608) ...
3 participants
This is a step to support OCSP, without boringocsp library that has never been open sourced.
ATS has been using
OCSP_REQ_CTXand related functions, which make HTTP requests for OCSP, but many of them were deprecated on OpenSSL 3.0 and they are unavailable on BoringSSL. On this PR, I replaced those functions with ATS's FetchSM and functions that are available on BoringSSL.It still doesn't work with BoringSSL because there are still other functions only available on OpenSSL. I'm going to replace them on next PR.
Changes on FetchSM are needed because OCSP stuff basically runs on ET_OCSP thread and the requests can be made before FetchSM gets ready. It should not affect existing plugins because those won't run until everything is initialized.