Final changes required for IP identifier support

.github/workflows/ci.yaml

@@ -127,6 +127,7 @@ jobs:
       - uses: perl-actions/install-with-cpm@8b1a9840b26cc3885ae2889749a48629be2501b0 # v1.9
         with:
           install: |
+            IO::Socket::INET6
             IO::Socket::SSL
             TimeDate
 

.github/workflows/sanitizers.yaml

@@ -18,6 +18,7 @@ env:
     make openssl patch which
     perl-Digest-SHA
     perl-FindBin
+    perl-IO-Socket-INET6
     perl-IO-Socket-SSL
     perl-Test-Harness
     perl-Test-Simple

README.md

@@ -15,11 +15,13 @@ The module implements following specifications:
     - Only HTTP-01 challenge type is supported
 - [RFC8737] (ACME TLS Application-Layer Protocol Negotiation (ALPN) Challenge
   Extension)
+- [RFC8738] (ACME IP Identifier Validation Extension)
 - [draft-ietf-acme-profiles] (ACME Profiles Extension, version 00)
 
 [NGINX]: https://nginx.org/
 [RFC8555]: https://datatracker.ietf.org/doc/html/rfc8555
 [RFC8737]: https://datatracker.ietf.org/doc/html/rfc8737
+[RFC8738]: https://datatracker.ietf.org/doc/html/rfc8738
 [draft-ietf-acme-profiles]: https://datatracker.ietf.org/doc/draft-ietf-acme-profiles/
 
 ## Getting Started
@@ -162,7 +164,10 @@ acme_shared_zone zone=ngx_acme_shared:1M;
 
 server {
     listen 443 ssl;
-    server_name  .example.test;
+    server_name  .example.test
+                 192.0.2.1      # not supported by some ACME servers
+                 2001:db8::1    # not supported by some ACME servers
+                 ;
 
     acme_certificate example;
 

src/conf.rs

@@ -53,7 +53,7 @@ pub struct AcmeServerConfig {
     pub issuer: ngx_str_t,
     // Only one certificate order per server block is currently allowed. For multiple entries we
     // will have to implement certificate selection in the variable handler.
-    pub order: Option<CertificateOrder<ngx_str_t, Pool>>,
+    pub order: Option<CertificateOrder<&'static str, Pool>>,
 }
 
 pub static mut NGX_HTTP_ACME_COMMANDS: [ngx_command_t; 4] = [
@@ -308,7 +308,7 @@ extern "C" fn cmd_add_certificate(
         return c"\"issuer\" is missing".as_ptr().cast_mut();
     }
 
-    let mut order = CertificateOrder::<ngx_str_t, Pool>::new_in(cf.pool());
+    let mut order = CertificateOrder::<&'static str, Pool>::new_in(cf.pool());
 
     for value in &args[2..] {
         if let Some(key) = value.strip_prefix(b"key=") {
@@ -320,7 +320,17 @@ extern "C" fn cmd_add_certificate(
             continue;
         }
 
-        if let Err(err) = order.try_add_identifier(value) {
+        if value.is_empty() {
+            return NGX_CONF_INVALID_VALUE;
+        }
+
+        // SAFETY: the value is not empty, well aligned, and the conversion result is assigned to an
+        // object in the same pool.
+        let Ok(value) = (unsafe { conf_value_to_str(value) }) else {
+            return NGX_CONF_INVALID_VALUE;
+        };
+
+        if let Err(err) = order.try_add_identifier(cf, value) {
             return cf.error(args[0], &err);
         }
     }
@@ -779,7 +789,7 @@ fn conf_check_nargs(cmd: &ngx_command_t, nargs: ngx_uint_t) -> bool {
 /// all the configuration objects. But this process role is not capable of serving connections or
 /// running background tasks, and thus will not create additional borrows with potentially extended
 /// lifetime.
-unsafe fn conf_value_to_str(value: &ngx_str_t) -> Result<&'static str, core::str::Utf8Error> {
+pub unsafe fn conf_value_to_str(value: &ngx_str_t) -> Result<&'static str, core::str::Utf8Error> {
     if value.len == 0 {
         Ok("")
     } else {

src/conf/identifier.rs

@@ -39,21 +39,6 @@ impl<S> Identifier<S> {
         }
     }
 
-    /// Borrows the current identifier with a str reference as an underlying data.
-    pub fn as_str(&self) -> Result<Identifier<&str>, str::Utf8Error>
-    where
-        S: AsRef<[u8]>,
-    {
-        match self {
-            Identifier::Dns(value) => str::from_utf8(value.as_ref()).map(Identifier::Dns),
-            Identifier::Ip(value) => str::from_utf8(value.as_ref()).map(Identifier::Ip),
-            Identifier::Other { kind, value } => Ok(Identifier::Other {
-                kind: str::from_utf8(kind.as_ref())?,
-                value: str::from_utf8(value.as_ref())?,
-            }),
-        }
-    }
-
     pub fn value(&self) -> &S {
         match self {
             Identifier::Dns(value) => value,

src/conf/issuer.rs

@@ -60,7 +60,7 @@ pub struct Issuer {
     // Generated fields
     // ngx_ssl_t stores a pointer to itself in SSL_CTX ex_data.
     pub ssl: Box<NgxSsl, Pool>,
-    pub orders: RbTreeMap<CertificateOrder<ngx_str_t, Pool>, CertificateContext, Pool>,
+    pub orders: RbTreeMap<CertificateOrder<&'static str, Pool>, CertificateContext, Pool>,
     pub pkey: Option<PKey<Private>>,
     pub data: Option<&'static RwLock<IssuerContext>>,
 }
@@ -228,7 +228,7 @@ impl Issuer {
     pub fn add_certificate_order(
         &mut self,
         cf: &mut ngx_conf_t,
-        order: &CertificateOrder<ngx_str_t, Pool>,
+        order: &CertificateOrder<&'static str, Pool>,
     ) -> Result<(), Status> {
         if self.orders.get(order).is_none() {
             ngx_log_debug!(
@@ -412,7 +412,7 @@ impl StateDir {
     pub fn load_certificate(
         &self,
         cf: &mut ngx_conf_t,
-        order: &CertificateOrder<ngx_str_t, Pool>,
+        order: &CertificateOrder<&'static str, Pool>,
     ) -> Result<CertificateContextInner<Pool>, CachedCertificateError> {
         use openssl_foreign_types::ForeignType;
         #[cfg(ngx_ssl_cache)]

src/conf/order.rs

@@ -3,9 +3,8 @@
 // This source code is licensed under the Apache License, Version 2.0 license found in the
 // LICENSE file in the root directory of this source tree.
 
-use core::fmt::{self, Write};
+use core::fmt;
 use core::hash::{self, Hash, Hasher};
-use core::net::IpAddr;
 use core::str::Utf8Error;
 
 use nginx_sys::{ngx_conf_t, ngx_http_server_name_t, ngx_str_t};
@@ -16,6 +15,7 @@ use ngx::ngx_log_error;
 use siphasher::sip::SipHasher;
 use thiserror::Error;
 
+use crate::conf::ext::NgxConfExt;
 use crate::conf::identifier::Identifier;
 use crate::conf::pkey::PrivateKey;
 
@@ -60,20 +60,6 @@ where
         dns.or_else(|| self.identifiers.first())
             .map(Identifier::value)
     }
-
-    pub fn to_str_order<NewA>(&self, alloc: NewA) -> CertificateOrder<&str, NewA>
-    where
-        NewA: Allocator + Clone,
-        S: AsRef<[u8]>,
-    {
-        let mut identifiers = Vec::<Identifier<&str>, NewA>::new_in(alloc);
-        identifiers.extend(self.identifiers.iter().map(|x| x.as_str().unwrap()));
-
-        CertificateOrder {
-            identifiers,
-            key: self.key.clone(),
-        }
-    }
 }
 
 impl<S: Hash, A> Hash for CertificateOrder<S, A>
@@ -150,8 +136,6 @@ where
 pub enum IdentifierError {
     #[error("memory allocation failed")]
     Alloc(#[from] AllocError),
-    #[error("empty server name")]
-    Empty,
     #[error("invalid server name")]
     Invalid,
     #[error("invalid UTF-8 string")]
@@ -160,9 +144,9 @@ pub enum IdentifierError {
     Wildcard,
 }
 
-impl CertificateOrder<ngx_str_t, Pool> {
+impl CertificateOrder<&'static str, Pool> {
     #[inline]
-    fn push(&mut self, id: Identifier<ngx_str_t>) -> Result<(), AllocError> {
+    fn push(&mut self, id: Identifier<&'static str>) -> Result<(), AllocError> {
         self.identifiers.try_reserve(1).map_err(|_| AllocError)?;
         self.identifiers.push(id);
         Ok(())
@@ -190,29 +174,30 @@ impl CertificateOrder<ngx_str_t, Pool> {
                 continue;
             }
 
-            self.try_add_identifier(&server_name.name)?;
+            // SAFETY: the value is not empty, well aligned, and the conversion result is assigned
+            // to an object in the same pool.
+            let value = unsafe { super::conf_value_to_str(&server_name.name)? };
+
+            self.try_add_identifier(cf, value)?;
         }
 
         Ok(())
     }
 
-    pub fn try_add_identifier(&mut self, value: &ngx_str_t) -> Result<(), IdentifierError> {
-        if value.is_empty() {
-            return Err(IdentifierError::Empty);
-        }
-
-        if core::str::from_utf8(value.as_ref())?
-            .parse::<IpAddr>()
-            .is_ok()
-        {
-            return self.push(Identifier::Ip(*value)).map_err(Into::into);
+    pub fn try_add_identifier(
+        &mut self,
+        cf: &ngx_conf_t,
+        value: &'static str,
+    ) -> Result<(), IdentifierError> {
+        if let Some(addr) = parse_ip_identifier(cf, value)? {
+            return self.push(Identifier::Ip(addr)).map_err(Into::into);
         }
 
-        if value.as_bytes().contains(&b'*') {
+        if value.contains('*') {
             return Err(IdentifierError::Wildcard);
         }
 
-        let host = validate_host(self.identifiers.allocator(), *value).map_err(|st| {
+        let host = validate_host(cf, value).map_err(|st| {
             if st == Status::NGX_ERROR {
                 IdentifierError::Alloc(AllocError)
             } else {
@@ -226,18 +211,14 @@ impl CertificateOrder<ngx_str_t, Pool> {
          * See <https://nginx.org/en/docs/http/server_names.html>
          */
 
-        if let Some(host) = host.strip_prefix(b".") {
-            let mut www = NgxString::new_in(self.identifiers.allocator());
-            www.try_reserve_exact(host.len + 4)
+        if let Some(host) = host.strip_prefix(".") {
+            let mut www = Vec::new_in(self.identifiers.allocator().clone());
+            www.try_reserve_exact(host.len() + 4)
                 .map_err(|_| AllocError)?;
-            // write to a buffer of sufficient size will succeed
-            let _ = write!(&mut www, "www.{host}");
-
-            let parts = www.into_raw_parts();
-            let www = ngx_str_t {
-                data: parts.0,
-                len: parts.1,
-            };
+            www.extend_from_slice(b"www.");
+            www.extend_from_slice(host.as_bytes());
+            // The buffer is owned by ngx_pool_t and does not leak.
+            let www = core::str::from_utf8(www.leak())?;
 
             self.push(Identifier::Dns(www))?;
             self.push(Identifier::Dns(host))?;
@@ -273,11 +254,52 @@ where
     }
 }
 
-fn validate_host(pool: &Pool, mut host: ngx_str_t) -> Result<ngx_str_t, Status> {
-    let mut pool = pool.clone();
-    let rc = Status(unsafe { nginx_sys::ngx_http_validate_host(&mut host, pool.as_mut(), 1) });
+/// Attempts to parse the value as an IP address, returning `Some(...)` on success.
+///
+/// The address will be converted to a canonical textual form and reallocated on the
+/// configuration pool if necessary.
+fn parse_ip_identifier(
+    cf: &ngx_conf_t,
+    value: &'static str,
+) -> Result<Option<&'static str>, AllocError> {
+    const INET6_ADDRSTRLEN: usize = "ffff:ffff:ffff:ffff:ffff:ffff:255.255.255.255".len();
+
+    let Ok(addr) = value.parse::<core::net::IpAddr>() else {
+        return Ok(None);
+    };
+
+    let mut buf = [0u8; INET6_ADDRSTRLEN];
+    let mut cur = std::io::Cursor::new(&mut buf[..]);
+    // Formatting IP address to a sufficiently large buffer should always succeed
+    let _ = std::io::Write::write_fmt(&mut cur, format_args!("{addr}"));
+    let len = cur.position() as usize;
+    let buf = &buf[..len];
+
+    if buf == value.as_bytes() {
+        return Ok(Some(value));
+    }
+
+    let mut out = Vec::new_in(cf.pool());
+    out.try_reserve_exact(buf.len()).map_err(|_| AllocError)?;
+    out.extend_from_slice(buf);
+    // SAFETY: formatted IpAddr is always a valid ASCII string.
+    // The buffer is owned by the ngx_pool_t and does not leak.
+    let out = unsafe { core::str::from_utf8_unchecked(out.leak()) };
+
+    Ok(Some(out))
+}
+
+/// Checks if the value is a valid domain name and returns a canonical (lowercase) form,
+/// reallocated on the configuration pool if necessary.
+fn validate_host(cf: &ngx_conf_t, host: &'static str) -> Result<&'static str, Status> {
+    let mut host = ngx_str_t {
+        data: host.as_ptr().cast_mut(),
+        len: host.len(),
+    };
+    let rc = Status(unsafe { nginx_sys::ngx_http_validate_host(&mut host, cf.pool, 0) });
     if rc != Status::NGX_OK {
         return Err(rc);
     }
-    Ok(host)
+
+    unsafe { super::conf_value_to_str(&host) }.map_err(|_| Status::NGX_ERROR)
 }

src/lib.rs

@@ -14,7 +14,6 @@ use nginx_sys::{
     ngx_conf_t, ngx_cycle_t, ngx_http_add_variable, ngx_http_module_t, ngx_int_t, ngx_module_t,
     ngx_uint_t, NGX_HTTP_MODULE, NGX_LOG_ERR, NGX_LOG_INFO, NGX_LOG_NOTICE, NGX_LOG_WARN,
 };
-use ngx::allocator::AllocError;
 use ngx::core::{Status, NGX_CONF_ERROR, NGX_CONF_OK};
 use ngx::http::{HttpModule, HttpModuleMainConf, HttpModuleServerConf, Merge};
 use ngx::log::ngx_cycle_log;
@@ -334,13 +333,7 @@ async fn ngx_http_acme_update_certificates_for_issuer(
             }
         }
 
-        let alloc = crate::util::OwnedPool::new(nginx_sys::NGX_DEFAULT_POOL_SIZE as _, log)
-            .map_err(|_| AllocError)?;
-
-        // Acme client wants &str and we already validated that the identifiers are valid UTF-8.
-        let str_order = order.to_str_order(&*alloc);
-
-        let cert_next = match client.new_certificate(&str_order).await {
+        let cert_next = match client.new_certificate(order).await {
             Ok(ref val) => {
                 let pkey = Zeroizing::new(val.pkey.private_key_to_pem_pkcs8()?);
                 let x509 = X509::from_pem(&val.chain)?;