letsencrypt: Improved Logging #3847
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi @marcohald
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
📝 WalkthroughWalkthroughThe changes introduce a new configuration option Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Config
participant Script
participant LogDir
User->>Config: Set max_logs_backup
Config->>Script: Retrieve max_logs_backup
Script->>LogDir: Create /ssl/letsencrypt-logs
Script->>Script: Use max_logs_backup in logging options
Script->>User: Log management updated
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: 1
🧹 Outside diff range and nitpick comments (4)
letsencrypt/translations/en.yaml (1)
45-51: Enhance the configuration description with additional detailsWhile the description effectively explains the basic functionality, consider adding more details to help users make informed decisions:
max_logs_backup: name: Max Logs Backup description: >- Specifies the maximum number of backup logs that - should be kept by Certbot's built in log rotation. - Setting this flag to 0 disables log rotation entirely, - causing Certbot to always append to the same log file. + should be kept by Certbot's built-in log rotation in the + /ssl/letsencrypt-logs directory. Defaults to 100 (previously 1000). + Setting this to 0 disables log rotation entirely, + causing Certbot to always append to the same log file. + Must be a non-negative integer.This enhanced description:
- Specifies the log directory location
- Mentions the default and previous values
- Clarifies the valid range for the setting
letsencrypt/rootfs/etc/cont-init.d/file-structure.sh (2)
7-7: Add error handling and set appropriate permissions for the logs directory.While creating the logs directory is good, consider:
- Adding error handling for directory creation
- Setting appropriate permissions to ensure the logs are readable by users but secure
-mkdir -p /ssl/letsencrypt-logs +if ! mkdir -p /ssl/letsencrypt-logs; then + bashio::log.error "Failed to create letsencrypt-logs directory" + exit 1 +fi +chmod 755 /ssl/letsencrypt-logs
Line range hint
87-92: Enhance security of TransIP RSA key handling.The current implementation might expose the API key in process listings. Consider:
- Adding error handling for the openssl command
- Using a more secure way to pass the API key to openssl
if bashio::config.exists 'dns.transip_api_key'; then TRANSIP_API_KEY=$(bashio::config 'dns.transip_api_key') - echo "${TRANSIP_API_KEY}" | openssl rsa -out /data/transip-rsa.key + if ! echo "${TRANSIP_API_KEY}" | openssl rsa -out /data/transip-rsa.key 2>/dev/null; then + bashio::log.error "Failed to process TransIP RSA key" + exit 1 + fi chmod 600 /data/transip-rsa.key filetsencrypt/rootfs/etc/services.d/lets-encrypt/run (1)
340-343: Consider reducing duplication of logging parameters.While the logging parameters are correctly implemented for both DNS and HTTP challenges, they are duplicated in both certbot commands.
Consider extracting the common parameters into a variable:
+ LOGGING_ARGS=("--logs-dir" "$LOGS_DIR" "--max-log-backups" "$MAX_LOGS_BACKUP") # For DNS challenge 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" \ - --logs-dir "$LOGS_DIR" --max-log-backups "$MAX_LOGS_BACKUP" \ + "${LOGGING_ARGS[@]}" \ --preferred-challenges "$CHALLENGE" "${PROVIDER_ARGUMENTS[@]}" \ --preferred-chain "ISRG Root X1" \ "${EAB_ARGUMENTS[@]}" # For HTTP challenge 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" \ - --logs-dir "$LOGS_DIR" --max-log-backups "$MAX_LOGS_BACKUP" \ + "${LOGGING_ARGS[@]}" \ --preferred-challenges "$CHALLENGE" "${ACME_CUSTOM_SERVER_ARGUMENTS[@]}" --standalone \ --preferred-chain "ISRG Root X1" \ "${EAB_ARGUMENTS[@]}"Also applies to: 350-353
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
letsencrypt/config.yaml(2 hunks)letsencrypt/rootfs/etc/cont-init.d/file-structure.sh(1 hunks)letsencrypt/rootfs/etc/services.d/lets-encrypt/run(5 hunks)letsencrypt/translations/en.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
letsencrypt/config.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 (8)
letsencrypt/config.yaml (1)
27-27: LGTM: New configuration option added correctly.
The default value of 100 log backups is a reasonable choice, being more conservative than certbot's default of 1000.
letsencrypt/rootfs/etc/cont-init.d/file-structure.sh (4)
Line range hint 9-71: LGTM! Secure handling of sensitive DNS configuration.
The code correctly:
- Uses bashio::config for safe configuration reading
- Sets appropriate file permissions (600) for the sensitive data
Line range hint 73-85: LGTM! Well-documented ClouDNS configuration handling.
The code correctly implements the special case for ClouDNS authentication options with clear documentation.
Line range hint 94-108: LGTM! Clean handling of deprecated options.
The code properly removes deprecated configuration options using the appropriate bashio::addon.option command.
7-7: Verify integration with max_logs_backup configuration.
Let's ensure that the new logs directory is properly integrated with the max_logs_backup configuration mentioned in the PR description.
✅ Verification successful
Based on the search results, I can now provide a final response since I found the relevant configuration and implementation details:
Verified: The logs directory is properly integrated with max_logs_backup configuration
The new /ssl/letsencrypt-logs directory is correctly integrated with the max_logs_backup configuration option:
- The
max_logs_backupoption is properly defined inletsencrypt/config.yamlwith a default value of 100 - The option is documented in the translations file with a clear description of its purpose
- The logs directory and configuration are properly used together in the service script (
letsencrypt/rootfs/etc/services.d/lets-encrypt/run), where both are passed to certbot:--logs-dir "$LOGS_DIR" --max-log-backups "$MAX_LOGS_BACKUP"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of the new logs directory with max_logs_backup configuration
# Check if max_logs_backup is defined in config.yaml
echo "Checking config.yaml for max_logs_backup:"
rg -A 2 "max_logs_backup" config.yaml
# Check if the logs directory is used in the service script
echo "Checking service script for logs directory usage:"
rg -A 2 "letsencrypt-logs" "rootfs/etc/services.d/lets-encrypt/run"
Length of output: 436
Script:
#!/bin/bash
# Let's first find the configuration and service files
echo "Finding configuration files:"
fd -e yaml -e json
echo -e "\nFinding service-related files:"
fd "run|lets-encrypt"
echo -e "\nChecking content of found files for logs and backup configuration:"
for file in $(fd -t f); do
if grep -q "max_logs\|letsencrypt-logs" "$file" 2>/dev/null; then
echo -e "\nFile: $file"
cat "$file"
fi
done
Length of output: 99231
letsencrypt/rootfs/etc/services.d/lets-encrypt/run (3)
7-7: LGTM! Variable declarations are well-structured.
The new variables LOGS_DIR and MAX_LOGS_BACKUP are appropriately declared and initialized. The log directory path under /ssl is a suitable choice for certificate-related logs.
Also applies to: 23-23
Line range hint 307-326: LGTM! Certificate detection logic is robust.
The certbot command has been correctly updated to include the new logging parameters while maintaining the existing robust certificate detection logic.
7-7: Verify log directory creation.
Ensure that the LOGS_DIR path (/ssl/letsencrypt-logs) is created before it's used by certbot.
✅ Verification successful
Log directory creation is properly handled
The /ssl/letsencrypt-logs directory is created by the initialization script at letsencrypt/rootfs/etc/cont-init.d/file-structure.sh before any service starts. The directory creation is handled with mkdir -p command which ensures the directory exists.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the log directory is created in the file-structure initialization script
rg -l "mkdir.*letsencrypt-logs" rootfs/etc/cont-init.d/
Length of output: 121
Script:
#!/bin/bash
# Let's first find all initialization scripts in the repository
fd "cont-init.d" --type d
# Then search for any file that creates the logs directory
rg -l "mkdir.*ssl"
Length of output: 366
Script:
#!/bin/bash
# Let's check the content of file-structure.sh to verify log directory creation
cat letsencrypt/rootfs/etc/cont-init.d/file-structure.sh
Length of output: 7292
Log Directory is now stored in /ssl/letsencrypt-logs to make it easier for the users to get access max_logs_backup is now a configurable option with a default of 100 Log Files (1000 if not specified)
|
Hm, we do log and store the output of the add-on. Does the Certbot internal logs have additional information then what is shown in the output? |
|
Yes they do, I should have mentioned that in the PR. |
I wonder if we could make it output all information on the console 🤔 There are the verbosity levels. Maybe we could just make this an add-on setting, and then the user can just run the add-on on sees the error there? I think this would be more user friendly. In a way, we want to logs all over the place. Having it just part of the regular add-on log would be kinda nice IMHO. Especially, now, with the new add-on logs where you can use the full history of the journald based log you can go back quite significantly. That said, if something logs massive amount of data, you'll loose the add-on log at one point. |
I had struggles to get access to the certbot Logs, the only way i found was with an Backup and there in the Data dir of the addon.
So I changed the Log Directory to /ssl/letsencrypt-logs to make it easier for the users to get access.
I also added max_logs_backup as a configurable option with a default of 100 Log Files.
If it is not specified certbot creates 1000 Files.
Summary by CodeRabbit
Release Notes
New Features
Documentation