Skip to content

fix: Restore detection of letsencrypt certificate file changes#2326

Merged
polarathene merged 8 commits intomasterfrom
fix/monitor-letsencrypt-paths
Dec 18, 2021
Merged

fix: Restore detection of letsencrypt certificate file changes#2326
polarathene merged 8 commits intomasterfrom
fix/monitor-letsencrypt-paths

Conversation

@polarathene
Copy link
Copy Markdown
Member

@polarathene polarathene commented Dec 16, 2021 •
edited
Loading

Description

The DYNAMIC_FILES var was quote wrapped, treating all filepaths to create checksums for as a single string that would be ignored instead of processed individually.

Removed the quotes, and changed the for loop to an array which accomplishes the same goal.

This was discovered while investigating #2321, but it's unclear if this will fix that issue.

This bug was introduced in #2279 with the v10.3.0 release.

We may want to branch that release tag and cherry pick this commit from master to publish a bugfix (as we've already merged v11 changes to master, the bugfix branch is the only way to backport it I think?). (RESOLVED: Decision was to release in v10.4.0 with the Debian upgrade)

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • If necessary I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

It'd probably be a good idea to add a test that verifies the cert path is in the checksum file, I'm just a bit pressed on time and resources currently but wanted to get this fix raised while I could spare the time.

If there's no rush to merge I can sort out tests within 7 days depending on how events pan out on my end 😅

@polarathene
fix: Restore detection of letsencrypt certificate file changes
1f0b71d 
The `DYNAMIC_FILES` var was quote wrapped, treating all filepaths to create checksums for as a single string that would be ignored instead of processed individually.

Removed the quotes, and changed the for loop to an array which accomplishes the same goal.
@polarathene polarathene self-assigned this Dec 16, 2021
@polarathene polarathene added this to the v11.0.0 milestone Dec 16, 2021
@polarathene
chore: Appease the lint gods
080a7f0 
`SC2068: Double quote array expansions to avoid re-splitting elements.`
@georglauterbach

This comment has been minimized.

Sign in to view
@georglauterbach

This comment has been minimized.

Sign in to view
@wernerfred

This comment has been minimized.

Sign in to view
@casperklein

This comment has been minimized.

Sign in to view
@wernerfred

This comment has been minimized.

Sign in to view
@polarathene

This comment has been minimized.

Sign in to view
@polarathene

This comment has been minimized.

Sign in to view
@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented Dec 17, 2021 •
edited
Loading

The GitHub WebUI does not support pre-commit hooks, correct.

I do not regard the Debian 11 commit as a necessity for a new major, so I'm very much fine with either

  1. just merging this on top, (waiting one more week and) releasing 10.4 (as a Christmas gift for sysadmins to complain about) and calling it a day (but feeling like Santa himself)

  2. or reverting the Debian 11 commit, merging this, releasing 10.3.1 and then re-reverting the Debian commit.

@wernerfred
Copy link
Copy Markdown
Member

wernerfred commented Dec 17, 2021 •
edited
Loading

as a Christmas gift for sysadmins to complain about

😆 😆 😆

@polarathene

This comment has been minimized.

Sign in to view
@polarathene polarathene marked this pull request as draft December 17, 2021 20:27
@polarathene
tests(fix): changedetector - Increase log lines scanned
8afd05a 
Failure is due to log truncation, increase size to 3000 bytes (arbitrary value that is large enough) like already used in this test file to resolve the failure.
@polarathene
fix: Ensure _strip_wildcard_prefix handles non-wildcard
b5e1d8a 
If the value provided does not have the wildcard prefix `*.`, then no value was returned.

`SSL_DOMAIN` does not have to have the wildcard prefix, nor should the method (with the current name and behaviour) output no result when used on FQDN without wildcard prefix. This would otherwise cause some unexpected results, which could hang some logic.
@polarathene
fix: Prevent unnecessary change detection event
0cd375a 
`acme.json` change would extract new cert files, which would then be hashed after restarting services and considered a change event, running through the logic again and restarting services once more when that was not required.

The checksum entries for those cert files are now replaced with new entries containing updated checksum hashes, after `acme.json` extraction.
@polarathene
chore: Appease the lint gods
e72af7e
@polarathene
shame: lint locally
e2222de
@polarathene
Copy link
Copy Markdown
Member Author

polarathene commented Dec 18, 2021 •
edited
Loading

Tests resolved, fixed another bug (didn't previously affect any tests as the method was only ever used with SSL_DOMAIN to test against documented wildcard prefix).

Refactored the check-for-changes.sh logic for acme.json a bit by removing the unnecessary for loop and switch statement. Just uses a regex conditional now. View diff with white-space hidden to reduce noise.

  • Cert files extracted for acme.json now have their hashes re-calculated afterwards to avoid the redundant change event that they otherwise triggered. Using sha512sum is run without subshell + exec + stderr redirect, I think that's fine?
  • The helper method _strip_wildcard_prefix now returns the original input if it didn't have a wildcard prefix to remove. Without that, the usage with CERT_DOMAIN and no SSL_DOMAIN filtering caused the logic to hang, likewise if SSL_DOMAIN with a wildcard was not stripped. I assume something went wrong with sed or sha512sum with the *..
  • Change detection test increased the amount of service log checked, the amount (3000 bytes) is matching an existing value used elsewhere in that test file which is more than enough.
  • The sed command could use the usual / pattern delimiter, originally I had the base filepath instead of a var, so to avoid escaping / from the filepath, I learned that you can substitute the delimiter with another character for any sed pattern matching.

The two tests passed locally, I expect all tests to be good now 👍

I'll add the warning for 10.3.0 release not reacting to cert file changes and prep a 10.4.0 release. (All done! 😀 )

@polarathene polarathene marked this pull request as ready for review December 18, 2021 01:28
@polarathene polarathene modified the milestones: v11.0.0, v10.4.0 Dec 18, 2021
georglauterbach
georglauterbach approved these changes Dec 18, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@georglauterbach georglauterbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

I will have a look at the if-then-else thing today.

Comment thread target/scripts/helper-functions.sh
@polarathene polarathene merged commit 6d06149 into master Dec 18, 2021
@polarathene polarathene deleted the fix/monitor-letsencrypt-paths branch December 18, 2021 22:25
@casperklein casperklein mentioned this pull request Feb 8, 2022
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@casperklein casperklein casperklein approved these changes

@georglauterbach georglauterbach georglauterbach approved these changes

Assignees

@polarathene polarathene

Labels

area/security priority/high

Projects

None yet

Milestone

v10.4.0

Development

Successfully merging this pull request may close these issues.

4 participants

@polarathene @georglauterbach @wernerfred @casperklein