Skip to content

Conversation

@bavshin-f5
Copy link
Member

@bavshin-f5 bavshin-f5 commented Dec 3, 2025

The signature of ngx_http_validate_host has changed in NGINX 1.29.4, breaking the build of the module. Instead of continuing to abuse an internal API and wrapping it with version check, this commit reimplements the validation in a more suitable for the module way.

Notable differences from the previous behavior:

  • Minimal supported NGINX version is now aligned to the ngx crate: 1.22

  • The new logic mostly follows ngx_http_validate_host behavior as of NGINX 1.29.4, and may reject some previously accepted values.

  • Wildcard DNS identifiers are now permitted. Some ACME servers allow validating wildcard identifiers with challenges other than dns-01.

@bavshin-f5 bavshin-f5 requested a review from Copilot December 3, 2025 22:39
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR reimplements DNS identifier validation to maintain compatibility with NGINX 1.29.4, which changed the signature of ngx_http_validate_host. Instead of adapting with version checks, the validation is now implemented directly in the module.

Key changes:

  • Custom DNS identifier validation adhering to RFC 1123 with support for wildcards and underscore characters
  • Support for wildcard DNS identifiers (e.g., *.example.com)
  • Minimum supported NGINX version updated from 1.25.0 to 1.22.0

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/conf/order.rs Replaces ngx_http_validate_host call with custom validate_dns_identifier function implementing RFC 1123 validation with wildcard support
src/acme/solvers/tls_alpn.rs Removes ngx_http_validate_host dependency, adds ASCII validation for DNS identifiers in TLS-ALPN solver
t/acme_conf_certificate.t Adds test cases for invalid server name patterns
README.md Updates minimum NGINX version requirement from 1.25.0 to 1.22.0
.github/workflows/ci.yaml Updates CI to test against NGINX 1.22 as the minimum supported version

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@bavshin-f5 bavshin-f5 marked this pull request as ready for review December 3, 2025 22:56
@bavshin-f5 bavshin-f5 requested review from ensh63 and xeioex December 3, 2025 22:59
Copy link
Contributor

@xeioex xeioex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise looks good.

@bavshin-f5 bavshin-f5 requested a review from Copilot December 5, 2025 19:48
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

The signature of ngx_http_validate_host has changed in NGINX 1.29.4,
breaking the build of the module.  Instead of continuing to abuse an
internal API and wrapping it with version check, this commit
reimplements the validation in a more suitable for the module way.

Notable differences from the previous behavior:

 - Minimal supported NGINX version is now aligned to the ngx crate: 1.22

 - The new logic mostly follows ngx_http_validate_host behavior as of
   NGINX 1.29.4, and may reject some previously accepted values.

 - Wildcard DNS identifiers are now permitted.  Some ACME servers allow
   validating wildcard identifiers with challenges other than dns-01.
@bavshin-f5 bavshin-f5 merged commit f7cff1a into nginx:main Dec 5, 2025
22 checks passed
@bavshin-f5 bavshin-f5 deleted the nginx-1.29.4-compat branch December 5, 2025 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants