Hello! On Fri, Jul 28, 2017 at 01:45:37PM -0500, Nate Karstens wrote:
> # HG changeset patch > # User Nate Karstens <[email protected]> > # Date 1501265849 18000 > # Fri Jul 28 13:17:29 2017 -0500 > # Node ID 9537b7d299131e41a3f5993257000d328e28b117 > # Parent 7e10e1dc1fae0f538a0a74fd6783f9b33a905933 > SSL: add connection support. It is not clear what this summary line is expected to mean. > > 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 Looks good now, thanks. > > PSK functionality can be easily tested with the OpenSSL s_client > using the "-psk" and "-psk_identity" options. Note that this stops working with OpenSSL 1.1.1 when using TLS 1.3. As of now, using PSK with TLS 1.3 in OpenSSL requires using a different callback (SSL_CTX_set_psk_find_session_callback()) with slightly different logic, and keys must match sha256 or sha384 length exactly. An example code can be found in OpenSSL's apps/s_server.c. This is probably something ok to postpone for later though. > > Signed-off-by: Nate Karstens <[email protected]> > > diff -r 7e10e1dc1fae -r 9537b7d29913 contrib/vim/syntax/nginx.vim > --- a/contrib/vim/syntax/nginx.vim Fri Jul 28 13:16:27 2017 -0500 > +++ b/contrib/vim/syntax/nginx.vim Fri Jul 28 13:17:29 2017 -0500 > @@ -550,6 +550,7 @@ syn keyword ngxDirective contained ssl_p > 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 7e10e1dc1fae -r 9537b7d29913 src/event/ngx_event_openssl.c > --- a/src/event/ngx_event_openssl.c Fri Jul 28 13:16:27 2017 -0500 > +++ b/src/event/ngx_event_openssl.c Fri Jul 28 13:17:29 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_password_callback(cha > 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(ngx_ssl_conn_t *ssl_conn, > + const char *identity, unsigned char *psk, unsigned int max_psk_len); This callback probably should be placed after the ngx_ssl_psk_file() function which uses it. And see below about ngx_ssl_psk_file() position. > 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); > @@ -114,6 +117,7 @@ int ngx_ssl_certificate_index; > 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 +229,13 @@ ngx_ssl_init(ngx_log_t *log) > return NGX_ERROR; > } > > + ngx_ssl_psk_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, NULL); You are allocating connection-specific index (for SSL structures), but use it as context-specific one (in SSL_CTX structures). This is not going to work. Also, please note that indexes are orderd based on their type. This particular position is wrong for both the type you've used and the type it should be. > + > + 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; > } > > @@ -875,6 +886,138 @@ ngx_ssl_info_callback(const ngx_ssl_conn > } > > > +static unsigned int > +ngx_ssl_psk_callback(ngx_ssl_conn_t *ssl_conn, const char *identity, > + unsigned char *psk, unsigned int max_psk_len) > +{ > + u_char *p, *last, *end, *psk_ptr; > + size_t len; > + ssize_t n; > + SSL_CTX *ssl_ctx; > + ngx_fd_t fd; > + ngx_str_t *psk_file; > + ngx_int_t psk_len; > + ngx_connection_t *c; > + u_char buf[NGX_SSL_PSK_BUFFER_SIZE]; > + > + c = ngx_ssl_get_connection(ssl_conn); > + > + ssl_ctx = c->ssl->session_ctx; Any reasons to use session-specific context here? This will preclude use of virtualhost-specific PSK files, which looks like a bad idea. Not to mention that the mere existence of session-specific contexts in c->ssl is a workaround for the OpenSSL session resumption logic, see http://hg.nginx.org/nginx/rev/97f102a13f33. Unless there are reasons, normal SSL context should be used instead, as available via SSL_get_SSL_CTX(). Note though that for proper virtualhost support additional changes in ngx_http_ssl_servername() might be also needed, especially if callbacks will be set conditionally (see below), as OpenSSL might not adjust callbacks properly on SSL_set_SSL_CTX(). > + psk_file = SSL_CTX_get_ex_data(ssl_ctx, ngx_ssl_psk_index); The "psk_file" name looks excessive. Just "file" would be fine here. > + > + if (psk_file == NULL) { > + return 0; > + } As the code only configures callback with a PSK file, this should never happen. > + > + fd = ngx_open_file(psk_file->data, NGX_FILE_RDONLY, NGX_FILE_OPEN, 0); > + if (fd == NGX_INVALID_FILE) { > + ngx_ssl_error(NGX_LOG_ERR, c->log, ngx_errno, > + ngx_open_file_n " \"%V\" failed", psk_file); > + return 0; > + } > + > + 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_ERR, c->log, ngx_errno, > + ngx_read_fd_n " \"%V\" failed", psk_file); > + psk_len = 0; > + goto cleanup; > + } > + > + end = last + n; > + > + if (len && n == 0) { > + *end++ = LF; > + } > + > + for (p = buf; /* void */; p = last) { > + 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, ':'); There are no reasons for the cast here, ngx_strlchr() returns u_char *. The "psk_ptr" name looks excessive. Instead, I would recommend using something like "colon" and moving the "p" pointer to colon + 1 after identity check. > + > + if (psk_ptr == NULL) { > + continue; > + } > + > + *psk_ptr = '\0'; > + psk_ptr++; > + > + if (ngx_strcmp(p, identity) != 0) { > + continue; > + } > + > + if (ngx_strncmp(psk_ptr, "{HEX}", ngx_strlen("{HEX}")) == 0) { > + psk_ptr += ngx_strlen("{HEX}"); Using ngx_strlen() with static strings is a bad idea. Instead, sizeof(...) - 1 should be used, much like in other parts of the code. > + psk_len = ngx_hex_decode(psk, max_psk_len, psk_ptr, > + p + len - psk_ptr); > + if (psk_len == NGX_ERROR) { > + ngx_memzero(psk, max_psk_len); > + psk_len = 0; > + } > + goto cleanup; Style: there should be more empty lines here, including one before "} else". I would recommend something like this: if (ngx_strncmp(psk_ptr, "{HEX}", sizeof("{HEX}") - 1) == 0) { psk_ptr += sizeof("{HEX}") - 1; psk_len = ngx_hex_decode(psk, max_psk_len, psk_ptr, p + len - psk_ptr); if (psk_len == NGX_ERROR) { ngx_memzero(psk, max_psk_len); psk_len = 0; } goto cleanup; } else ... > + } else if (ngx_strncmp(psk_ptr, "{PLAIN}", > + ngx_strlen("{PLAIN}")) == 0) { Style: indentation is wrong, ngx_strlen() should be aligned with ngx_strncmp() arguments, "== 0" should be on its own line and indented to the function checked, and "{" should be on its own line. And since there is no need to wrap ngx_strcmp() arguments, the result should look like: } else if (ngx_strncmp(psk_ptr, "{PLAIN}", sizeof("{PLAIN}") - 1) == 0) { > + psk_ptr += ngx_strlen("{PLAIN}"); > + } > + > + psk_len = last - 1 - psk_ptr; This logic is hardly readable and wrong. Notably, it will include trailing "CR" into the psk_len, despite the fact that the above code tries to ignore it. Instead, I would recommend to adjast "p" as suggested above, and adjust "len" at the same time. This will allow to get rid of such error-prone calculations, and simply use "p" and "len" here and in other places. > + > + if (psk_len > max_psk_len) { Compilation with at least clang fails here with the following message: src/event/ngx_event_openssl.c:984:25: error: comparison of integers of different signs: 'ngx_int_t' (aka 'int') and 'unsigned int' [-Werror,-Wsign-compare] if (psk_len > max_psk_len) { ~~~~~~~ ^ ~~~~~~~~~~~ Changing the psk_len type to "unsigned int" as it should be resolves this (though introduces a different problem elsewhere, to be resolved with ngx_hex_decode() API improvements as discussed in the previous patch review). > + psk_len = 0; > + goto cleanup; > + } > + > + ngx_memcpy(psk, psk_ptr, psk_len); > + goto cleanup; > + } > + > + len = end - p; > + > + if (len == NGX_SSL_PSK_BUFFER_SIZE) { > + ngx_ssl_error(NGX_LOG_ERR, c->log, 0, > + "too long line in \"%V\"", psk_file); > + psk_len = 0; > + goto cleanup; > + } > + > + ngx_memmove(buf, p, len); > + last = buf + len; > + > + } while (n != 0); > + > +cleanup: > + > + if (ngx_close_file(fd) == NGX_FILE_ERROR) { > + ngx_ssl_error(NGX_LOG_ALERT, c->log, ngx_errno, > + ngx_close_file_n " %V failed", psk_file); > + psk_len = 0; > + } > + > + ngx_memzero(buf, NGX_SSL_PSK_BUFFER_SIZE); > + > + return psk_len; Compilation fails here with the following message: src/event/ngx_event_openssl.c:1017:12: error: variable 'psk_len' may be uninitialized when used here [-Werror,-Wconditional-uninitialized] return psk_len; ^~~~~~~ This is indeed a bug in the code, as psk_len is never initialized if the identity is not found in the PSK file. > +} > + > + > RSA * > ngx_ssl_rsa512_key_callback(ngx_ssl_conn_t *ssl_conn, int is_export, > int key_length) > @@ -3137,6 +3280,23 @@ ngx_ssl_session_ticket_keys(ngx_conf_t * > #endif > > > +ngx_int_t > +ngx_ssl_psk_file(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *file) > +{ > +#if OPENSSL_VERSION_NUMBER >= 0x1000000fL This is not enough to compile with older OpenSSL versions, as there will be an unused ngx_ssl_psk_callback() function: src/event/ngx_event_openssl.c:890:1: error: unused function 'ngx_ssl_psk_callback' [-Werror,-Wunused-function] ngx_ssl_psk_callback(ngx_ssl_conn_t *ssl_conn, const char *identity, ^ This also breaks builds with at least LibreSSL 2.5.5, as it does not have SSL_CTX_set_ex_data() defined, yet provides large enough OPENSSL_VERSION_NUMBER. Checking for some related define (PSK_MAX_IDENTITY_LEN?) might be a better option. Also, there should be empty lines after #if and before #endif, as the code inside #if/#endif is large enough and contains empty lines. > + if (SSL_CTX_set_ex_data(ssl->ctx, ngx_ssl_psk_index, file) == 0) { This uses "file" as written in the configuration. Instead, it should resolve the name using the ngx_conf_full_name() function, to avoid dependencies on the working directory. See ngx_ssl_dhparam() for an example. > + ngx_ssl_error(NGX_LOG_ALERT, ssl->log, 0, > + "SSL_CTX_set_ex_data() failed"); The NGX_LOG_ALERT logging level is not appropriate here. As the error is fatal and will prevent nginx from starting, it should be NGX_LOG_EMERG. > + return NGX_ERROR; > + } > + > + SSL_CTX_set_psk_server_callback(ssl->ctx, ngx_ssl_psk_callback); Always configuring a callback is probably not a good idea. Instead, there should be a check if file is not empty. In particular, this will result in cryptic messages like: ... [error] ... open() "" failed (SSL:) (2: No such file or directory) while SSL handshaking on servers without PSK file configured if a client tries to use PSK. It also results in a critictal error like: ... [crit] ... SSL_do_handshake() failed (SSL: error:1419E0DF:SSL routines:tls_process_cke_psk_preamble:psk identity not found) while SSL handshaking which suggests that the patch needs to adjust severity levels in ngx_ssl_connection_error(). > +#endif > + > + return NGX_OK; > +} > + > + > void > ngx_ssl_cleanup_ctx(void *data) > { > diff -r 7e10e1dc1fae -r 9537b7d29913 src/event/ngx_event_openssl.h > --- a/src/event/ngx_event_openssl.h Fri Jul 28 13:16:27 2017 -0500 > +++ b/src/event/ngx_event_openssl.h Fri Jul 28 13:17:29 2017 -0500 > @@ -171,6 +171,7 @@ ngx_int_t ngx_ssl_session_cache(ngx_ssl_ > 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); The position chosen for for the function looks suboptimal, as it is placed between functions related to session caching. > @@ -255,6 +256,7 @@ extern int ngx_ssl_certificate_index; > 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 7e10e1dc1fae -r 9537b7d29913 src/http/modules/ngx_http_ssl_module.c > --- a/src/http/modules/ngx_http_ssl_module.c Fri Jul 28 13:16:27 2017 -0500 > +++ b/src/http/modules/ngx_http_ssl_module.c Fri Jul 28 13:17:29 2017 -0500 > @@ -234,6 +234,13 @@ static ngx_command_t ngx_http_ssl_comma > 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 @@ ngx_http_ssl_create_srv_conf(ngx_conf_t > * 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_http_ssl_merge_srv_conf(ngx_conf_t * > 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 @@ ngx_http_ssl_merge_srv_conf(ngx_conf_t * > > } > > + if (ngx_ssl_psk_file(cf, &conf->ssl, &conf->psk_file) > + != NGX_OK) > + { > + return NGX_CONF_ERROR; > + } > + Style: there is no need to wrap lines here, just using if (ngx_ssl_psk_file(cf, &conf->ssl, &conf->psk_file) != NGX_OK) { return NGX_CONF_ERROR; } would be fine. > return NGX_CONF_OK; > } > > diff -r 7e10e1dc1fae -r 9537b7d29913 src/http/modules/ngx_http_ssl_module.h > --- a/src/http/modules/ngx_http_ssl_module.h Fri Jul 28 13:16:27 2017 -0500 > +++ b/src/http/modules/ngx_http_ssl_module.h Fri Jul 28 13:17:29 2017 -0500 > @@ -55,6 +55,8 @@ typedef struct { > 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 > [email protected] > http://mailman.nginx.org/mailman/listinfo/nginx-devel -- Maxim Dounin http://nginx.org/ _______________________________________________ nginx-devel mailing list [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
