[PATCH 1 of 3] PSK: connection support

Maxim Dounin mdounin at mdounin.ru
Thu Jun 29 21:08:36 UTC 2017


Hello!

On Thu, Jun 22, 2017 at 01:24:54PM +0000, Karstens, Nate wrote:

> # HG changeset patch
> # User Nate Karstens <nate.karstens at garmin.com>
> # Date 1498137180 18000
> #      Thu Jun 22 08:13:00 2017 -0500
> # Node ID 3fb3c4928d06029ca1d57853a163c9f56fa90bca
> # Parent  6169dbad37d85fa8642b9d0a51f0f0f6c19dd3d1
> PSK: connection support

Please use "SSL: " prefix for all SSL-related commits.  Please use 
dot at the end of summary.  Overral, this should look like:

SSL: PSK cipher suites support.

Some additional comments mostly unrelated to the code:

- Please add something like

  [diff]
  showfunc=1

  to your Mercurial's ~/.hgrc to simplify further review.  This will 
  ensure that function names are directly visible in patches.

- When submitting a patch series, please do so in a single thread 
  using In-Reply-To / References.  This will happen automatically 
  when using "hg email".  Please also provide references to the 
  previous threads, if any, or use "hg email --in-reply-to 
  <messageid>" to submit a new / updated series to the same 
  thread.

> Adds support for TLS connections using PSK cipher suites. A new
> configuration directive, ssl_psk_file, specifies the file that
> contains a list of identities and associated PSKs. Each line of
> the file begins with the identity, followed by a colon character
> (':'), and ending with the PSK. As required by RFC 4279 section
> 5.4, PSKs may be entered either as plain text or using hexadecimal
> encoding. Hexadecimal PSKs must begin with "{HEX}". PSKs without
> this prefix are assumed to be plain text, but they may optionally
> begin with "{PLAIN}" to denote this. Some examples:
> 
> gary:plain_text_password
> min:{PLAIN}another_text_password
> cliff:{HEX}ab0123CD
> 
> The format of the given PSK file is checked at server startup.

This checking at server startup looks wrong.  As long as the file 
can be modified at any time, it doesn't provide anything but an 
additional maintanance problems - as syntax errors can be 
introduced (and fixed) at any time.

Note well that there is another problem with runtime-modifiable 
secrets file though.  Being able to access the file at runtime 
means that worker process have to able to read it, so the file 
can't be only readable by root.  Given that secrets are not 
encrypted (in contrast to auth basic passwords, which are hashed 
and generally non-recoverable even if leaked), this might not be 
secure enough.  It might be actually be a better idea to read all 
the keys at start as you initially suggested.  Not sure though.

> 
> PSK functionality can be easily tested with the OpenSSL s_client
> using the "-psk" and "-psk_identity" options.
> 
> Signed-off-by: Nate Karstens <nate.karstens at garmin.com>
> 
> diff -r 6169dbad37d8 -r 3fb3c4928d06 contrib/vim/syntax/nginx.vim
> --- a/contrib/vim/syntax/nginx.vim      Wed Jun 14 20:13:41 2017 +0300
> +++ b/contrib/vim/syntax/nginx.vim      Thu Jun 22 08:13:00 2017 -0500
> @@ -550,6 +550,7 @@
>  syn keyword ngxDirective contained ssl_prefer_server_ciphers
>  syn keyword ngxDirective contained ssl_preread
>  syn keyword ngxDirective contained ssl_protocols
> +syn keyword ngxDirective contained ssl_psk_file
>  syn keyword ngxDirective contained ssl_session_cache
>  syn keyword ngxDirective contained ssl_session_ticket_key
>  syn keyword ngxDirective contained ssl_session_tickets
> diff -r 6169dbad37d8 -r 3fb3c4928d06 src/event/ngx_event_openssl.c
> --- a/src/event/ngx_event_openssl.c     Wed Jun 14 20:13:41 2017 +0300
> +++ b/src/event/ngx_event_openssl.c     Thu Jun 22 08:13:00 2017 -0500
> @@ -11,6 +11,7 @@
> 
> 
>  #define NGX_SSL_PASSWORD_BUFFER_SIZE  4096
> +#define NGX_SSL_PSK_BUFFER_SIZE       4096
> 
> 
>  typedef struct {
> @@ -23,6 +24,8 @@
>  static int ngx_ssl_verify_callback(int ok, X509_STORE_CTX *x509_store);
>  static void ngx_ssl_info_callback(const ngx_ssl_conn_t *ssl_conn, int where,
>      int ret);
> +static unsigned int ngx_ssl_psk_callback(SSL *ssl, const char *identity,
> +    unsigned char *psk, unsigned int max_psk_len);
>  static void ngx_ssl_passwords_cleanup(void *data);
>  static void ngx_ssl_handshake_handler(ngx_event_t *ev);
>  static ngx_int_t ngx_ssl_handle_recv(ngx_connection_t *c, int n);
> @@ -65,6 +68,11 @@
>  #endif
>      ASN1_TIME *asn1time);
> 
> +static ngx_int_t ngx_ssl_psk_convert_hex(u_char **buf, const u_char *psk,
> +    const u_char *last, ngx_uint_t line);
> +static ngx_int_t ngx_ssl_psk_read(ngx_str_t *file, const char *identity,
> +    unsigned char *psk, int max_psk_len);
> +
>  static void *ngx_openssl_create_conf(ngx_cycle_t *cycle);
>  static char *ngx_openssl_engine(ngx_conf_t *cf, ngx_command_t *cmd, void *conf);
>  static void ngx_openssl_exit(ngx_cycle_t *cycle);
> @@ -114,6 +122,7 @@
>  int  ngx_ssl_next_certificate_index;
>  int  ngx_ssl_certificate_name_index;
>  int  ngx_ssl_stapling_index;
> +int  ngx_ssl_psk_index;
> 
> 
>  ngx_int_t
> @@ -225,6 +234,13 @@
>          return NGX_ERROR;
>      }
> 
> +    ngx_ssl_psk_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, NULL);
> +
> +    if (ngx_ssl_psk_index == -1) {
> +        ngx_ssl_error(NGX_LOG_ALERT, log, 0, "SSL_get_ex_new_index() failed");
> +        return NGX_ERROR;
> +    }
> +
>      return NGX_OK;
>  }
> 
> @@ -345,6 +361,7 @@
>      SSL_CTX_set_read_ahead(ssl->ctx, 1);
> 
>      SSL_CTX_set_info_callback(ssl->ctx, ngx_ssl_info_callback);
> +    SSL_CTX_set_psk_server_callback(ssl->ctx, ngx_ssl_psk_callback);

I would rather avoid using SSL_CTX_set_psk_server_callback() in a 
function which initializes a generic SSL context.  Instead, it 
would be better to set the callback explicitly when (and if) if a 
PSK file is provided.

Additionally, the SSL_CTX_set_psk_server_callback() function 
appeared only in OpenSSL 1.0.0.  While we are moving away from 
OpenSSL 0.9.7 support, we are certainly going to support 0.9.8 for 
some time, so this needs additional preprocessor checks.

> 
>      return NGX_OK;
>  }
> @@ -875,6 +892,29 @@
>  }
> 
> 
> +static unsigned int ngx_ssl_psk_callback(SSL *ssl, const char *identity,

Style: function name should start at its own line.

Style: please use "ngx_ssl_conn_t *ssl_conn" instead of "SSL *ssl" as 
in other callbacks.

> +    unsigned char *psk, unsigned int max_psk_len)
> +{
> +    SSL_CTX        *ssl_ctx;
> +    ngx_str_t      *psk_file;
> +    ngx_int_t       psk_len;

Style: there should less spaces.

> +
> +    ssl_ctx = SSL_get_SSL_CTX(ssl);
> +
> +    psk_file = SSL_CTX_get_ex_data(ssl_ctx, ngx_ssl_psk_index);
> +    if (psk_file == NULL) {
> +        return 0;
> +    }
> +
> +    psk_len = ngx_ssl_psk_read(psk_file, identity, psk, max_psk_len);
> +    if (psk_len < 0) {

It is not clear what "psk_len < 0" check is expected to mean.  As 
long as I see, ngx_ssl_psk_read() can either return positive 
number or NGX_ERROR.  Checking explicitly for NGX_ERROR instead 
might be a better idea.

Alternatively, ngx_ssl_psk_read() interface may be changed to 
return 0 on errors, so it will be possible to simply do

    return ngx_ssl_psk_read(...);

instead.

> +        return 0;
> +    }
> +
> +    return psk_len;
> +}
> +
> +
>  RSA *
>  ngx_ssl_rsa512_key_callback(ngx_ssl_conn_t *ssl_conn, int is_export,
>      int key_length)
> @@ -3137,6 +3177,24 @@
>  #endif
> 
> 
> +ngx_int_t
> +ngx_ssl_psk_file(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *file)
> +
> +{

Style: extra empty line.

> +    ngx_int_t   rc;
> +
> +    if (SSL_CTX_set_ex_data(ssl->ctx, ngx_ssl_psk_index, file) == 0) {
> +        ngx_ssl_error(NGX_LOG_ALERT, ssl->log, 0,
> +                      "SSL_CTX_set_ex_data() failed");
> +        return NGX_ERROR;
> +    }
> +
> +    rc = ngx_ssl_psk_read(file, NULL, NULL, 0);
> +
> +    return rc == 0 ? NGX_OK : NGX_ERROR;
> +}
> +
> +
>  void
>  ngx_ssl_cleanup_ctx(void *data)
>  {
> @@ -4127,6 +4185,223 @@
>  }
> 
> 
> +static ngx_int_t
> +ngx_ssl_psk_convert_hex(u_char **buf, const u_char *psk, const u_char *last,
> +                        ngx_uint_t line)

Style: there should be 4 spaces on function arguments 
continuation.

Style: a function is expected to be defined after the function 
which calls it.  That is, ngx_ssl_psk_convert_hex() is expected to 
go after ngx_ssl_psk_read() which calls it.

Additionally, both ngx_ssl_psk_convert_hex() and 
ngx_ssl_psk_read() seems to be badly placed in the file.  It 
should be better to put them right after ngx_ssl_psk_file().  And 
may be no where ngx_ssl_psk_file() currently placed, but rather 
after ngx_ssl_ecdh_curve(), following other configuration-related 
functions.

> +{
> +    ngx_int_t   len;
> +    u_char     *out = NULL;
> +    u_char      val;
> +
> +    if (buf != NULL) {
> +        *buf = NULL;
> +    }
> +
> +    if ((last - psk) & 1) {
> +        len = NGX_ERROR;
> +        goto error;
> +    }
> +
> +    len = (last - psk) / 2;
> +
> +    if (buf != NULL) {
> +        out = ngx_alloc(len, ngx_cycle->log);
> +        if (out == NULL) {
> +            return NGX_ERROR;
> +        }
> +        *buf = out;
> +    }

There should be no need to allocate any buffers here.

> +
> +    while (psk < last) {
> +        if (*psk >= '0' && *psk <= '9') {
> +            val = *psk - '0';
> +        } else if (*psk >= 'a' && *psk <= 'f') {
> +            val = *psk - 'a' + 10;
> +        } else if (*psk >= 'A' && *psk <= 'F') {
> +            val = *psk - 'A' + 10;

Testing lowercased version as in ngx_hextoi() and many other 
places might be a better idea.

It might be also a good idea to introduce a generic function to 
decode hex dumps instead of adding a PSK-specific function.

> +        } else {
> +            len = NGX_ERROR;
> +            goto error;
> +        }
> +
> +        val <<= 4;
> +        psk++;
> +
> +        if (*psk >= '0' && *psk <= '9') {
> +            val += *psk - '0';
> +        } else if (*psk >= 'a' && *psk <= 'f') {
> +            val += *psk - 'a' + 10;
> +        } else if (*psk >= 'A' && *psk <= 'F') {
> +            val += *psk - 'A' + 10;
> +        } else {
> +            len = NGX_ERROR;
> +            goto error;
> +        }
> +
> +        psk++;
> +
> +        if (out != NULL) {
> +            *out = val;
> +            out++;
> +        }
> +    }
> +
> +error:
> +
> +    if (len == NGX_ERROR) {
> +        ngx_ssl_error(NGX_LOG_EMERG, ngx_cycle->log, 0,
> +                      "The PSK on line %ui is not valid hex", line);

Logging to the cycle's log is generally a bad idea, unless no 
other options are available.  When parsing a PSK file for a given 
connection it should be possible to use connection log.  When 
parsing a configuration, ssl->log should be available.

Also, style of error messages here and in other places is far from 
the one generally used in nginx.  Please avoid uppercase unless it 
is really needed (as in function names).

The use of NGX_LOG_EMERG logging level looks incorrect, as the 
particular problem can appear at any time and is not fatal.

> +        if (buf != NULL) {
> +            ngx_free(*buf);
> +            *buf = NULL;
> +        }
> +    }
> +
> +    return len;
> +}
> +
> +
> +static ngx_int_t
> +ngx_ssl_psk_read(ngx_str_t *file, const char *identity, unsigned char *psk,
> +    int max_psk_len)
> +{
> +    ngx_int_t   rc = 0;
> +    ngx_fd_t    fd;
> +    ngx_uint_t  line;
> +    size_t      len;
> +    ssize_t     n;
> +    u_char     *p, *last, *end;
> +    u_char     *psk_ptr, *hex_buf = NULL;
> +    u_char      buf[NGX_SSL_PSK_BUFFER_SIZE];

Style: please avoid initialization in declarations.

Style: there should be more spaces.  General rule is that there 
should be 2 spaces between longest type and "*".

Style: variables should be ordered from shortest type to longest, 
with large buffers defined after everything.

Style: please avoid duplicate types unless there are specific 
reasons to.

So this should look like:

    u_char      *p, *last, *end, *psk_ptr, *hex_buf;
    size_t       len;
    ssize_t      n;
    ngx_fd_t     fd;
    ngx_int_t    rc;
    ngx_uint_t   line;
    u_char       buf[NGX_SSL_PSK_BUFFER_SIZE];

> +
> +    static const char plain_tag[7] = "{PLAIN}";
> +    static const char hex_tag[5] = "{HEX}";

I would rather avoid such constructions.  Instead, please either 
use strings directly like in src/core/ngx_crypt.c, or introduce 
appropriate string defines.

> +
> +    fd = ngx_open_file(file->data, NGX_FILE_RDONLY, NGX_FILE_OPEN, 0);
> +    if (fd == NGX_INVALID_FILE) {
> +        ngx_ssl_error(NGX_LOG_EMERG, ngx_cycle->log, ngx_errno,
> +                      ngx_open_file_n " \"%V\" failed", file);
> +        return NGX_ERROR;
> +    }
> +
> +    len = 0;
> +    last = buf;
> +
> +    do {
> +        n = ngx_read_fd(fd, last, NGX_SSL_PSK_BUFFER_SIZE - len);
> +
> +        if (n == -1) {
> +            ngx_ssl_error(NGX_LOG_EMERG, ngx_cycle->log, ngx_errno,
> +                          ngx_read_fd_n " \"%V\" failed", file);
> +            rc = NGX_ERROR;
> +            goto done;
> +        }
> +
> +        end = last + n;
> +
> +        if (len && n == 0) {
> +            *end++ = LF;
> +        }
> +
> +        for (p = buf, line = 1; ; p = last, line++) {

Style: if a condition is omitted, it should be explicitly 
indicated using /* void */ comment.

Overral, this construction looks unreadable.  It might be a better 
idea to simplify the code and either make it more similar to the 
ngx_ssl_read_password_file() you've used as a prototype, or to 
make it use the code similar to the one in 
ngx_http_auth_basic_module.c instead.

> +            last = ngx_strlchr(last, end, LF);
> +
> +            if (last == NULL) {
> +                break;
> +            }
> +
> +            len = last++ - p;
> +
> +            if (len && p[len - 1] == CR) {
> +                len--;
> +            }
> +
> +            if (len == 0) {
> +                continue;
> +            }
> +
> +            psk_ptr = (u_char *) ngx_strlchr(p, p + len, ':');
> +            if (psk_ptr == NULL) {
> +                ngx_ssl_error(NGX_LOG_EMERG, ngx_cycle->log, 0,
> +                              "Bad format on line %ui of PSK file \"%V\"",
> +                              line, file);
> +                rc = NGX_ERROR;
> +                goto done;
> +            }
> +
> +            *psk_ptr = '\0';
> +            psk_ptr++;
> +
> +            if (identity == NULL) {
> +                if (ngx_memcmp(psk_ptr, hex_tag, sizeof(hex_tag)) == 0) {
> +                    psk_ptr += sizeof(hex_tag);
> +                    if (ngx_ssl_psk_convert_hex(NULL, psk_ptr, p + len, line)
> +                        == NGX_ERROR)
> +                    {
> +                        rc = NGX_ERROR;
> +                        goto done;
> +                    }
> +                }
> +
> +                continue;
> +            }

Duplicate code under "if (identity == NULL)" might not be a good 
idea.  Instead, it should be beneficial to use the same code as in 
normal case, with additional checks after it.

Also, the whole idea about testing the file on start looks wrong, 
see above.

> +
> +            if (ngx_strcmp(p, identity) != 0) {
> +                continue;
> +            }
> +
> +            if (ngx_memcmp(psk_ptr, plain_tag, sizeof(plain_tag)) == 0) {
> +                psk_ptr += sizeof(plain_tag);
> +                rc = last - 1 - psk_ptr;

It might not be a good idea to use ngx_memcmp() unless you are 
sure both arguments have at least specified number of bytes 
available.  Using ngx_strncmp() might be a better idea.

> +            } else if (ngx_memcmp(psk_ptr, hex_tag, sizeof(hex_tag)) == 0) {
> +                psk_ptr += sizeof(hex_tag);
> +                rc = ngx_ssl_psk_convert_hex(&hex_buf, psk_ptr, p + len, line);
> +                if (rc == NGX_ERROR) {
> +                    goto done;
> +                }
> +                psk_ptr = hex_buf;
> +            } else {
> +                rc = last - 1 - psk_ptr;
> +            }
> +
> +            if (rc > max_psk_len) {
> +                rc = 0;
> +                goto done;
> +            }
> +
> +            ngx_memcpy(psk, psk_ptr, rc);
> +            goto done;
> +        }
> +
> +        len = end - p;
> +
> +        if (len == NGX_SSL_PSK_BUFFER_SIZE) {
> +            ngx_ssl_error(NGX_LOG_EMERG, ngx_cycle->log, 0,
> +                          "too long line in \"%V\"", file);
> +            rc = NGX_ERROR;
> +            goto done;
> +        }
> +
> +        ngx_memmove(buf, p, len);
> +        last = buf + len;
> +
> +    } while (n != 0);
> +
> +done:
> +
> +    if (ngx_close_file(fd) == NGX_FILE_ERROR) {
> +        ngx_ssl_error(NGX_LOG_ALERT, ngx_cycle->log, ngx_errno,
> +                      ngx_close_file_n " %V failed", file);
> +        rc = NGX_ERROR;
> +    }
> +
> +    ngx_memzero(buf, NGX_SSL_PSK_BUFFER_SIZE);
> +    ngx_free(hex_buf);
> +
> +    return rc;
> +}
> +
> +
>  static void *
>  ngx_openssl_create_conf(ngx_cycle_t *cycle)
>  {
> diff -r 6169dbad37d8 -r 3fb3c4928d06 src/event/ngx_event_openssl.h
> --- a/src/event/ngx_event_openssl.h     Wed Jun 14 20:13:41 2017 +0300
> +++ b/src/event/ngx_event_openssl.h     Thu Jun 22 08:13:00 2017 -0500
> @@ -171,6 +171,7 @@
>      ssize_t builtin_session_cache, ngx_shm_zone_t *shm_zone, time_t timeout);
>  ngx_int_t ngx_ssl_session_ticket_keys(ngx_conf_t *cf, ngx_ssl_t *ssl,
>      ngx_array_t *paths);
> +ngx_int_t ngx_ssl_psk_file(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *file);
>  ngx_int_t ngx_ssl_session_cache_init(ngx_shm_zone_t *shm_zone, void *data);
>  ngx_int_t ngx_ssl_create_connection(ngx_ssl_t *ssl, ngx_connection_t *c,
>      ngx_uint_t flags);
> @@ -255,6 +256,7 @@
>  extern int  ngx_ssl_next_certificate_index;
>  extern int  ngx_ssl_certificate_name_index;
>  extern int  ngx_ssl_stapling_index;
> +extern int  ngx_ssl_psk_index;
> 
> 
>  #endif /* _NGX_EVENT_OPENSSL_H_INCLUDED_ */
> diff -r 6169dbad37d8 -r 3fb3c4928d06 src/http/modules/ngx_http_ssl_module.c
> --- a/src/http/modules/ngx_http_ssl_module.c    Wed Jun 14 20:13:41 2017 +0300
> +++ b/src/http/modules/ngx_http_ssl_module.c    Thu Jun 22 08:13:00 2017 -0500
> @@ -234,6 +234,13 @@
>        offsetof(ngx_http_ssl_srv_conf_t, stapling_verify),
>        NULL },
> 
> +    { ngx_string("ssl_psk_file"),
> +      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_TAKE1,
> +      ngx_conf_set_str_slot,
> +      NGX_HTTP_SRV_CONF_OFFSET,
> +      offsetof(ngx_http_ssl_srv_conf_t, psk_file),
> +      NULL },
> +
>        ngx_null_command
>  };
> 
> @@ -539,6 +546,7 @@
>       *     sscf->shm_zone = NULL;
>       *     sscf->stapling_file = { 0, NULL };
>       *     sscf->stapling_responder = { 0, NULL };
> +     *     sscf->psk_file = { 0, NULL };
>       */
> 
>      sscf->enable = NGX_CONF_UNSET;
> @@ -620,6 +628,8 @@
>      ngx_conf_merge_str_value(conf->stapling_responder,
>                           prev->stapling_responder, "");
> 
> +    ngx_conf_merge_str_value(conf->psk_file, prev->psk_file, "");
> +
>      conf->ssl.log = cf->log;
> 
>      if (conf->enable) {
> @@ -800,6 +810,12 @@
> 
>      }
> 
> +    if (ngx_ssl_psk_file(cf, &conf->ssl, &conf->psk_file)
> +        != NGX_OK)
> +    {
> +        return NGX_CONF_ERROR;
> +    }
> +
>      return NGX_CONF_OK;
>  }
> 
> diff -r 6169dbad37d8 -r 3fb3c4928d06 src/http/modules/ngx_http_ssl_module.h
> --- a/src/http/modules/ngx_http_ssl_module.h    Wed Jun 14 20:13:41 2017 +0300
> +++ b/src/http/modules/ngx_http_ssl_module.h    Thu Jun 22 08:13:00 2017 -0500
> @@ -55,6 +55,8 @@
>      ngx_str_t                       stapling_file;
>      ngx_str_t                       stapling_responder;
> 
> +    ngx_str_t                       psk_file;
> +
>      u_char                         *file;
>      ngx_uint_t                      line;
>  } ngx_http_ssl_srv_conf_t;
> 
> ________________________________
> 
> CONFIDENTIALITY NOTICE: This email and any attachments are for the sole use of the intended recipient(s) and contain information that may be Garmin confidential and/or Garmin legally privileged. If you have received this email in error, please notify the sender by reply email and delete the message. Any disclosure, copying, distribution or use of this communication (including attachments) by someone other than the intended recipient is prohibited. Thank you.
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel

-- 
Maxim Dounin
http://nginx.org/


More information about the nginx-devel mailing list