Skip to content

Add hooks for outbound TLS start and close.#4377

Merged
shinrich merged 1 commit intoapache:masterfrom
shinrich:outbound_hooks
Oct 22, 2018
Merged

Add hooks for outbound TLS start and close.#4377
shinrich merged 1 commit intoapache:masterfrom
shinrich:outbound_hooks

Conversation

@shinrich
Copy link
Copy Markdown
Member

@shinrich shinrich commented Oct 9, 2018

Adds new hook points for plugins at the start and the close of an outbound TLS connection. Also adding two new Plugin APIs so the plugin can fetch the invoking Continuation or Transaction from the new outbound TLS connection.

The commit includes tests and documentation.

As this PR includes new APIs, I will send an API review email around.

@randall
Copy link
Copy Markdown
Contributor

randall commented Oct 11, 2018

[approve ci autest]

@shinrich shinrich force-pushed the outbound_hooks branch 3 times, most recently from 7f0cb6a to f5d5481 Compare October 12, 2018 18:57
Copy link
Copy Markdown
Member

@SolidWallOfCode SolidWallOfCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it matter if the close hook callback calls TSVConnReenable?


.. default-domain:: c

TSNetConnect
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this for TSNetConnect or TSNetInvokingContGet?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, copy error. Will update.

----------------------------

This hook is invoked after ATS has connected to the upstream server and before the SSL handshake has started. This gives the plugin the option of
overriding the default SSL connections on the SSL object.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"default SSL connections"? Do you mean "connection options"?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct. Will update.

Comment thread iocore/net/SSLNetVConnection.cc Outdated
@@ -834,7 +834,12 @@ void
SSLNetVConnection::do_io_close(int lerrno)
{
if (this->ssl != nullptr && sslHandShakeComplete) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the SSL handshake fails to complete, can it be the case that the START hook is called but not the CLOSE hook?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, as it stands that can be the case. Probably not a great thing. Will look into adding the hook invocation in the pre-handshake complete error cases.

@CrendKing
Copy link
Copy Markdown

I see the testing of whether the hooks are invoked, but it is not clear to me if we could alter the logic flow of the TLS handshake by manipulating the continuation.

In #4013, I wanted to achieve this: AFTER OpenSSL verifies the server certificate against CA, inspect the server certificate using plugin-supplied function, and continue/terminate the TLS handshake based on the return value of the function.

Can this implementation achieve the same?

@shinrich
Copy link
Copy Markdown
Member Author

@CrendKing I think you want to look at PR #4414 and PR #4385. In that PR, you can set server.verify.policy to ENFORCED and server.verify.propoerties to SIGNATURE (ignoring the standard name requirements) and then register your plugin on the TS_SSL_VERIFY_SERVER_HOOK hook. If your plugin is invoked and is not pleased with the certificate, it can terminate the connection.

I'll add a test for this case tomorrow in PR #4414.

@CrendKing
Copy link
Copy Markdown

CrendKing commented Oct 16, 2018

If your plugin is invoked and is not pleased with the certificate, it can terminate the connection.

I read #4414, and I found this code is used to handle the hook

    // If the previous configured checks passed, give the hook a try
+   netvc->callHooks(TS_EVENT_SSL_SERVER_VERIFY_HOOK);
  }
  return signature_ok;

I do not quite get how does the hook function from my plugin could alter the outcome of the OpenSSL verify function. The return value is not used, and I assume OpenSSL does not take ATS continuation into consideration in any way. Could you elaborate?

@shinrich
Copy link
Copy Markdown
Member Author

Updated to address @SolidWallOfCode's comments

@@ -0,0 +1,72 @@
'''
Test one delayed preaccept callback
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this different from test 13? It has the same comment.

It may be useful to delay the TLS handshake processing if other resources must be consulted to select or create
a certificate.

TS_VCONN_OUTBOUND_START_HOOK
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TS_VCONN_OUTBOUND_START_HOOK - The use case I need to address requires that the plugin set the certificate used in the origin side TLS mutual auth, based on information that came in on client side request. This hook looks like it will be perfectly timed. 👍🏻

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. that was our motivating use case as well.

@shinrich
Copy link
Copy Markdown
Member Author

Addressed @CredKing's issue in PR #4414

@shinrich shinrich merged commit 8e309ff into apache:master Oct 22, 2018
@bryancall bryancall removed this from the 9.0.0 milestone Apr 11, 2019
@bryancall
Copy link
Copy Markdown
Contributor

A backport PR was created for 8.1.0 because of conflicts: #5237

@bryancall bryancall added this to the Backported milestone Apr 11, 2019
@zwoop zwoop modified the milestones: Backported, 9.0.0 Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants