letsencrypt: Update certbot and dns plugins #3902
Conversation
|
Warning Rate limit exceeded@GoetzGoerisch has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe pull request introduces updates to the Let's Encrypt add-on, focusing on dependency version upgrades and standardizing DNS provider argument handling. Changes include the addition of a new version entry in the changelog, updates to the build configuration for various dependencies, modifications to a Bash script that manages Let's Encrypt certificates by replacing hardcoded values with dynamic references, an update to the configuration file version, and a simplification of the INWX DNS service URL. Additionally, certain DNS plugins have been disabled due to unresolved issues, as noted in the documentation. Changes
Sequence DiagramsequenceDiagram
participant Script as Let's Encrypt Run Script
participant DNS as DNS Provider
participant Certbot as Certbot
Script->>Script: Prepare DNS Provider Arguments
Script->>Certbot: Pass Dynamic DNS Provider Configuration
Certbot->>DNS: Request DNS Challenge
DNS-->>Certbot: Verify DNS Challenge
Certbot->>Certbot: Issue/Renew Certificate
The sequence diagram illustrates the updated process of dynamically constructing DNS provider arguments, which allows for more flexible and maintainable certificate management across different DNS providers. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
letsencrypt/rootfs/etc/services.d/lets-encrypt/run (1)
Line range hint
332-347: Consider adding error handling for certificate chain preference.The script uses a hardcoded preferred chain "ISRG Root X1". Consider making this configurable for environments that might require different root certificates.
Add a configuration option for the preferred certificate chain:
+PREFERRED_CHAIN=$(bashio::config 'preferred_chain' "ISRG Root X1") if [ "$CHALLENGE" == "dns" ]; then certbot certonly --non-interactive --keep-until-expiring --expand \ --email "$EMAIL" --agree-tos \ "${KEY_ARGUMENTS[@]}" \ --cert-name "${DOMAIN_ARR[1]}" "${DOMAIN_ARR[@]}" \ --config-dir "$CERT_DIR" --work-dir "$WORK_DIR" \ --preferred-challenges "$CHALLENGE" "${PROVIDER_ARGUMENTS[@]}" \ - --preferred-chain "ISRG Root X1" \ + --preferred-chain "${PREFERRED_CHAIN}" \ "${EAB_ARGUMENTS[@]}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
letsencrypt/CHANGELOG.md(1 hunks)letsencrypt/build.yaml(1 hunks)letsencrypt/rootfs/etc/services.d/lets-encrypt/run(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- letsencrypt/CHANGELOG.md
🧰 Additional context used
📓 Path-based instructions (1)
letsencrypt/build.yaml (6)
Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern */**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern */**(html|markdown|md): do not comment on HTML used for icons
Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
🔇 Additional comments (4)
letsencrypt/build.yaml (2)
13-13: Verify cryptography package compatibility.The cryptography package update to 44.0.0 is substantial. Ensure it's compatible with the Alpine 3.20 base image and Python 3.12.
12-14: Verify compatibility with major version updates.The significant version jumps (especially Cloudflare 4.0.0 and Certbot 3.1.0) might introduce breaking changes. Please ensure:
- The new Cloudflare plugin version is compatible with your DNS configuration
- The Certbot upgrade doesn't require additional configuration changes
letsencrypt/rootfs/etc/services.d/lets-encrypt/run (2)
106-106: LGTM! Standardized DNS provider argument handling.The changes consistently apply the same pattern for DNS provider arguments across all providers, improving maintainability.
Also applies to: 155-155, 203-203, 224-224, 245-245
Line range hint
208-214: Verify Azure credentials file permissions.The script sets 600 permissions for Azure credentials, which is good. However, ensure the credentials file is properly cleaned up after use.
Consider adding a cleanup trap:
+trap 'rm -f /data/azure_creds' EXIT if [ -f "/share/${AZURE_CREDS}" ]; then cp -f "/share/${AZURE_CREDS}" "/data/azure_creds" chmod 600 "/data/azure_creds" else bashio::log.info "Azure credentials file doesn't exist in folder share." fi
GoetzGoerisch
force-pushed
the
fix/letsencrypt
branch
from
02e0528 to
b80295e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
letsencrypt/rootfs/etc/cont-init.d/file-structure.sh (1)
Line range hint
1-115: Ensure secure handling of sensitive credentials.The script correctly:
- Sets restrictive permissions (600) on the credential files
- Uses environment variables for sensitive data
- Properly cleans up deprecated options
However, consider adding these security enhancements:
- Add input validation for the configuration values to prevent injection attacks
- Add logging for security-relevant events (file creation, permission changes)
- Consider using a secure secrets management solution instead of storing credentials in files
Example implementation for input validation:
+ # Validate input to prevent injection attacks + validate_input() { + local input=$1 + if [[ "$input" =~ [^a-zA-Z0-9_\-\.@] ]]; then + bashio::exit.nok "Invalid characters in input" + fi + } + # Setup Let's encrypt config echo -e "dns_desec_token = $(bashio::config 'dns.desec_token')\n" \ + validate_input "$(bashio::config 'dns.desec_token')"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
letsencrypt/rootfs/etc/cont-init.d/file-structure.sh(1 hunks)
🔇 Additional comments (1)
letsencrypt/rootfs/etc/cont-init.d/file-structure.sh (1)
70-70: Verify the INWX API endpoint change.The INWX API endpoint has been simplified from
https://api.domrobot.com/xmlrpc/tohttps://api.domrobot.com. This change appears to be related to the certbot-dns-inwx plugin update to version 3.0.1.Let's verify this change is aligned with the certbot-dns-inwx plugin requirements:
✅ Verification successful
The INWX API endpoint change is correct
The change from
https://api.domrobot.com/xmlrpc/tohttps://api.domrobot.comis verified to be correct, as it aligns with:
- The official INWX Python client (inwx-domrobot) which defines this as the production API endpoint
- The certbot-dns-inwx plugin's latest documentation and example configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the INWX API endpoint change in certbot-dns-inwx 3.0.1 # Check the certbot-dns-inwx package documentation or source code pip install certbot-dns-inwx==3.0.1 python3 -c " import pkg_resources import re try: dist = pkg_resources.get_distribution('certbot-dns-inwx') meta = dist.get_metadata('METADATA') print(re.search(r'Home-page: (.*)', meta).group(1)) # Try to find any mention of the API endpoint import certbot_dns_inwx import inspect source = inspect.getsource(certbot_dns_inwx) endpoints = re.findall(r'https://api\.domrobot\.com[^\s\'\"]*', source) print('\nFound endpoints:') for endpoint in endpoints: print(endpoint) except Exception as e: print(f'Error: {e}') "Length of output: 5352
Script:
#!/bin/bash # Check inwx-domrobot package and certbot-dns-inwx documentation python3 -c " import pkg_resources import re try: # Check inwx-domrobot package dist = pkg_resources.get_distribution('inwx-domrobot') meta = dist.get_metadata('METADATA') print('INWX Domrobot Package Info:') print(meta) print('\n---\n') # Check certbot-dns-inwx documentation dist = pkg_resources.get_distribution('certbot-dns-inwx') for name in dist.resource_listdir(''): if name.lower() in ['readme.md', 'readme.rst', 'readme.txt']: content = dist.get_resource_string('', name).decode('utf-8') print(f'Content of {name}:') print(content) except Exception as e: print(f'Error: {e}') "Length of output: 3993
Script:
#!/bin/bash # Check both repositories for API endpoint information echo "Checking certbot-dns-inwx repository..." gh api repos/oGGy990/certbot-dns-inwx/releases/latest --jq '.body' || true gh api repos/oGGy990/certbot-dns-inwx/contents/README.md | jq -r '.content' | base64 -d || true echo -e "\nChecking inwx/python-client repository..." gh api repos/inwx/python-client/releases/latest --jq '.body' || true gh api repos/inwx/python-client/contents/README.md | jq -r '.content' | base64 -d || true # Also check the actual configuration example in the certbot-dns-inwx package python3 -c " import pkg_resources import glob import os try: dist = pkg_resources.get_distribution('certbot-dns-inwx') for name in dist.resource_listdir(''): if 'example' in name.lower() or 'config' in name.lower(): content = dist.get_resource_string('', name).decode('utf-8') print(f'\nContent of {name}:') print(content) except Exception as e: print(f'Error: {e}') "Length of output: 1770
Script:
#!/bin/bash # Clone repositories and search for API endpoint information echo "Checking certbot-dns-inwx repository..." git clone https://github.com/oGGy990/certbot-dns-inwx.git cd certbot-dns-inwx echo "=== README.md ===" cat README.md || true echo -e "\n=== Searching for API endpoint references ===" rg -i "api.*domrobot" || true rg -i "inwx.*url" || true cd .. echo -e "\nChecking python-client repository..." git clone https://github.com/inwx/python-client.git cd python-client echo "=== README.md ===" cat README.md || true echo -e "\n=== Searching for API endpoint references ===" rg -i "api.*url" || true rg -i "domrobot.*url" || trueLength of output: 8920
GoetzGoerisch
force-pushed
the
fix/letsencrypt
branch
from
f9c9cb0 to
c48cb3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
letsencrypt/Dockerfile (1)
25-25: LGTM! Consider documenting the reason for plugin removals.The removal of mijn-host and websupport plugins is consistent with the PR objectives. However, it would be helpful to add comments explaining why these plugins were disabled.
Add comments above the commented-out lines to explain the reason for removal:
+ # Disabled due to unresolved issues # CERTBOT_DNS_MIJN_HOST_VERSION \ + # Disabled due to unresolved issues # certbot-dns-mijn-host==${CERTBOT_DNS_MIJN_HOST_VERSION} \ + # Disabled due to unresolved issues # certbot-dns-websupport==${CERTBOT_DNS_WEBSUPPORT_VERSION} \Also applies to: 77-77, 96-96
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
letsencrypt/CHANGELOG.md(1 hunks)letsencrypt/Dockerfile(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- letsencrypt/CHANGELOG.md
🧰 Additional context used
📓 Path-based instructions (1)
letsencrypt/Dockerfile (6)
Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern */**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern */**(html|markdown|md): do not comment on HTML used for icons
Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
GoetzGoerisch
force-pushed
the
fix/letsencrypt
branch
from
6e04146 to
355a3c0
Compare
|
Heads up that the Porkbun provider is broken, and needs this update to work again. This is because Porkbun recently changed the URL for their API: This can be fixed by updating the addon to use at least the 0.9.0 version of certbot-dns-porkbun The newest 0.9.1 release as proposed is the best bet. |
GoetzGoerisch
force-pushed
the
fix/letsencrypt
branch
2 times, most recently
from
122d419 to
1e5f9b7
Compare
There was a problem hiding this comment.
Oh hi there @GoetzGoerisch 👋
There is a merge conflict, could you please take a look?
Thanks 👍
../Frenck
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
GoetzGoerisch
force-pushed
the
fix/letsencrypt
branch
from
1e5f9b7 to
32f926f
Compare
GoetzGoerisch
force-pushed
the
fix/letsencrypt
branch
from
32f926f to
9c98d1b
Compare
There was a problem hiding this comment.
Thanks a lot for the PR! It does generally LGTM.
Since this breaks at least two DNS validation providers let's add the breaking_versions configuration to config.yaml (see https://developers.home-assistant.io/docs/add-ons/configuration). This will prevent auto-update so folks with those DNS validation providers can opt to not (yet) update to the latest version.
GoetzGoerisch
force-pushed
the
fix/letsencrypt
branch
from
4080b8e to
29536ac
Compare
|
@coderabbitai review |
|
Thank you @angers for the review and the suggestion with the breaking versions,
Fixed with e3ccb12 |
GoetzGoerisch
force-pushed
the
fix/letsencrypt
branch
from
29536ac to
20ea0bd
Compare
GoetzGoerisch
force-pushed
the
fix/letsencrypt
branch
from
20ea0bd to
a184677
Compare
GoetzGoerisch
force-pushed
the
fix/letsencrypt
branch
from
3c6b1bd to
723308b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
letsencrypt/CHANGELOG.md (1)
5-8: Verify the impact of disabled DNS providers.The breaking changes disabling certbot-dns-mijn-host and certbot-dns-websupport are well-documented with issue links. However, we should ensure users are properly notified.
Consider adding migration guidance for users of these disabled providers to help them transition to alternative solutions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
letsencrypt/CHANGELOG.md(1 hunks)letsencrypt/DOCS.md(6 hunks)letsencrypt/Dockerfile(2 hunks)letsencrypt/build.yaml(1 hunks)letsencrypt/config.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- letsencrypt/config.yaml
- letsencrypt/Dockerfile
- letsencrypt/DOCS.md
- letsencrypt/build.yaml
🧰 Additional context used
📓 Path-based instructions (6)
`*/**(html|markdown|md)`: - For instructional content in doc...
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
letsencrypt/CHANGELOG.md
`*/**(html|markdown|md)`: - Use bold to mark UI strings. - I...
*/**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
letsencrypt/CHANGELOG.md
`*/**(html|markdown|md)`: - Be brief in your replies and don...
*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
letsencrypt/CHANGELOG.md
`*/**(html|markdown|md)`: - Use sentence-style capitalizatio...
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
letsencrypt/CHANGELOG.md
`*/**(html|markdown|md)`: do not comment on HTML used for ic...
*/**(html|markdown|md): do not comment on HTML used for icons
letsencrypt/CHANGELOG.md
`*/**(html|markdown|md)`: Avoid flagging inline HTML for emb...
*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
letsencrypt/CHANGELOG.md
🔇 Additional comments (2)
letsencrypt/CHANGELOG.md (2)
3-4: LGTM!The version bump to 5.3.0 is appropriate given the breaking changes and significant updates.
9-10:❓ Verification inconclusive
Verify compatibility with Python 3.13 and Alpine 3.21.
The infrastructure updates to Python 3.13 and Alpine 3.21 are significant changes.
🏁 Script executed:
#!/bin/bash # Description: Check if Python 3.13 and Alpine 3.21 are available in the Alpine package repositories # Check Alpine version availability curl -s https://dl-cdn.alpinelinux.org/alpine/v3.21/main/x86_64/APKINDEX.tar.gz | tar -xz -O 2>/dev/null | grep -A1 "P:python3" # Check Python version in PyPI curl -s https://pypi.org/pypi/python/json | jq '.releases | keys[]' | grep "3.13"Length of output: 291
Action Required: Manually Verify Python 3.13 and Alpine 3.21 Compatibility
The current automated checks did not produce clear outputs for both components. Please verify manually that:
- Alpine 3.21: The APKINDEX extraction returns the expected package entries for Python (consider re-running the command using
grep -a -A1 "P:python3"to force text-mode processing).- Python 3.13: The expected release details are available—note that querying PyPI for Python may not be effective since CPython isn’t distributed as a PyPI package. It might be better to consult official sources (e.g., python.org) for confirmation.
GoetzGoerisch
force-pushed
the
fix/letsencrypt
branch
from
723308b to
8718ea9
Compare
* plugins are currently outdated and/or unmaintained * add breaking_versions to config.yaml Co-authored-by: Stefan Agner <[email protected]> Signed-off-by: Goetz Goerisch <[email protected]>
Signed-off-by: Goetz Goerisch <[email protected]>
GoetzGoerisch
force-pushed
the
fix/letsencrypt
branch
from
8718ea9 to
4b2bab2
Compare
|
@agners thank you for the fix I have incorporated it into the correct commit attributing you as the co-author. Could you please give the CI another go? |
Fixed
Breaking
Updated
Summary by CodeRabbit
Dependency Updates
Improvements
certbot-dns-mijn-hostandcertbot-dns-websupportplugins due to unresolved issues.