@a.bavs...@nginx.com <a.bavs...@nginx.com>
Gentle Reminder for review. This feature to make ssl_client_certificate
optional may help us here at Nvidia.
Thanks in advance. Kindly let me know if any more modification is needed in
fix.
Note: AFAIK, mTLS was not supported with SSLv2. I kept the NGX_SSL_SSLv2
flag in fix, if Nginx today supports SSLv2.
On Mon, Aug 19, 2024 at 4:22 PM Praveen Chaudhary <pclico...@gmail.com>
wrote:
> Thanks Aleksei for the review.
>
> Agree, It makes sense to have explicit error message to require either
> ssl_client_certificate or ssl_trusted_certificate. Because:
> Nginx prints error number from SSL to identify SSL error\routine,
> but for a client or admin, it may be still hard to find why "SSL
> certificate error"
> is seen.
>
> V2 Patch.
> [Keeping same Subject]
>
> # HG changeset patch
> # User Praveen Chaudhary <praveen5...@gmail.com>
> # Date 1723406727 25200
> # Sun Aug 11 13:05:27 2024 -0700
> # Node ID a5525b8eac0e1f10da42b7367cd5296fb29f4787
> # Parent 8796dfbe7177cb0be2a53bcdb4d25cc64a58d2a7
> Make ssl_client_certificate directive optional with TLSv1.1+.
>
> - With TLS 1.1+, Certificate Authorities(CAs) are optional
> in the Certificate Request packet. This makes directive
> ssl_client_certificate also optional for mutual TLS
> configurations.
>
> - For TLS 1.1, check either ssl_client_certificate or
> ssl_trusted_certificate is non empty.
>
> diff -r 8796dfbe7177 -r a5525b8eac0e src/http/modules/ngx_http_ssl_module.c
> --- a/src/http/modules/ngx_http_ssl_module.c Mon Aug 12 18:21:01 2024 +0400
> +++ b/src/http/modules/ngx_http_ssl_module.c Sun Aug 11 13:05:27 2024 -0700
> @@ -788,9 +788,20 @@
> if (conf->verify) {
>
> if (conf->client_certificate.len == 0 && conf->verify != 3) {
> - ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
> - "no ssl_client_certificate for
> ssl_verify_client");
> - return NGX_CONF_ERROR;
> +
> + if (conf->protocols &
> + (NGX_SSL_SSLv2|NGX_SSL_SSLv3|NGX_SSL_TLSv1)) {
> + ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
> + "no ssl_client_certificate for
> ssl_verify_client");
> + return NGX_CONF_ERROR;
> + }
> + /* For TLS 1.1+. */
> + if (conf->trusted_certificate.len == 0) {
> + ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
> + "no ssl_client_certificate or "
> + "ssl_trusted_certificate for
> ssl_verify_client");
> + return NGX_CONF_ERROR;
> + }
> }
>
> if (ngx_ssl_client_certificate(cf, &conf->ssl,
> diff -r 8796dfbe7177 -r a5525b8eac0e src/mail/ngx_mail_ssl_module.c
> --- a/src/mail/ngx_mail_ssl_module.c Mon Aug 12 18:21:01 2024 +0400
> +++ b/src/mail/ngx_mail_ssl_module.c Sun Aug 11 13:05:27 2024 -0700
> @@ -451,9 +451,20 @@
> if (conf->verify) {
>
> if (conf->client_certificate.len == 0 && conf->verify != 3) {
> - ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
> - "no ssl_client_certificate for
> ssl_verify_client");
> - return NGX_CONF_ERROR;
> +
> + if (conf->protocols &
> + (NGX_SSL_SSLv2|NGX_SSL_SSLv3|NGX_SSL_TLSv1)) {
> + ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
> + "no ssl_client_certificate for
> ssl_verify_client");
> + return NGX_CONF_ERROR;
> + }
> + /* For TLS 1.1+. */
> + if (conf->trusted_certificate.len == 0) {
> + ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
> + "no ssl_client_certificate or "
> + "ssl_trusted_certificate for
> ssl_verify_client");
> + return NGX_CONF_ERROR;
> + }
> }
>
> if (ngx_ssl_client_certificate(cf, &conf->ssl,
> diff -r 8796dfbe7177 -r a5525b8eac0e src/stream/ngx_stream_ssl_module.c
> --- a/src/stream/ngx_stream_ssl_module.c Mon Aug 12 18:21:01 2024 +0400
> +++ b/src/stream/ngx_stream_ssl_module.c Sun Aug 11 13:05:27 2024 -0700
> @@ -933,9 +933,20 @@
> if (conf->verify) {
>
> if (conf->client_certificate.len == 0 && conf->verify != 3) {
> - ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
> - "no ssl_client_certificate for
> ssl_verify_client");
> - return NGX_CONF_ERROR;
> +
> + if (conf->protocols &
> + (NGX_SSL_SSLv2|NGX_SSL_SSLv3|NGX_SSL_TLSv1)) {
> + ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
> + "no ssl_client_certificate for
> ssl_verify_client");
> + return NGX_CONF_ERROR;
> + }
> + /* For TLS 1.1+. */
> + if (conf->trusted_certificate.len == 0) {
> + ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
> + "no ssl_client_certificate or "
> + "ssl_trusted_certificate for
> ssl_verify_client");
> + return NGX_CONF_ERROR;
> + }
> }
>
> if (ngx_ssl_client_certificate(cf, &conf->ssl,
>
> On Mon, Aug 19, 2024 at 11:41 AM Aleksei Bavshin <a.bavs...@nginx.com>
> wrote:
>
>> On 8/16/2024 8:02 AM, Praveen Chaudhary wrote:
>> > Hi Nginx Devs
>> >
>> > Bumping patch to the top for review.
>> >
>> > CC: @Sergey Kandaurov
>> > Thanks for contributing client certificate validation with OSCP. It is
>> > a long awaited feature.
>> > In this patch, I am trying to fix another lingering concern. It will be
>> > great, if you can have a look.
>>
>> Hello,
>>
>> Sending an empty list of CAs is explicitly mentioned starting from TLS
>> 1.1; RFC 4346 Section 7.4.4:
>>
>> If the certificate_authorities list is empty then the client MAY
>> send any certificate of the appropriate ClientCertificateType,
>> unless there is some external arrangement to the contrary.
>>
>> TLS 1.0 (RFC 2246 Section 7.4.4) does not specify any behavior. While
>> it's known that some 1.0 or SSL 3.0 clients can accept an empty list, it
>> could be safer to limit the ability to the TLS 1.1+ configurations.
>>
>> As for the means of doing so, simply skipping the
>> conf->client_certificate check is not correct. For any ssl_client_verify
>> mode other than 'optional_no_ca' nginx must have a list of known trusted
>> CAs, and the configuration without any is not valid.
>> A better approach is to require either 'ssl_client_certificate' or
>> 'ssl_trusted_certificate' to be set when the client cert verification is
>> enabled.
>>
>> >
>> > # HG changeset patch
>> > # User Praveen Chaudhary <praveen5...@gmail.com
>> > <mailto:praveen5...@gmail.com>>
>> > # Date 1723406727 25200
>> > # Sun Aug 11 13:05:27 2024 -0700
>> > # Node ID 199a35c74b60437da9d22a70d257507b4afb1878
>> > # Parent b5550a7f16c795f394f9d1ac87132dd2b7ef0e41
>> > Make ssl_client_certificate directive optional with TLSv1.3.
>> >
>> > - As per RFC 8446 Section 4.2.4, server MAY (not SHOULD or MUST)
>> > send Certificate Authorities (CAs) in the Certificate Request
>> > packet. This makes ssl_client_certificate directive optional
>> > when only TLS 1.3 is used for mutual TLS configurations.
>> > > - Today, Nginx requires ssl_client_certificate directive to
>> > be set to CA Certificates file, if ssl_verify_client is
>> > enabled, even when using only TLS 1.3. Else Nginx does not
>> > reload or restart.
>> >
>> > diff -r b5550a7f16c7 -r 199a35c74b60
>> src/http/modules/ngx_http_ssl_module.c
>> > --- a/src/http/modules/ngx_http_ssl_module.c Fri Aug 09 19:12:26 2024
>> +0400
>> > +++ b/src/http/modules/ngx_http_ssl_module.c Sun Aug 11 13:05:27 2024
>> -0700
>> > @@ -787,10 +787,16 @@
>> >
>> > if (conf->verify) {
>> >
>> > - if (conf->client_certificate.len == 0 && conf->verify != 3) {
>> > - ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
>> > - "no ssl_client_certificate for
>> > ssl_verify_client");
>> > - return NGX_CONF_ERROR;
>> > + if (conf->protocols & (NGX_SSL_TLSv1|NGX_SSL_TLSv1_1|
>> > NGX_SSL_TLSv1_2)) {
>> > + /*
>> > + For TLS 1.3, It is optional to send Certificate
>> Authorities in
>> > + Certificate Request Packet. RFC 8446#section-4.2.4
>> > + */
>> > + if (conf->client_certificate.len == 0 && conf->verify !=
>> 3) {
>> > + ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
>> > + "no ssl_client_certificate for
>> > ssl_verify_client");
>> > + return NGX_CONF_ERROR;
>> > + }
>> > }
>> >
>> > if (ngx_ssl_client_certificate(cf, &conf->ssl,
>> > diff -r b5550a7f16c7 -r 199a35c74b60 src/mail/ngx_mail_ssl_module.c
>> > --- a/src/mail/ngx_mail_ssl_module.c Fri Aug 09 19:12:26 2024 +0400
>> > +++ b/src/mail/ngx_mail_ssl_module.c Sun Aug 11 13:05:27 2024 -0700
>> > @@ -450,12 +450,19 @@
>> >
>> > if (conf->verify) {
>> >
>> > - if (conf->client_certificate.len == 0 && conf->verify != 3) {
>> > - ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
>> > - "no ssl_client_certificate for
>> > ssl_verify_client");
>> > - return NGX_CONF_ERROR;
>> > + if (conf->protocols & (NGX_SSL_TLSv1|NGX_SSL_TLSv1_1|
>> > NGX_SSL_TLSv1_2)) {
>> > + /*
>> > + For TLS 1.3, It is optional to send Certificate
>> Authorities in
>> > + Certificate Request Packet. RFC 8446#section-4.2.4
>> > + */
>> > + if (conf->client_certificate.len == 0 && conf->verify !=
>> 3) {
>> > + ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
>> > + "no ssl_client_certificate for
>> > ssl_verify_client");
>> > + return NGX_CONF_ERROR;
>> > + }
>> > }
>> >
>> > +
>> > if (ngx_ssl_client_certificate(cf, &conf->ssl,
>> > &conf->client_certificate,
>> > conf->verify_depth)
>> > diff -r b5550a7f16c7 -r 199a35c74b60 src/stream/ngx_stream_ssl_module.c
>> > --- a/src/stream/ngx_stream_ssl_module.c Fri Aug 09 19:12:26 2024 +0400
>> > +++ b/src/stream/ngx_stream_ssl_module.c Sun Aug 11 13:05:27 2024 -0700
>> > @@ -932,10 +932,16 @@
>> >
>> > if (conf->verify) {
>> >
>> > - if (conf->client_certificate.len == 0 && conf->verify != 3) {
>> > - ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
>> > - "no ssl_client_certificate for
>> > ssl_verify_client");
>> > - return NGX_CONF_ERROR;
>> > + if (conf->protocols & (NGX_SSL_TLSv1|NGX_SSL_TLSv1_1|
>> > NGX_SSL_TLSv1_2)) {
>> > + /*
>> > + For TLS 1.3, It is optional to send Certificate
>> Authorities in
>> > + Certificate Request Packet. RFC 8446#section-4.2.4
>> > + */
>> > + if (conf->client_certificate.len == 0 && conf->verify !=
>> 3) {
>> > + ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
>> > + "no ssl_client_certificate for
>> > ssl_verify_client");
>> > + return NGX_CONF_ERROR;
>> > + }
>> > }
>> >
>> > if (ngx_ssl_client_certificate(cf, &conf->ssl,
>> >
>> > _______________________________________________
>> > nginx-devel mailing list
>> > nginx-devel@nginx.org
>> > https://mailman.nginx.org/mailman/listinfo/nginx-devel
>> _______________________________________________
>> nginx-devel mailing list
>> nginx-devel@nginx.org
>> https://mailman.nginx.org/mailman/listinfo/nginx-devel
>>
>
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
