mailcow-dockerized icon indicating copy to clipboard operation
mailcow-dockerized copied to clipboard

Add ECDSA support

Open OpenLarry opened this issue 5 years ago โ€ข 29 comments

I added the ECDSA certificate support like we have discussed in #2426 .

I tried to modify the bash script as less as possible to reduce the time for code review/testing and I tried to use the same coding style like before. However, I decided to move some code into separate functions to avoid code duplication.

A few notes:

  • I did not add a separate option (at least not yet) to disable ECDSA certificates when using the integrated Letsencrypt client. I don't think that it makes sense to disable ECDSA, because there should be no drawback when it stays enabled for everyone?
  • If SKIP_LETSENCRYPT=y (or the installed certificate isn't issued by Letsencrypt) and there is no custom ECDSA cert, the script will add a symlink from the ECDSA certificate to the RSA certificate.
  • ECDSA and RSA certificates are checked and renewed independently from each other, so it's likely that they have different validity periods after migration.
  • I've combined this if-statement with the statement above. I think it's a bug to continue the script if there isn't a certificate, but we have an old one that has non-matching hashes. (However it should happen rarely - or even never?)

I've tested migration (using custom and Letsencrypt certificates) and fresh installations. I've checked all services using testssl.sh and it reports two certificates or one certificate depending on whether an ECDSA certificate exists or not. Looks good to me, so I'm looking forward to feedback! :-)

Of course I can add a few lines to the documentation as well.

Closes #2426

OpenLarry avatar Mar 16 '19 14:03 OpenLarry

I have not looked into it at all. What about TLSA? Any changes when updating?

I

andryyy avatar Mar 16 '19 14:03 andryyy

Damn... yes.

Didn't think about that yet because I don't use DNSSEC. A second certificate also requires a second TLSA record for each offered service. The automatic generation of DNS records in Mailcow's UI could probably be extended easily to show the required records for both certificates.

But that makes migration from an RSA-only setup indeed more complex for users that are already using DNSSEC.

Any ideas how to solve that easily?

  • Check if TLSA records are present and disable the ECDSA certificate if it's not listed?
  • Show a warning during the update process and let the user decide (and add an option to disable the ECDSA certificates in mailcow.conf)?
  • Combination of both?

OpenLarry avatar Mar 16 '19 15:03 OpenLarry

And thatโ€™s where hacky starts and delivery may fail.

andryyy avatar Mar 16 '19 15:03 andryyy

That means we need a configuration option where users explicitly have to turn on ECDSA. I was hoping to avoid that (we have enough such options already), but there is no way around it. I guess we could turn it on by default on new installs though (from generate_config.sh).

mkuron avatar Mar 16 '19 15:03 mkuron

Can we get docs for this? :)

andryyy avatar Mar 17 '19 12:03 andryyy

Sure! :-) If the PR is ready to get merged and the functionality is more or less complete, then I'll add something about it to the documentation.

OpenLarry avatar Mar 17 '19 12:03 OpenLarry

Looks complete. :) Do you think this is ready to be merged?

A few words in the docs are enough.

andryyy avatar Mar 18 '19 01:03 andryyy

I've added a few words about it to documentation and I updated the chapter slightly. I hope it's all right. :-)

I tried to test my code with every scenario that came into my mind. So I hope it works. ๐Ÿ˜ƒ

OpenLarry avatar Mar 18 '19 15:03 OpenLarry

Is anything still missing here? Do you need help? :-)

OpenLarry avatar Apr 01 '19 10:04 OpenLarry

@andryyy Anything new in terms of merging this?

patschi avatar May 05 '19 01:05 patschi

I wonder what happens with all the Software when they load the same certificate twice.

Knight1 avatar Aug 05 '19 18:08 Knight1

I've already checked the services implementation and documentation here.

Since it's documented OpenSSL functionality that loading the same certificate twice actually does nothing and postfix/dovecot/nginx don't care about the certificate handling/parsing itself, it should (and does!) work flawlessly in my opinion. :-)

OpenLarry avatar Aug 07 '19 18:08 OpenLarry

What's the exact state with this PR, @OpenLarry? Now as #2509 was merged in the past, I'd guess that most of the code is outdated and not compatible anymore. Any input from @mhofer117 for this?

patschi avatar Nov 16 '19 22:11 patschi

I had this included in #2509 initially because I thought ECDSA would be merged first. Then removed it so #2509 could be merged. However my changes are still ready here: https://github.com/OpenLarry/mailcow-dockerized/pull/1

Personally I have no need for ECDSA certs so i'm not maintaining it, but that PR should solve almost all conflicts

mhofer117 avatar Nov 16 '19 23:11 mhofer117

I've already merged the mentioned PR by mhofer117 locally and it seems to work. But I wasn't able to test everything yet.

I will try to do that in the next two days and update this PR afterwards. :-)

OpenLarry avatar Nov 17 '19 12:11 OpenLarry

Sorry for the late reply, had a lot of stuff to do during the last days...

I've merged the PR by @mhofer117 containing the ECDSA adjustments for the SNI part and it looks good to me. But I haven't tested the SNI part yet, because I have no need for that. Maybe someone else wants to try that out and verify that everything works properly?

OpenLarry avatar Dec 07 '19 17:12 OpenLarry

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar May 20 '20 08:05 stale[bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] avatar Jun 03 '21 00:06 github-actions[bot]

Just saw your rebase on master, possible to change the base branch to staging and rerebase? :p

MAGICCC avatar Nov 11 '21 16:11 MAGICCC

Sure! I will do that in the coming days and also clean up this branch. :-)

OpenLarry avatar Nov 13 '21 13:11 OpenLarry

Using ECDSA with last acme.sh deploy hook. Really need that to be merged.

Any news ?

SecT0uch avatar Jan 29 '22 09:01 SecT0uch

@SecT0uch Do you mean this? I added the ECDSA support over a year ago.

christianbur avatar Jan 29 '22 16:01 christianbur

@christianbur No, I mean mailcow recognizing ecdsa-cert.pem and ecdsa-key.pem intruduced with that commit

I had a previous version of the deploy script that was working well.

Edit: I have mailcow up-to-date and don't want to play with symlinks in docker

SecT0uch avatar Feb 02 '22 15:02 SecT0uch

I've squashed all messy commits together and rebased it on the current staging branch. Seems to work (and is already working for me for years).

OpenLarry avatar May 31 '22 16:05 OpenLarry

Yes, keep in mind there are like 47838374 different setups and thinks will get dirty. We will need to be prepared. Thank you, Aaron! :)

andryyy avatar May 31 '22 16:05 andryyy

What's the status on that PR ?

SecT0uch avatar Jul 07 '22 12:07 SecT0uch

@SecT0uch Why did you break my acme.sh deploy plugin? -> https://github.com/acmesh-official/acme.sh/pull/4170

If you want to store ECDSA certificates in the (RSA) cert files (cert.pem, key.pem), this would have been possible simply with symlinks. With your change in acme.sh, you destroyed my plugin and also my mail setup, because DANE/TLSA was broken.

@SecT0uch -> Please undo the change as soon as possible.

Create symlink in directory ./mailcow-dockerized/data/assets/ssl

ln -s ecdsa-cert.pem cert.pem
ln -s ecdsa-key.pem key.pem
$ ./mailcow-dockerized/data/assets/ssl# ls -al 
lrwxrwxrwx 1 root root 10 Aug 24 12:11 cert.pem -> ecdsa-cert.pem
lrwxrwx 1 root root 9 Aug 24 12:11 key.pem -> ecdsa-key.pem
-rw-r--r-- 1 root root 5934 Aug 24 11:54 ecdsa-cert.pem
-rw-r--r-- 1 root root 3247 Aug 24 11:54 ecdsa-key.pem

christianbur avatar Aug 24 '22 10:08 christianbur

Because you are including custom changes in a public repository that breaks a default mailcow installation.

You broke my installation in the first place with your changes on the acme plugin. Since clearly this commit won't be merged, I don't see the point in breaking everyone else's installation.

If you want to keep using the custom changes you made on mailcow, you should fork the acme plugin and make your changes on your side.

SecT0uch avatar Aug 24 '22 12:08 SecT0uch

Hello everyone,

as i have taken the position to maintain the repository and i'm not 100% sure what your PR does i would kindly ask you to describe what the benefits of ECDSA are and if it will break existing installations.

I've saw, that you've changed also the dovecot.conf for alt_certs. How does dovecot/postfix behave if these certs are not generated?

DerLinkman avatar Mar 20 '23 10:03 DerLinkman