Conversation
t8m
added
branch: master
tmshort
left a comment
tmshort
left a comment
There was a problem hiding this comment.
LGTM (pending CI; which is currently just Windows)
|
|
||
| B<StrictCertCheck>: Enable strict certificate checking. Equivalent to | ||
| setting B<SSL_CERT_FLAG_TLS_STRICT> with SSL_CTX_set_cert_flags(). | ||
|
|
There was a problem hiding this comment.
Just noting that this flag is the equivalent to -strict on the s_client/s_server command line, but none of the command-line options reference config options (just the flags).
| SSL_FLAG_TBL_INV("CANames", SSL_OP_DISABLE_TLSEXT_CA_NAMES), | ||
| SSL_FLAG_TBL("KTLS", SSL_OP_ENABLE_KTLS) | ||
| SSL_FLAG_TBL("KTLS", SSL_OP_ENABLE_KTLS), | ||
| SSL_FLAG_TBL_CERT("StrictCertCheck", SSL_CERT_FLAG_TLS_STRICT) |
There was a problem hiding this comment.
Also noting that this is the first time SSL_FLAG_TBL_CERT is being used, it had been previously defined but never used.
tmshort
removed
the
approval: review pending
|
ping for second review |
| "MaxProtocol" => $protocol, | ||
| "Certificate" => test_pem("client-pss-restrict-cert.pem"), | ||
| "PrivateKey" => test_pem("client-pss-restrict-key.pem"), | ||
| "Options" => "StrictCertCheck", |
There was a problem hiding this comment.
I am slightly confused by this. We are expecting a server fail due to StrictCertCheck....but the option is set client side? Why does the server fail because of a client side option?
There was a problem hiding this comment.
Because the client certificate as configured at the client will not be used by the client because it is rejected by the StrictCertCheck. As the issuer DN is not on the list sent by the server. The server will fail because it is configured to require a client cert and none is provided.
|
This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago |
|
Ping again for the second review. |
|
This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago |
paulidale
added
approval: done
openssl-machine
added
approval: ready to merge
|
This pull request is ready to merge |
|
Merged to master. |
Reviewed-by: Todd Short <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #17989)
Reviewed-by: Todd Short <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#17989)
5 participants
This can be useful for ssl_new testcases.
Checklist