Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: letsencrypt implicit location discovery #2525

Merged

Conversation

polarathene
Copy link
Member

@polarathene polarathene commented Apr 3, 2022

Description

Small refactor to better support check-for-changes.sh functionality.

These changes should allow some additional test coverage to be enabled. This implements Option B I described in comment for a PR trying to enable that additional coverage.

  • Changes to ssl.sh is minimal, effectively lifting the logic out of _setup_ssl to separate methods so that check-for-changes.sh can use one.
  • check-for-changes.sh is primarily simplifying the logic by dropping the loop and inner conditional statement (so some lines only have indentation changes), calling _setup_ssl and _find_letsencrypt_domain instead. A separate commit improves relevant maintainer comments.
  • Unnecessary to this PR is wrapping the change detection into a function that the loop calls instead of inlined. I did this to use local annotation.
All commit messages for easy reference

chore: Extract letsencrypt logic into methods
This allows other scripts to share the functionality to discover the correct letsencrypt folder from the 3 possible locations (where specific order is important).

As these methods should now return a string value, the return 1 after a panic is now dropped.

The _find_letsencrypt_key method can take a domain as an arg, otherwise it will retrieve the internal domain the same way as a fallback, which is slightly inefficient to repeat but should be a non-issue. (No longer the case)

chore: Update comments
The todo is resolved with this PR; _setup_ssl will be called by both cert conditional statements, the purpose for each is better documented for maintainers.

refactor: Defer most logic to helper/ssl.sh
The loop is no longer required, extraction is delegated to _setup_ssl now.

For the change event prevention, we retrieve the relevant FQDN via the new helper method, beyond that it's just indentation diff.

chore: Appease the lint gods
The lint warning for the unused optional parameter could alternatively be resolved by prepending #shellcheck disable=SC2120 to the function definition.
As there are no other consumers, I changed to a required arg with a hard failure instead of providing a fallback.

check-for-changes.sh adjusted to allow locally scoped var declarations by wrapping a function. Presently no loop control flow is needed so this seems fine. Made it clear that CHANGED is local and CHKSUM_FILE is not.

The panic scope arg doesn't need SSL_TYPE for added context, it's clearlyletsencrypt, removed.

Type of change

  • Improvement (non-breaking change that does improve existing functionality)

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
  • New and existing unit tests pass locally with my changes

This allows other scripts to share the functionality to discover the correct letsencrypt folder from the 3 possible locations (where specific order is important).

As these methods should now return a string value, the `return 1` after a panic is now dropped.

The `_find_letsencrypt_key` method can take a domain as an arg, otherwise it will retrieve the internal domain the same way as a fallback, which is slightly inefficient to repeat but should be a non-issue.
The todo is resolved with this PR, `_setup_ssl` will be called by both cert conditional statements with purpose for each better documented to maintainers at the start of the logic block.
The loop is no longer required, extraction is delegated to `_setup_ssl` now.

For the change event prevention, we retrieve the relevant FQDN via the new helper method, beyond that it's just indentation diff.
Optional parameter warning could alternative be resolved by prepending `#shellcheck disable=SC2120` to the function definition. As there is no other consumer presently, I adjusted to a required arg with hard failure instead of fallback.

`check-for-changes.sh` adjusted to allow locally scoped var declarations by wrapping a function. Presently no loop control flow is needed so this seems fine. Made it clear that `CHANGED` is local and `CHKSUM_FILE` is not.

Panic scope doesn't require `SSL_TYPE` for context, it's clearly`letsencrypt`.
@georglauterbach georglauterbach added area/scripts kind/improvement Improve an existing feature, configuration file or the documentation labels Apr 3, 2022
@georglauterbach georglauterbach added this to the v11.0.0 milestone Apr 3, 2022
Now that the service configs are properly updated, when the services restart they will return a cert with the SAN `DNS:*.example.test`,  which is valid for `mail.example.test`, however the test function did not properly account for this in the regexp query.

Resolved by truncating the left-most DNS label from FQDN and adding a third check to match a returned wildcard DNS result.

Extracted out the common logic to create the regexp query and renamed the methods to be communicate more clearly that they check the FQDN is supported, not necessarily explicitly listed by the cert.
These will now pass. Adjusted comments accordingly.


Added an additional test on a fake FQDN that should still be valid to a wildcard cert (SNI validation in a proper setup would reject the connection afterwards).
@polarathene

This comment was marked as resolved.

@polarathene
Copy link
Member Author

I have enabled the related tests in this PR but can revert that if preferred?

I did so due to renaming the helper method and reworking the comments.

@georglauterbach
Copy link
Member

I have enabled the related tests in this PR but can revert that if preferred?

I did so due to renaming the helper method and reworking the comments.

That's not a problem. I will just close #2524.

Copy link
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

@polarathene
Copy link
Member Author

polarathene commented Apr 18, 2022

@georglauterbach as the v11 release PR has been open for testing, do you still want this merged for v11 or for me to wait until after that release?


From release PR:

During this testing period, only PRs that do not change functionality will be merged.

This doesn't really change any functionality, other than a refactor to enable some extra tests and supporting a TLS cert edge case in check-for-changes.sh that those tests cover.

@georglauterbach
Copy link
Member

#2537 is currently only WIP. I would very much like to see this PR go into v11. Therefore, I will update the branch and you can then merge it straight away :D

@polarathene polarathene merged commit 1b1877f into docker-mailserver:master Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/scripts kind/improvement Improve an existing feature, configuration file or the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants