feat: support OCSP validation of the client certificate #11351
Conversation
|
It seem the ocsp responder didn't exit normally, I will check it. |
|
any reviewer? |
apisix/plugins/ocsp-stapling.lua
Outdated
| end | ||
|
|
||
| local matched_ssl = ctx.matched_ssl | ||
| local required_verify_client = matched_ssl.value.client |
There was a problem hiding this comment.
why do you name it required_verify_client just name it client
apisix/plugins/ocsp-stapling.lua
Outdated
| return | ||
| end | ||
|
|
||
| local required_ssl_ocsp = matched_ssl.value.ocsp_stapling.ssl_ocsp |
There was a problem hiding this comment.
ditto, you can choose a simpler name like ssl_ocsp
apisix/plugins/ocsp-stapling.lua
Outdated
|
|
||
| local matched_ssl = ctx.matched_ssl | ||
| local required_verify_client = matched_ssl.value.client | ||
| if not required_verify_client then |
There was a problem hiding this comment.
how about
| if not required_verify_client then | |
| if not matched_ssl.value.client then |
apisix/plugins/ocsp-stapling.lua
Outdated
| local full_chain_pem_cert = ctx.var.ssl_client_raw_cert .. matched_ssl.value.client.ca | ||
| local der_cert_chain, err = ngx_ssl.cert_pem_to_der(full_chain_pem_cert) | ||
| if not der_cert_chain then | ||
| core.log.error("failed to convert clinet certificate from PEM to DER: ", err) |
There was a problem hiding this comment.
| core.log.error("failed to convert clinet certificate from PEM to DER: ", err) | |
| core.log.error("failed to convert client certificate from PEM to DER: ", err) |
| default = "off", | ||
| enum = {"off", "leaf"}, | ||
| }, | ||
| ssl_ocsp_responder = { |
There was a problem hiding this comment.
these fields are just for customising the values of default nginx directives right?
There was a problem hiding this comment.
yes, but no all same as nginx directives, just part of it, nginx directives allows "off", "leaf" and "on". the openresty api can not get full chain cert, so we don't support "on" here.
apisix/plugins/ocsp-stapling.lua
Outdated
| @@ -166,6 +179,7 @@ local function set_cert_and_key(sni, value) | |||
| end | |||
|
|
|||
| local ok, err = set_ocsp_resp(fin_pem_cert, | |||
| value.ocsp_stapling.ssl_stapling_responder, | |||
There was a problem hiding this comment.
this value is being passed around through many functions, I would recommend checking if this value is nil. If it is nil, use get_ocsp_responder.
There was a problem hiding this comment.
this value will always be the params of function get_ocsp_responder on line 49, the function will do a check.
There was a problem hiding this comment.
yes but instead of passing this values through mulitple functions, I think a simple check before hand can avoid this and make code simpler.
|
The issue you have linked with this PR is not focused, please create a new issue describing your proposal. |
|
Hi, @yuweizzz. The centos7 related image that was causing the CI error has been removed, please merge the changes on the master branch to resolve the CI error. |
|
Merged. |
| local ocsp_url, err = ngx_ocsp.get_ocsp_responder_from_der_chain(der_cert_chain) | ||
|
|
||
| if not ocsp_url then | ||
| local function get_ocsp_responder(der_cert_chain) |
There was a problem hiding this comment.
can we skip this unnecessary function name modification/breaking down of one function into two?
I am not a OCSP expert and it makes difficult to review.
There was a problem hiding this comment.
break down this function because we could reuse it in client verify.
Description
Fixes #11385
Releated: #10309
Checklist