letsencrypt: refactor addon #3943
Conversation
GoetzGoerisch
changed the title
GoetzGoerisch
force-pushed
the
fix/letsencrypt-directadmin
branch
4 times, most recently
from
456d624 to
f72b247
Compare
|
@agners would appreciate an initial review. I have one todo which I can hopefully finish during the next days. |
📝 WalkthroughWalkthroughThe changes update the versioning and documentation while modifying build and runtime configurations. A new version entry for 5.4.0 has been added to the changelog, highlighting enhancements such as new arguments for certbot and updates to DNS provider support. Documentation clarifies HTTP/DNS challenge details and introduces new configuration options. The Dockerfile and build configuration remove several older version arguments and add new ones for ACME and updated DNS plugins. Additionally, new conditional logic for various DNS providers has been introduced in the runtime service script. Changes
Sequence Diagram(s)sequenceDiagram
participant S as Run Script
participant C as Configuration
participant D as DNS Provider (dns-desec)
S->>C: Check CHALLENGE and DNS_PROVIDER
alt If CHALLENGE=="dns" and DNS_PROVIDER=="dns-desec"
S->>C: Retrieve dns.desec_token
S->>D: Send authenticator type, credentials, and propagation seconds
D-->>S: Acknowledge and apply settings
else
S->>S: Process alternative provider logic
end
✨ Finishing Touches
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: 2
🧹 Nitpick comments (4)
letsencrypt/CHANGELOG.md (1)
3-7: Confirm version bump necessity.
The new version entry 5.3.4 introduces documentation improvements and configuration refactoring. Verify that this version bump aligns with the add-on’s release strategy—especially in light of previous feedback noting that store metadata updates might not require an explicit version change.letsencrypt/DOCS.md (3)
95-184: Validate updated DNS provider configuration schema.
The YAML configuration block now includes many new fields for various DNS providers. Verify that:
- All key names and placeholder values (empty strings) are correct and remain non-sensitive.
- The fields follow the expected alphabetical and naming conventions to support the refactored configuration.
If any adjustments are needed to align the schema with deployment practices, consider updating the documentation accordingly.
385-392: Revise Cloudflare authentication description.
The Cloudflare section currently reads:-Previously, Cloudflare’s “Global API Key” was used for authentication, however this key can access the entire Cloudflare API for all domains in your account, meaning it could cause a lot of damage if leaked. +Previously, Cloudflare’s “Global API Key” was used for authentication. However, this key can access the entire Cloudflare API for all domains in your account, meaning it could cause a lot of damage if leaked.Splitting the sentence improves grammatical accuracy and readability.
🧰 Tools
🪛 LanguageTool
[typographical] ~387-~387: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...bal API Key” was used for authentication, however this key can access the entire Cloudfla...(HOWEVER_SENTENCE)
1428-1493: General documentation review and security note.
The sections for Supported DNS providers and Support are well organized. As a final note:
• Review minor grammatical and stylistic points (e.g. consider replacing verbose phrases like “in order to” with “to” and ensure punctuation is consistent).
• Ensure that all sample API keys and token fields remain as empty placeholders to prevent any accidental exposure of sensitive information.
These adjustments will further enhance clarity and maintain operational security.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
letsencrypt/CHANGELOG.md(1 hunks)letsencrypt/DOCS.md(24 hunks)letsencrypt/Dockerfile(3 hunks)letsencrypt/README.md(0 hunks)letsencrypt/build.yaml(1 hunks)letsencrypt/config.yaml(4 hunks)letsencrypt/rootfs/etc/services.d/lets-encrypt/run(3 hunks)
💤 Files with no reviewable changes (1)
- letsencrypt/README.md
🧰 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.mdletsencrypt/build.yamlletsencrypt/DOCS.mdletsencrypt/Dockerfileletsencrypt/config.yaml
`*/**(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.mdletsencrypt/build.yamlletsencrypt/DOCS.mdletsencrypt/Dockerfileletsencrypt/config.yaml
`*/**(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.mdletsencrypt/build.yamlletsencrypt/DOCS.mdletsencrypt/Dockerfileletsencrypt/config.yaml
`*/**(html|markdown|md)`: - Use sentence-style capitalizatio...
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
letsencrypt/CHANGELOG.mdletsencrypt/build.yamlletsencrypt/DOCS.mdletsencrypt/Dockerfileletsencrypt/config.yaml
`*/**(html|markdown|md)`: do not comment on HTML used for ic...
*/**(html|markdown|md): do not comment on HTML used for icons
letsencrypt/CHANGELOG.mdletsencrypt/build.yamlletsencrypt/DOCS.mdletsencrypt/Dockerfileletsencrypt/config.yaml
`*/**(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.mdletsencrypt/build.yamlletsencrypt/DOCS.mdletsencrypt/Dockerfileletsencrypt/config.yaml
🪛 Gitleaks (8.21.2)
letsencrypt/DOCS.md
397-397: Detected a Cloudflare Global API Key, potentially compromising cloud application deployments and operational security.
(cloudflare-global-api-key)
406-406: Detected a Cloudflare Global API Key, potentially compromising cloud application deployments and operational security.
(cloudflare-global-api-key)
657-657: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
676-676: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
951-951: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1044-1044: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1093-1093: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1168-1168: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1169-1169: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1170-1170: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1221-1221: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1222-1222: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1293-1293: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1393-1396: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🪛 LanguageTool
letsencrypt/DOCS.md
[typographical] ~387-~387: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...bal API Key” was used for authentication, however this key can access the entire Cloudfla...
(HOWEVER_SENTENCE)
[uncategorized] ~507-~507: Possible missing article found.
Context: ...ord. Instructions on how to create such key can be found at <https://help.directadm...
(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~604-~604: Possible missing comma found.
Context: ...ummary>DreamHost Use of this plugin an API key from DreamHost with dns-* ...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~665-~665: Consider a shorter alternative to avoid wordiness.
Context: ...PI access must be requested and granted in order to use this module: <https://cp.easydns.co...
(IN_ORDER_TO_PREMIUM)
[misspelling] ~707-~707: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...est a certificate for. If you only have an Gandi LiveDNS API key, please refer t...
(EN_A_VS_AN)
[uncategorized] ~857-~857: Possible missing comma found.
Context: ...XXXXXXXXXX ``` To obtain the DNS API token follow the instructions here: <https:/...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~940-~940: Possible missing comma found.
Context: ...o DNS; no other permissions are needed. Newly keys will likely use API version '4.' *...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~1080-~1080: Possible missing comma found.
Context: ...settings/api/>. If you have a static IP-address restrict the access to your IP. I you a...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~1080-~1080: Consider using only one pronoun here.
Context: ...address restrict the access to your IP. I you are not sure, you probably don't have a...
(PRP_REPITION)
[style] ~1206-~1206: Consider a shorter alternative to avoid wordiness.
Context: ...
Porkbun
In order to use a domain with this challenge, API a...(IN_ORDER_TO_PREMIUM)
[style] ~1207-~1207: Consider a shorter alternative to avoid wordiness.
Context: ...ccess will need enabling on the domain. In order to do this go to domain management -> sele...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~1208-~1208: Possible missing comma found.
Context: ... enabling on the domain. In order to do this go to domain management -> select the d...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~1210-~1210: To elevate your writing, try using an alternative expression here.
Context: ...nd then create a new api key. The title does not matter and is not used by certbot, make note o...
(MATTERS_RELEVANT)
[uncategorized] ~1297-~1297: Possible missing comma found.
Context: ... statement to your domain's hosted zone ARN or use * for all. ```json { ...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
letsencrypt/DOCS.md
1297-1297: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
1297-1297: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
1297-1297: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
1297-1297: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
🔇 Additional comments (9)
letsencrypt/rootfs/etc/services.d/lets-encrypt/run (1)
234-237: LGTM! The dns-desec provider integration is well implemented.The implementation follows the established pattern for DNS providers, correctly handling required configuration and propagation seconds.
letsencrypt/Dockerfile (2)
5-40: LGTM! The ARG declarations are well organized.The developer note about alphabetical ordering is helpful for maintainability. The ARG declarations are properly sorted and consistently formatted.
98-99:✅ Verification successful
Verify the status of the disabled websupport package.
The websupport package is disabled with a comment about unresolved issues.
Let's check the status of the reported issues:
🏁 Script executed:
#!/bin/bash # Description: Check for open issues related to the websupport package. # Search for issues mentioning websupport gh search issues "websupport" --repo home-assistant/addons --state openLength of output: 185
Re: Certbot-dns-websupport Package Status – No Relevant Open Issues Detected
The GitHub issues check for “websupport” returned one open issue (ID 3927) concerning a Namecheap zone identifier error—which is unrelated to the certbot-dns-websupport integration. There are no active unresolved issues directly impacting the websupport package.
- File:
letsencrypt/Dockerfile(lines 98-99)
- The disabled line and accompanying comment appear to be based on outdated concerns.
- Recommendation: If your integration tests confirm that
certbot-dns-websupportworks correctly, consider updating or removing the outdated comment.letsencrypt/config.yaml (2)
2-2: LGTM! Version bump to 5.3.4.The version update aligns with the changes in DNS provider support.
44-174: LGTM! The DNS configuration schema is well organized.The developer note about alphabetical ordering is helpful for maintainability. The schema fields are properly sorted and consistently formatted. The provider list is comprehensive and well-structured.
letsencrypt/DOCS.md (4)
29-29: Clarify HTTP challenge limitation.
The HTTP challenge description now explicitly states that wildcard certificates (e.g. *.domain.tld) are not allowed. Confirm that this limitation remains consistent with Let’s Encrypt policy.
33-34: Confirm DNS challenge capability.
The DNS challenge section now notes that wildcard certificates can be requested. This change accurately reflects current capabilities, so no further adjustments are needed here.
39-40: Developer note on alphabetical ordering.
The embedded developer note instructs that new plugins be added alphabetically. Ensure that this convention is maintained consistently throughout the document.
238-246: ACME server override instructions clarity.
This section explains how to set a custom ACME server via theacme_serverfield. The instructions are clear; however, double-check that terms like “add-on” are used consistently and consider a brief review of capitalization for clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
letsencrypt/DOCS.md (1)
986-1001:⚠️ Potential issueLuaDNS provider identifier typo.
The LuaDNS example incorrectly sets the provider as dns-luadna. The correct identifier is dns-luadns. This duplicate comment reiterates a previous review remark.- provider: dns-luadna + provider: dns-luadns
🧹 Nitpick comments (5)
letsencrypt/rootfs/etc/services.d/lets-encrypt/run (3)
40-52: Handle missing Azure credentials file more strictly.Currently, the code logs an info message if the Azure credentials file is not found but does not halt execution. This may lead to silent misconfigurations if the file is essential. Consider failing early or using a fallback mechanism when the file is missing.
145-158: Improve missing Google credentials handling.Similar to Azure, the logic only logs a message when the file is missing. Consider stopping the process if credentials are critical, or provide a clear fallback to avoid partial misconfiguration.
254-265: Store AWS credentials securely.Credentials are exported as environment variables, which can present a security risk if logs or process inspection occurs. Consider storing these credentials in an external file with exclusive permissions (similar to other providers) to limit potential exposure.
letsencrypt/translations/en.yaml (1)
45-56: Descriptions look good but could be more explicit.The new fields—Dry Run, Issue test certificates, and Verbose Mode—are clearly labeled and described. Consider adding a note indicating that the test certificate is not trusted in production environments and explaining that a "dry run" does not produce valid certificates for usage.
letsencrypt/DOCS.md (1)
39-40: Developer note: Remove if not intended for end users.
A developer note (line 39) instructs to “add a new plugin alphabetically into all lists.” If this note is solely for internal development, consider removing it from the public documentation to avoid confusion for end users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
letsencrypt/CHANGELOG.md(1 hunks)letsencrypt/DOCS.md(24 hunks)letsencrypt/Dockerfile(3 hunks)letsencrypt/build.yaml(1 hunks)letsencrypt/config.yaml(4 hunks)letsencrypt/rootfs/etc/cont-init.d/file-structure.sh(1 hunks)letsencrypt/rootfs/etc/services.d/lets-encrypt/run(11 hunks)letsencrypt/translations/en.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- letsencrypt/Dockerfile
- letsencrypt/build.yaml
- letsencrypt/config.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.mdletsencrypt/DOCS.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.mdletsencrypt/DOCS.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.mdletsencrypt/DOCS.md
`*/**(html|markdown|md)`: - Use sentence-style capitalizatio...
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
letsencrypt/CHANGELOG.mdletsencrypt/DOCS.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.mdletsencrypt/DOCS.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.mdletsencrypt/DOCS.md
🪛 Gitleaks (8.21.2)
letsencrypt/DOCS.md
658-658: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
677-677: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
395-395: Detected a Cloudflare Global API Key, potentially compromising cloud application deployments and operational security.
(cloudflare-global-api-key)
406-406: Detected a Cloudflare Global API Key, potentially compromising cloud application deployments and operational security.
(cloudflare-global-api-key)
952-952: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1045-1045: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1094-1094: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1169-1169: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1170-1170: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1171-1171: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1222-1222: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1223-1223: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1294-1294: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1394-1397: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🪛 LanguageTool
letsencrypt/DOCS.md
[uncategorized] ~398-~398: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...l API Key” was used for authentication. However this key can access the entire Cloudfla...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[style] ~413-~413: Consider a shorter alternative to avoid wordiness.
Context: ...
ClouDNS
In order to use a domain with this challenge, you f...(IN_ORDER_TO_PREMIUM)
[uncategorized] ~508-~508: Possible missing article found.
Context: ...ord. Instructions on how to create such key can be found at <https://help.directadm...
(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~605-~605: Possible missing comma found.
Context: ...ummary>DreamHost Use of this plugin an API key from DreamHost with dns-* ...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~666-~666: Consider a shorter alternative to avoid wordiness.
Context: ...PI access must be requested and granted in order to use this module: <https://cp.easydns.co...
(IN_ORDER_TO_PREMIUM)
[misspelling] ~708-~708: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...est a certificate for. If you only have an Gandi LiveDNS API key, please refer t...
(EN_A_VS_AN)
[uncategorized] ~802-~802: Possible missing comma found.
Context: ...dynamically create and delete these TXT records as doing so would subsequently remove y...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~858-~858: Possible missing comma found.
Context: ...XXXXXXXXXX ``` To obtain the DNS API token follow the instructions here: <https:/...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~966-~966: The preposition ‘to’ seems more likely in this position.
Context: ... records - removeSubdomain - Required for complete cleanup Example configuration...
(AI_HYDRA_LEO_REPLACE_FOR_TO)
[grammar] ~1081-~1081: Consider using only one pronoun here.
Context: ...address restrict the access to your IP. I you are not sure, you probably don't have a...
(PRP_REPITION)
[style] ~1207-~1207: Consider a shorter alternative to avoid wordiness.
Context: ...
Porkbun
In order to use a domain with this challenge, API a...(IN_ORDER_TO_PREMIUM)
[style] ~1208-~1208: Consider a shorter alternative to avoid wordiness.
Context: ...ccess will need enabling on the domain. In order to do this go to domain management -> sele...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~1209-~1209: Possible missing comma found.
Context: ... enabling on the domain. In order to do this go to domain management -> select the d...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~1210-~1210: Possible missing comma found.
Context: ... toggle. Then go to the menu in the top right select API access and then create a new...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~1211-~1211: To elevate your writing, try using an alternative expression here.
Context: ...nd then create a new api key. The title does not matter and is not used by certbot, make note o...
(MATTERS_RELEVANT)
[uncategorized] ~1298-~1298: Possible missing comma found.
Context: ... statement to your domain's hosted zone ARN or use * for all. ```json { ...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
letsencrypt/DOCS.md
1298-1298: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
1298-1298: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
1298-1298: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
1298-1298: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build i386 letsencrypt add-on
- GitHub Check: Build armv7 letsencrypt add-on
- GitHub Check: Build armhf letsencrypt add-on
- GitHub Check: Build aarch64 letsencrypt add-on
🔇 Additional comments (23)
letsencrypt/rootfs/etc/services.d/lets-encrypt/run (15)
24-26: Ensure type safety for new booleans.Reading
DRY_RUN,TEST_CERT, andVERBOSEdirectly from configuration may cause unexpected behavior if they are not strictly boolean. You may wish to validate them or convert them to booleans upstream.
67-76: ClouDNS and deSEC provider blocks look consistent.The added checks and required config look aligned with the rest of the DNS logic. No issues found.
94-99: Dreamhost block is consistent.The configuration requirements and appended arguments match the general pattern used across providers.
110-116: easyDNS handling is properly aligned.No issues found. The config requirements and propagation-seconds usage appear sound.
159-164: Hurricane Electric DNS integration looks good.No concerns. The approach follows the established pattern for DNS providers.
175-181: INWX provider block is correct.All required fields are enforced, and arguments are structured consistently.
205-209: mijn.host block is consistent.Looks good. The provider credentials are properly required and appended.
210-215: Namecheap block is standard.No issues identified. Required credentials and approach align with other DNS providers.
216-222: Netcup block is consistent.The pattern of requiring tokens and adding arguments follows the rest of the script.
223-232: Njalla and noris blocks are properly added.No immediate concerns. Each block is logically consistent with existing providers.
244-248: Porkbun logic mirrors other providers well.No issues found in these lines.
266-269: Simply.com DNS logic is uniform.The approach for credentials and propagation seconds is consistent. No issues found.
272-279: TransIP block is correct.Minimum propagation limit is enforced, which aligns with known provider constraints. Looks good.
352-363: Additional configuration arguments for Certbot look clean.Dry-run, test-cert, and verbose flags are properly appended. No concerns about correctness or maintainability.
369-369: In-line additional arguments insertion is well-structured.Passing
ADDITIONAL_ARGShere ensures consistency between DNS and non-DNS challenges. No issues found.Also applies to: 379-379
letsencrypt/CHANGELOG.md (1)
3-9: Version bump note mirrors a previous discussion.A prior reviewer mentioned that minor documentation updates alone might not require a version bump if the store metadata is refreshed regularly. Since this change also includes new configuration options, the bump might still be justified.
letsencrypt/rootfs/etc/cont-init.d/file-structure.sh (4)
10-87: Parameter list update: Verify consistency with removal expectations.
The updated echo command includes many new configuration parameters (e.g.aws_access_key_id,dns_domainoffensive_api_token, etc.). However, note that parameters such asdns_eurodns_applicationId,dns_gehirn_api_token,dns_godaddy_secret, anddns_ionos_endpointare still present even though the enriched summary indicates these should be removed. Align these with the PR objectives and updated documentation.Likely an incorrect or invalid review comment.
89-101: ClouDNS configuration: Ensure single non‐empty auth option.
The conditional blocks for appending ClouDNS options (lines 91–101) correctly add a single configuration when the option exists. Confirm that the downstream certbot integration accepts only one non‐empty auth parameter to avoid errors.
105-110: TransIP RSA key preparation: Secure and correct.
The TransIP RSA key block (lines 105–110) usesopenssl rsawith proper file permissions. Confirm that the method of reading the API key and generating the RSA key meets your security guidelines.
112-127: Cleanup of removed add-on options: Logic is clear.
The cleanup section (lines 112–127) checks for deprecated options and callsbashio::addon.optionto remove them. This logic is concise and aligns with best practices for cleaning up obsolete configuration.letsencrypt/DOCS.md (3)
28-30: HTTP challenge: Confirm wildcard certificate limitation.
The revised bullet now states that wildcard certificates (e.g. *.domain.tld) are not allowed. Ensure that this restriction is consistent with the behavior of the HTTP challenge implementation.
31-35: DNS challenge: Confirm wildcard certificate support.
The DNS challenge description now indicates that wildcard certificates are allowed. Validate that this documentation change accurately reflects the plugin’s capability and that users understand the differences between the two challenges.
95-184: YAML configuration fields: Verify alphabetical order and accuracy.
The configuration examples (lines 95–184) have been updated with many new fields (e.g.aws_access_key_id,cloudflare_email, etc.). Confirm that the fields are listed in alphabetical order (as per the PR objective) and that each field exactly matches the parameter names used in the add-on’s configuration file.
GoetzGoerisch
force-pushed
the
fix/letsencrypt-directadmin
branch
from
5f5ab6d to
9990fc1
Compare
GoetzGoerisch
changed the title
GoetzGoerisch
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
letsencrypt/rootfs/etc/services.d/lets-encrypt/run (1)
89-93: 🛠️ Refactor suggestionRemove redundant arguments for domainoffensive provider.
The script already applies
--agree-tos -m ${EMAIL}arguments globally in later lines (e.g., lines 367-368). Remove them from this block for consistency.- PROVIDER_ARGUMENTS+=("--authenticator" "${DNS_PROVIDER}" "--${DNS_PROVIDER}-credentials" "/data/dnsapikey") + PROVIDER_ARGUMENTS+=("--authenticator" "${DNS_PROVIDER}" "--${DNS_PROVIDER}-credentials" "/data/dnsapikey")
🧹 Nitpick comments (2)
letsencrypt/DOCS.md (2)
395-395: Enhance security by clearly marking example API keys.While the API keys are placeholders, they should be more clearly marked as examples to prevent confusion. Consider using obvious placeholder text like
<YOUR-API-KEY>instead of realistic-looking values.Also applies to: 406-406, 952-952, 1045-1045, 1094-1094, 1169-1171, 1222-1223, 1294-1294
🧰 Tools
🪛 Gitleaks (8.21.2)
395-395: Detected a Cloudflare Global API Key, potentially compromising cloud application deployments and operational security.
(cloudflare-global-api-key)
398-398: Improve documentation readability with style enhancements.Consider these style improvements:
- Add missing commas after introductory phrases
- Fix article usage (e.g., "a" vs "an")
- Reduce wordiness by replacing "in order to" with "to"
- Use consistent formatting for menu paths and UI elements
Also applies to: 413-413, 508-508, 605-605, 666-666, 708-708, 802-802, 858-858, 966-966, 1081-1081, 1207-1211
🧰 Tools
🪛 LanguageTool
[uncategorized] ~398-~398: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...l API Key” was used for authentication. However this key can access the entire Cloudfla...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
letsencrypt/CHANGELOG.md(1 hunks)letsencrypt/DOCS.md(24 hunks)letsencrypt/Dockerfile(3 hunks)letsencrypt/build.yaml(1 hunks)letsencrypt/config.yaml(4 hunks)letsencrypt/rootfs/etc/cont-init.d/file-structure.sh(1 hunks)letsencrypt/rootfs/etc/services.d/lets-encrypt/run(11 hunks)letsencrypt/translations/en.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- letsencrypt/CHANGELOG.md
- letsencrypt/translations/en.yaml
- letsencrypt/build.yaml
- letsencrypt/config.yaml
- letsencrypt/Dockerfile
🧰 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/DOCS.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/DOCS.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/DOCS.md
`*/**(html|markdown|md)`: - Use sentence-style capitalizatio...
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
letsencrypt/DOCS.md
`*/**(html|markdown|md)`: do not comment on HTML used for ic...
*/**(html|markdown|md): do not comment on HTML used for icons
letsencrypt/DOCS.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/DOCS.md
🪛 Gitleaks (8.21.2)
letsencrypt/DOCS.md
658-658: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
677-677: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
395-395: Detected a Cloudflare Global API Key, potentially compromising cloud application deployments and operational security.
(cloudflare-global-api-key)
406-406: Detected a Cloudflare Global API Key, potentially compromising cloud application deployments and operational security.
(cloudflare-global-api-key)
952-952: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1045-1045: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1094-1094: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1169-1169: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1170-1170: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1171-1171: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1222-1222: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1223-1223: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1294-1294: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1394-1397: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🪛 LanguageTool
letsencrypt/DOCS.md
[uncategorized] ~398-~398: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...l API Key” was used for authentication. However this key can access the entire Cloudfla...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[style] ~413-~413: Consider a shorter alternative to avoid wordiness.
Context: ...
ClouDNS
In order to use a domain with this challenge, you f...(IN_ORDER_TO_PREMIUM)
[uncategorized] ~508-~508: Possible missing article found.
Context: ...ord. Instructions on how to create such key can be found at <https://help.directadm...
(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~605-~605: Possible missing comma found.
Context: ...ummary>DreamHost Use of this plugin an API key from DreamHost with dns-* ...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~666-~666: Consider a shorter alternative to avoid wordiness.
Context: ...PI access must be requested and granted in order to use this module: <https://cp.easydns.co...
(IN_ORDER_TO_PREMIUM)
[misspelling] ~708-~708: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...est a certificate for. If you only have an Gandi LiveDNS API key, please refer t...
(EN_A_VS_AN)
[uncategorized] ~802-~802: Possible missing comma found.
Context: ...dynamically create and delete these TXT records as doing so would subsequently remove y...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~858-~858: Possible missing comma found.
Context: ...XXXXXXXXXX ``` To obtain the DNS API token follow the instructions here: <https:/...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~966-~966: The preposition ‘to’ seems more likely in this position.
Context: ... records - removeSubdomain - Required for complete cleanup Example configuration...
(AI_HYDRA_LEO_REPLACE_FOR_TO)
[uncategorized] ~1081-~1081: Possible missing comma found.
Context: ...settings/api/>. If you have a static IP-address restrict the access to your IP. I you a...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~1081-~1081: Consider using only one pronoun here.
Context: ...address restrict the access to your IP. I you are not sure, you probably don't have a...
(PRP_REPITION)
[style] ~1207-~1207: Consider a shorter alternative to avoid wordiness.
Context: ...
Porkbun
In order to use a domain with this challenge, API a...(IN_ORDER_TO_PREMIUM)
[style] ~1208-~1208: Consider a shorter alternative to avoid wordiness.
Context: ...ccess will need enabling on the domain. In order to do this go to domain management -> sele...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~1209-~1209: Possible missing comma found.
Context: ... enabling on the domain. In order to do this go to domain management -> select the d...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~1210-~1210: Possible missing comma found.
Context: ... toggle. Then go to the menu in the top right select API access and then create a new...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~1211-~1211: To elevate your writing, try using an alternative expression here.
Context: ...nd then create a new api key. The title does not matter and is not used by certbot, make note o...
(MATTERS_RELEVANT)
[uncategorized] ~1298-~1298: Possible missing comma found.
Context: ... statement to your domain's hosted zone ARN or use * for all. ```json { ...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
letsencrypt/DOCS.md
1298-1298: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
1298-1298: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
1298-1298: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
1298-1298: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build armv7 letsencrypt add-on
- GitHub Check: Build armhf letsencrypt add-on
- GitHub Check: Build aarch64 letsencrypt add-on
🔇 Additional comments (5)
letsencrypt/rootfs/etc/services.d/lets-encrypt/run (3)
24-26: LGTM! New configuration options added.The new variables
DRY_RUN,TEST_CERT, andVERBOSEare properly defined using bashio::config and follow the existing pattern.
40-52: LGTM! Azure DNS provider implementation follows security best practices.The implementation:
- Validates required configuration
- Securely handles credentials by copying to a protected location
- Sets proper file permissions (600)
352-362: LGTM! Additional arguments block properly implemented.The implementation cleanly adds optional certbot arguments based on configuration flags.
letsencrypt/rootfs/etc/cont-init.d/file-structure.sh (1)
10-87: LGTM! Configuration parameters properly updated.The new DNS provider parameters are:
- Added in alphabetical order
- Follow the existing pattern
- Properly formatted
letsencrypt/DOCS.md (1)
29-30: LGTM! Challenge descriptions updated for consistency.The domain format examples have been standardized to use
*.domain.tldinstead of*.yourdomain.com.Also applies to: 34-35
|
@coderabbitai full review |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
letsencrypt/DOCS.md (3)
95-184: YAML examples and provider configurations.
The numerous YAML examples throughout the document have been updated to include the new fields and reflect correct provider identifiers. Ensure that:
• The field names (e.g.,cloudflare_api_token,domainoffensive_token, etc.) match those in the configuration schema.
• Placeholder values do not accidentally expose sensitive information.
Additionally, consider switching to asterisk-based emphasis for UI strings to align with the documentation guidelines.
254-264: ACME server override example clarity.
The section demonstrating how to override the default ACME server (lines 246–252) is clearly written. Verify punctuation and avoid excess wording for improved readability per Microsoft Style Guide recommendations.🧰 Tools
🪛 LanguageTool
[uncategorized] ~259-~259: Did you mean: “By default,”?
Context: ...ary>Selecting the Key Type By default the ECDSA key type is used. You can cho...(BY_DEFAULT_COMMA)
1298-1301: Markdown emphasis consistency.
Static analysis has flagged instances where underscores are used for emphasis instead of asterisks. Switching to asterisk-based emphasis consistently throughout the document will help conform to markdown style guidelines.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1298-1298: Emphasis style
Expected: asterisk; Actual: underscore(MD049, emphasis-style)
1298-1298: Emphasis style
Expected: asterisk; Actual: underscore(MD049, emphasis-style)
1298-1298: Emphasis style
Expected: asterisk; Actual: underscore(MD049, emphasis-style)
1298-1298: Emphasis style
Expected: asterisk; Actual: underscore(MD049, emphasis-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
letsencrypt/CHANGELOG.md(1 hunks)letsencrypt/DOCS.md(24 hunks)letsencrypt/Dockerfile(3 hunks)letsencrypt/README.md(0 hunks)letsencrypt/build.yaml(1 hunks)letsencrypt/config.yaml(4 hunks)letsencrypt/rootfs/etc/cont-init.d/file-structure.sh(1 hunks)letsencrypt/rootfs/etc/services.d/lets-encrypt/run(11 hunks)letsencrypt/translations/en.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- letsencrypt/README.md
🧰 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.mdletsencrypt/config.yamlletsencrypt/Dockerfileletsencrypt/DOCS.mdletsencrypt/build.yaml
`*/**(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.mdletsencrypt/config.yamlletsencrypt/Dockerfileletsencrypt/DOCS.mdletsencrypt/build.yaml
`*/**(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.mdletsencrypt/config.yamlletsencrypt/Dockerfileletsencrypt/DOCS.mdletsencrypt/build.yaml
`*/**(html|markdown|md)`: - Use sentence-style capitalizatio...
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
letsencrypt/CHANGELOG.mdletsencrypt/config.yamlletsencrypt/Dockerfileletsencrypt/DOCS.mdletsencrypt/build.yaml
`*/**(html|markdown|md)`: do not comment on HTML used for ic...
*/**(html|markdown|md): do not comment on HTML used for icons
letsencrypt/CHANGELOG.mdletsencrypt/config.yamlletsencrypt/Dockerfileletsencrypt/DOCS.mdletsencrypt/build.yaml
`*/**(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.mdletsencrypt/config.yamlletsencrypt/Dockerfileletsencrypt/DOCS.mdletsencrypt/build.yaml
🪛 Gitleaks (8.21.2)
letsencrypt/DOCS.md
658-658: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
677-677: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
395-395: Detected a Cloudflare Global API Key, potentially compromising cloud application deployments and operational security.
(cloudflare-global-api-key)
406-406: Detected a Cloudflare Global API Key, potentially compromising cloud application deployments and operational security.
(cloudflare-global-api-key)
952-952: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1045-1045: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1094-1094: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1169-1169: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1170-1170: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1171-1171: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1222-1222: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1223-1223: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1294-1294: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1394-1397: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🪛 LanguageTool
letsencrypt/DOCS.md
[uncategorized] ~398-~398: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...l API Key” was used for authentication. However this key can access the entire Cloudfla...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[style] ~413-~413: Consider a shorter alternative to avoid wordiness.
Context: ...
ClouDNS
In order to use a domain with this challenge, you f...(IN_ORDER_TO_PREMIUM)
[style] ~666-~666: Consider a shorter alternative to avoid wordiness.
Context: ...PI access must be requested and granted in order to use this module: <https://cp.easydns.co...
(IN_ORDER_TO_PREMIUM)
[misspelling] ~708-~708: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...est a certificate for. If you only have an Gandi LiveDNS API key, please refer t...
(EN_A_VS_AN)
[grammar] ~1081-~1081: Consider using only one pronoun here.
Context: ...address restrict the access to your IP. I you are not sure, you probably don't have a...
(PRP_REPITION)
[style] ~1207-~1207: Consider a shorter alternative to avoid wordiness.
Context: ...
Porkbun
In order to use a domain with this challenge, API a...(IN_ORDER_TO_PREMIUM)
[style] ~1208-~1208: Consider a shorter alternative to avoid wordiness.
Context: ...ccess will need enabling on the domain. In order to do this go to domain management -> sele...
(IN_ORDER_TO_PREMIUM)
[style] ~1211-~1211: To elevate your writing, try using an alternative expression here.
Context: ...nd then create a new api key. The title does not matter and is not used by certbot, make note o...
(MATTERS_RELEVANT)
🪛 markdownlint-cli2 (0.17.2)
letsencrypt/DOCS.md
1298-1298: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
1298-1298: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
1298-1298: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
1298-1298: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
🔇 Additional comments (41)
letsencrypt/rootfs/etc/services.d/lets-encrypt/run (21)
24-26: Introduce new certbot parameters
Setting these environment variables for DRY_RUN, TEST_CERT, and VERBOSE is a helpful expansion of the addon’s configurability.
40-52: Consider strict error handling for missing Azure credentials
Currently, if the credentials file is missing, the script only logs an info message. This can lead to incomplete DNS challenge configurations. Consider exiting with an error to prevent silent failures.
67-71: Check propagation seconds usage
The block for ClouDNS looks consistent with other DNS providers. Ensure that the default or configured propagation time is sufficient, given ClouDNS caching.
72-76: Desec integration
The integration logic is straightforward. Confirm that the credentials file is properly populated before use.
89-93: Domainoffensive arguments
Thank you for removing the redundant agreement/email arguments. The current approach is consistent with global arguments.
94-99: Dreamhost DNS integration
The logic is consistent with the overall pattern. No immediate issues noted.
110-116: easyDNS credentials
Ensure that thedns.easydns_key,dns.easydns_token, anddns.easydns_endpointare all required. Missing any field might cause a partial failure.
159-164: dns-he naming alignment
Double-check that using"dns-hurricane_electric"aligns with upstream plugin naming for"dns-he".
175-181: INWX block
Requiringdns.inwx_username,dns.inwx_password, anddns.inwx_shared_secretis correct. No immediate concerns.
205-209: dns-mijn-host integration
Ensure the correct naming for the plugin is used. Otherwise, looks consistent.
210-215: dns-namecheap
The approach is consistent with other DNS providers. No issues found.
216-222: dns-netcup
The block is consistent. Properly requiring all config fields.
223-227: dns-njalla
No immediate issues. The config requires the token. Implementation is consistent.
228-232: dns-noris
All required fields appear to be enforced. The logic is consistent with other providers.
244-248: dns-porkbun
The config requires the key and secret. This approach is consistent.
254-265: Exporting AWS credentials
Storing AWS credentials as environment variables is typical, but confirm there's no risk of them leaking into logs.
266-271: dns-simply
Block is consistent with the pattern. No issues identified.
272-281: dns-transip
Increasing the propagation limit to 240 seconds is a good approach for reliability.
352-363: Introducing DRY_RUN, TEST_CERT, and VERBOSE
Appending these certbot arguments on the fly is a great addition. This helps with debugging and staging.
369-369: Add additional arguments for DNS challenge
IncorporatingADDITIONAL_ARGS[@]into the certbot command ensures the new config flags are applied.
379-379: Add additional arguments for HTTP challenge
Similarly, passingADDITIONAL_ARGS[@]to the standalone challenge helps unify the usage of these flags.letsencrypt/build.yaml (7)
12-13: Developer note + ACME_VERSION
Adding theACME_VERSIONpinned at 3.2.0 is consistent with the upstream changes. Keep the note about alphabetical insertion for clarity.
18-19: Certbot DNS Domainoffensive & Dreamhost
Listing these versions directly in alphabetical order clarifies maintenance.
23-27: EuroDNS, Gandi, GoDaddy, Hurricane Electric
These plugin version additions are coherent. Double-check that 1.6.0 for Gandi matches your tested environment if any compatibility issues arise.
35-36: Netcup and Njalla plugin versions
Pinned versions look correct. Confirm that 1.4.4 and 2.0.2 remain stable with Certbot 3.2.0.
38-38: Plesk plugin version
Pinning at 0.3.0 is consistent with the alphabetical approach.
41-42: TransIP and WebSupport
The pinned versions appear correct. No immediate incompatibilities found.
43-45: CERTBOT_VERSION, CLOUDFLARE_VERSION, and CRYPTOGRAPHY_VERSION
These pinned versions ensure consistent builds. If a future upgrade is needed, consider updating them carefully due to potential dependencies.letsencrypt/translations/en.yaml (1)
45-56: New configuration labels
Adding descriptive labels and help text fordry_run,test_cert, andverboseimproves user understanding.letsencrypt/Dockerfile (3)
5-8: Alphabetical plugin note and new ARG additions.
The developer note on line 5 reminds contributors to insert new plugins alphabetically, and the added ARGs (e.g.ACME_VERSION,CERTBOT_DNS_DOMAINOFFENSIVE_VERSION, among others) clearly extend the build configuration. Verify that these new version arguments consistently match related entries in build and configuration files.
6-41: Expanded build arguments for DNS plugins.
The ARG block now includes several new DNS plugin version variables (e.g. for EuroDNS, Gandi, Godaddy, Hurricane Electric, Netcup, Njalla, Plesk, and Porkbun) as well as updates forCERTBOT_VERSION,CLOUDFLARE_VERSION, andCRYPTOGRAPHY_VERSION. This refactoring centralizes version control and supports alphabetical order.
42-104: Refactored pip installation commands with updated plugin references.
The RUN command correctly references the new version ARGs in the pip install call. Notably, the switch to a maintained Gandi DNS plugin (certbot-plugin-gandi-modern) is implemented (line 71). Ensure that the commented-out installation ofcertbot-dns-websupportremains disabled intentionally.letsencrypt/config.yaml (3)
2-2: Configuration version bump.
The version is updated to5.3.4, which reflects the changes in plugin support and new configuration fields. Confirm that consumers of this add-on are aware of the version bump and its impact.
43-45: New certbot options added to schema.
The new schema fieldsdry_run,test_cert, andverboseare introduced (lines 43–45). These options will allow additional certbot arguments, aligning with the recent changes in the Dockerfile and changelog.
46-48: DNS schema reordering and developer note.
The DNS section now includes a developer note (line 47) requesting alphabetical additions, ensuring consistency across the project. Verify that the provider fields and their types remain consistent with the expected configuration.letsencrypt/CHANGELOG.md (2)
3-10: Changelog entry for version 5.3.4.
The new changelog entry clearly documents the improvements: documentation enhancements, configuration refactoring, script updates, the switch to a maintained Gandi DNS plugin, and the addition of new certbot arguments. This provides clear traceability for the updates in this release.
11-14: Preservation of previous version history.
The subsequent entries for previous versions (e.g. 5.3.3 and earlier) are preserved and require no additional changes.letsencrypt/rootfs/etc/cont-init.d/file-structure.sh (1)
112-124: Cleanup of removed options.
The conditional blocks that remove deprecated configuration options (e.g. fordns.cloudxns_*,keytype, and Google Domains) are retained, ensuring that obsolete settings do not interfere with the new configuration.letsencrypt/DOCS.md (3)
28-30: HTTP challenge clarification.
The documentation now explicitly states that the HTTP challenge does not allow wildcard certificates (line 29). This clear guidance is essential for users configuring the add-on.
31-35: DNS challenge enhancement.
The DNS challenge section correctly notes that wildcard certificates are permitted (line 34) and provides clear instructions. Ensure that the examples align with the updated configuration schema.
39-40: Developer note for plugin ordering in documentation.
The embedded developer note reminds contributors to add new DNS plugins in alphabetical order within the supported provider list. This note is consistent with similar comments in the Dockerfile and config files.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
letsencrypt/rootfs/etc/services.d/lets-encrypt/run (1)
145-157: 🛠️ Refactor suggestionStrengthen Google credentials handling.
Similar to a previous review comment, the script only logs an info message if the Google credentials file is missing.
- else - bashio::log.info "Google Credentials File doesn't exists in folder share." + else + bashio::log.error "Google credentials file '${GOOGLE_CREDS}' not found in /share directory" + exit 1 + fi
🧹 Nitpick comments (2)
letsencrypt/rootfs/etc/cont-init.d/file-structure.sh (2)
36-37: Hurricane Electric Credential Addition
The new values for:
dns_hurricane_electric_pass(sourced from'dns.he_pass')dns_hurricane_electric_user(sourced from'dns.he_user')have been added. For consistency, consider whether these keys should be renamed (e.g. to
'dns.hurricane_electric_pass'and'dns.hurricane_electric_user') to fully match the naming convention used for other providers.
10-87: Alphabetical Sorting Consistency Check
The PR objective mentions sorting configuration entries alphabetically. While many new parameters have been added, the overall ordering in the echo block appears to be grouped by provider rather than strictly alphabetically sorted. Please confirm whether this grouping is intentional for clarity or if a strict alphabetical reorder is required to match documentation guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
letsencrypt/rootfs/etc/cont-init.d/file-structure.sh(1 hunks)letsencrypt/rootfs/etc/services.d/lets-encrypt/run(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build i386 letsencrypt add-on
- GitHub Check: Build armv7 letsencrypt add-on
- GitHub Check: Build armhf letsencrypt add-on
- GitHub Check: Build amd64 letsencrypt add-on
- GitHub Check: Build aarch64 letsencrypt add-on
🔇 Additional comments (18)
letsencrypt/rootfs/etc/services.d/lets-encrypt/run (3)
24-26: LGTM! New configuration options enhance flexibility.The addition of
DRY_RUN,TEST_CERT, andVERBOSEoptions provides users with better control over certificate operations.
355-366: LGTM! Well-structured additional arguments handling.The implementation of additional arguments is clean and modular, making it easy to add more options in the future.
372-372: LGTM! Consistent integration of additional arguments.The additional arguments are properly integrated into both DNS and HTTP challenge certbot commands.
Also applies to: 382-382
letsencrypt/rootfs/etc/cont-init.d/file-structure.sh (15)
10-13: New AWS & Basic DNS Parameter Additions
The added lines now include configuration for:
aws_access_key_idaws_secret_access_keydns_cloudns_auth_passworddns_desec_tokenPlease verify that each configuration key (e.g.
'dns.aws_access_key_id', etc.) correctly maps to the intended values from the add-on configuration. The output is written to/data/dnsapikeyand later permission-restricted, which is good practice for handling sensitive data.
15-15: DirectAdmin Password Update
The update fordns_directadmin_passwordnow uses the configuration key'dns.directadmin_password'. Confirm that this update is intentional and consistent with the documentation.
21-23: Addition of Domainoffensive & DreamHost Parameters
The new parameters:
dns_domainoffensive_tokendns_dreamhost_api_keydns_dreamhost_baseurlhave been introduced to support additional DNS integrations. Please ensure these keys exactly match those in
config.yamland that the corresponding documentation (e.g., in DOCS.md) is updated accordingly.
26-28: Integration of easyDNS Parameters
The new entries for:
dns_easydns_endpointdns_easydns_userkeydns_easydns_usertokenare now added. Verify that the configuration keys are in sync with the updated docs and consider whether the ordering meets the intended alphabetical (or grouped) structure for clarity.
39-43: INWX & IONOS Configuration Updates
The updates for INWX parameters:
dns_inwx_passworddns_inwx_shared_secretdns_inwx_url(hardcoded ashttps://api.domrobot.com)dns_inwx_usernameand the IONOS parameter:
dns_ionos_endpointare now included. The hardcoded URL may be acceptable, but consider making it configurable in the future if provider endpoints change. Verify that
'dns.ionos_endpoint'is the correct key for the IONOS integration.
47-48: Joker DNS Credentials Update
The new parameters:
dns_joker_passworddns_joker_usernamehave been added. Please confirm that these keys are consistent with what is expected in the configuration documentation.
52-52: Loopia User Parameter Update
The update todns_loopia_userappears to be intentional. Verify that this change is reflected in the help docs and is used consistently elsewhere in the add-on.
57-57: Namecheap Username Update
The added parameterdns_namecheap_usernameis now included. Ensure that it adheres to the naming conventions used throughout the configuration and is noted in the documentation.
60-60: Netcup Customer ID Addition
The new settingdns_netcup_customer_idhas been added. Confirm that it’s correctly positioned among the other Netcup settings and aligns with the documented configuration schema.
67-70: OVH & Plesk Parameter Updates
The updated parameters for OVH and Plesk:
dns_ovh_endpointdns_plesk_api_urldns_plesk_passworddns_plesk_usernameshould be rechecked to ensure they conform with the latest API specifications and that their naming is consistent with other DNS provider keys.
71-73: Porkbun & RFC2136 Algorithm Parameters Addition
The new entries for:
dns_porkbun_keydns_porkbun_secretdns_rfc2136_algorithmhave been added. These look appropriate; please verify that they follow the project's conventions and the intended alphabetical or logical grouping if that is part of the refactoring objective.
75-75: RFC2136 Port Parameter Update
The update todns_rfc2136_portis present. Confirm that this change matches the expected values or configuration requirements for the RFC2136 provider.
77-77: RFC2136 Server Parameter Update
Ensure that the updated value fordns_rfc2136_servercorrectly corresponds to the desired DNS server configuration for RFC2136 challenges.
80-82: Sakuracloud & Simply DNS Parameters Addition
The new configuration parameters:
dns_sakuracloud_api_tokendns_simply_account_namedns_simply_api_keyare now part of the setup. Verify that their configuration keys reflect those described in the updated documentation and meet provider requirements.
85-85: TransIP Username Update
The updateddns_transip_usernameparameter is added. Ensure that this change is documented and that any downstream integration or usage of this key is updated accordingly.
GoetzGoerisch
force-pushed
the
fix/letsencrypt-directadmin
branch
from
6f895ad to
cd95154
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
letsencrypt/rootfs/etc/services.d/lets-encrypt/run (1)
40-52:⚠️ Potential issueStrengthen Azure credentials handling.
The script only logs an info message if the Azure credentials file is missing. This could lead to silent failures during certificate generation.
Apply this diff to handle missing credentials more robustly:
else - bashio::log.info "Azure credentials file doesn't exist in folder share." + bashio::log.error "Azure credentials file '${AZURE_CREDS}' not found in /share directory" + exit 1 fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
letsencrypt/rootfs/etc/services.d/lets-encrypt/run(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build i386 letsencrypt add-on
- GitHub Check: Build armv7 letsencrypt add-on
- GitHub Check: Build armhf letsencrypt add-on
- GitHub Check: Build amd64 letsencrypt add-on
- GitHub Check: Build aarch64 letsencrypt add-on
🔇 Additional comments (3)
letsencrypt/rootfs/etc/services.d/lets-encrypt/run (3)
24-26: LGTM! New configuration options enhance flexibility.The new configuration options
DRY_RUN,TEST_CERT, andVERBOSEprovide useful debugging and testing capabilities.
355-366: LGTM! Well-structured additional arguments handling.The implementation of additional arguments based on configuration is clean and modular.
367-388: LGTM! Consistent application of additional arguments.The additional arguments are correctly applied to both DNS and HTTP challenge modes.
GoetzGoerisch
force-pushed
the
fix/letsencrypt-directadmin
branch
from
cd95154 to
89a8267
Compare
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
* as per comment home-assistant#3855 (comment) * fixes home-assistant#3876 Co-authored-by: Goopher <goopher@live.nl>
Signed-off-by: Goetz Goerisch <47734341+GoetzGoerisch@users.noreply.github.com>
GoetzGoerisch
force-pushed
the
fix/letsencrypt-directadmin
branch
from
89a8267 to
6bfa2e2
Compare
|
Thank you for the review and suggestions, @agners, appreciated. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
letsencrypt/rootfs/etc/services.d/lets-encrypt/run (1)
275-280:⚠️ Potential issueFix TransIP global key configuration.
The global key configuration has a syntax error and is not properly written to the credentials file.
Apply this diff to fix the issues:
- if bashio::config.has_value 'dns.transip_global_key'; then - "dns_transip_global_key=$(bashio::config 'dns.transip_global_key')" + if bashio::config.exists 'dns.transip_global_key'; then + echo "dns_transip_global_key = $(bashio::config 'dns.transip_global_key')" >> "/data/dnsapikey"
🧹 Nitpick comments (2)
letsencrypt/DOCS.md (2)
398-401: Improve clarity in Cloudflare authentication section.Add a comma after "However" and rephrase for better clarity:
-However this key can access the entire Cloudflare API for all domains in your account, meaning it could cause a lot of damage if leaked. +However, this key can access the entire Cloudflare API for all domains in your account and could cause significant damage if leaked.🧰 Tools
🪛 LanguageTool
[uncategorized] ~398-~398: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...l API Key” was used for authentication. However this key can access the entire Cloudfla...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
1208-1211: Simplify Porkbun configuration instructions.The current wording is verbose and could be more concise:
-In order to use a domain with this challenge, API access will need enabling on the domain. In order to do this go to domain management -> select the domain -> details and click the API access toggle. +To use a domain with this challenge: +1. Go to domain management +2. Select the domain +3. Click details +4. Enable API access using the toggle🧰 Tools
🪛 LanguageTool
[style] ~1208-~1208: Consider a shorter alternative to avoid wordiness.
Context: ...ccess will need enabling on the domain. In order to do this go to domain management -> sele...(IN_ORDER_TO_PREMIUM)
[style] ~1211-~1211: To elevate your writing, try using an alternative expression here.
Context: ...nd then create a new api key. The title does not matter and is not used by certbot, make note o...(MATTERS_RELEVANT)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
letsencrypt/CHANGELOG.md(1 hunks)letsencrypt/DOCS.md(24 hunks)letsencrypt/Dockerfile(3 hunks)letsencrypt/README.md(0 hunks)letsencrypt/build.yaml(1 hunks)letsencrypt/config.yaml(4 hunks)letsencrypt/rootfs/etc/cont-init.d/file-structure.sh(1 hunks)letsencrypt/rootfs/etc/services.d/lets-encrypt/run(11 hunks)letsencrypt/translations/en.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- letsencrypt/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- letsencrypt/CHANGELOG.md
- letsencrypt/build.yaml
- letsencrypt/config.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/DOCS.mdletsencrypt/Dockerfile
`*/**(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/DOCS.mdletsencrypt/Dockerfile
`*/**(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/DOCS.mdletsencrypt/Dockerfile
`*/**(html|markdown|md)`: - Use sentence-style capitalizatio...
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
letsencrypt/DOCS.mdletsencrypt/Dockerfile
`*/**(html|markdown|md)`: do not comment on HTML used for ic...
*/**(html|markdown|md): do not comment on HTML used for icons
letsencrypt/DOCS.mdletsencrypt/Dockerfile
`*/**(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/DOCS.mdletsencrypt/Dockerfile
🪛 Gitleaks (8.21.2)
letsencrypt/DOCS.md
395-395: Detected a Cloudflare Global API Key, potentially compromising cloud application deployments and operational security.
(cloudflare-global-api-key)
406-406: Detected a Cloudflare Global API Key, potentially compromising cloud application deployments and operational security.
(cloudflare-global-api-key)
658-658: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
677-677: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
952-952: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1045-1045: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1094-1094: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1169-1169: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1170-1170: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1171-1171: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1222-1222: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1223-1223: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1294-1294: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1394-1397: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🪛 LanguageTool
letsencrypt/DOCS.md
[uncategorized] ~398-~398: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...l API Key” was used for authentication. However this key can access the entire Cloudfla...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[style] ~413-~413: Consider a shorter alternative to avoid wordiness.
Context: ...
ClouDNS
In order to use a domain with this challenge, you f...(IN_ORDER_TO_PREMIUM)
[style] ~666-~666: Consider a shorter alternative to avoid wordiness.
Context: ...PI access must be requested and granted in order to use this module: <https://cp.easydns.co...
(IN_ORDER_TO_PREMIUM)
[misspelling] ~708-~708: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...est a certificate for. If you only have an Gandi LiveDNS API key, please refer t...
(EN_A_VS_AN)
[grammar] ~1081-~1081: Consider using only one pronoun here.
Context: ...address restrict the access to your IP. I you are not sure, you probably don't have a...
(PRP_REPITION)
[style] ~1207-~1207: Consider a shorter alternative to avoid wordiness.
Context: ...
Porkbun
In order to use a domain with this challenge, API a...(IN_ORDER_TO_PREMIUM)
[style] ~1208-~1208: Consider a shorter alternative to avoid wordiness.
Context: ...ccess will need enabling on the domain. In order to do this go to domain management -> sele...
(IN_ORDER_TO_PREMIUM)
[style] ~1211-~1211: To elevate your writing, try using an alternative expression here.
Context: ...nd then create a new api key. The title does not matter and is not used by certbot, make note o...
(MATTERS_RELEVANT)
🪛 markdownlint-cli2 (0.17.2)
letsencrypt/DOCS.md
1298-1298: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
1298-1298: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
1298-1298: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
1298-1298: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build i386 letsencrypt add-on
- GitHub Check: Build armv7 letsencrypt add-on
- GitHub Check: Build armhf letsencrypt add-on
- GitHub Check: Build amd64 letsencrypt add-on
- GitHub Check: Build aarch64 letsencrypt add-on
🔇 Additional comments (27)
letsencrypt/translations/en.yaml (1)
45-56: LGTM! New configuration options enhance testing and debugging capabilities.The new options
dry_run,test_cert, andverboseare well-documented and provide valuable functionality for testing certificate requests and debugging issues.letsencrypt/rootfs/etc/services.d/lets-encrypt/run (3)
24-26: LGTM! New configuration variables properly declared.The new variables align with the configuration options defined in translations/en.yaml.
40-53: LGTM! Azure DNS provider implementation is secure and robust.The implementation:
- Properly validates required configuration
- Securely handles credentials file with appropriate permissions
- Includes proper error handling for missing credentials
357-367: LGTM! Additional arguments properly implemented.The implementation correctly handles the new configuration options and applies them consistently to both DNS and HTTP challenges.
letsencrypt/Dockerfile (23)
5-5: Ensure alphabetical ordering note clarity.
The developer note now reminds contributors to insert new plugins in alphabetical order across all lists—a clear instruction to maintain consistency.
8-8: Add ACME_VERSION ARG.
The addition of theACME_VERSIONargument enables explicit version control for the acme package.
13-13: Include domain offensive DNS plugin argument.
The newCERTBOT_DNS_DOMAINOFFENSIVE_VERSIONargument is correctly added and placed in alphabetical order.
18-18: Add EuroDNS plugin version argument.
IntroducingCERTBOT_DNS_EURODNS_VERSIONprovides controlled versioning for the EuroDNS plugin.
19-19: Update Gandi DNS plugin version argument.
The update toCERTBOT_DNS_GANDI_VERSIONis intended to work with the maintained version, as seen later with the switch tocertbot-plugin-gandi-modern. Confirm that both ARG and installation use the same version value.
20-20: Add Godaddy DNS plugin version argument.
The inclusion ofCERTBOT_DNS_GODADDY_VERSIONfits within the alphabetical order and supports independent version management.
22-22: Include Hurricane Electric plugin version argument.
AddingCERTBOT_DNS_HURRICANE_ELECTRIC_VERSIONensures that the Hurricane Electric DNS plugin is installed with its proper version.
30-30: Add Netcup DNS plugin version argument.
The newCERTBOT_DNS_NETCUP_VERSIONargument is correctly declared, maintaining proper alphabetical ordering.
31-31: Add Njalla DNS plugin version argument.
The argumentCERTBOT_DNS_NJALLA_VERSIONis introduced appropriately and is in the correct alphabetical sequence.
33-33: Add Plesk DNS plugin version argument.
The declaration ofCERTBOT_DNS_PLESK_VERSIONsupports version control for the Plesk plugin.
34-34: Add Porkbun DNS plugin version argument.
The addition ofCERTBOT_DNS_PORKBUN_VERSIONis properly positioned and manages versioning for the Porkbun DNS plugin.
38-40: Reorder version arguments for certbot, cloudflare, and cryptography.
GroupingCERTBOT_VERSION,CLOUDFLARE_VERSION, andCRYPTOGRAPHY_VERSIONat the end of the ARG declarations aids in consistency. Verify that these values match their usage in the installation commands below.
56-56: Install acme package with new version argument.
The RUN command now installs theacmepackage using theACME_VERSIONargument. This change aligns with the refactoring efforts toward ACME-based functionality.
65-66: Update DNS plugin installations for domain offensive and Dreamhost.
Bothcertbot-dns-domainoffensiveandcertbot-dns-dreamhostcommands now reference their corresponding version ARGs, ensuring that each is installed with the correct version.
69-69: Update installation command for EasyDNS plugin.
UsingCERTBOT_DNS_EASYDNS_VERSIONfor the EasyDNS package enhances consistency across DNS plugin installations.
71-71: Switch to maintained Gandi plugin.
The command now installscertbot-plugin-gandi-modernusingCERTBOT_DNS_GANDI_VERSION, ensuring that the plugin is both up-to-date and maintained. Confirm that the ARG value properly reflects the intended version.
76-76: Update installation for Hurricane Electric DNS plugin.
The installation command appliesCERTBOT_DNS_HURRICANE_ELECTRIC_VERSIONcorrectly, which helps maintain version integrity for this plugin.
78-78: Specify version for INWX DNS plugin.
UsingCERTBOT_DNS_INWX_VERSIONensures that the INWX DNS plugin is installed at the intended version.
85-85: Apply Namecheap DNS plugin version argument.
The reference toCERTBOT_DNS_NAMECHEAP_VERSIONin the installation command ensures accurate version control for the Namecheap DNS plugin.
86-86: Apply Netcup DNS plugin version argument in installation.
The installation ofcertbot-dns-netcupnow properly usesCERTBOT_DNS_NETCUP_VERSIONfor precise version management.
87-87: Ensure Njalla DNS plugin installation uses new version argument.
The command now explicitly referencesCERTBOT_DNS_NJALLA_VERSION, confirming that the Njalla DNS plugin is installed with the correct version.
91-92: Update installation commands for Plesk and Porkbun DNS plugins.
Both thecertbot-dns-pleskandcertbot-dns-porkbuninstallations have been updated to reflect their respective version ARGs, supporting proper version tracking.
100-102: Update installation for certbot, Cloudflare, and cryptography packages.
The RUN command now tiescertbot,cloudflare, andcryptographyinstallations to their respective version arguments, ensuring all dependencies are locked to the specified versions. Verify that these versions are aligned with the overall build configuration.
| "dns_dynu_auth_token = $(bashio::config 'dns.dynu_auth_token')\n" \ | ||
| "dns_eurodns_applicationId = $(bashio::config 'dns.eurodns_applicationId')\n" \ | ||
| "dns_easydns_endpoint = $(bashio::config 'dns.easydns_endpoint')\n" \ | ||
| "dns_easydns_userkey = $(bashio::config 'dns.easydns_key')\n" \ | ||
| "dns_easydns_usertoken = $(bashio::config 'dns.easydns_token')\n" \ | ||
| "dns_eurodns_apiKey = $(bashio::config 'dns.eurodns_apiKey')\n" \ | ||
| "dns_gehirn_api_token = $(bashio::config 'dns.gehirn_api_token')\n" \ | ||
| "dns_eurodns_applicationId = $(bashio::config 'dns.eurodns_applicationId')\n" \ | ||
| "dns_gehirn_api_secret = $(bashio::config 'dns.gehirn_api_secret')\n" \ | ||
| "dns_godaddy_secret = $(bashio::config 'dns.godaddy_secret')\n" \ | ||
| "dns_gehirn_api_token = $(bashio::config 'dns.gehirn_api_token')\n" \ | ||
| "dns_godaddy_key = $(bashio::config 'dns.godaddy_key')\n" \ | ||
| "dns_godaddy_secret = $(bashio::config 'dns.godaddy_secret')\n" \ | ||
| "dns_hetzner_api_token = $(bashio::config 'dns.hetzner_api_token')\n" \ | ||
| "dns_hurricane_electric_pass = $(bashio::config 'dns.he_pass')\n" \ | ||
| "dns_hurricane_electric_user = $(bashio::config 'dns.he_user')\n" \ | ||
| "dns_infomaniak_token = $(bashio::config 'dns.infomaniak_api_token')\n" \ | ||
| "dns_inwx_password = $(bashio::config 'dns.inwx_password')\n" \ | ||
| "dns_inwx_shared_secret = $(bashio::config 'dns.inwx_shared_secret')\n" \ | ||
| "dns_inwx_url = https://api.domrobot.com\n" \ | ||
| "dns_inwx_username = $(bashio::config 'dns.inwx_username')\n" \ | ||
| "dns_ionos_endpoint = $(bashio::config 'dns.ionos_endpoint')\n" \ | ||
| "dns_ionos_prefix = $(bashio::config 'dns.ionos_prefix')\n" \ | ||
| "dns_ionos_secret = $(bashio::config 'dns.ionos_secret')\n" \ | ||
| "dns_ionos_endpoint = $(bashio::config 'dns.ionos_endpoint')\n" \ | ||
| "dns_joker_username = $(bashio::config 'dns.joker_username')\n" \ | ||
| "dns_joker_password = $(bashio::config 'dns.joker_password')\n" \ | ||
| "dns_joker_domain = $(bashio::config 'dns.joker_domain')\n" \ | ||
| "dns_plesk_username = $(bashio::config 'dns.plesk_username')\n" \ | ||
| "dns_plesk_password = $(bashio::config 'dns.plesk_password')\n" \ | ||
| "dns_plesk_api_url = $(bashio::config 'dns.plesk_api_url')\n" \ | ||
| "dns_joker_password = $(bashio::config 'dns.joker_password')\n" \ | ||
| "dns_joker_username = $(bashio::config 'dns.joker_username')\n" \ | ||
| "dns_linode_key = $(bashio::config 'dns.linode_key')\n" \ | ||
| "dns_linode_version = $(bashio::config 'dns.linode_version')\n" \ | ||
| "dns_loopia_user = $(bashio::config 'dns.loopia_user')\n" \ | ||
| "dns_loopia_password = $(bashio::config 'dns.loopia_password')\n" \ | ||
| "dns_loopia_user = $(bashio::config 'dns.loopia_user')\n" \ | ||
| "dns_luadns_email = $(bashio::config 'dns.luadns_email')\n" \ | ||
| "dns_luadns_token = $(bashio::config 'dns.luadns_token')\n" \ | ||
| "dns_mijn_host_api_key = $(bashio::config 'dns.mijn_host_api_key')\n" \ | ||
| "dns_namecheap_username = $(bashio::config 'dns.namecheap_username')\n" \ | ||
| "dns_namecheap_api_key = $(bashio::config 'dns.namecheap_api_key')\n" \ | ||
| "dns_netcup_customer_id = $(bashio::config 'dns.netcup_customer_id')\n" \ | ||
| "dns_namecheap_username = $(bashio::config 'dns.namecheap_username')\n" \ | ||
| "dns_netcup_api_key = $(bashio::config 'dns.netcup_api_key')\n" \ | ||
| "dns_netcup_api_password = $(bashio::config 'dns.netcup_api_password')\n" \ | ||
| "dns_simply_account_name = $(bashio::config 'dns.simply_account_name')\n" \ | ||
| "dns_simply_api_key = $(bashio::config 'dns.simply_api_key')\n" \ | ||
| "dns_netcup_customer_id = $(bashio::config 'dns.netcup_customer_id')\n" \ | ||
| "dns_njalla_token = $(bashio::config 'dns.njalla_token')\n" \ | ||
| "dns_noris_token = $(bashio::config 'dns.noris_token')\n" \ | ||
| "dns_nsone_api_key = $(bashio::config 'dns.nsone_api_key')\n" \ | ||
| "dns_porkbun_key = $(bashio::config 'dns.porkbun_key')\n" \ | ||
| "dns_porkbun_secret = $(bashio::config 'dns.porkbun_secret')\n" \ | ||
| "dns_ovh_endpoint = $(bashio::config 'dns.ovh_endpoint')\n" \ | ||
| "dns_ovh_application_key = $(bashio::config 'dns.ovh_application_key')\n" \ | ||
| "dns_ovh_application_secret = $(bashio::config 'dns.ovh_application_secret')\n" \ | ||
| "dns_ovh_consumer_key = $(bashio::config 'dns.ovh_consumer_key')\n" \ | ||
| "dns_rfc2136_server = $(bashio::config 'dns.rfc2136_server')\n" \ | ||
| "dns_rfc2136_port = $(bashio::config 'dns.rfc2136_port')\n" \ | ||
| "dns_ovh_endpoint = $(bashio::config 'dns.ovh_endpoint')\n" \ | ||
| "dns_plesk_api_url = $(bashio::config 'dns.plesk_api_url')\n" \ | ||
| "dns_plesk_password = $(bashio::config 'dns.plesk_password')\n" \ | ||
| "dns_plesk_username = $(bashio::config 'dns.plesk_username')\n" \ | ||
| "dns_porkbun_key = $(bashio::config 'dns.porkbun_key')\n" \ | ||
| "dns_porkbun_secret = $(bashio::config 'dns.porkbun_secret')\n" \ | ||
| "dns_rfc2136_algorithm = $(bashio::config 'dns.rfc2136_algorithm')\n" \ | ||
| "dns_rfc2136_name = $(bashio::config 'dns.rfc2136_name')\n" \ | ||
| "dns_rfc2136_port = $(bashio::config 'dns.rfc2136_port')\n" \ | ||
| "dns_rfc2136_secret = $(bashio::config 'dns.rfc2136_secret')\n" \ | ||
| "dns_rfc2136_algorithm = $(bashio::config 'dns.rfc2136_algorithm')\n" \ | ||
| "dns_rfc2136_server = $(bashio::config 'dns.rfc2136_server')\n" \ | ||
| "dns_rfc2136_sign_query = $(bashio::config 'dns.rfc2136_sign_query')\n" \ | ||
| "aws_access_key_id = $(bashio::config 'dns.aws_access_key_id')\n" \ | ||
| "aws_secret_access_key = $(bashio::config 'dns.aws_secret_access_key')\n" \ | ||
| "dns_sakuracloud_api_token = $(bashio::config 'dns.sakuracloud_api_token')\n" \ | ||
| "dns_sakuracloud_api_secret = $(bashio::config 'dns.sakuracloud_api_secret')\n" \ | ||
| "dns_transip_username = $(bashio::config 'dns.transip_username')\n" \ | ||
| "dns_sakuracloud_api_token = $(bashio::config 'dns.sakuracloud_api_token')\n" \ | ||
| "dns_simply_account_name = $(bashio::config 'dns.simply_account_name')\n" \ | ||
| "dns_simply_api_key = $(bashio::config 'dns.simply_api_key')\n" \ | ||
| "dns_transip_global_key = $(bashio::config 'dns.transip_global_key')\n" \ | ||
| "dns_transip_key_file = /data/transip-rsa.key\n" \ | ||
| "dns_inwx_url = https://api.domrobot.com\n" \ | ||
| "dns_inwx_username = $(bashio::config 'dns.inwx_username')\n" \ | ||
| "dns_inwx_password = $(bashio::config 'dns.inwx_password')\n" \ | ||
| "dns_inwx_shared_secret = $(bashio::config 'dns.inwx_shared_secret')\n" \ | ||
| "dns_cloudns_auth_password = $(bashio::config 'dns.cloudns_auth_password')\n" \ | ||
| "dns_dreamhost_baseurl = $(bashio::config 'dns.dreamhost_baseurl')\n" \ | ||
| "dns_dreamhost_api_key = $(bashio::config 'dns.dreamhost_api_key')\n" \ | ||
| "dns_hurricane_electric_user = $(bashio::config 'dns.he_user')\n" \ | ||
| "dns_hurricane_electric_pass = $(bashio::config 'dns.he_pass')\n" \ | ||
| "dns_easydns_endpoint = $(bashio::config 'dns.easydns_endpoint')\n" \ | ||
| "dns_easydns_usertoken = $(bashio::config 'dns.easydns_token')\n" \ | ||
| "dns_easydns_userkey = $(bashio::config 'dns.easydns_key')\n" \ | ||
| "dns_domainoffensive_api_token = $(bashio::config 'dns.domainoffensive_token')\n" \ | ||
| "dns_transip_username = $(bashio::config 'dns.transip_username')\n" \ | ||
| "dns_websupport_identifier = $(bashio::config 'dns.websupport_identifier')\n" \ | ||
| "dns_websupport_secret_key = $(bashio::config 'dns.websupport_secret_key')\n" > /data/dnsapikey |
There was a problem hiding this comment.
Replace sensitive data in configuration examples.
The configuration contains actual API keys and tokens in examples, which could pose a security risk if they are real credentials.
Replace all API keys, tokens, and secrets with placeholder values like 'XXXX' or descriptive placeholders like 'your-api-key'.
Additionally, verify that the parameter names match those in config.yaml, particularly for:
dns_domainoffensive_api_tokendns_easydns_userkeydns_easydns_usertoken
| linode_key: 865c9f462c7d54abc1ad2dbf79c938bc5c55575fdaa097ead2178ee68365ab3e | ||
| linode_version: '4' |
There was a problem hiding this comment.
Replace API keys in examples with obvious placeholders.
Several examples contain real-looking API keys and tokens. Replace them with obvious placeholder values to prevent accidental use:
- linode_key: 865c9f462c7d54abc1ad2dbf79c938bc5c55575fdaa097ead2178ee68365ab3e
+ linode_key: 'your-linode-api-key'
- namecheap_api_key: 0123456789abcdef0123456789abcdef01234567
+ namecheap_api_key: 'your-namecheap-api-key'
- njalla_token: 0123456789abcdef0123456789abcdef01234567
+ njalla_token: 'your-njalla-token'
- ovh_application_key: 0123456789abcdef0123456789abcdef01234
- ovh_application_secret: 0123456789abcdef0123456789abcdef01234
- ovh_consumer_key: 0123456789abcdef0123456789abcdef01234
+ ovh_application_key: 'your-ovh-application-key'
+ ovh_application_secret: 'your-ovh-application-secret'
+ ovh_consumer_key: 'your-ovh-consumer-key'
- porkbun_key: 0123456789abcdef0123456789abcdef01234
- porkbun_secret: 0123456789abcdef0123456789abcdef01234
+ porkbun_key: 'your-porkbun-key'
+ porkbun_secret: 'your-porkbun-secret'
- aws_access_key_id: 0123456789ABCDEF0123
- aws_secret_access_key: 0123456789abcdef0123456789/abcdef0123456
+ aws_access_key_id: 'your-aws-access-key-id'
+ aws_secret_access_key: 'your-aws-secret-access-key'Also applies to: 1045-1046, 1094-1095, 1169-1172, 1222-1223, 1294-1295
🧰 Tools
🪛 Gitleaks (8.21.2)
952-952: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Summary by CodeRabbit
Summary by CodeRabbit