Re: [PATCH] OpenSSL-1.1.0锛歋upport Asynchronous Operations of SSL with openSSL-1.1.0

[Maxim Dounin]

Hello!

On Tue, Sep 07, 2021 at 04:48:08PM +0800, Junli Liu wrote:

> # HG changeset patch
> # User Junli Liu<jlliudh at isoftstone.com>
> # Date 1631003347 -28800
> #      Tue Sep 07 16:29:07 2021 +0800
> # Node ID 301a837387ed63bb2e455942ef2ef79bc9aaa972
> # Parent  2245324a507abc54cf0274fd1b1e81bfac7c1c73
> OpenSSL-1.1.0：Support Asynchronous Operations of SSL with openSSL-1.1.0
> 
> Security is critical to the foundation of networking and Transport Layer Security (TLS) is the backbone protocol for Internet security today. But normally, the introduction of TLS usually leads to network performance degradation, because encryption and decryption need to consume more compute resources.OpenSSL-1.1.0 has involved features of asynchronous operations to improve the performance of network and often combined with hardware feature.
> 
> This changeset make Nginx can work well with  OpenSSL asynchronous operations when process http request.

This patch seems to be borrowed from Intel's async work 
(https://github.com/intel/asynch_mode_nginx).  Do you have legal 
rights to submit this code?

Also, our previous testing of the code by Intel suggests that the 
code is buggy and unstable, and only beneficial when using Intel 
QAT with RSA certificates, but not with ECDSA.  Do you have any 
testing results which indicate that the patch is beneficial?

See below for some obvious comments about the code (not a full 
review).

> 
> diff -r 2245324a507a -r 301a837387ed auto/lib/openssl/conf
> --- a/auto/lib/openssl/conf	Thu Sep 02 12:25:37 2021 +0300
> +++ b/auto/lib/openssl/conf	Tue Sep 07 16:29:07 2021 +0800
> @@ -139,4 +139,33 @@
>          exit 1
>      fi
>  
> +    OPENSSL_ASYNC=
> +    if [ "$NGX_SSL_ASYNC" != NO ]; then
> +
> +        OPENSSL_ASYNC=NO
> +
> +        ngx_feature="OpenSSL library"
> +        ngx_feature_name=
> +        ngx_feature_run=no
> +        ngx_feature_incs="#include <openssl/ssl.h>"
> +        ngx_feature_path=
> +        ngx_feature_libs="-lssl -lcrypto"
> +        ngx_feature_test="#ifndef SSL_MODE_ASYNC
> +        error: not define async
> +        #endif
> +        "
> +        . auto/feature
> +        if [ $ngx_found = yes ]; then
> +            have=NGX_SSL_ASYNC . auto/have
> +            OPENSSL_ASYNC=YES
> +        fi
> +    fi
> +
> +    if [ -n "$OPENSSL_ASYNC"  -a  "$OPENSSL_ASYNC" != YES  ]; then
> +cat << END
> +$1: error: For using asynchronous mode, The OpenSSL must be version 1.1.0 or greater.
> +
> +END
> +        exit 1
> +    fi
>  fi

Environment variables is not something nginx configure is using to 
enable or disable features.

Note well that these checks are not going to work when building 
OpenSSL with nginx as per "./configure --with-openssl=...".  Given 
that it tests the SSL_MODE_ASYNC macro, a better idea 
would be to use compile-time checks in the code instead.

> diff -r 2245324a507a -r 301a837387ed src/core/nginx.c
> --- a/src/core/nginx.c	Thu Sep 02 12:25:37 2021 +0300
> +++ b/src/core/nginx.c	Tue Sep 07 16:29:07 2021 +0800
> @@ -182,6 +182,12 @@
>  static ngx_uint_t   ngx_show_help;
>  static ngx_uint_t   ngx_show_version;
>  static ngx_uint_t   ngx_show_configure;
> +#if (NGX_SSL && NGX_SSL_ASYNC)
> +/* indicate that nginx start without ngx_ssl_init()
> + * which will involve OpenSSL configuration file to
> + * start OpenSSL engine */
> +static ngx_uint_t   ngx_no_ssl_init;
> +#endif
>  static u_char      *ngx_prefix;
>  static u_char      *ngx_error_log;
>  static u_char      *ngx_conf_file;
> @@ -238,7 +244,13 @@
>  
>      /* STUB */
>  #if (NGX_OPENSSL)
> -    ngx_ssl_init(log);
> +#if (NGX_SSL && NGX_SSL_ASYNC)
> +    if (!ngx_no_ssl_init) {
> +#endif
> +        ngx_ssl_init(log);
> +#if (NGX_SSL && NGX_SSL_ASYNC)
> +    }
> +#endif
>  #endif
>  
>      /*
> @@ -248,6 +260,9 @@
>  
>      ngx_memzero(&init_cycle, sizeof(ngx_cycle_t));
>      init_cycle.log = log;
> +#if (NGX_SSL && NGX_SSL_ASYNC)
> +    init_cycle.no_ssl_init = ngx_no_ssl_init;
> +#endif
>      ngx_cycle = &init_cycle;
>  
>      init_cycle.pool = ngx_create_pool(1024, log);
> @@ -782,11 +797,17 @@
>  
>              case 't':
>                  ngx_test_config = 1;
> +#if (NGX_SSL && NGX_SSL_ASYNC)
> +                ngx_no_ssl_init = 1;
> +#endif
>                  break;

Note that this approach looks incorrect: testing nginx 
configuration may require properly initialized OpenSSL library, 
including various engines loaded, for example, to load default 
settings and/or private keys from hardware tokens.  While skipping 
OpenSSL initialization might work for a proof-of-concept code such 
as Intel's async work, this is not the approach usable for 
generic-purpose server such as nginx.

[...]

-- 
Maxim Dounin
http://mdounin.ru/