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

Dual certificate support (eg ECDSA with RSA fallback) #1801

Conversation

polarathene
Copy link
Member

@polarathene polarathene commented Feb 9, 2021

Description

Continuation of my original PR: #1629

TL;DR: This feature enables support for multiple certificates, eg for serving a modern ECDSA cert with an RSA cert as fallback.

See the original PR and/or commit messages for added details. Additionally (from original PR), there's also fixes for bugs related to modifying internal cert configs.

NOTE: This does not enable support for ECDSA certificates. Only the ability to serve a primary certificate with a fallback (in future there will be EdDSA that differs from ECDSA which would in future replace RSA as a fallback). ECDSA support is dependent upon a separate PR updating the TLS ciphersuites.

Potential blockers:

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (non-breaking change that does improve existing functionality)
  • This change requires a documentation update

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
  • I have made corresponding changes to the documentation (README.md or ENVIRONMENT.md or the Wiki)
  • 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

@wernerfred wernerfred added area/enhancement kind/new feature A new feature is requested in this issue or implemeted with this PR meta/needs triage This issue / PR needs checks and verification from maintainers labels Feb 9, 2021
@georglauterbach
Copy link
Member

Tests obviously need some adjustments as they also check some thing related to TLS levels and cipher suites. But that shouldn't be a big deal - just changing the defaults in the tests to use the new, appropriate and updated suites and levels.

@polarathene
Copy link
Member Author

polarathene commented Feb 9, 2021

For tests I need advice on existing tests that need to be updated or added. I'm not familiar with the testing infrastructure, and it's directory is full of many directories and files.


One of my commits updates a test for letsencrypt:

@test "checking ssl: letsencrypt configuration is correct"
#test domain has certificate files
#test hostname has certificate files

For both of those it removes two separate tests (that individually configure key or cert files), and updates to the new config setting (smtpd_tls_chain_files, postfix only) which takes a sequence of files containing the private key and cert.

Perhaps I need to write something here that does similar for the fallback certificate? (postfix uses the same new config setting but includes both primary and fallback, dovecot has two new config settings specifically for fallback cert)


Tests obviously need some adjustments as they also check some thing related to TLS levels and cipher suites.

I assume this is about ECDSA cipher suites?

That's fair, but as this doesn't enable those cipher suites (my other PR does), I think tests for this just need to pass the current certificate related tests and add some new tests that somehow test the fallback support is working? (basically, if the client doesn't accept ECDSA, they should still be able to accept RSA certificate with cipher suites using RSA digital signature)

just changing the defaults in the tests to use the new, appropriate and updated suites and levels.

I think you got this mixed up with my other PR? This one is just about dual certificate support (and fixes / improvements on start-mailserver.sh config issues).

Perhaps getting the related PR merged first is a good idea? This PR probably can't be properly tested without it.

@polarathene
Copy link
Member Author

Regarding the removal of the -combined.pem file for Dovecot when using self-signed.

I've found that it was added to generate-ssl-certificate in 2015 when Courier was used (required a fullchain bundle file) instead of Dovecot. When Dovecot replaced it, -combined.pem was still used instead of -cert.pem like Postfix was using. No additional action required, just documenting the change better.


generate-ssl-certificate appears to also have comments and roughly the same commands as described in the Postfix docs for setting up a private CA.

cacert.pem does not appear to be used for anything else beyond postfix with self-signed (Dovecot also can support specifying a CA, but when it replaced Courier.. this was missed). I don't think it's required that a self-signed CA be added to sign a single self-signed certificate. This is probably legacy as well and can be dropped.

By doing so, we could remove the generate-ssl-certificate file/command which requires user interaction for adding details, but as self-signed mode is strictly meant for non-production usage, we shouldn't require those details to be accurate. manual mode can be used by someone who insists otherwise.

There's a test TODO task related to self-signed / generate-ssl-certificate:

# TODO: To create a test generate-ssl-certificate must be non interactive
#@test "checking setup.sh: setup.sh config ssl" {
# run ./setup.sh -c mail_ssl config ssl
# assert_success
#}

Should we remove that along with the setup.sh command config ssl? The cert generation code could be migrated to start-mailserver.sh and live there instead for self-signed mode, implicitly setting up the certificate.


These changes could be deferred to another PR if that's more suitable.

@georglauterbach
Copy link
Member

georglauterbach commented Feb 10, 2021

I think you got this mixed up with my other PR?

Indeed.

Perhaps getting the related PR merged first is a good idea?

Indeed as well. I will mark this as a draft again :)

Perhaps I need to write something here that does similar for the fallback certificate?

I guess it's worth a shot:) Testing it locally should be quite easy: test/bats/bin/bats <THE BATS TEST FILE>. If it's the large file, just comment the other tests or delete them and revert this change later with GIT, just for testing.


Should we remove that along with the setup.sh command config ssl?

I think this would be a good idea, since we are going to publish this with version 8.1.0 anyway. A bump in minor version can justify this change.

These changes could be deferred to another PR if that's more suitable.

If it's more comfortable for you.


About the tests: Is there anything you have a question about? The infrastructure is best-described here: #1643 (comment)

@georglauterbach georglauterbach marked this pull request as draft February 10, 2021 16:50
@georglauterbach georglauterbach changed the title FEAT: Dual certificate support (eg ECDSA with RSA fallback) Dual certificate support (eg ECDSA with RSA fallback) Feb 11, 2021
@georglauterbach georglauterbach marked this pull request as ready for review February 18, 2021 09:25
@georglauterbach
Copy link
Member

@polarathene Out of draft state since #1802 was merged :)

polarathene and others added 5 commits February 18, 2021 22:49
Since Postfix 3.4, `smtpd_tls_cert_file` and `smtpd_tls_key_file` have been deprecated in favor of `smtpd_tls_chain_files` which supports a list of values where a single or sequence of file paths provide a private key followed by it's certificate chain.
`smtpd_tls_chain_files` allows for multiple key+cert bundles so that you can provide different key types, such as ECDSA and RSA.

To maintain compatibility with the current CERT/KEY ENV vars only a 2nd certificate is supported.

Since Dovecot 2.2.31 a related feature is also available, but it is limited to only providing one alternative certificate via separate cert and key settings.

---

This feature enables support for multiple certificates, eg for serving modern ECDSA certs with RSA as fallback.
@polarathene polarathene force-pushed the feat/hybrid-cert-support-refactor branch from ebad93a to 3018edd Compare February 18, 2021 09:53
@georglauterbach
Copy link
Member

What do you think @polarathene, how fast can you get this to be ready for merging? We are assessing whether this should be in v9.0.0 or not.

@polarathene
Copy link
Member Author

how fast can you get this to be ready for merging?

I just need to look into why tests are failing:

Failing test snippets
not ok 80 checking postfix: remove privacy details of the sender
# (in test file test/mail_privacy.bats, line 43)
#   `docker exec mail_privacy /bin/sh -c "openssl s_client -quiet -starttls smtp -connect 0.0.0.0:587 < /tmp/docker-mailserver-test/email-templates/send-privacy-email.txt"' failed
# 140464342774912:error:14094410:SSL routines:ssl3_read_bytes:sslv3 alert handshake failure:../ssl/record/rec_layer_s3.c:1544:SSL alert number 40
@test "checking postfix: remove privacy details of the sender" {
  docker exec mail_privacy /bin/sh -c "openssl s_client -quiet -starttls smtp -connect 0.0.0.0:587 < /tmp/docker-mailserver-test/email-templates/send-privacy-email.txt"

 not ok 139 checking saslauthd: ldap smtp authentication
# (from function `assert_success' in file test/test_helper/bats-assert/src/assert.bash, line 114,
#  in test file test/mail_with_ldap.bats, line 210)
#   `assert_success' failed
# 
# -- command failed --
# status : 1
# output : 140615162684544:error:14094410:SSL routines:ssl3_read_bytes:sslv3 alert handshake failure:../ssl/record/rec_layer_s3.c:1544:SSL alert number 40
# --
# 
@test "checking saslauthd: ldap smtp authentication" {
  run docker exec mail_with_ldap /bin/sh -c "nc -w 5 0.0.0.0 25 < /tmp/docker-mailserver-test/auth/sasl-ldap-smtp-auth.txt | grep 'Authentication successful'"
  assert_success
  run docker exec mail_with_ldap /bin/sh -c "openssl s_client -quiet -connect 0.0.0.0:465 < /tmp/docker-mailserver-test/auth/sasl-ldap-smtp-auth.txt | grep 'Authentication successful'"
  assert_success

 not ok 267 checking system: /var/log/mail/mail.log is error free
# (from function `assert_failure' in file test/test_helper/bats-assert/src/assert.bash, line 140,
#  in test file test/tests.bats, line 516)
#   `assert_failure' failed
# 
# -- command succeeded, but it was expected to fail --
# output : Feb 18 10:13:27 mail postfix/submission/smtpd[3651]: warning: TLS library problem: error:1417A0C1:SSL routines:tls_post_process_client_hello:no shared cipher:../ssl/statem/statem_srvr.c:2259:
# --
# 
@test "checking system: /var/log/mail/mail.log is error free" {
  run docker exec mail grep 'non-null host address bits in' /var/log/mail/mail.log
  assert_failure
  run docker exec mail grep 'mail system configuration error' /var/log/mail/mail.log
  assert_failure
  run docker exec mail grep ': error:' /var/log/mail/mail.log
  assert_failure

not ok 312 checking setup.sh: setup.sh dkim help
# (from function `assert_success' in file test/test_helper/bats-assert/src/assert.bash, line 114,
#  in test file test/tests.bats, line 1132)
#   `assert_success' failed
# 
# -- command failed --
# status : 1
# output (65 lines):
#   setup.sh Bootstrapping Script
#   
#   Usage: ./setup.sh [-i IMAGE_NAME] [-c CONTAINER_NAME] <subcommand> <subcommand> [args]
#   
#   OPTIONS:
#   
#     -i IMAGE_NAME     The name of the docker-mailserver image
#                       The default value is
#                       'docker.io/mailserver/docker-mailserver:latest'
#   
#     -c CONTAINER_NAME The name of the running container.
#   
#     -p PATH           Config folder path (default: /home/runner/work/docker-mailserver/docker-mailserver/config)
#   
#     -h                Show this help dialogue
#   
#     -z                Allow container access to the bind mount content
#                       that is shared among multiple containers
#                       on a SELinux-enabled host.
#   
#     -Z                Allow container access to the bind mount content
#                       that is private and unshared with other containers
#                       on a SELinux-enabled host.
#   
#   SUBCOMMANDS:
#   
#     email:
#   
#       ./setup.sh email add <email> [<password>]
#       ./setup.sh email update <email> [<password>]
#       ./setup.sh email del [OPTIONS] <email>
#       ./setup.sh email restrict <add|del|list> <send|receive> [<email>]
#       ./setup.sh email list
#   
#     alias:
#   
#       ./setup.sh alias add <email> <recipient>
#       ./setup.sh alias del <email> <recipient>
#       ./setup.sh alias list
#   
#     quota:
#   
#       ./setup.sh quota set <email> [<quota>]
#       ./setup.sh quota del <email>
#   
#     config:
#   
#       ./setup.sh config dkim [keysize <size>] [domain '<domain1.tld>[,<domain2.tld>...]']
#       ./setup.sh config ssl <fqdn>
#   
#     relay:
#   
#       ./setup.sh relay add-domain <domain> <host> [<port>]
#       ./setup.sh relay add-auth <domain> <username> [<password>]
#       ./setup.sh relay exclude-domain <domain>
#   
#     debug:
#   
#       ./setup.sh debug fetchmail
#       ./setup.sh debug fail2ban [<unban> <ip-address>]
#       ./setup.sh debug show-mail-logs
#       ./setup.sh debug inspect
#       ./setup.sh debug login <commands>
#   
#     help: Show this help dialogue
# --
# 
@test "checking setup.sh: setup.sh dkim help" {
  run ./setup.sh -c mail dkim help
  assert_success

not ok 325 checking postfix submission: only A grade TLS ciphers are used
# (from function `assert_success' in file test/test_helper/bats-assert/src/assert.bash, line 114,
#  in test file test/tests.bats, line 1260)
#   `assert_success' failed
# 
# -- command failed --
# status : 1
# output : 
# --
# 
# postfix submission TLS
@test "checking postfix submission: only A grade TLS ciphers are used" {
  run docker run --rm -i --link mail:postfix \
    --entrypoint sh instrumentisto/nmap -c \
      'nmap --script ssl-enum-ciphers -p 587 postfix | grep "least strength: A"'
  assert_success
}

ok 326 checking postfix submission: nmap produces no warnings on TLS ciphers verifying
not ok 327 checking postfix smtps: only A grade TLS ciphers are used
# (from function `assert_success' in file test/test_helper/bats-assert/src/assert.bash, line 114,
#  in test file test/tests.bats, line 1276)
#   `assert_success' failed
# 
# -- command failed --
# status : 1
# output : 
# --
# 
@test "checking postfix submission: nmap produces no warnings on TLS ciphers verifying" {
  run docker run --rm -i --link mail:postfix \
    --entrypoint sh instrumentisto/nmap -c \
      'nmap --script ssl-enum-ciphers -p 587 postfix | grep "warnings" | wc -l'
  assert_success
  assert_output 0
}

# postfix smtps SSL
@test "checking postfix smtps: only A grade TLS ciphers are used" {
  run docker run --rm -i --link mail:postfix \
    --entrypoint sh instrumentisto/nmap -c \
      'nmap --script ssl-enum-ciphers -p 465 postfix | grep "least strength: A"'
  assert_success
}

Other than that, adding another cipher list test shouldn't be an issue. Just need to handle the Dovecot concerned mentioned above and add env var docs. Should be good before monday?

I'm not sure why these tests are failing however.. I'll look into them but any assistance would be helpful. eg for the last two tests that test 'nmap --script ssl-enum-ciphers -p <port> postfix | grep "least strength: A"' (where <port> is 587 or 465), I'm unable to reproduce this error out of the CI at present.

The tests.bats file is fairly large, and the initialization in setup() is a fair bit, I haven't reproduced that locally yet, perhaps I'll run the test file with all other test cases stripped.

@polarathene
Copy link
Member Author

polarathene commented Feb 19, 2021

Slimmed tests.bats down to just the failing test cases, and checked docker logs mail while that ran to see if anything useful was there:

mail postfix/smtpd[2831]: warning: error opening chain file: /etc/ssl/certs/ssl-fullkeychain-snakeoil.pem: No such file or directory

I guess that means my PR is at fault under a certain configuration 😅

tests.bats doesn't have SSL_TYPE setup? And start-mailserver.sh doesn't have any default, as the ENV docs indicate. Are the tests really meant to run without SSL setup?

@polarathene
Copy link
Member Author

This was due to my config change of:

smtpd_tls_cert_file=/etc/ssl/certs/ssl-cert-snakeoil.pem
smtpd_tls_key_file=/etc/ssl/private/ssl-cert-snakeoil.key

with non-existent file:

smtpd_tls_chain_files = /etc/ssl/certs/ssl-fullkeychain-snakeoil.pem

I thought that by the name snakeoil these were fake files that didn't actually exist, and if they did that they weren't sourced officially from somewhere. Turns out they are, but they're still not intended to be used in production.

Technically this failure should be valid, and TLS shouldn't be enabled as the start-mailserver.sh indicates should be the case if no SSL_TYPE is given.

Should probably change the handling from ssl=required to ssl=yes, to instead change to ssl=no for dovecot, and similarly disable TLS for postfix too?

That or I can use these snakeoil files as they were handled before? (and perhaps outputting a clear warning about insecure setup, since the current log for this case doesn't imply a bad SSL setup).

@georglauterbach
Copy link
Member

Should probably change the handling from ssl=required to ssl=yes, to instead change to ssl=no for dovecot, and similarly disable TLS for postfix too?

That or I can use these snakeoil files as they were handled before? (and perhaps outputting a clear warning about insecure setup, since the current log for this case doesn't imply a bad SSL setup).

TBH, I don't know what's better. I would leave it up to you to decide. This change can be breaking too, since it will be included in the next major release v9.0.0.

@polarathene
Copy link
Member Author

polarathene commented Feb 20, 2021

Not sure what's going on here but it seems ClamAV in setup() is failing. I can see that memory usage climbs up to 2GB+ (from ~300MB), then it drops and this failure output is given:

./test/bats/bin/bats test/tests.bats
 ✗ 
   (from function `repeat_until_success_or_timeout' in file test/test_helper/common.bash, line 37,
    from function `repeat_in_container_until_success_or_timeout' in file test/test_helper/common.bash, line 70,
    from function `setup_file' in test file test/tests.bats, line 53)
     `repeat_in_container_until_success_or_timeout 60 mail test -e /var/run/clamav/clamd.ctl' failed
   26d3295fc834e4e959f5f4959439240de6958f36f0804f1c81583ab98b7969b1
   [ TASKLOG ]  mail.my-domain.com is up and running
   Timed out on command: docker exec mail test -e /var/run/clamav/clamd.ctl
   mail
   bats warning: Executed 1 instead of expected 6 tests

6 tests, 1 failure, 5 not run

It looks like a timeout is one of the expected outcomes, but this is failing the tests before they run for me, as this is run in setup(), is that intentional? I'm not sure of it's relevance for the test cases in tests.bats.


I had to disable this one and the final setup() method wait_for_empty_mail_queue_in_container mail. All tests passed with the PR related change I made. I'll see how it fares with the CI I guess..

I did notice /var/log/mail/mail.log revealed the issue with clamav: Feb 20 06:27:53 mail amavis[1136]: (01136-01) (!)connect to /var/run/clamav/clamd.ctl failed, attempt #1: Can't connect to a UNIX socket /var/run/clamav/clamd.ctl: No such file or directory

That may be related to this, assuming there are no tests to check ClamAV is working properly, we might be shipping an image that has non-functional ClamAV, or something with the CI builds differ from creating a fresh build that causes this (caching?).

When `SSL_TYPE` isn't properly setup, we're still offering SSL connections but not warning in logs about the insecurity of such, or why a misconfiguration may have occurred.

This commit more clearly communicates to the user that they should look into the issue before considering deploying to production.

The `TODO` comments communicate to any future maintainer to consider treating these improper configs as disabling TLS instead.
I mistakenly thought this was placeholder text, which broke some tests. This adds the two files in the correct order (private key followed by cert/chain), to fix that issue.
Certain scenarios may persist state of previously configured alt cert via ENV vars that are removed from a future run. If the config is not reset to original immutable state, this will correctly disable the config from using alt cert unintentionally.
@georglauterbach
Copy link
Member

georglauterbach commented Feb 20, 2021

Not sure what happened here, but at least on 8.0.1 ClamAV works. I will test on :edge now.

EDIT: Seems to work on :edge too. I just tested with supervisorctl status clamav and checked whether /Var/run/clamav/clamd.ctl is there.

By switching from string var to array / list expansion, this better stores the extracted result and applies it in a manner that ShellCheck linting approves, removing the need to disable the rule.
Few tweaks to the test script allows re-purposing it for covering dual cert support as well.
@polarathene
Copy link
Member Author

polarathene commented Feb 20, 2021

EDIT: Seems to work on :edge too. I just tested with supervisorctl status clamav and checked whether /Var/run/clamav/clamd.ctl is there.

This is not building locally right? (git clone of repo, init with submodules, docker build -t mailserver-testing:ci ., running the make command to prep testing data and running tests.bats against the newly built image)

I assume that there must be something different with the CI environment.


Test is updated, sorting out docs tomorrow (it's late here), might have a few other minor changes, then I'll add some release note snippets for you and we'll be good for review and merge :)

@georglauterbach
Copy link
Member

This is not building locally right? (git clone of repo, init with submodules, docker build -t mailserver-testing:ci ., running the make command to prep testing data and running tests.bats against the newly built image)

You're probably missing build arguments. Have a look at make build, which essentially wraps docker build ....

I assume that there must be something different with the CI environment.

Not really. CI executes make build in the same way.

you and we'll be good for review and merge :)

Very nice.

@polarathene
Copy link
Member Author

polarathene commented Feb 20, 2021

You're probably missing build arguments. Have a look at make build, which essentially wraps docker build ....

Nope, like I said I ran the make command as the CI seems to, I don't see anything missing there. I'll investigate another time.

Not really. CI executes make build in the same way.

Seems other users had the issue in the past, I was testing with a 1GB VPS that had plenty of swap but perhaps I need to spin up a larger VPS instance to confirm if it was memory pressure related.


Related to this PR, SSL_TYPE=custom came before SSL_TYPE=manual in the same year (2016). Prior there was only letsencrypt (Dec 2015) and self-signed (original, with improvements in Aug 2015, including generate-ssl-certificate function) support.

Also in 2016, letsencrypt mode added support for simp_le (fork from original that ceased maintenance in 2016); I'll need to investigate further for future PR, but I'm not sure about this support being useful (additionally a 2020 PR introduced the _extract_certs_from_acme method, specifically to cater to traefik v2, just a note for me).


I'm going to look into refactoring / consolidating the SSL_TYPE handling in a future PR. I think we have some unnecessary redundancy shoved in complicating things for no reason and either no one was bothered enough or not comfortable touching this area of the script so I'll be happy to tackle that :)

Like wise, the Wiki - Configure SSL docs aren't being touched with this PR (not that they can be in a PR atm). I'll be tackling the wiki migration into this repo for static doc gen and hosting next, after that I'll revise the SSL docs.

A little reorganization, mostly placing private key ahead of related cert lines.
This should make the parameters a little less confusing.

Previously was 3 parameters, but the Postfix parameter (1st) may look like two variables if you don't pay attention to the surrounding quotes; while the Dovecot parameters (2nd + 3rd) would have an opposing order. There was also a variant where the `FULLKEYCHAIN` var was passed in three times.

Now it's two params, with the 2nd param as an optional one. If the 2nd param is provided, then the two params are in the order of private key then certificate, otherwise if only a single parameter it's a single PEM file with the full cert chain and private key bundled.

This avoids implying that Postfix and Dovecot might use different files.
Inlined for the benefit of anyone else maintaining this section if I'm unable to address the concerns within my own time.
`TLS_LEVEL=old` isn't in the codebase anymore, not likely to be relevant to retain.

No point in documenting what is considered invalid / unsupported config value in the first place for `SSL_TYPE`.

`SSL_TYPE=manual` was missing documentation for both related file path ENV vars, they've been added along with their alt fallback variants.
Not sure how relevant this is, the file isn't complete sync with the main dovecot `10-ssl.conf` config, adding the support just in case.
There doesn't appear to be a standardized name for this type of file bundle, and `keychain` may be misleading (fullkeychain often provides macOS keychain  results on search engines).

Opting for a more explicit `KEY_WITH_FULLCHAIN` name instead.
@georglauterbach
Copy link
Member

SGTM

`_set_certificate` refactor commit accidentally changed a var name and committed that breaking the dual cert support (thanks tests!).
Proper test return values instead of `wc -l` based checking.

Tests with dual cert support active, tests that feature (to better detect failure case.

Third test case was unable to verify new self-signed certificate, added new certs signed with self-signed root CA.

Adjusted openssl `CApath` parameter to use `CAfile` instead as `letsencrypt` cert was replaced thus CA cert is missing from the system trust store.
Fixes lint error.

Also realized I was accidentally asserting a file exists in the test environment, not within the container.

Resolved that and also added an additional test case to ensure the ENV var files are valid when passed in, in the event a change misconfigures them and that the issue is identified earlier.
@polarathene
Copy link
Member Author

Should be good to go now 👍

Reviewing may be easier via commits if the full PR diff is difficult to work through. I personally like Gitkraken as a GUI where you can easily navigate through the commits or select multiple commits at once to view an aggregated diff with different viewing layouts.


Release Notes

  • [fix] TLS: For some docker-compose setups when restarting the docker-mailserver container, internal config state may have been persisted despite making changes that should reconfigure TLS (eg changing SSL_TYPE or replacing the certificate file).
  • [feat] TLS: Dual (aka hybrid) certificate support! (eg ECDSA certificate with an RSA fallback for broader compatibility)
    • This feature is presently only for SSL_TYPE=manual, all you need to do is provide your fallback certificate to the SSL_ALT_CERT_PATH and SSL_ALT_KEY_PATH ENV vars, just like your primary certificate would be setup for manual mode.

@georglauterbach
Copy link
Member

georglauterbach commented Feb 21, 2021

Other than a few styling and apostrophes issues, this LGTM. Just correct them and this can be merged.

Better format some strings that had mixed quotes when they weren't necessary.

Additionally DRYed up the config path for Postfix and Dovecot within the `_setup_ssl` method.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/new feature A new feature is requested in this issue or implemeted with this PR meta/needs triage This issue / PR needs checks and verification from maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants