Update documentation for SSL VERIFY hooks.#4385
Conversation
|
Hmmm, is this documentation for a yet to be landed hook? If so, why not merge this in with that commit? |
|
Also, not that it matters much, but I think we (for some reason :-), try to keep documentation lines < 80 characters. |
|
Both hooks already exist in the code. Just wasn't documented yet. Hmm. But looking more closely, it looks like the SERVER version of the name is a bit different. Back in again to tidy up. |
shinrich
force-pushed
the
update-verify-hook-documentation
branch
from
342a93b to
c17e893
Compare
|
I updated the docs and the constants to be consistent. Both hooks already existing, but one was TS_SSL_VERIFY_CLIENT_HOOK and the other was TS_SSL_SERVER_VERIFY_HOOK. I changed them both to the TS_SSL_VERIFY_* form. Updated the enums, but left aliases of the old versions for backwards compatibility. Will be adding tests in a separate PR. |
| TS_SSL_VERIFY_SERVER_HOOK | ||
| ------------------------- | ||
|
|
||
| This hooks is called when a client connects to Traffic Server and presents a |
There was a problem hiding this comment.
This comment does not sound correct. It seems to be copied verbatim from the TS_SSL_VERIFY_CLIENT_HOOK section.
| TS_SSL_VERIFY_CLIENT_HOOK | ||
| ------------------------- | ||
|
|
||
| This hooks is called when a client connects to Traffic Server and presents a |
shinrich
force-pushed
the
update-verify-hook-documentation
branch
from
c17e893 to
9c909bb
Compare
|
Updated to address @CrendKing's review comments. |
| TS_SSL_VERIFY_SERVER_HOOK | ||
| ------------------------- | ||
|
|
||
| This hooks is called when a Traffic Server connections to an origin and the origin |
shinrich
force-pushed
the
update-verify-hook-documentation
branch
from
9c909bb to
a7a1b57
Compare
|
Updated to address @CrendKing's comment. |
|
@CrendKing @zwoop Any other comments or are we good to go? |
| presents a certificate. The callback can get the SSL object from the TSVConn | ||
| argument and use that to access the origin certificate and make any additional checks. | ||
|
|
||
| Processing will continue regardless of whether the hook callback executes |
There was a problem hiding this comment.
Correct me if I'm wrong. #4427 adds the possibility to "interrupt" the workflow by calling TSVConnReenableEx() with TS_EVENT_ERROR. Seems contradictory once both PRs are committed.
There was a problem hiding this comment.
Correct. The underlying openssl structure limits what we can do. However, in the case of the verify_server hook, we cannot leave the callback not-enabled. But we can return an ERROR and have the core ATS verify_callback interpret that to fail the certificate verification.
There was a problem hiding this comment.
The underlying openssl structure limits what we can do
Just to make sure we are on the side page. We agree that if the callback function from SSL_set_verify() returns 0, the OpenSSL will stop the TLS handshake, right? So what do you mean here?
https://www.openssl.org/docs/man1.0.2/ssl/SSL_set_verify.html
The return value of verify_callback controls the strategy of the further verification process. If verify_callback returns 0, the verification process is immediately stopped with "verification failed" state.
There was a problem hiding this comment.
Correct. The verify_callback() function is registered by Traffic Server via the SSL_CTX_set_verify or SSL_set_verify openssl function. It will eventually return either 0 or 1. If it returns 0, the handshake will fail . It it returns 1, the handshake will continue on.
The VERIFY_SERVER_HOOK gives a place for plugins to insert logic and influence the return value of the verify_callback(). If the hook reenables with a TS_EVENT_ERROR, the verify_callback() will return 0.
|
Because of conflicts a new PR was created to merge into 8.1.0: #5236 |
5 participants
No description provided.