trafficserver-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Peach <jpe...@apache.org>
Subject Re: [1/2] trafficserver git commit: [TS-3153]: Ability to disable or modify npn advertisement based on SNI
Date Tue, 18 Nov 2014 18:28:02 GMT

> On Nov 17, 2014, at 1:12 PM, sudheerv@apache.org wrote:
> 
> Repository: trafficserver
> Updated Branches:
>  refs/heads/master 08f19da9b -> 34ca6f2dc
> 
> 
> [TS-3153]: Ability to disable or modify npn advertisement based on SNI
> 
> 
> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
> Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/24262d8f
> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/24262d8f
> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/24262d8f
> 
> Branch: refs/heads/master
> Commit: 24262d8f6a14b6bb7bf7288f6309a68f6dc8589b
> Parents: 08f19da
> Author: Sudheer Vinukonda <sudheerv@yahoo-inc.com>
> Authored: Mon Nov 17 21:10:59 2014 +0000
> Committer: Sudheer Vinukonda <sudheerv@yahoo-inc.com>
> Committed: Mon Nov 17 21:10:59 2014 +0000
> 
> ----------------------------------------------------------------------
> configure.ac                                    |   1 +
> iocore/net/P_SSLNetVConnection.h                |   6 +
> iocore/net/SSLNetVConnection.cc                 |  82 +++++++-
> iocore/net/SSLUtils.cc                          |   5 +
> plugins/experimental/Makefile.am                |   1 +
> plugins/experimental/sni_proto_nego/Makefile.am |  21 ++
> .../sni_proto_nego/sni_proto_nego.cc            | 194 +++++++++++++++++++
> proxy/InkAPI.cc                                 |  10 +
> proxy/api/ts/ts.h                               |   1 +
> 9 files changed, 320 insertions(+), 1 deletion(-)
> ----------------------------------------------------------------------
> 
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/24262d8f/configure.ac
> ----------------------------------------------------------------------
> diff --git a/configure.ac b/configure.ac
> index 3e4465b..91e9874 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1945,6 +1945,7 @@ AS_IF([test "x$enable_experimental_plugins" = xyes], [
>     plugins/experimental/regex_revalidate/Makefile
>     plugins/experimental/remap_stats/Makefile
>     plugins/experimental/s3_auth/Makefile
> +    plugins/experimental/sni_proto_nego/Makefile
>     plugins/experimental/sslheaders/Makefile
>     plugins/experimental/ssl_cert_loader/Makefile
>     plugins/experimental/stale_while_revalidate/Makefile
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/24262d8f/iocore/net/P_SSLNetVConnection.h
> ----------------------------------------------------------------------
> diff --git a/iocore/net/P_SSLNetVConnection.h b/iocore/net/P_SSLNetVConnection.h
> index c481c8b..1dc7071 100644
> --- a/iocore/net/P_SSLNetVConnection.h
> +++ b/iocore/net/P_SSLNetVConnection.h
> @@ -122,6 +122,9 @@ public:
>   static int advertise_next_protocol(SSL * ssl, const unsigned char ** out, unsigned
* outlen, void *);
>   static int select_next_protocol(SSL * ssl, const unsigned char ** out, unsigned char
* outlen, const unsigned char * in, unsigned inlen, void *);
> 
> +  bool modify_npn_advertisement(const unsigned char ** list, unsigned cnt);
> +  bool setAdvertiseProtocols(const unsigned char ** list, unsigned cnt);
> +
>   Continuation * endpoint() const {
>     return npnEndpoint;
>   }
> @@ -198,6 +201,9 @@ private:
> 
>   const SSLNextProtocolSet * npnSet;
>   Continuation * npnEndpoint;
> +  unsigned char * npnAdvertised;
> +  size_t npnszAdvertised;
> +  int npnAdvertisedBufIndex;
> };
> 
> typedef int (SSLNetVConnection::*SSLNetVConnHandler) (int, void *);
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/24262d8f/iocore/net/SSLNetVConnection.cc
> ----------------------------------------------------------------------
> diff --git a/iocore/net/SSLNetVConnection.cc b/iocore/net/SSLNetVConnection.cc
> index 4a9ec29..60fcbf9 100644
> --- a/iocore/net/SSLNetVConnection.cc
> +++ b/iocore/net/SSLNetVConnection.cc
> @@ -27,6 +27,8 @@
> #include "P_SSLUtils.h"
> #include "InkAPIInternal.h"	// Added to include the ssl_hook definitions
> 
> +extern unsigned char * append_protocol(const char * proto, unsigned char * buf);

This is non-static by accident. Please make a static helper API in SSLNextProtocolSet to construct
the advertisement. Maybe something like this:

size_t SSLNextProtocolSet::create_advertisement(const char **, unsigned, unsigned char * buf,
size_t bufsz) 

> +
> // Defined in SSLInternal.c, should probably make a separate include
> // file for this at some point
> void SSL_set_rbio(SSLNetVConnection *sslvc, BIO *rbio);
> @@ -776,7 +778,10 @@ SSLNetVConnection::SSLNetVConnection():
>   sslPreAcceptHookState(SSL_HOOKS_INIT),
>   sslSNIHookState(SNI_HOOKS_INIT),
>   npnSet(NULL),
> -  npnEndpoint(NULL)
> +  npnEndpoint(NULL),
> +  npnAdvertised(NULL),
> +  npnszAdvertised(0),
> +  npnAdvertisedBufIndex(-1)

You don't need to store npnAdvertisedBufIndex, since the size tells you which iobuf allocator
to use.

> {
> }
> 
> @@ -815,6 +820,9 @@ SSLNetVConnection::free(EThread * t) {
>   hookOpRequested = TS_SSL_HOOK_OP_DEFAULT;
>   npnSet = NULL;
>   npnEndpoint= NULL;
> +  npnAdvertised = NULL;
> +  npnszAdvertised = 0;
> +  npnAdvertisedBufIndex = -1;

I think you need to release npnAdvertised here.

> 
>   if (from_accept_thread) {
>     sslNetVCAllocator.free(this);
> @@ -1160,6 +1168,14 @@ SSLNetVConnection::advertise_next_protocol(SSL *ssl, const unsigned
char **out,
> 
>   ink_release_assert(netvc != NULL);
> 
> +  // check if there's a SNI based customized advertisement
> +  if (netvc->npnAdvertised && netvc->npnszAdvertised) {
> +    *out = netvc->npnAdvertised;
> +    *outlen = netvc->npnszAdvertised;
> +    return SSL_TLSEXT_ERR_OK;
> +  }

I believe that Alan's suggestion was to construct a new SSLNextProtocolSet and attach it to
SSLNetVConnection::npnSet. You could use the lower pointer bits to know whether you have a
shared or unique set.

You also need to make some changes to SSLNetVConnection::select_next_protocol(). We don't
distinguish between NPN and ALPN.

> +  // use default endPoint advertisement
>   if (netvc->npnSet && netvc->npnSet->advertiseProtocols(out, outlen))
{
>     // Successful return tells OpenSSL to advertise.
>     return SSL_TLSEXT_ERR_OK;
> @@ -1168,6 +1184,70 @@ SSLNetVConnection::advertise_next_protocol(SSL *ssl, const unsigned
char **out,
>   return SSL_TLSEXT_ERR_NOACK;
> }
> 
> +bool
> +SSLNetVConnection::modify_npn_advertisement(const unsigned char ** list, unsigned cnt)

I don't really see why you need this. Just do it inline in the caller.

> +{
> +  unsigned char* advertised = npnAdvertised;
> +
> +  for (unsigned int i=0; i<cnt; i++) {
> +    const char* proto = (const char*) list[i];
> +    Debug("ssl", "advertising protocol %s", proto);
> +    advertised = append_protocol(proto, advertised);
> +  }
> +
> +  return true;
> +}
> +
> +bool
> +SSLNetVConnection::setAdvertiseProtocols(const unsigned char ** list, unsigned cnt)

This should be spelled setAdvertisedProtocols

> +{
> +  size_t total_len = 0;
> +
> +  if (cnt == 0) {

list == NULL or cnt == 0 should just fail. The npn set already is the default. I can see that
this would let you return to the default after you have set the advertisement once, but I
don't think that would ever get used.

> +    // set default list based on server_ports config
> +    if (npnAdvertised) {
> +      ink_assert (npnAdvertisedBufIndex >= 0);
> +      ioBufAllocator[npnAdvertisedBufIndex].free_void(npnAdvertised);
> +      npnAdvertised = NULL;
> +      npnszAdvertised = 0;
> +      npnAdvertisedBufIndex = -1;
> +    }
> +    return true;
> +  }
> +
> +  // validate the modified npn list
> +  for (unsigned int i=0; i<cnt; i++) {

Style -- there should be single space around binary operators. i = 0; i < cnt

> +    const char* proto = (const char*) list[i];
> +    size_t len = strlen(proto);
> +
> +    // Both ALPN and NPN only allow 255 bytes of protocol name.
> +    if (len > 255) {
> +      return false;
> +    }

There's no way for this to happen since you require the protocols to already be in the set.

> +
> +    if (!npnSet->findEndpoint((const unsigned char *)proto, len)) {
> +      return false;
> +    }
> +    total_len += (len + 1);
> +  }
> +
> +  if (npnAdvertised) {
> +    ink_assert (npnAdvertisedBufIndex >= 0);
> +    ioBufAllocator[npnAdvertisedBufIndex].free_void(npnAdvertised);
> +  }
> +
> +  npnszAdvertised = total_len;
> +  npnAdvertisedBufIndex = buffer_size_to_index(npnszAdvertised);

Should this be iobuffer_size_to_index()?

> +  npnAdvertised = (unsigned char *)ioBufAllocator[npnAdvertisedBufIndex].alloc_void();
> +  if (npnAdvertised == NULL) {
> +    npnszAdvertised = 0;
> +    npnAdvertisedBufIndex = -1;
> +    return false;
> +  }
> +
> +  return modify_npn_advertisement(list, cnt);
> +}
> +
> // ALPN TLS extension callback. Given the client's set of offered
> // protocols, we have to select a protocol to use for this session.
> int
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/24262d8f/iocore/net/SSLUtils.cc
> ----------------------------------------------------------------------
> diff --git a/iocore/net/SSLUtils.cc b/iocore/net/SSLUtils.cc
> index 537cc9d..a6ef4b7 100644
> --- a/iocore/net/SSLUtils.cc
> +++ b/iocore/net/SSLUtils.cc
> @@ -305,6 +305,11 @@ ssl_servername_callback(SSL * ssl, int * ad, void * /*arg*/)
>     goto done;
>   }
> 
> +  // set the default 
> +#if TS_USE_TLS_NPN
> +  SSL_CTX_set_next_protos_advertised_cb(ctx, SSLNetVConnection::advertise_next_protocol,
NULL);
> +#endif /* TS_USE_TLS_NPN */

What is this change for?

> +
>   // Call the plugin SNI code
>   reenabled = netvc->callHooks(TS_SSL_SNI_HOOK);
>   // If it did not re-enable, return the code to
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/24262d8f/plugins/experimental/Makefile.am
> ----------------------------------------------------------------------
> diff --git a/plugins/experimental/Makefile.am b/plugins/experimental/Makefile.am
> index 51b06f0..091557d 100644
> --- a/plugins/experimental/Makefile.am
> +++ b/plugins/experimental/Makefile.am
> @@ -33,6 +33,7 @@ SUBDIRS = \
>  regex_revalidate \
>  remap_stats \
>  s3_auth \
> + sni_proto_nego \
>  ssl_cert_loader \
>  sslheaders \
>  stale_while_revalidate \
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/24262d8f/plugins/experimental/sni_proto_nego/Makefile.am
> ----------------------------------------------------------------------
> diff --git a/plugins/experimental/sni_proto_nego/Makefile.am b/plugins/experimental/sni_proto_nego/Makefile.am
> new file mode 100644
> index 0000000..958634c
> --- /dev/null
> +++ b/plugins/experimental/sni_proto_nego/Makefile.am
> @@ -0,0 +1,21 @@
> +#  Licensed to the Apache Software Foundation (ASF) under one
> +#  or more contributor license agreements.  See the NOTICE file
> +#  distributed with this work for additional information
> +#  regarding copyright ownership.  The ASF licenses this file
> +#  to you under the Apache License, Version 2.0 (the
> +#  "License"); you may not use this file except in compliance
> +#  with the License.  You may obtain a copy of the License at
> +#
> +#      http://www.apache.org/licenses/LICENSE-2.0
> +#
> +#  Unless required by applicable law or agreed to in writing, software
> +#  distributed under the License is distributed on an "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> +#  See the License for the specific language governing permissions and
> +#  limitations under the License.
> +
> +include $(top_srcdir)/build/plugins.mk
> +
> +pkglib_LTLIBRARIES = sni_proto_nego.la
> +sni_proto_nego_la_SOURCES = sni_proto_nego.cc
> +sni_proto_nego_la_LDFLAGS = $(TS_PLUGIN_LDFLAGS)
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/24262d8f/plugins/experimental/sni_proto_nego/sni_proto_nego.cc
> ----------------------------------------------------------------------
> diff --git a/plugins/experimental/sni_proto_nego/sni_proto_nego.cc b/plugins/experimental/sni_proto_nego/sni_proto_nego.cc
> new file mode 100644
> index 0000000..cd1f4db
> --- /dev/null
> +++ b/plugins/experimental/sni_proto_nego/sni_proto_nego.cc
> @@ -0,0 +1,194 @@
> +#include <stdio.h>
> +#include <ts/ts.h>
> +#include <ts/apidefs.h>
> +#include <openssl/ssl.h>
> +#include <string>
> +#include <map>
> +#include <string.h>
> +
> +using namespace std;
> +
> +const char* PLUGIN_NAME = "sni_proto_nego";
> +const int MAX_BUFFER_SIZE = 1024;
> +const int MAX_FILE_PATH_SIZE = 1024;
> +const unsigned int MAX_PROTO_LIST_LEN = 100;
> +const unsigned int MAX_PROTO_NAME_LEN = 255;
> +
> +typedef struct {
> +  bool enableNpn;
> +  unsigned int npn_proto_list_count;
> +  unsigned char npn_proto_list [MAX_PROTO_LIST_LEN] [MAX_PROTO_NAME_LEN];
> +} SNIProtoConfig;
> +
> +typedef map<string, SNIProtoConfig> stringMap;
> +static  stringMap _sniProtoMap;
> +
> +static
> +bool read_config(char* config_file) {
> +  char file_path[MAX_FILE_PATH_SIZE];
> +  TSFile file;
> +  if (config_file == NULL) {
> +    TSError("invalid config file");
> +    return false;
> +  }
> +  TSDebug(PLUGIN_NAME, "trying to open config file in this path: %s", file_path);
> +  file = TSfopen(config_file, "r");
> +  if (file == NULL) {
> +    snprintf(file_path, sizeof(file_path), "%s/%s", TSInstallDirGet(), config_file);

Config files are always relative to TSConfigDirGet()

> +    file = TSfopen(file_path, "r");
> +    if (file == NULL) {
> +      TSError("Failed to open config file %s", config_file);
> +      return false;
> +    }
> +  }
> +  char buffer[MAX_BUFFER_SIZE];
> +  memset(buffer, 0, sizeof(buffer));
> +  while (TSfgets(file, buffer, sizeof(buffer) - 1) != NULL) {
> +    char *eol = 0;
> +    // make sure line was not bigger than buffer
> +    if ((eol = strchr(buffer, '\n')) == NULL && (eol = strstr(buffer, "\r\n"))
== NULL) {
> +      TSError("sni_proto_nego line too long, did not get a good line in cfg, skipping,
line: %s", buffer);
> +      memset(buffer, 0, sizeof(buffer));
> +      continue;
> +    }
> +    // make sure line has something useful on it
> +    if (eol - buffer < 2 || buffer[0] == '#') {
> +      memset(buffer, 0, sizeof(buffer));
> +      continue;
> +    }
> +    char* cfg = strtok(buffer, "\n\r\n");
> +
> +    if (cfg != NULL) {
> +        TSDebug(PLUGIN_NAME, "setting SniProto based on string: %s", cfg);
> +
> +        char* domain = strtok(buffer, " ");
> +        SNIProtoConfig sniProtoConfig = {1, 1};
> +
> +        if (domain) {
> +          if ((*domain == '*') && (domain+1) && (*(domain+1)=='.'))
{
> +            domain += 2;
> +            if (domain == NULL) {
> +              continue;
> +            }
> +          }
> +          char* sni_proto_config = strtok (NULL, " ");
> +          if (sni_proto_config) {
> +            sniProtoConfig.enableNpn = atoi(sni_proto_config);
> +            TSDebug(PLUGIN_NAME, "npn_proto_config %d", sniProtoConfig.enableNpn);
> +            sni_proto_config = strtok (NULL, " ");
> +            // now get the npn proto advertisment list
> +            sni_proto_config = strtok (NULL, " ");
> +            sniProtoConfig.npn_proto_list_count = 0;
> +            while (sni_proto_config != NULL) {
> +              char* proto = strtok(NULL, "|");
> +              if ((proto == NULL) ||
> +                  (sniProtoConfig.npn_proto_list_count >= MAX_PROTO_LIST_LEN) ||
> +                  (strlen(proto) >= MAX_PROTO_NAME_LEN)) {
> +                break;
> +              }
> +              _TSstrlcpy((char*)sniProtoConfig.npn_proto_list[sniProtoConfig.npn_proto_list_count++],
proto, (strlen(proto) + 1));
> +            }
> +          }
> +          _sniProtoMap.insert(make_pair(domain, sniProtoConfig));
> +        }
> +
> +        memset(buffer, 0, sizeof(buffer));
> +    }
> +  }
> +
> +  TSfclose(file);
> +
> +  TSDebug(PLUGIN_NAME, "Done parsing config");
> +
> +  return true;
> +}
> +
> +
> +static void
> +init_sni_callback(void *sslNetVC)
> +{
> +  TSVConn ssl_vc = reinterpret_cast<TSVConn>(sslNetVC);
> +  TSSslConnection sslobj = TSVConnSSLConnectionGet(ssl_vc);
> +  SSL *ssl = reinterpret_cast<SSL *>(sslobj);
> +  const char *serverName = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name);
> +  SSL_CTX * ctx = SSL_get_SSL_CTX(ssl);
> +
> +  if (serverName == NULL) {
> +    TSDebug(PLUGIN_NAME, "invalid ssl netVC %p, servername %s for ssl obj %p", sslNetVC,
serverName, ssl);
> +    return;
> +  }
> +
> +  TSDebug(PLUGIN_NAME, "ssl netVC %p, servername %s for ssl obj %p", sslNetVC, serverName,
ssl);
> +
> +  stringMap::iterator it; 
> +  it=_sniProtoMap.find(serverName);
> +
> +  // check for wild-card domains
> +  if(it==_sniProtoMap.end()) {
> +    char* domain = strstr((char*)serverName, ".");
> +    if (domain && (domain+1)) {
> +      it=_sniProtoMap.find(domain+1);  
> +    }
> +  }
> +
> +  if (it!=_sniProtoMap.end()) {
> +    SNIProtoConfig sniProtoConfig = it->second; 
> +    if (!sniProtoConfig.enableNpn) {
> +      TSDebug(PLUGIN_NAME, "disabling NPN for serverName %s", serverName);
> +      SSL_CTX_set_next_protos_advertised_cb(ctx, NULL, NULL);
> +    } else {
> +      TSDebug(PLUGIN_NAME, "setting NPN advertised list for %s", serverName);
> +      TSSslAdvertiseProtocolSet(ssl_vc, (const unsigned char **)sniProtoConfig.npn_proto_list,
sniProtoConfig.npn_proto_list_count);
> +    }
> +  } else {
> +    TSDebug(PLUGIN_NAME, "setting NPN advertised list for %s", serverName);
> +    TSSslAdvertiseProtocolSet(ssl_vc, NULL, 0);
> +  }
> +}
> +
> +int
> +SSLSniInitCallbackHandler(TSCont cont, TSEvent id, void* sslNetVC) {
> +  (void) cont;
> +  TSDebug(PLUGIN_NAME, "SSLSniInitCallbackHandler with id %d", id);
> +  switch (id) {
> +  case TS_SSL_SNI_HOOK:
> +      {
> +        init_sni_callback(sslNetVC);
> +      }
> +      break;
> +
> +  default:
> +    TSDebug(PLUGIN_NAME, "Unexpected event %d", id);
> +    break;
> +  }
> +
> +  return TS_EVENT_NONE;
> +}
> +
> +void
> +TSPluginInit(int argc, const char *argv[])
> +{
> +  (void) argc;
> +  TSPluginRegistrationInfo info;
> +
> +  info.plugin_name = (char *)("sni_proto_nego");
> +  info.vendor_name = (char *)("ats");
> +
> +  if (TSPluginRegister(TS_SDK_VERSION_3_0, &info) != TS_SUCCESS) {
> +    TSError("Plugin registration failed.");
> +  }
> +
> +  char* config_file = (char*)"conf/sni_proto_nego/sni_proto_nego.config";

Is this some Yahoo thing?

> +
> +  if (argc >= 2) {
> +    config_file = (char*)argv[1];
> +  }
> +  
> +  if (!read_config(config_file)) {
> +    TSDebug(PLUGIN_NAME, "nothing to do..");
> +    return;
> +  }
> +
> +  TSCont cont = TSContCreate(SSLSniInitCallbackHandler, NULL);
> +  TSHttpHookAdd(TS_SSL_SNI_HOOK, cont);
> +}
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/24262d8f/proxy/InkAPI.cc
> ----------------------------------------------------------------------
> diff --git a/proxy/InkAPI.cc b/proxy/InkAPI.cc
> index 62f0870..d61e997 100644
> --- a/proxy/InkAPI.cc
> +++ b/proxy/InkAPI.cc
> @@ -8757,6 +8757,16 @@ tsapi int TSVConnIsSsl(TSVConn sslp)
>   return ssl_vc != NULL;
> }
> 
> +tsapi TSReturnCode
> +TSSslAdvertiseProtocolSet(TSVConn sslp, const unsigned char ** list, unsigned int count)

list should be "const char **", there's no unsigned there.

> +{
> +  NetVConnection *vc = reinterpret_cast<NetVConnection*>(sslp);
> +  SSLNetVConnection *ssl_vc = dynamic_cast<SSLNetVConnection*>(vc);
> +  sdk_assert(sdk_sanity_check_null_ptr((void*)ssl_vc) == TS_SUCCESS);

Don't crash if it's not a SSL VC, just fail.

> +  ssl_vc->setAdvertiseProtocols(list, count);

Propagate the return code.

> +  return TS_SUCCESS;
> +}
> +
> void
> TSVConnReenable(TSVConn vconn)
> {
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/24262d8f/proxy/api/ts/ts.h
> ----------------------------------------------------------------------
> diff --git a/proxy/api/ts/ts.h b/proxy/api/ts/ts.h
> index b5b0abe..8950b5c 100644
> --- a/proxy/api/ts/ts.h
> +++ b/proxy/api/ts/ts.h
> @@ -1238,6 +1238,7 @@ extern "C"
>   tsapi TSSslContext TSSslContextFindByAddr(struct sockaddr const*);
>   // Returns 1 if the sslp argument refers to a SSL connection
>   tsapi int TSVConnIsSsl(TSVConn sslp);
> +  tsapi TSReturnCode TSSslAdvertiseProtocolSet(TSVConn sslp, const unsigned char **
list, unsigned int count);
> 
>   /* --------------------------------------------------------------------------
>      HTTP transactions */
> 


Mime
View raw message