Cleaning up TLS server verify options by shinrich · Pull Request #4414 · apache/trafficserver

shinrich · 2018-10-15T21:44:28Z

Split verify.server into policy and properties components. As discussed on the dev mailing list on Oct 10, 2018. Repeated below

"Currently there is a records.config entry, proxy.config.ssl.client.verify.server, which can be set to 0 (no verify), 1 (strict verify), or 2 (check but only log).

This global setting can be overridden in the ssl_server_name.yaml file using the verify_origin_server parameter which can be set to NONE (no verify), MODERATE (check but only log), and STRICT (verify and enforce).

In PR #4013 CrendKing identified a need to allow for the openssl signature checking but bypassing the Traffic Server verification that the requested SNI is in the certificate. They have their own logic for verifying the validity of the name in the cert and could implement that in a callback on the TS_SSL_SERVER_VERIFY_HOOK.

With another option to our enumeration, I propose rearranging our verify options as follows.

Break the configuration into a policy component and a properties component. In records.config have proxy.config.ssl.client.verify.server.policy and proxy.config.ssl.client.verify.server.properties. The policy entry is one of DISABLED, PERMISSIVE, and ENFORCED (following the selinux nomenclature). The properties entry is a list of the following: SIGNATURE, NAME, or ALL. As we come across more things to check in the future the properties list could be expanded.

A similar change would be made for the ssl_server_name.yaml attributes."

CrendKing · 2018-10-16T00:55:13Z

The properties entry is a list of the following: SIGNATURE, NAME, or ALL. As we come across more things to check in the future the properties list could be expanded.

When more options are added, it will be increasingly difficult to allow user to choose which option combinations to use. For example, when there are A, B, C and ALL, with ALL equivalent to A + B + C, how does user choose A + B only? Does that mean we need to start adding AB, AC, BC along with ALL?

In classic programming world, people use bit wise OR to solve the problem. I wonder if 1) my concern is actually a concern (how likely would C exist); and if yes 2) can we improve the config syntax to make it more future proof?

SolidWallOfCode · 2018-10-16T01:59:49Z

I thought the policy entry would be a list.

shinrich · 2018-10-16T13:34:14Z

The plan is that the properties entry would be a list. Just didn't need to be yet for two properties, so I didn't bother with that parsing logic yet.

In the future if the properties that can be checked by the core Traffic Server logic are SIGNATURE, NAME, FOO, then valid values for proxy.config.ssl.client.verify.server.properties would include
ALL
SIGNATURE,FOO
NAME,FOO
SIGNATURE,NAME
SIGNATURE
NAME
FOO
NONE

Internally, I'm assigning masks to the properties (though with two values the numerical values are the same as a straight enumeration). So in the foo case, FOO=0x4 and ALL=0x7. So we can add bit checks like

if (my_properties & FOO) {
// Do the foo check
}

I completely agree that the internal implementation should involve bitmasks, and that is how it is written in this PR. But I am unclear on how you are suggesting that should be revealed through the configuration interface.

SolidWallOfCode · 2018-10-16T15:29:42Z

It might be reasonable to leave it for now and wait for the YAML conversion where handling lists will be easier.

bryancall · 2018-10-16T17:52:37Z


 #. Enable or disable, per your security policy, server SSL certificate
-   verification using :ts:cv:`proxy.config.ssl.client.verify.server` in
+   verification using :ts:cv:`proxy.config.ssl.client.verify.serve.policyr` in


Looks to be a typo "policyr". Also, since this is supposed to be a compatible change we should not remove the documentation for "proxy.config.ssl.client.verify.server".

I'm adding back the docs of the original settings in the file-based documentation with notes about the old version being deprecated. This file is a to-do list to set up TLS security. I think it is ok to only talk about the new interface here.

Fixing typo. I am adding back the docs on the file-based documentations. Since this seems to be a how-to list, I think it is ok to just discuss the new mechanism here.

bryancall · 2018-10-16T17:53:56Z

+   * Set to DISABLED, PERFMISSIVE, or ENFORCED
+   * Controls how the server certificate verification is handled
+   */
+  uint8_t verifyServerPolicy = 0;


An enum class would would better here and you can specify that it is of size uint8_t.

Yep. Updating. There was already an enum around but for some reason we were storing as the uint8_t. Fixing this eliminated a fair amount of casting,

bryancall · 2018-10-16T17:55:38Z

-  int8_t verifyLevel = 0;       // whether to verify the next hop
-  SSL_CTX *ctx       = nullptr; // ctx generated off the certificate to present to this server
+  const char *name              = nullptr; // name of the server
+  int8_t verifyServerPolicy     = 0;       // whether to verify the next hop


Again, I think an enum class would be better.

shinrich · 2018-10-16T19:38:10Z

Just pushed up another set of changes that includes a test that exercises the case of a plugin with code on the TS_EVENT_SSL_SERVER_VERIFY_HOOK. This code also changes the signature of the TSVConnReenable() to add an event argument. The plugin sets TS_EVENT_VCONN_ERROR in the TSVConnReenable when it determines there is a problem with the origin certificate and the TLS handshake again.

This is another API change, and perhaps should be moved into a separate PR, so the rest of the changes in this PR can be made in the 8.x branch with some hope of backwards compatibility.

shinrich · 2018-10-16T20:54:49Z

Pushed up new commit to fix the plugin examples and address @bryancall 's comments

shinrich · 2018-10-16T21:03:12Z

+
+int
+CB_server_verify(TSCont cont, TSEvent event, void *edata)
+{


@CrendKing here is the test plugin callback to take over for the origin certificate verification. Since this is a canned test, it is kind of a silly test case. If the sni name matches one of the preconfigured "bad name", the plugin reenables with an error which will cause the cert verification to fail.

In the test, I have the base case configured to check no properties. But if you had a real certificate set up, you could set verify_server_properties to SIGNATURE.

Does this meet your use case?

The only part not immediately apparent when comparing to the OpenSSL SSL_set_verify() approach is the assurance of the validity of the peer certificate. From what I understand the code, if signature_ok is 0, the hook callback will not be called at all, correct? So we should be able to assume that the certificate is good at the moment.

If you have server.verify.properties set to include SIGNATURE, then a failure in the signature check will short cut the verification process with a failure and not call the hook.

The hook will be called if the built in checks were enabled and succeeded.

shinrich · 2018-10-17T18:21:01Z

Splitting the TSVConnReenable change and related verify plugin test into a separate PR. So the API changes in this PR should be backwards compatible. Will set up another PR with the verify plugin test changes.

SolidWallOfCode · 2018-10-19T15:02:59Z

+
+   You can override this global setting on a per domain basis in the ssl_servername.yaml file using the :ref:`verify_server_policy attribute<override-verify-server-policy>`.
+
+:DISABLED: Server Certificate will not be verified


You might reformat this as

:code:`DISABLED` Server certificate ... :code:`PERMISSIVE` Server certificate will ...

SolidWallOfCode · 2018-10-19T15:03:44Z

+.. ts:cv:: CONFIG proxy.config.ssl.client.verify.server.properties STRING ALL
   :reloadable:

+   Configures Traffic Server for what the default verify callback should check during origin server verification.


It's better to use |TS| for "Traffic Server".

SolidWallOfCode · 2018-10-19T15:04:30Z

+:ALL: Check both the signature and the name.
+
+.. ts:cv:: CONFIG proxy.config.ssl.client.verify.server INT 0
+   :reloadable


":reloadable:"
Isn't there a ":deprecated:" also?

SolidWallOfCode · 2018-10-19T15:06:05Z

-                        If :code:`false` then HTTP/2 is removed from
-                        the valid next protocol list. It is not an error to set this to :code:`false`
-                        for proxy ports on which HTTP/2 is not enabled.
+client_cert               The client certificate to use for the outbound connection.


Do you mean the path or file containing the client certificate? Or is this the actual certificate?

SolidWallOfCode · 2018-10-19T15:07:00Z


+In addition ``verify_server_properties`` specifies what Traffic Server will check when performing the verification.
+
+:code:`NONE`


This is how the other enumerations should be formatted. See my previous comments.

SolidWallOfCode · 2018-10-19T15:21:49Z

@@ -92,18 +103,34 @@ verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
    if (validate_hostname(cert, sni_name, false, &matched_name)) {


Must this allocate a string?

In the bowel's of the validate_hostname it allocates space for a copy of the peer name if there is a match of a ASN string generated from mucking about in the X509 cert. Afraid that this copying is necessary given out the openssl interface for grubbing about in the cert. The code in question is in src/tscore/X509HostnameValidator.cc.
If we passed in a nullptr, no string would be created.

SolidWallOfCode · 2018-10-19T15:22:27Z

-      return preverify_ok;
+    } else { // Name validation failed
+      // Get the server address if we did't already compute it
+      if (netvc->options.sni_servername) {


Isn't this test backwards?

You mean stylistically? Having the error case as the else?
Or you mean filling in buff? If the sni_servername was not null, we already filled in the buff. In fast sni_name and buff will be pointing at the same memory.

SolidWallOfCode · 2018-10-19T15:29:43Z

+    }
+  }
+
+  if (!verify_backward_compatible) {


Why check this flag? I would think you should just check the source - if both the new and old are set, the new should take precedence.

Oh, we don't want to always use the old if it is set?

SolidWallOfCode · 2018-10-19T15:59:45Z

+  if (!verify_backward_compatible) {
+    char *verify_server = nullptr;
+    REC_ReadConfigStringAlloc(verify_server, "proxy.config.ssl.client.verify.server.policy");
+    if (strcmp(verify_server, "DISABLED") == 0) {


strcasecmp?

We're defining the values as upper case. Should we allow both?

SolidWallOfCode · 2018-10-19T16:03:50Z

-constexpr char TS_verify_origin_server[] = "verify_origin_server";
-constexpr char TS_client_cert[]          = "client_cert";
-constexpr char TS_ip_allow[]             = "ip_allow";
+#define TSDECL(id) constexpr char TS_##id[] = #id


How could you do this to me?

This was @ywkaras's suggestion. Eliminates errors in double typing

@SolidWallOfCode yeah template this one homie!

shinrich · 2018-10-19T21:34:41Z

Updated to address @SolidWallOfCode's comments.

Also rebased against master to pick up conflicting merged changes.

My previous commit updates addressed Bryan's concerns.

Split verify.server into policy and propoerties components.

shinrich · 2018-10-22T14:10:08Z

Updated to rebase against updated master

CrendKing · 2018-11-08T00:40:40Z

Hi @shinrich, I created an issue about this change at #4569. Could you take a look? Thanks.

bryancall · 2018-11-19T18:21:59Z

This PR also needs "Fix chained server cert verify #4497" to fix an issue

zwoop · 2020-03-19T19:01:51Z

This has merge conflicts, and we'll need a new 8.1.x PR. Also, feel free to recommend / suggest other PRs that this depends on, such that it makes the back porting easier.

shinrich added Configuration Tests Documentation labels Oct 15, 2018

shinrich added this to the 9.0.0 milestone Oct 15, 2018

shinrich self-assigned this Oct 15, 2018

shinrich mentioned this pull request Oct 16, 2018

Add hooks for outbound TLS start and close. #4377

Merged

bryancall previously requested changes Oct 16, 2018

View reviewed changes

shinrich force-pushed the update-servrer-verify branch from e665f1c to de7a416 Compare October 16, 2018 19:25

shinrich force-pushed the update-servrer-verify branch from de7a416 to 159af65 Compare October 16, 2018 20:54

shinrich commented Oct 16, 2018

View reviewed changes

shinrich force-pushed the update-servrer-verify branch from 159af65 to 5ae5141 Compare October 17, 2018 18:19

shinrich mentioned this pull request Oct 17, 2018

Add vconn reenable event #4427

Merged

shinrich force-pushed the update-servrer-verify branch from 5ae5141 to e85b293 Compare October 18, 2018 15:14

SolidWallOfCode suggested changes Oct 19, 2018

View reviewed changes

shinrich force-pushed the update-servrer-verify branch from e85b293 to 38ed35d Compare October 19, 2018 21:34

SolidWallOfCode previously approved these changes Oct 21, 2018

View reviewed changes

Cleaning up TLS server verify options

db825d3

Split verify.server into policy and propoerties components.

shinrich dismissed SolidWallOfCode’s stale review via db825d3 October 22, 2018 14:09

shinrich force-pushed the update-servrer-verify branch from 38ed35d to db825d3 Compare October 22, 2018 14:09

SolidWallOfCode approved these changes Oct 22, 2018

View reviewed changes

shinrich merged commit 5cd7b75 into apache:master Oct 22, 2018

shinrich mentioned this pull request Oct 25, 2018

Fix chained server cert verify #4497

Merged

bryancall added Incompatible and removed Incompatible labels Nov 19, 2018

shinrich mentioned this pull request Nov 29, 2018

Add back in the option to conf_remap the verify_server settings. #4663

Merged


		You can override this global setting on a per domain basis in the ssl_servername.yaml file using the :ref:`verify_server_policy attribute<override-verify-server-policy>`.

		:DISABLED: Server Certificate will not be verified


		In addition ``verify_server_properties`` specifies what Traffic Server will check when performing the verification.

		:code:`NONE`

		@@ -92,18 +103,34 @@ verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
		if (validate_hostname(cert, sni_name, false, &matched_name)) {

Conversation

shinrich commented Oct 15, 2018

Uh oh!

CrendKing commented Oct 16, 2018

Uh oh!

SolidWallOfCode commented Oct 16, 2018

Uh oh!

shinrich commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SolidWallOfCode commented Oct 16, 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!

Uh oh!

shinrich commented Oct 16, 2018

Uh oh!

shinrich commented Oct 16, 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!

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!

shinrich commented Oct 19, 2018

Uh oh!

shinrich commented Oct 22, 2018

Uh oh!

CrendKing commented Nov 8, 2018

Uh oh!

bryancall commented Nov 19, 2018

Uh oh!

zwoop commented Mar 19, 2020

Uh oh!

Reviewers

shinrich commented Oct 16, 2018 •

edited

Loading