RE: [PATCH] Proxy: support configuration of socket buffer sizes
Maxim, I wanted to follow up on this because our software has been released and so I can provide a few more details about our setup. Our use case involves the user transferring a large amount of data from a mobile device (running iOS or Android) through the HTTP proxy and storing it onto an SD card (the SD card is provided by the user). If the SD card's write speed is too slow, then the proxy send buffer will eventually fill up, causing a long delay between when the HTTP client sends packets, which ultimately leads to the client timing out. Because the request contains a large amount of data, "proxy_request_buffering" is set to "off". We looked into adjusting the behavior of the client, but this was not possible. To explain, there are generally two ways a mobile app can use HTTP to transfer data. The most straightforward way is to actively transfer the data using either an HTTP client integrated into the app or the HTTP libraries provided by the platform. However, for large transfers this requires the user to keep the mobile app in the foreground -- if the mobile OS detects that the user is not actively using the app then it will suspend the app, which interrupts the transfer. The alternative method to transfer data is to utilize the HTTP background transfer functionality provided by the mobile OS. With this method, the app configures the HTTP transfer and then asks the mobile OS to complete it on the app's behalf. The advantage here is that the user can use other apps while the mobile OS completes the transfer. Unfortunately, the mobile OS provides no ability to customize connection parameters, such as timeouts. I read through the Cloudflare blog entry (thank you -- that was interesting). Their case is different because they were having problems with a download, while we are having problems with an upload. As such, perhaps the "proxy_send_timeout" setting applies in this case instead of the "send_timeout" that they were using. Still, it seems undesirable to increase the timeout value to be too large because it increases the time before you can detect a problem with the connection. The Cloudflare team also considered reducing the TCP buffer size, but ultimately decided on a different solution. I noticed that their solution has not been incorporated into the main distribution, do you know why that is? For us, the option to configure the TCP buffer sizes seems to be the most straightforward. In our system, the default TCP send buffer size is 16kB. However, this can grow to a maximum of 4MB (the Linux stack adjusts this in tcp_sndbuf_expand() -- see tcp_input.c). We do not think that reducing the system-wide maximum TCP buffer size is ideal in this case because the server provides many other TCP applications that work well with the default value; adjusting that value presents additional risk and may lead to more widespread application-specific tuning. Thanks for your consideration, Nate -Original Message- From: nginx-devel [mailto:nginx-devel-boun...@nginx.org] On Behalf Of Maxim Dounin Sent: Tuesday, May 23, 2017 3:25 PM To: nginx-devel@nginx.org Subject: Re: [PATCH] Proxy: support configuration of socket buffer sizes Hello! On Mon, May 22, 2017 at 07:02:04PM +, Karstens, Nate wrote: > Maxim, > > I'd be happy to explain. Our application is actually relying more on > the change to support to SO_SNDBUF option, but I noticed the SO_RCVBUF > option there and thought it was worth exposing that at the same time. > > The application we're having issues with is essentially a file > transfer through a proxy to a relatively slow storage medium. > Because we're using a proxy there are three buffers involved on the > receive end of the HTTP request: 1) receive buffer on external nginx > socket, 2) send buffer in nginx proxy module, and > 3) receive buffer on proxied server. So, in a system where each buffer > is a maximum of 5 MB, you can have 3 x 5 = 15 MB of data in the TCP > buffers at a given point in time. Send buffer on the client contributes to overral buffering as well, and probably should be counted too. But, frankly, 5MB is a lot, and much higher than typical default, and may result in problems by itself. See https://blog.cloudflare.com/the-curious-case-of-slow-downloads/ for a description of a problem Cloudflare faced with such socket buffer sizes. > In most circumstances, I don't think this would be a problem. > However, writing the data is so slow that the HTTP client times out > waiting for a reply (which is only sent once all data has been written > out). Unfortunately, we cannot solve this by increasing the client's > timeout. We found that reducing the size Are you using some custom client, or a browser? Even with buffers as huge as 3x5MB, a really slow backend will be required to trigger a timeout in a typical browser, as browsers
RE: [PATCH] [PATCH 4 of 4] SSL: add identity hint config directive
Yes, I see what you were saying now, sorry for the confusion. Your updates look good. How do you plan to get additional feedback? Maybe at nginx.conf? Our particular use case is more of an embedded environment, so our security constraints are different from a high traffic web server. Perhaps we can extrapolate the best approach for that environment given what we know? As I see it, we have a few alternatives: 1) Read the file from the PSK worker process (as is being done in the current patch) 2) Read the file when the configuration is loaded and cache results in memory (file only needs to be read by master process) 3) Allow the user to choose between 1 and 2 (using a config file setting) 4) Adding encryption to PSK file (encryption key is loaded when configuration is loaded and PSK file is decrypted by worker process) Can you think of any others? The main disadvantage of the second option is that you have to reload the configuration file whenever a PSK is added/removed/changed, right? Can you help me understand the performance implications of reloading the configuration file? If severe, maybe those performance implications could be mitigated by functionality specific to the environment the server is operating in? For example, in an environment with thousands of users maybe the administrators could institute a policy of updating the PSK file and reloading the configuration only once every 5 minutes? Other, less complicated environments could operate without any additional functionality. Nate -Original Message- From: nginx-devel [mailto:nginx-devel-boun...@nginx.org] On Behalf Of Maxim Dounin Sent: Monday, September 04, 2017 11:01 AM To: nginx-devel@nginx.org Subject: Re: [PATCH] [PATCH 4 of 4] SSL: add identity hint config directive Hello! On Fri, Sep 01, 2017 at 01:18:56PM +, Karstens, Nate wrote: > Maxim, > > Your changes look good and test well -- works for me! > > Thanks for your work on this, and for your patience! Thank you for your work, too. Some additionall problems I'v found looking into this: 1. In the previous review, http://mailman.nginx.org/pipermail/nginx-devel/2017-August/010419.html, I've asked to not set callback if file is not empty: : 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. I think you've probably get me wrong, and instead tried to check the file contents. This is not what I mean, I've asked to check the "file" string to see if it is empty, and do nothing if it is - that is, if there is no PSK file specified in the configuration. There should be something like this in the code, similar to ngx_ssl_dhparam(): @@ -1181,6 +1181,10 @@ ngx_ssl_psk_file(ngx_conf_t *cf, ngx_ssl { #ifdef PSK_MAX_IDENTITY_LEN +if (file->len == 0) { +return NGX_OK; +} + if (ngx_conf_full_name(cf->cycle, file, 1) != NGX_OK) { return NGX_ERROR; } This should prevent configurations without ssl_psk_file configured from trying to use PSK callback and reading the configuration directory in an attempt to find a secret as the code without the check does. 2. Error logging in the ngx_ssl_psk_callback() function is not correct. It uses ngx_ssl_error() function, which is intended to log OpenSSL-specific errors with error details extracted from the OpenSSL error stack. This function uses normal file operations though, and the OpenSSL error stack is always empty. As such, all messages will contain meaningless "(SSL:)" part. To avoid this, errors should be logged using normal ngx_log_error(). Updated patches below. Note well that I'm still not sure if it is good idea to read ssl_psk_file from worker processes instead of reading it once on startup as we do with SSL certificate keys (or, for example, ticket keys). This and the fact that PSK keys are not protected anyhow, neither with additional password nor hashing, implies that PSK keys can be easily compromissed if an attacker is able to read files accessible to nginx user. I'm going to postpone further work on this till at least some feedback from people using it in the real world. # HG changeset patch # User Nate Karstens <nate.karst...@garmin.com> # Date 1503540018 18000 # Wed Aug 23 21:00:18 2017 -0500 # Node ID a87e224e8d6b2993dfcd8903bfb0e7eb7fd934fa # Parent c7d4017c8876af6d8570e400320537d7d39e9578 Core: add function to decode hexadecimal strings. Adds functionality to convert a hexadecimal string into binary data. This will be used to decode PSKs stored in hexadecimal representation. Signed-off-by: Nate Karstens <nate.karst...@garmin.com> diff --git a/src/core/ngx_string.c b/s
RE: [PATCH] [PATCH 4 of 4] SSL: add identity hint config directive
Maxim, Your changes look good and test well -- works for me! Thanks for your work on this, and for your patience! Nate -Original Message- From: nginx-devel [mailto:nginx-devel-boun...@nginx.org] On Behalf Of Maxim Dounin Sent: Thursday, August 31, 2017 9:45 AM To: nginx-devel@nginx.org Subject: Re: [PATCH] [PATCH 4 of 4] SSL: add identity hint config directive Hello! On Wed, Aug 23, 2017 at 09:22:43PM -0500, Nate Karstens wrote: > # HG changeset patch > # User Nate Karstens# Date 1503540237 > 18000 > # Wed Aug 23 21:03:57 2017 -0500 > # Node ID 62b4032371bd45217d40e2f0daf8ecd6956601d8 > # Parent a11e114a2bcde4afb515dd0b70f3ef39693f475a > [PATCH 4 of 4] SSL: add identity hint config directive. As in previous patches, there should be no "[PATCH 4 of 4] ". Otherwise loos good. Following this and previous patches review, here are all four patches adjusted according to the comments. Please take a look if it works for you. # HG changeset patch # User Nate Karstens # Date 1503540018 18000 # Wed Aug 23 21:00:18 2017 -0500 # Node ID a87e224e8d6b2993dfcd8903bfb0e7eb7fd934fa # Parent c7d4017c8876af6d8570e400320537d7d39e9578 Core: add function to decode hexadecimal strings. Adds functionality to convert a hexadecimal string into binary data. This will be used to decode PSKs stored in hexadecimal representation. Signed-off-by: Nate Karstens diff --git a/src/core/ngx_string.c b/src/core/ngx_string.c --- a/src/core/ngx_string.c +++ b/src/core/ngx_string.c @@ -1118,6 +1118,56 @@ ngx_hex_dump(u_char *dst, u_char *src, s } +ngx_int_t +ngx_hex_decode(u_char *dst, u_char *src, size_t len) { +u_char ch, decoded; + +if (len & 1) { +return NGX_ERROR; +} + +while (len) { +ch = *src++; +len -= 2; + +if (ch >= '0' && ch <= '9') { +decoded = ch - '0'; +goto second; +} + +ch |= 0x20; + +if (ch >= 'a' && ch <= 'f') { +decoded = ch - 'a' + 10; +goto second; +} + +return NGX_ERROR; + +second: + +ch = *src++; + +if (ch >= '0' && ch <= '9') { +*dst++ = (u_char) ((decoded << 4) + ch - '0'); +continue; +} + +ch |= 0x20; + +if (ch >= 'a' && ch <= 'f') { +*dst++ = (u_char) ((decoded << 4) + ch - 'a' + 10); +continue; +} + +return NGX_ERROR; +} + +return NGX_OK; +} + + void ngx_encode_base64(ngx_str_t *dst, ngx_str_t *src) { diff --git a/src/core/ngx_string.h b/src/core/ngx_string.h --- a/src/core/ngx_string.h +++ b/src/core/ngx_string.h @@ -177,6 +177,7 @@ time_t ngx_atotm(u_char *line, size_t n) ngx_int_t ngx_hextoi(u_char *line, size_t n); u_char *ngx_hex_dump(u_char *dst, u_char *src, size_t len); +ngx_int_t ngx_hex_decode(u_char *dst, u_char *src, size_t len); #define ngx_base64_encoded_length(len) (((len + 2) / 3) * 4) # HG changeset patch # User Nate Karstens # Date 1503540059 18000 # Wed Aug 23 21:00:59 2017 -0500 # Node ID d89f77108fa8a20bc1fb9cdbaf43fefbc5e07119 # Parent a87e224e8d6b2993dfcd8903bfb0e7eb7fd934fa SSL: add support for PSK cipher suites. 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 PSK functionality can be easily tested with the OpenSSL s_client using the "-psk" and "-psk_identity" options. Signed-off-by: Nate Karstens diff --git a/contrib/vim/syntax/nginx.vim b/contrib/vim/syntax/nginx.vim --- a/contrib/vim/syntax/nginx.vim +++ b/contrib/vim/syntax/nginx.vim @@ -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 --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c --- a/src/event/ngx_event_openssl.c +++ b/src/event/ngx_event_openssl.c @@ -11,6 +11,7 @@ #define NGX_SSL_PASSWORD_BUFFER_SIZE 4096 +#define NGX_SSL_PSK_BUFFER_SIZE 4096 typedef struct { @@ -24,6
RE: [PATCH] [PATCH 2 of 4] SSL: add support for PSK cipher suites
> In the previous review I've pointed out that the patch needs to adjust > severity levels in ngx_ssl_connection_error(): Sorry, I think I got confused with something else. Nate -Original Message- From: nginx-devel [mailto:nginx-devel-boun...@nginx.org] On Behalf Of Maxim Dounin Sent: Thursday, August 31, 2017 9:44 AM To: nginx-devel@nginx.org Subject: Re: [PATCH] [PATCH 2 of 4] SSL: add support for PSK cipher suites Hello! On Wed, Aug 23, 2017 at 09:20:50PM -0500, Nate Karstens wrote: > # HG changeset patch > # User Nate Karstens# Date 1503540059 > 18000 > # Wed Aug 23 21:00:59 2017 -0500 > # Node ID 97953fe374455a04973268c4b2fbadd7ced91ffe > # Parent 17c038b56051f922ec440d40e23e8d1b23d835df > [PATCH 2 of 4] SSL: add support for PSK cipher suites. There is no need to include "[PATCH 2 of 4] " into patch summary line. [...] > @@ -1163,6 +1177,211 @@ ngx_ssl_ecdh_curve(ngx_conf_t *cf, ngx_s > > > ngx_int_t > +ngx_ssl_psk_file(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *file) { > +#ifdef PSK_MAX_IDENTITY_LEN > + > +ngx_fd_tfd; > +ngx_int_t err; > +ngx_file_info_t fi; Style: there should be two spaces between the longest type and a variable. I've already explained details in one of the previous reviews here: http://mailman.nginx.org/pipermail/nginx-devel/2017-June/010247.html > + > +err = NGX_OK; > + > +if (ngx_conf_full_name(cf->cycle, file, 1) != NGX_OK) { > +return NGX_ERROR; > +} > + > +fd = ngx_open_file(file->data, NGX_FILE_RDONLY, NGX_FILE_OPEN, 0); > +if (fd == NGX_INVALID_FILE) { > +ngx_ssl_error(NGX_LOG_EMERG, ssl->log, ngx_errno, > + ngx_open_file_n " \"%V\" failed", file); > +return NGX_ERROR; > +} > + > +if (ngx_fd_info(fd, ) == NGX_FILE_ERROR) { > +ngx_ssl_error(NGX_LOG_EMERG, ssl->log, ngx_errno, > + ngx_fd_info_n " \"%V\" failed", file); > +err = NGX_ERROR; > +goto failed; > +} > + > +if (ngx_file_size() == 0) { > +ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0, > + "PSK file \"%V\" is empty", file); > +err = NGX_ERROR; > +goto failed; > +} What's the point in opening the file here? As long the file is being read at run-time, it doesn't seem to make sense. The file can be legitimately created later. Checking ngx_file_size() looks completely wrong as well. It will prevent configurations with no currently configured PSK keys from running if the file size is 0 - but won't do this as long as there is a comment and/or some garbage in the file. > + > +failed: > + > +if (ngx_close_file(fd) == NGX_FILE_ERROR) { > +ngx_ssl_error(NGX_LOG_EMERG, ssl->log, ngx_errno, > + ngx_close_file_n " %V failed", file); > +err = NGX_ERROR; > +} > + > +if (err != NGX_OK) { > +return err; > +} > + > +if (SSL_CTX_set_ex_data(ssl->ctx, ngx_ssl_psk_index, file) == 0) { > +ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0, > + "SSL_CTX_set_ex_data() failed"); > +return NGX_ERROR; > +} > + > +SSL_CTX_set_psk_server_callback(ssl->ctx, ngx_ssl_psk_callback); > + > +#endif > + > +return NGX_OK; > +} > + > + > +#ifdef PSK_MAX_IDENTITY_LEN > + > +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, *colon; > +size_t len; > +ssize_t n; > +SSL_CTX*ssl_ctx; > +ngx_fd_tfd; > +ngx_str_t *file; > +unsigned intpsk_len; > +ngx_connection_t *c; > +u_char buf[NGX_SSL_PSK_BUFFER_SIZE]; Style: there should be two spaces between "ngx_connection_t" and "*c". > + > +psk_len = 0; This looks to early for the psk_len initialization. Rather, I would move the initialization to somewhere near "len = 0; last = buf" below where it will look more logical. > +c = ngx_ssl_get_connection(ssl_conn); > + > +ssl_ctx = SSL_get_SSL_CTX(ssl_conn); > +file = SSL_CTX_get_ex_data(ssl_ctx, ngx_ssl_psk_index); > + > +fd = ngx_open_file(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", 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", file); > +psk_len = 0; There is no need to set psk_len to 0 here, as it is already initialized to 0 and never set to anything else except before "goto cleanup". > +
RE: [PATCH 1 of 3] PSK: connection support
Maxim, I'm starting to get back to this now and wanted to follow up on your comments in http://mailman.nginx.org/pipermail/nginx-devel/2017-June/010247.html to see if you had a preference on when the PSKs should be read from the file. I believe we have two options: 1) At startup into a hash. Advantages: Faster lookups, PSK file can be read-only by root, can warn on syntax errors Disadvantages: Must reload to add/remove PSKs, more config options (for the hash max size and bucket size) 2) As needed. This is what was done in the last patch, but changed to no longer check for syntax errors at startup. Advantages: Do not need to reload to add/remove users, can control access using groups Disadvantages: Slower lookups (linear, but this can be improved using a RAM disk) Thanks, Nate -Original Message- From: nginx-devel [mailto:nginx-devel-boun...@nginx.org] On Behalf Of Maxim Dounin Sent: Friday, June 30, 2017 6:58 AM To: nginx-devel@nginx.org Subject: Re: [PATCH 1 of 3] PSK: connection support Hello! On Thu, Jun 29, 2017 at 10:00:45PM +, Karstens, Nate wrote: > Thanks for the comments. I'll try to start on those in a couple of > days. Just to make it clear: there is no need to hurry. Likely I won't be able to review new patches in at least a couple of weeks, so feel free to spend more time polishing the patches. > 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. The "hg email" command can work with any SMTP server, including Exchange. Or you can ensure proper threading manually by using a "reply" function. > 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: TLS v1.3 session resumption uses PSK internally, but it is very different from internal usage point of view. It is handled well enough with existing session cache / session tickets mechanisms. [...] > > +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; } [...] > > @@ -800,6 +810,12 @@ > > > > } > > > > +if (ngx_ssl_psk_file(cf, >ssl, >psk_file) > > +!= NGX_OK) > > +{ > > +return NGX_CONF_ERROR; > > +} Note: this calls ngx_ssl_psk_file() unconditionally, and ngx_ssl_psk_file() also doesn't check if a file is configured. As a result, a configuration without ssl_psk_file fails. -- Maxim Dounin http://nginx.org/ ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel 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
RE: [PATCH 1 of 3] PSK: connection support
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-boun...@nginx.org] On Behalf Of Maxim Dounin Sent: Thursday, June 29, 2017 4:09 PM To: nginx-devel@nginx.org Subject: Re: [PATCH 1 of 3] PSK: connection support Hello! On Thu, Jun 22, 2017 at 01:24:54PM +, Karstens, Nate wrote: > # HG changeset patch > # User Nate Karstens <nate.karst...@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 " 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.karst...@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 keyw
RE: Testing TLSv1.3 session resumption in Nginx using PSK
Hello, I submitted some patches a few days ago that implement PSK support in nginx, maybe you could take a look and comment if these will meet your requirements (or at least serve as a starting point)? Please see: * http://mailman.nginx.org/pipermail/nginx-devel/2017-June/010203.html * http://mailman.nginx.org/pipermail/nginx-devel/2017-June/010202.html * http://mailman.nginx.org/pipermail/nginx-devel/2017-June/010204.html Thanks, Nate Karstens Garmin International, Inc. From: nginx-devel [mailto:nginx-devel-boun...@nginx.org] On Behalf Of Utkarsh Tewari Sent: Wednesday, June 28, 2017 5:08 PM To: nginx-devel@nginx.org Subject: Testing TLSv1.3 session resumption in Nginx using PSK Hi, I have been trying to implement a TLSv1.3 session resumption mechanism with Nginx and openssl-1.1.1-dev. Basic TLSv1.3 handshake works fine. Now I wanna test session resumption via PSK but couldn't find a way to do it. Please guide me on how to enable the Nginx server to send a new_session_ticket to the client in order to enable session resumption using PSK in TLSv1.3 Thank you, Utkarsh Tewari Graduate student at San Jose State University [Image removed by sender.]ᐧ 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
[PATCH 3 of 3] PSK: add identity hint config directive
# HG changeset patch # User Nate Karstens# Date 1498137243 18000 # Thu Jun 22 08:14:03 2017 -0500 # Node ID b706695658216c88716904519467a36c1aac7ac9 # Parent a4635fa4a0cabf5312cda617b8010ea14279ab1c PSK: add identity hint config directive Adds the directive "ssl_psk_identity_hint" to the ngx_http_ssl_module. This allows the user to specify the PSK identity hint given to the connecting client. Signed-off-by: Nate Karstens diff -r a4635fa4a0ca -r b70669565821 contrib/vim/syntax/nginx.vim --- a/contrib/vim/syntax/nginx.vim Thu Jun 22 08:13:27 2017 -0500 +++ b/contrib/vim/syntax/nginx.vim Thu Jun 22 08:14:03 2017 -0500 @@ -551,6 +551,7 @@ syn keyword ngxDirective contained ssl_preread syn keyword ngxDirective contained ssl_protocols syn keyword ngxDirective contained ssl_psk_file +syn keyword ngxDirective contained ssl_psk_identity_hint syn keyword ngxDirective contained ssl_session_cache syn keyword ngxDirective contained ssl_session_ticket_key syn keyword ngxDirective contained ssl_session_tickets diff -r a4635fa4a0ca -r b70669565821 src/http/modules/ngx_http_ssl_module.c --- a/src/http/modules/ngx_http_ssl_module.cThu Jun 22 08:13:27 2017 -0500 +++ b/src/http/modules/ngx_http_ssl_module.cThu Jun 22 08:14:03 2017 -0500 @@ -241,6 +241,13 @@ offsetof(ngx_http_ssl_srv_conf_t, psk_file), NULL }, +{ ngx_string("ssl_psk_identity_hint"), + 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_identity_hint), + NULL }, + ngx_null_command }; @@ -550,6 +557,7 @@ * sscf->stapling_file = { 0, NULL }; * sscf->stapling_responder = { 0, NULL }; * sscf->psk_file = { 0, NULL }; + * sscf->psk_identity_hint = { 0, NULL }; */ sscf->enable = NGX_CONF_UNSET; @@ -632,6 +640,7 @@ prev->stapling_responder, ""); ngx_conf_merge_str_value(conf->psk_file, prev->psk_file, ""); +ngx_conf_merge_str_value(conf->psk_identity_hint, prev->psk_identity_hint, ""); conf->ssl.log = cf->log; @@ -819,6 +828,15 @@ return NGX_CONF_ERROR; } +if (conf->psk_identity_hint.len != 0) { +if (SSL_CTX_use_psk_identity_hint(conf->ssl.ctx, + (char *) conf->psk_identity_hint.data) +!= 1) +{ +return NGX_CONF_ERROR; +} +} + return NGX_CONF_OK; } diff -r a4635fa4a0ca -r b70669565821 src/http/modules/ngx_http_ssl_module.h --- a/src/http/modules/ngx_http_ssl_module.hThu Jun 22 08:13:27 2017 -0500 +++ b/src/http/modules/ngx_http_ssl_module.hThu Jun 22 08:14:03 2017 -0500 @@ -56,6 +56,7 @@ ngx_str_t stapling_responder; ngx_str_t psk_file; +ngx_str_t psk_identity_hint; u_char *file; ngx_uint_t line; 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
[PATCH 1 of 3] PSK: connection support
# HG changeset patch # User Nate Karstens# Date 1498137180 18000 # Thu Jun 22 08:13:00 2017 -0500 # Node ID 3fb3c4928d06029ca1d57853a163c9f56fa90bca # Parent 6169dbad37d85fa8642b9d0a51f0f0f6c19dd3d1 PSK: connection support 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. PSK functionality can be easily tested with the OpenSSL s_client using the "-psk" and "-psk_identity" options. Signed-off-by: Nate Karstens 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); return NGX_OK; } @@ -875,6 +892,29 @@ } +static unsigned int ngx_ssl_psk_callback(SSL *ssl, const char *identity, +unsigned char *psk, unsigned int max_psk_len) +{ +SSL_CTX*ssl_ctx; +ngx_str_t *psk_file; +ngx_int_t psk_len; + +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) { +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) + +{ +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
RE: PSK Support
Maxim, OK, we can skip the patch for turning off the certificate warnings (and just use a dummy certificate) and just support a single PSK file. The {HEX} prefix seems OK. I think it would also be good to support an {ASC}. It is unlikely that anyone would have an ASCII-based PSK that starts with {HEX}, but using {ASC} would provide a way to make prevent that case. Also, instead of referring to text-based PSKs as ASCII, maybe they should be UTF8-encoded and referred to as {TXT}? Nate -Original Message- From: nginx-devel [mailto:nginx-devel-boun...@nginx.org] On Behalf Of Maxim Dounin Sent: Wednesday, June 07, 2017 1:13 PM To: nginx-devel@nginx.org Subject: Re: PSK Support Hello! On Wed, Jun 07, 2017 at 05:02:52AM +, Karstens, Nate wrote: > Maxim, > > The biggest downside that I can see to the dummy certificate approach > is documentation. Using a dummy certificate wasn't immediately obvious > to me, though perhaps my familiarity with OpenSSL's "nocert" option > may have affected that. Which do you think would be easier for the > user to find in the documentation: > 1) a description of the dummy certificate approach under the > "ssl_certificate" directive, 2) a separate directive ("ssl_nocert"), > or 3) an explicit option to the "ssl_certificate" directive (e.g., " > Syntax: ssl_certificate file | off;")? In most cases, normal users won't need PSK at all, or will use it combined with certificates anyway. Describing in the documentation that "if you need to use PSK only, you still have to configure a certificate" doesn't looks like a big problem to me. Note well that there is a side problem with "listen ... ssl" used in server block but no ssl_certificate configured for the server. As of now, it is not reported during configuration parsing and may result in non-obvious behaviour in some cases, see ticket #178 (https://trac.nginx.org/nginx/ticket/178). This is a separate problem though, and it can't be fixed with the changes suggested. On the other hand, properly fixing it will greatly improve user experience in many cases, including PSK. > I'm OK with changing it to read from a password file (formatted in a > manner similar to stunnel) that is searched as needed (an > "ssl_psk_file" directive). Would it be OK to support multiple files > and stipulate that files are searched in the order that they are > included in nginx.conf? I don't think that supporting multiple files is a good idea, a single one should be enough. Possible alternative names: - "ssl_psk_secrets", similar to the one used by stunnel; - "ssl_pre_shared_keys". Not sure if these are better though. > Can we support both ASCII and binary PSKs? RFC 4279 section 5.4 seems > to require both types, and I need binary keys for my application :). > Maybe a parameter to the "ssl_psk_file" > directive could indicate how the PSKs are stored in the file? We can consider providing an alternative form to allow arbitrary keys, e.g., by using hex-encoded keys. This can be done using some explicit prefix, something like this: identity:key identity:0x6b6579 identity:{HEX}6b6579 (The last variant is in line with "{scheme}data" syntax as used in auth_basic_user_file. Not sure if we need it here, just "0x" might be easier.) -- Maxim Dounin http://nginx.org/ ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel 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
RE: PSK Support
Maxim, The biggest downside that I can see to the dummy certificate approach is documentation. Using a dummy certificate wasn't immediately obvious to me, though perhaps my familiarity with OpenSSL's "nocert" option may have affected that. Which do you think would be easier for the user to find in the documentation: 1) a description of the dummy certificate approach under the "ssl_certificate" directive, 2) a separate directive ("ssl_nocert"), or 3) an explicit option to the "ssl_certificate" directive (e.g., " Syntax: ssl_certificate file | off;")? I'm OK with changing it to read from a password file (formatted in a manner similar to stunnel) that is searched as needed (an "ssl_psk_file" directive). Would it be OK to support multiple files and stipulate that files are searched in the order that they are included in nginx.conf? Can we support both ASCII and binary PSKs? RFC 4279 section 5.4 seems to require both types, and I need binary keys for my application :). Maybe a parameter to the "ssl_psk_file" directive could indicate how the PSKs are stored in the file? Thanks, Nate -Original Message- From: nginx-devel [mailto:nginx-devel-boun...@nginx.org] On Behalf Of Maxim Dounin Sent: Monday, June 05, 2017 1:40 PM To: nginx-devel@nginx.org Subject: Re: PSK Support Hello! On Mon, Jun 05, 2017 at 02:08:15PM +, Karstens, Nate wrote: > Maxim, > > Thanks for the reply. I understand your concerns about PSK. We > discussed it quite a bit, but ultimately decided that a PKI was not > practical for our environment. We have to rely on the end user to > configure security and any solution using PKI would be so difficult to > work with that they just wouldn't bother with security at all. Ok, understood. I think that PSK can be a reasonable alternative to using plain http in many cases. > I considered some alternatives on the "ssl_nocert" option. My > preference would have been to analyze the supported cipher suites > (from "ssl_ciphers") and determine if any include a PSK, but it does > not look like OpenSSL exposes APIs to accomplish this. By default, nginx uses "HIGH:!aNULL:!MD5" as ciphers list, and this includes various PSK ciphers as well, so this approach doesn't look working even if there were appropriate APIs. > Using a dummy certificate seemed more complicated than the other two > suggestions you had (using "ssl_certificate" with a value of "off" or > disabling the tests if there are PSK secrets), so I'd prefer one of > those two. What is your preference? Using a dummy certificate has an obvious benefit of not requiring any changes to the code, and might actually be a good starting option. Disabling the tests with PSK secrets might not work as expected when they are defined at the http{} level. Using "ssl_certificate off" is obviously most explicit of all options, but I would rather consider a dummy certificate instead for now, as long as there are no other downsides. > One advantage of the PSK path concept is that it provides a lot of > flexibility. It allows, for example, multiple applications to each > independently manage their own PSKs without the need to coordinate > changes to a single file (note that in this scenario each application > would want to use $ssl_psk_identity to check the key). On the one hand, it is a plus. On the other - it is a nightmare when something goes wrong. I would rather avoid such approach. > stunnel uses a single file and seems to assume that keys will be ASCII > strings. Its format, for example, would not allow NUL to appear in the > string, as that would terminate the key early and, at best, lead to a > reduced key size. Yes, and stunnel author considers this to be a feature, see https://www.stunnel.org/pipermail/stunnel-users/2015-October/005275.html. If you are targeting end-users, it might be actually easier to use sufficiently long printable keys then arbitrary binary strings. > I might be mistaken, but wouldn't changing a certificate also require > reloading the configuration? Do you have some ideas on how this could > be done without requiring a reload? Yes, changing a certificate requires a reload. But the "path" concept is generally used in SSL where appropriate filesystem lookups are done on the fly, in contrast to loading a file into memory and then working with the data from memory. Consider "openssl verify -CApath" vs. "openssl verify -CAfile". Additionally, PSK keys look much more dynamic than certificates, as adding a user requires configuration changes. With PKI, you don't need any certificate changes on the server to add a user. With PSK, you have to add a key to introduce a new user. Overall, PSK seems to be very close to basic authentication, and it
RE: PSK Support
Maxim, Thanks for the reply. I understand your concerns about PSK. We discussed it quite a bit, but ultimately decided that a PKI was not practical for our environment. We have to rely on the end user to configure security and any solution using PKI would be so difficult to work with that they just wouldn't bother with security at all. I considered some alternatives on the "ssl_nocert" option. My preference would have been to analyze the supported cipher suites (from "ssl_ciphers") and determine if any include a PSK, but it does not look like OpenSSL exposes APIs to accomplish this. Using a dummy certificate seemed more complicated than the other two suggestions you had (using "ssl_certificate" with a value of "off" or disabling the tests if there are PSK secrets), so I'd prefer one of those two. What is your preference? One advantage of the PSK path concept is that it provides a lot of flexibility. It allows, for example, multiple applications to each independently manage their own PSKs without the need to coordinate changes to a single file (note that in this scenario each application would want to use $ssl_psk_identity to check the key). stunnel uses a single file and seems to assume that keys will be ASCII strings. Its format, for example, would not allow NUL to appear in the string, as that would terminate the key early and, at best, lead to a reduced key size. I might be mistaken, but wouldn't changing a certificate also require reloading the configuration? Do you have some ideas on how this could be done without requiring a reload? I agree on the proxy_ssl_* directives; the same could probably be said for the mail and stream SSL modules. Regards, Nate -Original Message- From: nginx-devel [mailto:nginx-devel-boun...@nginx.org] On Behalf Of Maxim Dounin Sent: Monday, June 05, 2017 8:09 AM To: nginx-devel@nginx.org Subject: Re: PSK Support Hello! On Thu, Jun 01, 2017 at 05:20:53PM +, Karstens, Nate wrote: > Greetings, > > I'm about push 3 patches that add support for PSK TLS cipher suites to > nginx and thought it would be good to discuss the feature itself in a > separate thread. > > First, PSK support is useful in certain environments that are not > conducive to a full public key infrastructure. The environment I'm > personally working with is the recreational boating market; we are > developing a new industry standard that relies on HTTPS, secured by > PSK, for much of its underlying security protocol. I think this would > also be useful to the IoT market. A quick search shows that some other > users have been interested in this feature: > > https://forum.nginx.org/read.php?2,272443,272443 > https://stackoverflow.com/questions/22513641/pre-shared-keys-tls-psk-n > ginx-configuration I have mixed feelings about PSK. On the one hand, it is expected to be better for constrained devices and also may have various performance benefits for internal connections. On the other hand, using a shared key is a big step backwards compared to public-key cryptography. > After applying the patches, one can enable PSK support by adding a few > directives to their nginx.conf: > > 1) "ssl_nocert" -- This disables checks for a certificate within > nginx. By default these checks are enabled because most users will > need a certificate. This is analogous to the "-nocert" > option in the OpenSSL s_server. > 2) "ssl_psk_path" -- This is a local folder that contains all of the > valid PSKs. Each file in the folder is loaded into memory as a PSK, > and its file name is used as the PSK identity. When the client > connects it specifies the identity of the PSK it is using for the > connection. The server looks up the key using hash of the loaded PSKs > and if the keys match then the TLS handshake is successful. Note that > the identity of the PSK is made available in the variable > $ssl_psk_identity. > 3) Add some PSK ciphers to the "ssl_ciphers" directive. Some comments, in no particular order: - the "ssl_nocert" directive looks strange / unneeded, there should be a better way to do this (ssl_certificate with a special value "off"? just assume a certificate is not needed if there are PSK secrets? always require a certificate, and let users who don't need specify a dummy one?); - "ssl_psk_path" seems to be overcomplicated, and yet non-flexible as any change to keys requires configuration reloads; - the only server software with PSK support seems to be stunnel, it might be good to be compatible with the form it uses for PSK secrets (a file with "IDENTITY:KEY" lines); - probably eventually there should be a proxy_ssl_* counterpart, though it is perfectly ok to don't have it for now. -- Maxim Dounin http://nginx.org/ ___
[PATCH 3 of 3] PSK: add PSK identity variable
# HG changeset patch # User Nate Karstens# Date 1496332963 18000 # Thu Jun 01 11:02:43 2017 -0500 # Node ID cb09937f63834ab74b49a76b9b158dd0a5871309 # Parent 7aa7771191d61ef635478460017446bca1f6db55 PSK: add PSK identity variable Adds the variable $ssl_psk_identity to get the PSK identity used in a connnection secured with a PSK cipher suite. Signed-off-by: Nate Karstens diff -r 7aa7771191d6 -r cb09937f6383 src/event/ngx_event_openssl.c --- a/src/event/ngx_event_openssl.c Thu Jun 01 11:01:05 2017 -0500 +++ b/src/event/ngx_event_openssl.c Thu Jun 01 11:02:43 2017 -0500 @@ -4286,6 +4286,33 @@ } +ngx_int_t +ngx_ssl_get_psk_identity(ngx_connection_t *c, ngx_pool_t *pool, ngx_str_t *s) +{ +const char *identity; +size_t len; + +identity = SSL_get_psk_identity(c->ssl->connection); + +if (identity == NULL) { +s->len = 0; +return NGX_OK; +} + +len = ngx_strlen(identity); + +s->data = ngx_pnalloc(pool, len); +if (s->data == NULL) { +return NGX_ERROR; +} + +ngx_memcpy(s->data, identity, len); +s->len = len; + +return NGX_OK; +} + + static time_t ngx_ssl_parse_time( #if OPENSSL_VERSION_NUMBER > 0x1010L diff -r 7aa7771191d6 -r cb09937f6383 src/event/ngx_event_openssl.h --- a/src/event/ngx_event_openssl.h Thu Jun 01 11:01:05 2017 -0500 +++ b/src/event/ngx_event_openssl.h Thu Jun 01 11:02:43 2017 -0500 @@ -235,6 +235,8 @@ ngx_str_t *s); ngx_int_t ngx_ssl_get_client_v_remain(ngx_connection_t *c, ngx_pool_t *pool, ngx_str_t *s); +ngx_int_t ngx_ssl_get_psk_identity(ngx_connection_t *c, ngx_pool_t *pool, +ngx_str_t *s); ngx_int_t ngx_ssl_handshake(ngx_connection_t *c); diff -r 7aa7771191d6 -r cb09937f6383 src/http/modules/ngx_http_ssl_module.c --- a/src/http/modules/ngx_http_ssl_module.cThu Jun 01 11:01:05 2017 -0500 +++ b/src/http/modules/ngx_http_ssl_module.cThu Jun 01 11:02:43 2017 -0500 @@ -357,6 +357,9 @@ { ngx_string("ssl_client_v_remain"), NULL, ngx_http_ssl_variable, (uintptr_t) ngx_ssl_get_client_v_remain, NGX_HTTP_VAR_CHANGEABLE, 0 }, +{ ngx_string("ssl_psk_identity"), NULL, ngx_http_ssl_variable, + (uintptr_t) ngx_ssl_get_psk_identity, NGX_HTTP_VAR_CHANGEABLE, 0 }, + { ngx_null_string, NULL, NULL, 0, 0, 0 } }; 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
[PATCH 2 of 3] PSK: add configuration directives
# HG changeset patch # User Nate Karstens# Date 1496332865 18000 # Thu Jun 01 11:01:05 2017 -0500 # Node ID 7aa7771191d61ef635478460017446bca1f6db55 # Parent a38066b79d71b6ecb62a9f7618afe2cf3ed8a4f9 PSK: add configuration directives Adds the directive "ssl_psk_dir" to the ngx_http_ssl_module. This allows the user to specify a folder that contains the set of PSKs that can be used to form a connection. Each key in the directory is read into memory and the file name is saved as the key identity. Also added the "ssl_psk_hash_max_size" and "ssl_psk_hash_bucket_size" directives to configure the hash that stores PSK data. This functionality can be easily tested with the OpenSSL s_client using the "-psk" and "-psk_identity" options. Signed-off-by: Nate Karstens diff -r a38066b79d71 -r 7aa7771191d6 contrib/vim/syntax/nginx.vim --- a/contrib/vim/syntax/nginx.vim Thu Jun 01 10:55:04 2017 -0500 +++ b/contrib/vim/syntax/nginx.vim Thu Jun 01 11:01:05 2017 -0500 @@ -551,6 +551,9 @@ 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_path +syn keyword ngxDirective contained ssl_psk_hash_bucket_size +syn keyword ngxDirective contained ssl_psk_hash_max_size syn keyword ngxDirective contained ssl_session_cache syn keyword ngxDirective contained ssl_session_ticket_key syn keyword ngxDirective contained ssl_session_tickets diff -r a38066b79d71 -r 7aa7771191d6 src/event/ngx_event_openssl.c --- a/src/event/ngx_event_openssl.c Thu Jun 01 10:55:04 2017 -0500 +++ b/src/event/ngx_event_openssl.c Thu Jun 01 11:01:05 2017 -0500 @@ -23,6 +23,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); @@ -114,6 +116,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 +228,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 +355,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); return NGX_OK; } @@ -875,6 +886,40 @@ } +static unsigned int ngx_ssl_psk_callback(SSL *ssl, const char *identity, +unsigned char *psk, unsigned int max_psk_len) +{ +SSL_CTX*ssl_ctx; +size_t identity_len; +ngx_hash_t *psk_hash; +ngx_uint_t key; +ngx_str_t *psk_str; + +ssl_ctx = SSL_get_SSL_CTX(ssl); +identity_len = strlen(identity); + +psk_hash = SSL_CTX_get_ex_data(ssl_ctx, ngx_ssl_psk_index); +if (psk_hash == NULL) { +return 0; +} + +key = ngx_hash_key((u_char *)identity, identity_len); + +psk_str = ngx_hash_find(psk_hash, key, (u_char *)identity, identity_len); +if (psk_str == NULL) { +return 0; +} + +if (psk_str->len > max_psk_len) { +return 0; +} + +ngx_memcpy(psk, psk_str->data, psk_str->len); + +return psk_str->len; +} + + RSA * ngx_ssl_rsa512_key_callback(ngx_ssl_conn_t *ssl_conn, int is_export, int key_length) @@ -3137,6 +3182,158 @@ #endif +ngx_int_t +ngx_ssl_psk_path(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *path, + ngx_uint_t psk_hash_max_size, ngx_uint_t psk_hash_bucket_size) + +{ +ngx_err_t err; +ngx_uint_t level; +ngx_dir_t dir; +ngx_file_t file; +ssize_t read; +ngx_str_t *psk_str; +ngx_hash_keys_arrays_t psk_keys; +ngx_str_t key; +ngx_hash_t *psk_hash; +ngx_hash_init_t hash; + +if (path->len == 0) { +return NGX_OK; +} + +psk_keys.pool = cf->pool; +psk_keys.temp_pool = cf->temp_pool; + +ngx_hash_keys_array_init(_keys, NGX_HASH_SMALL); + +if (ngx_open_dir(path, ) == NGX_ERROR) { +err = ngx_errno; + +level = (err == NGX_ENOENT + || err == NGX_ENOTDIR + || err == NGX_ENAMETOOLONG + || err == NGX_EACCES) ? NGX_LOG_ERR : NGX_LOG_CRIT; + +ngx_ssl_error(level, ssl->log, err, + ngx_open_dir_n "
PSK Support
Greetings, I'm about push 3 patches that add support for PSK TLS cipher suites to nginx and thought it would be good to discuss the feature itself in a separate thread. First, PSK support is useful in certain environments that are not conducive to a full public key infrastructure. The environment I'm personally working with is the recreational boating market; we are developing a new industry standard that relies on HTTPS, secured by PSK, for much of its underlying security protocol. I think this would also be useful to the IoT market. A quick search shows that some other users have been interested in this feature: https://forum.nginx.org/read.php?2,272443,272443 https://stackoverflow.com/questions/22513641/pre-shared-keys-tls-psk-nginx-configuration After applying the patches, one can enable PSK support by adding a few directives to their nginx.conf: 1) "ssl_nocert" -- This disables checks for a certificate within nginx. By default these checks are enabled because most users will need a certificate. This is analogous to the "-nocert" option in the OpenSSL s_server. 2) "ssl_psk_path" -- This is a local folder that contains all of the valid PSKs. Each file in the folder is loaded into memory as a PSK, and its file name is used as the PSK identity. When the client connects it specifies the identity of the PSK it is using for the connection. The server looks up the key using hash of the loaded PSKs and if the keys match then the TLS handshake is successful. Note that the identity of the PSK is made available in the variable $ssl_psk_identity. 3) Add some PSK ciphers to the "ssl_ciphers" directive. Thanks, Nate Karstens Garmin International, Inc. 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
[PATCH 1 of 3] PSK: make server certificates optional
# HG changeset patch # User Nate Karstens# Date 1496332504 18000 # Thu Jun 01 10:55:04 2017 -0500 # Node ID a38066b79d71b6ecb62a9f7618afe2cf3ed8a4f9 # Parent 716852cce9136d977b81a2d1b8b6f9fbca0dce49 PSK: make server certificates optional Adds the directive "ssl_nocert" to the ngx_http_ssl_module to allow the user to indicate that the absence of a certificate is intentional. Any cipher suites that rely on certificates will not function properly. Servers that only use PSK will error out without this change. Signed-off-by: Nate Karstens diff -r 716852cce913 -r a38066b79d71 contrib/vim/syntax/nginx.vim --- a/contrib/vim/syntax/nginx.vim Thu Jun 01 15:44:23 2017 +0300 +++ b/contrib/vim/syntax/nginx.vim Thu Jun 01 10:55:04 2017 -0500 @@ -546,6 +546,7 @@ syn keyword ngxDirective contained ssl_ecdh_curve syn keyword ngxDirective contained ssl_engine syn keyword ngxDirective contained ssl_handshake_timeout +syn keyword ngxDirective contained ssl_nocert syn keyword ngxDirective contained ssl_password_file syn keyword ngxDirective contained ssl_prefer_server_ciphers syn keyword ngxDirective contained ssl_preread diff -r 716852cce913 -r a38066b79d71 src/http/modules/ngx_http_ssl_module.c --- a/src/http/modules/ngx_http_ssl_module.cThu Jun 01 15:44:23 2017 +0300 +++ b/src/http/modules/ngx_http_ssl_module.cThu Jun 01 10:55:04 2017 -0500 @@ -101,6 +101,13 @@ 0, NULL }, +{ ngx_string("ssl_nocert"), + NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_FLAG, + ngx_conf_set_flag_slot, + NGX_HTTP_SRV_CONF_OFFSET, + offsetof(ngx_http_ssl_srv_conf_t, nocert), + NULL }, + { ngx_string("ssl_dhparam"), NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_TAKE1, ngx_conf_set_str_slot, @@ -546,6 +553,7 @@ sscf->buffer_size = NGX_CONF_UNSET_SIZE; sscf->verify = NGX_CONF_UNSET_UINT; sscf->verify_depth = NGX_CONF_UNSET_UINT; +sscf->nocert = NGX_CONF_UNSET; sscf->certificates = NGX_CONF_UNSET_PTR; sscf->certificate_keys = NGX_CONF_UNSET_PTR; sscf->passwords = NGX_CONF_UNSET_PTR; @@ -595,6 +603,7 @@ ngx_conf_merge_uint_value(conf->verify, prev->verify, 0); ngx_conf_merge_uint_value(conf->verify_depth, prev->verify_depth, 1); +ngx_conf_merge_value(conf->nocert, prev->nocert, 0); ngx_conf_merge_ptr_value(conf->certificates, prev->certificates, NULL); ngx_conf_merge_ptr_value(conf->certificate_keys, prev->certificate_keys, NULL); @@ -622,50 +631,52 @@ conf->ssl.log = cf->log; -if (conf->enable) { + if (!conf->nocert) { +if (conf->enable) { -if (conf->certificates == NULL) { -ngx_log_error(NGX_LOG_EMERG, cf->log, 0, - "no \"ssl_certificate\" is defined for " - "the \"ssl\" directive in %s:%ui", - conf->file, conf->line); -return NGX_CONF_ERROR; -} +if (conf->certificates == NULL) { +ngx_log_error(NGX_LOG_EMERG, cf->log, 0, + "no \"ssl_certificate\" is defined for " + "the \"ssl\" directive in %s:%ui", + conf->file, conf->line); +return NGX_CONF_ERROR; +} -if (conf->certificate_keys == NULL) { -ngx_log_error(NGX_LOG_EMERG, cf->log, 0, - "no \"ssl_certificate_key\" is defined for " - "the \"ssl\" directive in %s:%ui", - conf->file, conf->line); -return NGX_CONF_ERROR; -} +if (conf->certificate_keys == NULL) { +ngx_log_error(NGX_LOG_EMERG, cf->log, 0, + "no \"ssl_certificate_key\" is defined for " + "the \"ssl\" directive in %s:%ui", + conf->file, conf->line); +return NGX_CONF_ERROR; +} -if (conf->certificate_keys->nelts < conf->certificates->nelts) { -ngx_log_error(NGX_LOG_EMERG, cf->log, 0, - "no \"ssl_certificate_key\" is defined " - "for certificate \"%V\" and " - "the \"ssl\" directive in %s:%ui", - ((ngx_str_t *) conf->certificates->elts) - + conf->certificates->nelts - 1, - conf->file, conf->line); -return NGX_CONF_ERROR; -} +if (conf->certificate_keys->nelts < conf->certificates->nelts) { +ngx_log_error(NGX_LOG_EMERG, cf->log, 0, + "no \"ssl_certificate_key\" is defined " + "for certificate \"%V\" and " + "the \"ssl\" directive in %s:%ui", + ((ngx_str_t
RE: [PATCH] Proxy: support configuration of socket buffer sizes
Greetings, I just wanted to follow up on this patch and make sure that the fraud detection notice or confidentiality notice added by my company wasn't precluding it from consideration. Thanks! Nate -Original Message- From: nginx-devel [mailto:nginx-devel-boun...@nginx.org] On Behalf Of Karstens, Nate Sent: Sunday, April 30, 2017 12:19 PM To: nginx-devel@nginx.org Subject: [PATCH] Proxy: support configuration of socket buffer sizes [This sender failed our fraud detection checks and may not be who they appear to be. Learn about spoofing at http://aka.ms/LearnAboutSpoofing] # HG changeset patch # User Nate Karstens # Date 1493467011 18000 # Sat Apr 29 06:56:51 2017 -0500 # Node ID 1251a543804b17941b2c96b84bd1f4e58a37bc15 # Parent 8801ff7d58e1650c9d1abb50e09f5979e4f9ffbf Proxy: support configuration of socket buffer sizes Allows the size of the buffers used by the TCP sockets for HTTP proxy connections to be configured. The new configuration directives are: * proxy_socket_rcvbuf * proxy_socket_sndbuf These correspond with the SO_RCVBUF and SO_SNDBUF socket options, respectively. This is be useful in cases where the proxy processes received data slowly. Data was being buffered in three separate TCP buffers (nginx-from-client receive, nginx- to-proxy send, and proxy-from-nginx receive). The cumulative effect is that the client thinks it has sent all of the data, but times out waiting for a reply from the proxy, which cannot reply because it is still processing the data in its buffers. Signed-off-by: Nate Karstens diff -r 8801ff7d58e1 -r 1251a543804b src/event/ngx_event_connect.c --- a/src/event/ngx_event_connect.c Tue Apr 25 23:39:13 2017 +0300 +++ b/src/event/ngx_event_connect.c Sat Apr 29 06:56:51 2017 -0500 @@ -73,6 +73,16 @@ } } +if (pc->sndbuf) { +if (setsockopt(s, SOL_SOCKET, SO_SNDBUF, + (const void *) >sndbuf, sizeof(int)) == -1) +{ +ngx_log_error(NGX_LOG_ALERT, pc->log, ngx_socket_errno, + "setsockopt(SO_SNDBUF) failed"); +goto failed; +} +} + if (ngx_nonblocking(s) == -1) { ngx_log_error(NGX_LOG_ALERT, pc->log, ngx_socket_errno, ngx_nonblocking_n " failed"); diff -r 8801ff7d58e1 -r 1251a543804b src/event/ngx_event_connect.h --- a/src/event/ngx_event_connect.h Tue Apr 25 23:39:13 2017 +0300 +++ b/src/event/ngx_event_connect.h Sat Apr 29 06:56:51 2017 -0500 @@ -57,6 +57,7 @@ int type; int rcvbuf; +int sndbuf; ngx_log_t *log; diff -r 8801ff7d58e1 -r 1251a543804b src/http/modules/ngx_http_proxy_module.c --- a/src/http/modules/ngx_http_proxy_module.c Tue Apr 25 23:39:13 2017 +0300 +++ b/src/http/modules/ngx_http_proxy_module.c Sat Apr 29 06:56:51 2017 +++ -0500 @@ -90,6 +90,9 @@ ngx_uint_t headers_hash_max_size; ngx_uint_t headers_hash_bucket_size; +ngx_int_t rcvbuf; +ngx_int_t sndbuf; + #if (NGX_HTTP_SSL) ngx_uint_t ssl; ngx_uint_t ssl_protocols; @@ -629,6 +632,20 @@ offsetof(ngx_http_proxy_loc_conf_t, http_version), _http_proxy_http_version }, +{ ngx_string("proxy_socket_rcvbuf"), + NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1, + ngx_conf_set_num_slot, + NGX_HTTP_LOC_CONF_OFFSET, + offsetof(ngx_http_proxy_loc_conf_t, rcvbuf), + NULL }, + +{ ngx_string("proxy_socket_sndbuf"), + NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1, + ngx_conf_set_num_slot, + NGX_HTTP_LOC_CONF_OFFSET, + offsetof(ngx_http_proxy_loc_conf_t, sndbuf), + NULL }, + #if (NGX_HTTP_SSL) { ngx_string("proxy_ssl_session_reuse"), @@ -905,6 +922,13 @@ u->buffering = plcf->upstream.buffering; +if (plcf->rcvbuf != NGX_CONF_UNSET) { +u->peer.rcvbuf = plcf->rcvbuf; +} +if (plcf->sndbuf != NGX_CONF_UNSET) { +u->peer.sndbuf = plcf->sndbuf; +} + u->pipe = ngx_pcalloc(r->pool, sizeof(ngx_event_pipe_t)); if (u->pipe == NULL) { return NGX_HTTP_INTERNAL_SERVER_ERROR; @@ -2902,6 +2926,9 @@ conf->headers_hash_max_size = NGX_CONF_UNSET_UINT; conf->headers_hash_bucket_size = NGX_CONF_UNSET_UINT; +conf->rcvbuf = NGX_CONF_UNSET; +conf->sndbuf = NGX_CONF_UNSET; + ngx_str_set(>upstream.module, "proxy"); return conf; @@ -3297,6 +3324,12 @@ ngx_conf_merge_uint_value(conf->headers_hash_bucket_size, prev->headers_hash_bucket_size, 64); +ngx_conf_merge_value(conf->rcvbuf, +