Add vconn reenable event by shinrich · Pull Request #4427 · apache/trafficserver

shinrich · 2018-10-17T18:50:18Z

Depends on PR #4414. So this PR is only the second commit. Most of the changes are in the first commit (PR #4414).

Adds TSVConnReenableEx(), which is like TSVConnReenable() but it takes a TSEvent argument as the Txn and Ssn reenable functions do. With the event argument the plugin can indicate back to the core whether the plugin fails and the underlying VConn should error out.

This PR adds a test to exercise the TS_SSL_SERVER_VERIFY_HOOK with a plugin. The plugin fails some of the handshakes based on detecting a "bad" cert.

As an API change, I will send an API review email on the dev mailing list.

ywkaras · 2018-10-17T19:28:31Z

+
+An extended verion of TSVConnEnable that allows the plugin to return a status to
+the core logic.  If all goes well this is TS_EVENT_VCONN_CONTINUE.  However, if
+the plugin wants to stop the processing it can set the event to TS_EVENT_VCONN_CONTINUE.


Do you mean TS_EVENT_VCONN_ERROR?

Yep, will fix.

ywkaras · 2018-10-17T19:37:36Z

-  TS_EVENT_SSL_VERIFY_CLIENT                    = 60206
+  TS_EVENT_SSL_VERIFY_CLIENT                    = 60206,
+  TS_EVENT_VCONN_CONTINUE                       = 60207,
+  TS_EVENT_VCONN_ERROR                          = 60208


Why can't TS_EVENT_CONTINUE and TS_EVENT_ERROR (generic) be used? Or, why do we need an event at all, why not:

tsapi void TSVConnErrorTermination(TSVConn sslvcp);

instead of:

tsapi void TSVConnReenableEx(TSVConn sslvcp, TSEvent event);

Hmmm. There is a already a TS_EVENT_ERROR/CONTINUE. I was following the pattern of TSHttpTxnReenable/TSHttpSsnReenable with TS_EVENT_HTTP_ERROR/CONTINUE.

We could use TS_EVENT_ERROR/CONTINUE instead of adding another pair of events.

I do like the Reenable pattern with a general event argument rather than having an explicit error reenable method. This gives you the option of inserting different events to impact processing in the future.

ywkaras · 2018-10-17T19:45:57Z

+  if (netvc && netvc->options.verifyServerPolicy == YamlSNIConfig::Policy::DISABLED) {
+    return true;       // Tell them that all is well
+  } else if (!netvc) { // No netvc, very bad.  Go away.  Things are not good.
+    return false;


We don't need an entry in diags.log, or even ink_assert in this case?

Probably. This is in the other commit. Will update in the other PR.

ywkaras · 2018-10-17T19:47:08Z

+  cert  = X509_STORE_CTX_get_current_cert(ctx);
+  err   = X509_STORE_CTX_get_error(ctx);
+
+  bool enforce_mode = (netvc && netvc->options.verifyServerPolicy == YamlSNIConfig::Policy::ENFORCED);


After line 59 no more checks of netvc being non-null are needed.

True. Will fix in other PR

ywkaras · 2018-10-17T19:55:05Z

+      Warning("%s is invalid for proxy.config.ssl.client.verify.server.policy.  Should be one of DISABLED, PERMISSIVE, or ENFORCED",
+              verify_server);
+      verifyServerPolicy = YamlSNIConfig::Policy::DISABLED;
+      ;


Emoji? @SolidWallOfCode may get jealous wondering who you're winking at.

ywkaras · 2018-10-17T19:55:27Z

+      verifyServerProperties = YamlSNIConfig::Property::ALL_MASK;
+    } else if (strcmp(verify_server, "NONE") == 0) {
+      verifyServerProperties = YamlSNIConfig::Property::NONE;
+      ;


More cause for jealously?

ywkaras · 2018-10-17T19:57:33Z

        if (!this->getSSLHandShakeComplete()) {
-          this->sslHandShakeComplete = true;
+          this->sslHandshakeStatus = SSL_HANDSHAKE_DONE;
+          ;


ywkaras · 2018-10-17T19:58:27Z

+    ats_free(verify_server);
+
+    REC_ReadConfigStringAlloc(verify_server, "proxy.config.ssl.client.verify.server.properties");
+    if (strcmp(verify_server, "SIGNATURE") == 0) {


should these be strcasecmp()?

No the keyword is upcase

ywkaras · 2018-10-17T20:10:14Z

-constexpr char TS_verify_origin_server[] = "verify_origin_server";
-constexpr char TS_client_cert[]          = "client_cert";
-constexpr char TS_ip_allow[]             = "ip_allow";
+constexpr char TS_fqdn[]                     = "fqdn";


#define X(ID) constexpr char TS_##ID #ID; X(fqdn)

Ok, I see. A means to avoid double typing (and messing up) the same string.

It's just a thought, I suppose some might find it overly tricky. To me, this sort of technique makes it more clear when there's a consistent pattern. Also helps avoid typos.

ywkaras · 2018-10-17T20:10:37Z

+constexpr char TS_verify_server_properties[] = "verify_server_properties";
+constexpr char TS_verify_origin_server[]     = "verify_origin_server";
+constexpr char TS_client_cert[]              = "client_cert";
+constexpr char TS_ip_allow[]                 = "ip_allow";


shinrich · 2018-10-18T15:14:58Z

Addressed the remainder of @ywkaras's comments on PR #4414

shinrich · 2018-10-18T19:56:59Z

Accidentally closed.

shinrich · 2018-10-22T20:31:23Z

Updated the PR to rebase against the pre-cursor PR which has been merged in.

shinrich · 2018-10-24T20:02:24Z

updated to actually add the test files to the PR.

shinrich added Plugins TLS Tests TS API Documentation labels Oct 17, 2018

shinrich added this to the 9.0.0 milestone Oct 17, 2018

shinrich self-assigned this Oct 17, 2018

ywkaras reviewed Oct 17, 2018

View reviewed changes

shinrich force-pushed the add-vconn-reenable-event branch 2 times, most recently from ec9c52c to 949e83a Compare October 18, 2018 14:45

shinrich closed this Oct 18, 2018

shinrich reopened this Oct 18, 2018

CrendKing mentioned this pull request Oct 19, 2018

Update documentation for SSL VERIFY hooks. #4385

Merged

shinrich force-pushed the add-vconn-reenable-event branch from 949e83a to 25119ea Compare October 22, 2018 20:29

Add VConn Reenable event so plugins can error processing.

64359be

shinrich force-pushed the add-vconn-reenable-event branch from 25119ea to 64359be Compare October 24, 2018 20:01

SolidWallOfCode approved these changes Oct 25, 2018

View reviewed changes

shinrich merged commit 63873d4 into apache:master Oct 29, 2018

shinrich mentioned this pull request Oct 31, 2018

Create a plugin API to indicate that the SSL connection should shutdown #1922

Closed

Conversation

shinrich commented Oct 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shinrich commented Oct 18, 2018

Uh oh!

shinrich commented Oct 18, 2018

Uh oh!

shinrich commented Oct 22, 2018

Uh oh!

shinrich commented Oct 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants