feature: Added ngx_http_lua_ffi_ssl_ocsp_get_nextupdate #1041
Conversation
|
Also, two newbie questions:
|
|
@alubbe To answer your questions:
|
src/ngx_http_lua_ssl_ocsp.c
Outdated
| @@ -433,6 +433,182 @@ ngx_http_lua_ffi_ssl_validate_ocsp_response(const u_char *resp, | |||
| } | |||
|
|
|||
|
|
|||
| static time_t | |||
| ngx_ssl_stapling_time(ASN1_GENERALIZEDTIME *asn1time) | |||
There was a problem hiding this comment.
You should use the ngx_http_lua_ prefix, no matter what.
Fair enough. I will copy the contents and rename it to
Can we also remove /~https://github.com/openresty/lua-nginx-module/pull/782/files#diff-c4f8697714ffd4ae8e3d403bc9a55715R17 ? |
| goto error; | ||
| } | ||
|
|
||
| id = OCSP_cert_to_id(NULL, cert, issuer); |
There was a problem hiding this comment.
hmm, I am not sure what this cert_to_id do though.
There was a problem hiding this comment.
It extracts the id that I believe is mandatory to call OCSP_resp_find_status, which in turn extracts the nextUpdate value.
https://www.openssl.org/docs/man1.1.0/crypto/OCSP_cert_to_id.html
https://www.openssl.org/docs/man1.1.0/crypto/OCSP_resp_find_status.html
There was a problem hiding this comment.
Is issuer supposed to be the issuer of the leaf certificate, or the issuer of the OCSP response? (they don't always match).
If I understand the documentation and the OpenSSL code correctly it is the former (issuer of certificate) so this implementation is correct, but I'm not 100% sure.
There was a problem hiding this comment.
This function body borrows heavily from the function right above it. This particular line is the same as /~https://github.com/openresty/lua-nginx-module/pull/1041/files#diff-50267b7dd63c740bc5c1d29c7387e789R357
| goto error; | ||
| } | ||
|
|
||
| cert = d2i_X509_bio(bio, NULL); |
There was a problem hiding this comment.
We already have FFI functions to parse certificates into STACK_OF(X509) (from PEM, but still), so IMO this function should just take an already parsed certificate chain instead of a DER blob.
| } | ||
|
|
||
| if (nupdate_ans1 == NULL) | ||
| { |
There was a problem hiding this comment.
Should not be on its own line.
|
|
||
| if (OCSP_resp_find_status(basic, id, NULL, NULL, NULL, NULL, &nupdate_ans1) | ||
| != 1) | ||
| { |
|
|
||
| int | ||
| ngx_http_lua_ffi_ssl_ocsp_get_nextupdate(const u_char *resp, size_t resp_len, | ||
| const char *chain_data, size_t chain_len, time_t* nextupdate, |
There was a problem hiding this comment.
Should be time_t *nextupdate not time_t* nextupdate.
There was a problem hiding this comment.
Also, next_update might be better.
There was a problem hiding this comment.
I agree that next_update is better, since the official field name is nextUpdate. However, that naming decision was already made in existing code: /~https://github.com/openresty/lua-nginx-module/pull/1041/files#diff-50267b7dd63c740bc5c1d29c7387e789L284
If @agentzh would allow me, I could open a PR renaming all existing instances to next_update, but until then I'd stick to the existing convention of nextupdate.
zhuizhuhaomeng
force-pushed
the
master
branch
from
f924579 to
fef2581
Compare
|
This feature has been implemented in another PR. |
This PR is a possible solution to openresty/lua-resty-core#75 and is complemented by openresty/lua-resty-core#99.
Looking forward to your feedback.
I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.