[PATCH 1 of 3] PSK: connection support
Karstens, Nate
Nate.Karstens at garmin.com
Thu Jun 29 22:00:45 UTC 2017
Maxim,
Thanks for the comments. I'll try to start on those in a couple of days. My company uses Outlook/Exchange for email, so I don't think I'll be able to use hg email, do you have any other suggestions? Thanks also for your patience, I've used Git quite a bit but am new to Mercurial.
Utkarsh sounds like he is trying to use PSK for TLS v1.3 session resumption. Given that each TLS connection could potentially result in a new PSK I think only reading them at startup could result in too many refreshes. I think there might be some benefit to the original approach in regards to storing each PSK in its own file in a designated directory. Benefits include:
1) We can utilize the file system to avoid name collisions. With TLS v1.3 session resumption the PSK identity doesn't have to be user-friendly, so something like mkstemp() could be used to generate the identity/filename.
2) Files include metadata about when they were created, so you could have a cron job that periodically cleans up old entries. Without this you have to maintain creation time in a separate database.
3) No need to worry about hex vs. text -- the contents of the file are the PSK and can be plain text or binary data as appropriate.
Utkarsh -- do you have any thoughts on the preferred way to set up the PSKs?
Nate
-----Original Message-----
From: nginx-devel [mailto:nginx-devel-bounces at nginx.org] On Behalf Of Maxim Dounin
Sent: Thursday, June 29, 2017 4:09 PM
To: nginx-devel at nginx.org
Subject: Re: [PATCH 1 of 3] PSK: connection support
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/
_______________________________________________
nginx-devel mailing list
nginx-devel at nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
More information about the nginx-devel
mailing list