shreemaan-abhishek commented on code in PR #11351:
URL: https://github.com/apache/apisix/pull/11351#discussion_r1663944597
##########
apisix/plugins/ocsp-stapling.lua:
##########
@@ -176,6 +190,58 @@ local function set_cert_and_key(sni, value)
end
+function _M.rewrite(conf, ctx)
+ local scheme = ctx.var.scheme
+ if scheme ~= "https" then
+ return
+ end
+
+ local matched_ssl = ctx.matched_ssl
+ local required_verify_client = matched_ssl.value.client
+ if not required_verify_client then
Review Comment:
how about
```suggestion
if not matched_ssl.value.client then
```
##########
apisix/plugins/ocsp-stapling.lua:
##########
@@ -199,6 +265,19 @@ function _M.init()
type = "boolean",
default = false,
},
+ ssl_stapling_responder = {
+ type = "string",
+ pattern = [[^http://]],
+ },
+ ssl_ocsp = {
+ type = "string",
+ default = "off",
+ enum = {"off", "leaf"},
+ },
+ ssl_ocsp_responder = {
Review Comment:
these fields are just for customising the values of default nginx directives
right?
##########
apisix/plugins/ocsp-stapling.lua:
##########
@@ -176,6 +190,58 @@ local function set_cert_and_key(sni, value)
end
+function _M.rewrite(conf, ctx)
+ local scheme = ctx.var.scheme
+ if scheme ~= "https" then
+ return
+ end
+
+ local matched_ssl = ctx.matched_ssl
+ local required_verify_client = matched_ssl.value.client
+ if not required_verify_client then
+ return
+ end
+
+ local required_ssl_ocsp = matched_ssl.value.ocsp_stapling.ssl_ocsp
Review Comment:
ditto, you can choose a simpler name like `ssl_ocsp`
##########
apisix/plugins/ocsp-stapling.lua:
##########
@@ -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,
Review 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`.
##########
apisix/plugins/ocsp-stapling.lua:
##########
@@ -176,6 +190,58 @@ local function set_cert_and_key(sni, value)
end
+function _M.rewrite(conf, ctx)
+ local scheme = ctx.var.scheme
+ if scheme ~= "https" then
+ return
+ end
+
+ local matched_ssl = ctx.matched_ssl
+ local required_verify_client = matched_ssl.value.client
Review Comment:
why do you name it `required_verify_client` just name it `client`
##########
apisix/plugins/ocsp-stapling.lua:
##########
@@ -176,6 +190,58 @@ local function set_cert_and_key(sni, value)
end
+function _M.rewrite(conf, ctx)
+ local scheme = ctx.var.scheme
+ if scheme ~= "https" then
+ return
+ end
+
+ local matched_ssl = ctx.matched_ssl
+ local required_verify_client = matched_ssl.value.client
+ if not required_verify_client then
+ return
+ end
+
+ local required_ssl_ocsp = matched_ssl.value.ocsp_stapling.ssl_ocsp
+ local client_verify_res = ctx.var.ssl_client_verify
+ -- only client verify ok and ssl_ocsp is "leaf" need to validate ocsp
response
+ if required_ssl_ocsp == "leaf" and client_verify_res == "SUCCESS" then
+ -- ssl_client_raw_cert will return client cert only, need to combine
ca cert
+ 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)
Review Comment:
```suggestion
core.log.error("failed to convert client certificate from PEM to
DER: ", err)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
