Batching to avoid acme recursive domain safeguard#8
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces domain batching functionality to handle ACME certificate issuance more safely, particularly for domains with many labels that might trigger rate limits or safety heuristics. It includes a new domain_batching module, updates to the CLI for custom keystore paths, and enhancements to the API and client to support batched issuance. However, the core batching logic in domain_batching.py contains a critical error where it splits single FQDNs into invalid label fragments rather than grouping full domain strings. Additionally, the CLI lacks a flag to actually trigger this new batching behavior, and there is a minor non-idiomatic type check in the certificate manager.
| labels = [label for label in normalized.split(".") if label] | ||
| if len(labels) <= 3: | ||
| compact_batch.append(normalized) | ||
| continue | ||
|
|
||
| for i in range(0, len(labels), 3): | ||
| singleton_batches.append([".".join(labels[i : i + 3])]) |
There was a problem hiding this comment.
The logic for handling domains with more than 3 labels is fundamentally incorrect for ACME certificate issuance.
- Invalid Hostnames: It splits a single FQDN into label fragments (e.g.,
a.b.c.d.e.fbecomesa.b.candd.e.f). ACME requires the full FQDN for validation; you cannot obtain a certificate for a fragment of a domain to cover the original FQDN. - Loss of Wildcards: The
_normalize_domainfunction removes*.prefixes. If a user requests a wildcard certificate, this logic will attempt to issue a standard certificate for the base domain instead. - Unused Logic: The function ignores the
blocked_labelsparameter and thewould_triggerlogic defined earlier in the file, blindly splitting any domain with more than 3 labels.
If the goal is to isolate potentially 'unsafe' domains to avoid triggering the safeguard for the entire order, you should group the full domain strings into separate batches (singleton batches) rather than splitting the labels of a single domain.
| labels = [label for label in normalized.split(".") if label] | |
| if len(labels) <= 3: | |
| compact_batch.append(normalized) | |
| continue | |
| for i in range(0, len(labels), 3): | |
| singleton_batches.append([".".join(labels[i : i + 3])]) | |
| labels = _split_labels(domain) | |
| # Preserve the original domain (stripped/lowered) for issuance to keep wildcards | |
| issuance_domain = domain.strip().lower().rstrip(".") | |
| if len(labels) <= 3: | |
| compact_batch.append(issuance_domain) | |
| continue | |
| singleton_batches.append([issuance_domain]) |
| renew_threshold_days: Optional[int] = None, | ||
| batch_generator: Optional[Callable[[List[str]], List[List[str]]]] = None, | ||
| ) -> CertificateResponse: | ||
| if type(hosts) == str: |
There was a problem hiding this comment.
It is recommended to use isinstance(hosts, str) instead of type(hosts) == str. This is more idiomatic in Python and correctly handles potential subclasses of str.
| if type(hosts) == str: | |
| if isinstance(hosts, str): |
| obtain_parser.add_argument( | ||
| "--path", | ||
| default="/etc/ssl", | ||
| help="Directory where key/cert files will be written.", | ||
| ) |
There was a problem hiding this comment.
The CLI obtain command is missing a flag to enable the new batching functionality. While the API and CertManagerClient were updated to support batch_domains, the CLI still performs a single issuance for all domains because it calls AcmeCertIssuer directly instead of using the AcmeCertManager wrapper. To ensure consistency and make the feature accessible to CLI users, consider adding a --batch argument and updating the implementation to use AcmeCertManager.issue_certificate_in_batches.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8 +/- ##
==========================================
+ Coverage 52.29% 55.24% +2.95%
==========================================
Files 41 42 +1
Lines 2289 2355 +66
==========================================
+ Hits 1197 1301 +104
+ Misses 1092 1054 -38 ☔ View full report in Codecov by Sentry. |
mesudip
force-pushed
the
feat/recursive-batching-solution
branch
from
d669657 to
e3c97b9
Compare
1 participant
Acme has added a resurcive domain detection in this PR
This pr adds batching support to avoid triggering such detection