Dual certificate support (eg ECDSA with RSA fallback) #1801
Conversation
wernerfred
added
area/enhancement
kind/new feature
|
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. |
|
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:
For both of those it removes two separate tests (that individually configure key or cert files), and updates to the new config setting ( 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)
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)
I think you got this mixed up with my other PR? This one is just about dual certificate support (and fixes / improvements on Perhaps getting the related PR merged first is a good idea? This PR probably can't be properly tested without it. |
|
Regarding the removal of the I've found that it was added to
By doing so, we could remove the There's a test TODO task related to docker-mailserver/test/tests.bats Lines 1443 to 1447 in f168e3b Should we remove that along with the These changes could be deferred to another PR if that's more suitable. |
Indeed.
Indeed as well. I will mark this as a draft again :)
I guess it's worth a shot:) Testing it locally should be quite easy:
I think this would be a good idea, since we are going to publish this with version
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
changed the title
|
@polarathene Out of draft state since #1802 was merged :) |
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.
Improved some comments too.
polarathene
force-pushed
the
feat/hybrid-cert-support-refactor
branch
from
ebad93a to
3018edd
Compare
|
What do you think @polarathene, how fast can you get this to be ready for merging? We are assessing whether this should be in |
I just need to look into why tests are failing: Failing test snippets@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"@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@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@test "checking setup.sh: setup.sh dkim help" {
run ./setup.sh -c mail dkim help
assert_success# 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
}@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 The |
|
Slimmed
I guess that means my PR is at fault under a certain configuration 😅
|
|
This was due to my config change of: with non-existent file: I thought that by the name Technically this failure should be valid, and TLS shouldn't be enabled as the Should probably change the handling from 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 |
|
Not sure what's going on here but it seems ClamAV in ./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 runIt 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 I had to disable this one and the final I did notice 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.
|
Not sure what happened here, but at least on EDIT: Seems to work on |
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.
This is not building locally right? (git clone of repo, init with submodules, 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 :) |
You're probably missing build arguments. Have a look at
Not really. CI executes
Very nice. |
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.
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, Also in 2016, I'm going to look into refactoring / consolidating the Like wise, the |
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.
|
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.
|
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
|
|
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.
Description
Continuation of my original PR: #1629
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
Checklist: