Final changes required for IP identifier support #79
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This allows us to skip revalidation of UTF-8 identifier strings and reallocation of the order object. See also 511be2a.
There was a problem hiding this comment.
Pull Request Overview
This PR implements full RFC 8738 compliance for ACME IP identifier support, enabling certificate requests using IPv4 and IPv6 addresses as identifiers instead of just domain names.
Key Changes:
- Refactored identifier handling to use
&'static strinstead ofngx_str_tfor improved type safety and canonical representation - Added IP address parsing and canonicalization logic to ensure RFC-compliant formatting
- Added comprehensive test coverage for both IPv4 and IPv6 identifier validation
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| t/acme_ipv6_identifier.t | New test suite validating IPv6 address identifier support via HTTP-01 and TLS-ALPN-01 challenges |
| t/acme_ipv4_identifier.t | New test suite validating IPv4 address identifier support via HTTP-01 and TLS-ALPN-01 challenges |
| src/lib.rs | Simplified certificate order processing by removing intermediate string conversion |
| src/conf/order.rs | Major refactoring to handle IP identifiers with canonical formatting and improved validation |
| src/conf/issuer.rs | Updated type signatures to use &'static str for certificate orders |
| src/conf/identifier.rs | Removed unused as_str() method as part of refactoring |
| src/conf.rs | Updated configuration parsing to convert values to static strings early and made conf_value_to_str public |
| README.md | Updated documentation to note RFC 8738 support and provide examples of IP identifier usage |
Comments suppressed due to low confidence (1)
t/acme_ipv6_identifier.t:1
- Port 8080 should use an IPv6 address
[::1]instead of127.0.0.1to match the IPv6 test context. This hardcoded IPv4 address is inconsistent with the IPv6-focused test and server configuration.
#!/usr/bin/perl
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bavshin-f5
force-pushed
the
8738-ip-identifiers
branch
from
e25db35 to
7ef429f
Compare
As stated in RFC8738 § 3: : The value field of the identifier MUST contain the textual form of the : address as defined in Section 2.1 of [RFC1123] for IPv4 and in Section : 4 of [RFC5952] for IPv6. This is the last piece required before declaring RFC8738 support. Fixes nginx#5.
bavshin-f5
force-pushed
the
8738-ip-identifiers
branch
from
3c2cfd8 to
628fa62
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This gets us fully compliant with the RFC, so we can mark it as supported.