Hello!

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

> # HG changeset patch
> # User Nate Karstens <nate.karstens@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@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@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel

--
Maxim Dounin
http://nginx.org/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel