[PATCH] services: nginx: Add support for ssl-stapling in server blocks.

  • Open
  • quality assurance status badge
Details
3 participants
  • Christopher Baines
  • Maxim Cournoyer
  • mirai
Owner
unassigned
Submitted by
mirai
Severity
normal
M
M
mirai wrote on 27 Nov 2022 00:59
(address . guix-patches@gnu.org)(name . Bruno Victal)(address . mirai@makinata.eu)
9a18d0c03940cfe0d8ab01964f12d08fcc972e30.1669507155.git.mirai@makinata.eu
From: Bruno Victal <mirai@makinata.eu>

* gnu/services/web.scm (<nginx-server-configuration>): Add
ssl-stapling? and ssl-stapling-verify?.
* doc/guix.texi (NGINX): Document this.
---
doc/guix.texi | 7 +++++
gnu/services/web.scm | 69 +++++++++++++++++++++++++-------------------
2 files changed, 46 insertions(+), 30 deletions(-)

Toggle diff (128 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index e547d469f4..f116798dba 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -29339,6 +29339,13 @@ you don't have a certificate or you don't want to use HTTPS.
Where to find the private key for secure connections. Set it to @code{#f} if
you don't have a key or you don't want to use HTTPS.
+@item @code{ssl-stapling?} (default: @code{#f})
+Whether the server should @uref{https://datatracker.ietf.org/doc/html/rfc6066#section-8,staple OCSP responses}.
+Requires at least one @samp{resolver} directive in @code{raw-content}.
+
+@item @code{ssl-stapling-verify?} (default: @code{#f})
+Whether the server should verify the OCSP responses.
+
@item @code{server-tokens?} (default: @code{#f})
Whether the server should add its configuration to response.
diff --git a/gnu/services/web.scm b/gnu/services/web.scm
index 83aa97055f..8ab4050d47 100644
--- a/gnu/services/web.scm
+++ b/gnu/services/web.scm
@@ -510,48 +510,52 @@ (define httpd-service-type
(define-record-type* <nginx-server-configuration>
nginx-server-configuration make-nginx-server-configuration
nginx-server-configuration?
- (listen nginx-server-configuration-listen
- (default '("80" "443 ssl")))
- (server-name nginx-server-configuration-server-name
- (default (list 'default)))
- (root nginx-server-configuration-root
- (default "/srv/http"))
- (locations nginx-server-configuration-locations
- (default '()))
- (index nginx-server-configuration-index
- (default (list "index.html")))
- (try-files nginx-server-configuration-try-files
- (default '()))
- (ssl-certificate nginx-server-configuration-ssl-certificate
- (default #f))
- (ssl-certificate-key nginx-server-configuration-ssl-certificate-key
- (default #f))
- (server-tokens? nginx-server-configuration-server-tokens?
- (default #f))
- (raw-content nginx-server-configuration-raw-content
- (default '())))
+ (listen nginx-server-configuration-listen
+ (default '("80" "443 ssl")))
+ (server-name nginx-server-configuration-server-name
+ (default (list 'default)))
+ (root nginx-server-configuration-root
+ (default "/srv/http"))
+ (locations nginx-server-configuration-locations
+ (default '()))
+ (index nginx-server-configuration-index
+ (default (list "index.html")))
+ (try-files nginx-server-configuration-try-files
+ (default '()))
+ (ssl-certificate nginx-server-configuration-ssl-certificate
+ (default #f))
+ (ssl-certificate-key nginx-server-configuration-ssl-certificate-key
+ (default #f))
+ (ssl-stapling? nginx-server-configuration-ssl-stapling?
+ (default #f))
+ (ssl-stapling-verify? nginx-server-configuration-ssl-stapling-verify?
+ (default #f))
+ (server-tokens? nginx-server-configuration-server-tokens?
+ (default #f))
+ (raw-content nginx-server-configuration-raw-content
+ (default '())))
(define-record-type* <nginx-upstream-configuration>
nginx-upstream-configuration make-nginx-upstream-configuration
nginx-upstream-configuration?
- (name nginx-upstream-configuration-name)
- (servers nginx-upstream-configuration-servers)
- (extra-content nginx-upstream-configuration-extra-content
- (default '())))
+ (name nginx-upstream-configuration-name)
+ (servers nginx-upstream-configuration-servers)
+ (extra-content nginx-upstream-configuration-extra-content
+ (default '())))
(define-record-type* <nginx-location-configuration>
nginx-location-configuration make-nginx-location-configuration
nginx-location-configuration?
- (uri nginx-location-configuration-uri
- (default #f))
- (body nginx-location-configuration-body))
+ (uri nginx-location-configuration-uri
+ (default #f))
+ (body nginx-location-configuration-body))
(define-record-type* <nginx-named-location-configuration>
nginx-named-location-configuration make-nginx-named-location-configuration
nginx-named-location-configuration?
- (name nginx-named-location-configuration-name
- (default #f))
- (body nginx-named-location-configuration-body))
+ (name nginx-named-location-configuration-name
+ (default #f))
+ (body nginx-named-location-configuration-body))
(define-record-type* <nginx-configuration>
nginx-configuration make-nginx-configuration
@@ -628,6 +632,9 @@ (define (emit-nginx-server-config server)
(ssl-certificate (nginx-server-configuration-ssl-certificate server))
(ssl-certificate-key
(nginx-server-configuration-ssl-certificate-key server))
+ (ssl-stapling? (nginx-server-configuration-ssl-stapling? server))
+ (ssl-stapling-verify?
+ (nginx-server-configuration-ssl-stapling-verify? server))
(root (nginx-server-configuration-root server))
(index (nginx-server-configuration-index server))
(try-files (nginx-server-configuration-try-files server))
@@ -647,6 +654,8 @@ (define-syntax-rule (and/l x tail ...)
" server_name " (config-domain-strings server-name) ";\n"
(and/l ssl-certificate " ssl_certificate " <> ";\n")
(and/l ssl-certificate-key " ssl_certificate_key " <> ";\n")
+ " ssl_stapling " (if ssl-stapling? "on" "off") ";\n"
+ " ssl_stapling_verify " (if ssl-stapling-verify? "on" "off") ";\n"
(if (not (equal? "" root))
(list " root " root ";\n")
"")

base-commit: 68925b5ee7e0d96b0c84ae98a633eea5097bf511
--
2.38.1
C
C
Christopher Baines wrote on 7 Jan 2023 18:21
(address . mirai@makinata.eu)
87o7ramay8.fsf@cbaines.net
mirai@makinata.eu writes:

Toggle quote (10 lines)
> From: Bruno Victal <mirai@makinata.eu>
>
> * gnu/services/web.scm (<nginx-server-configuration>): Add
> ssl-stapling? and ssl-stapling-verify?.
> * doc/guix.texi (NGINX): Document this.
> ---
> doc/guix.texi | 7 +++++
> gnu/services/web.scm | 69 +++++++++++++++++++++++++-------------------
> 2 files changed, 46 insertions(+), 30 deletions(-)

Hi Bruno,

Thanks for the patch, and sorry it's taken so long to reply.

Toggle quote (12 lines)
> @@ -647,6 +654,8 @@ (define-syntax-rule (and/l x tail ...)
> " server_name " (config-domain-strings server-name) ";\n"
> (and/l ssl-certificate " ssl_certificate " <> ";\n")
> (and/l ssl-certificate-key " ssl_certificate_key " <> ";\n")
> + " ssl_stapling " (if ssl-stapling? "on" "off") ";\n"
> + " ssl_stapling_verify " (if ssl-stapling-verify? "on" "off") ";\n"
> (if (not (equal? "" root))
> (list " root " root ";\n")
> "")
>
> base-commit: 68925b5ee7e0d96b0c84ae98a633eea5097bf511

Generally this looks good to me. There's some unnecessary indentation
changes that should probably go in another commit if they're made, but I
did spot something in the above diff.

I'm no expert in NGinx configs, but I do wonder if this change will
break using nginx if it's built without the ngx_http_ssl_module? With
the other module specific configuration (e.g. ssl_certificate), it's
possible to specify a value in the <nginx-server-configuration> that
means the line won't be included in the configuration. I think it would
be good to continue that here.

I'm not sure how to enable not including these config lines. Maybe a
symbol value like 'noval could be used (this should also be the default,
rather than #f), or maybe 'on and 'off could be used as the values with
#f meaning the line isn't included.

Does that make sense?

Thanks,

Chris
-----BEGIN PGP SIGNATURE-----

iQKlBAEBCgCPFiEEPonu50WOcg2XVOCyXiijOwuE9XcFAmO5wA9fFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcRHG1haWxAY2Jh
aW5lcy5uZXQACgkQXiijOwuE9XcBFw//SeCdplyuIbCgasFPf6DHGBuLT7l74xpu
xGuAD33ABueYs7Gf/qRZ2qUuCqNXJ3yly4ovCRfYTKLOlhLN1xPWn9sBFKXAd2l8
qhBHHLO+k+3gnjONLjyWpHBkq94xrzkMHAjDftuw47LUerzU4t/vmAYBligohZSy
7XxA1Dz4pdVI31MyeJW2yh6roVzW/ow3bQl3rLCpG/Jz64nUZlEbnv0QGT8bGojP
crblVhyE+9A3+iZ9uXRks+9GgnTPfRr42y+7nwnFhS//l18VTwaoWAgirngg+C7P
E86KJl0wXBe5De2iZgWL4EjovNPwh13Q12JYrbHoBAvJHMZpvoU8ea7IeS5NMIdE
6R9az7wu3HncPDX8/h3jKSaB2h2bcuM9wNm0711Hs01dW57YWIarEz5kxfItfQW2
JSRHikK2oj1SkxB0HGIfX4Um/T7cXCjKifQWEkK9iL2+fQa4RK5TKGB/TH9E5sss
GCUNB6WVflWTbFmG+RrwfEGU9d4iAoIH6hhM/5pqNaZ2hHJhMsDCRIJpFp2bsUUR
CW3o06OGwo8K6PzVA+JzeIIsIG/ETLeDjuXqdvEQ/yVlMhqlkqrr5FkyS8x4MLVG
nut6KOActyO08xvmhMT9526/Y3eDnrJxtHHRvPl+iNRalsco6XQ8i/jldN5grvfx
qzTg4T7M/Bs=
=U08J
-----END PGP SIGNATURE-----

B
B
Bruno Victal wrote on 7 Jan 2023 21:07
(name . Christopher Baines)(address . mail@cbaines.net)
b9513572-b409-e6c4-cec5-ee1c7d5f2f04@makinata.eu
Hi

On 2023-01-07 17:21, Christopher Baines wrote:
Toggle quote (33 lines)
>
> mirai@makinata.eu writes:
>
>> From: Bruno Victal <mirai@makinata.eu>
>>
>> * gnu/services/web.scm (<nginx-server-configuration>): Add
>> ssl-stapling? and ssl-stapling-verify?.
>> * doc/guix.texi (NGINX): Document this.
>> ---
>> doc/guix.texi | 7 +++++
>> gnu/services/web.scm | 69 +++++++++++++++++++++++++-------------------
>> 2 files changed, 46 insertions(+), 30 deletions(-)
>
> Hi Bruno,
>
> Thanks for the patch, and sorry it's taken so long to reply.
>
>> @@ -647,6 +654,8 @@ (define-syntax-rule (and/l x tail ...)
>> " server_name " (config-domain-strings server-name) ";\n"
>> (and/l ssl-certificate " ssl_certificate " <> ";\n")
>> (and/l ssl-certificate-key " ssl_certificate_key " <> ";\n")
>> + " ssl_stapling " (if ssl-stapling? "on" "off") ";\n"
>> + " ssl_stapling_verify " (if ssl-stapling-verify? "on" "off") ";\n"
>> (if (not (equal? "" root))
>> (list " root " root ";\n")
>> "")
>>
>> base-commit: 68925b5ee7e0d96b0c84ae98a633eea5097bf511
>
> Generally this looks good to me. There's some unnecessary indentation
> changes that should probably go in another commit if they're made, but I
> did spot something in the above diff.

I was afraid that doing it in a separate commit would have
made it less clearer as it would have looked like a trivial cosmetic
change without any purpose.

Toggle quote (8 lines)
>
> I'm no expert in NGinx configs, but I do wonder if this change will
> break using nginx if it's built without the ngx_http_ssl_module? With
> the other module specific configuration (e.g. ssl_certificate), it's
> possible to specify a value in the <nginx-server-configuration> that
> means the line won't be included in the configuration. I think it would
> be good to continue that here.

I haven't tested this with a nginx that is built without ngx_http_ssl_module,
it would be a rather esoteric nginx build as TLS support presence is a
common expectation of web servers.
Toggle quote (7 lines)
> I'm not sure how to enable not including these config lines. Maybe a
> symbol value like 'noval could be used (this should also be the default,
> rather than #f), or maybe 'on and 'off could be used as the values with
> #f meaning the line isn't included.
>
> Does that make sense?

I'm not a fan of this approach as there's define-configuration
and define-maybe value types that should be used here rather than
making up a custom value, though I'm afraid reworking nginx-configuration
and writing the serialize- procedures to use the gnu/services/configuration.scm
facilities is a much bigger effort than what's done in this patch.

Before such effort is to be considered, a plan to solve [1] is required as I don't
think define-configuration is enough to represent the structure of nginx.conf
(nested locations, if branches, configuration for custom modules, etc.)



Cheers,
Bruno
M
M
Maxim Cournoyer wrote on 21 Mar 2023 14:20
Re: bug#59621: [PATCH] services: nginx: Add support for ssl-stapling in server blocks.
(name . Bruno Victal)(address . mirai@makinata.eu)
87y1nqqlzi.fsf_-_@gmail.com
Hi Bruno, Chris,

Bruno Victal <mirai@makinata.eu> writes:

Toggle quote (52 lines)
> Hi
>
> On 2023-01-07 17:21, Christopher Baines wrote:
>>
>> mirai@makinata.eu writes:
>>
>>> From: Bruno Victal <mirai@makinata.eu>
>>>
>>> * gnu/services/web.scm (<nginx-server-configuration>): Add
>>> ssl-stapling? and ssl-stapling-verify?.
>>> * doc/guix.texi (NGINX): Document this.
>>> ---
>>> doc/guix.texi | 7 +++++
>>> gnu/services/web.scm | 69 +++++++++++++++++++++++++-------------------
>>> 2 files changed, 46 insertions(+), 30 deletions(-)
>>
>> Hi Bruno,
>>
>> Thanks for the patch, and sorry it's taken so long to reply.
>>
>>> @@ -647,6 +654,8 @@ (define-syntax-rule (and/l x tail ...)
>>> " server_name " (config-domain-strings server-name) ";\n"
>>> (and/l ssl-certificate " ssl_certificate " <> ";\n")
>>> (and/l ssl-certificate-key " ssl_certificate_key " <> ";\n")
>>> + " ssl_stapling " (if ssl-stapling? "on" "off") ";\n"
>>> + " ssl_stapling_verify " (if ssl-stapling-verify? "on" "off") ";\n"
>>> (if (not (equal? "" root))
>>> (list " root " root ";\n")
>>> "")
>>>
>>> base-commit: 68925b5ee7e0d96b0c84ae98a633eea5097bf511
>>
>> Generally this looks good to me. There's some unnecessary indentation
>> changes that should probably go in another commit if they're made, but I
>> did spot something in the above diff.
>
> I was afraid that doing it in a separate commit would have
> made it less clearer as it would have looked like a trivial cosmetic
> change without any purpose.
>
>>
>> I'm no expert in NGinx configs, but I do wonder if this change will
>> break using nginx if it's built without the ngx_http_ssl_module? With
>> the other module specific configuration (e.g. ssl_certificate), it's
>> possible to specify a value in the <nginx-server-configuration> that
>> means the line won't be included in the configuration. I think it would
>> be good to continue that here.
>
> I haven't tested this with a nginx that is built without ngx_http_ssl_module,
> it would be a rather esoteric nginx build as TLS support presence is a
> common expectation of web servers.

The only nginx package in Guix has TLS support; I wouldn't expect people
will go out of the way to define TLS-less variants just to run a local
HTTP-only web server; perhaps it's OK to not give to much importance to
that for now?

--
Thanks,
Maxim
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 59621@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 59621
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch