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

ECC certs generated with acme.sh not detected

Open SecT0uch opened this issue 2 years ago • 7 comments

Contribution guidelines

I've found a bug and checked that ...

  • [X] ... I understand that not following the below instructions will result in immediate closure and/or deletion of my issue.
  • [X] ... I have understood that this bug report is dedicated for bugs, and not for support-related inquiries.
  • [X] ... I have understood that answers are voluntary and community-driven, and not commercial support.
  • [X] ... I have verified that my issue has not been already answered in the past. I also checked previous issues.

Description

Since this commit

Acme.sh is naming ECDSA cert and key ecdsa-cert.pem and ecdsa-key.pem. As a result mailcow is unable to detect it.

This is covered in the PR #2443.

Why isn't it merged already ? Every 3 months I have to manually rename the certs..

Logs

Irrelevant

Steps to reproduce

  1. Use acme.sh with an --ecc cert
  2. Use the acme.sh mailcow deploy-hook
  3. When triggering a deploy acme.sh writes:
  • data/assets/ssl/ecdsa-cert.pem
  • data/assets/ssl/ecdsa-key.pem
  1. Mailcow is looking for:
  • data/assets/ssl/cert.pem
  • data/assets/ssl/cert.pem

System information

Irrelevant

SecT0uch avatar Apr 29 '22 06:04 SecT0uch

How is this an issue when even the base is not merged?

I mean, why aren't you just fixing your hooks?

andryyy avatar Apr 29 '22 08:04 andryyy

What do you mean by the base is not merged ? I'm actually asking for a merge of this PR.

Well, I'm often updating acme.sh which also updates the hooks.

Wouldn't that be better if the existing mailcow hook would actually work as expected?

Do you think I should rather I should submit a PR at acme.sh to revert these changes ?

SecT0uch avatar Apr 29 '22 09:04 SecT0uch

Ah, I see. Hm. Hmmm. Okay, let's finally get that moving.

@mkuron I need you and @Thomas2500 I need you too. What do you guys think?

andryyy avatar Apr 29 '22 10:04 andryyy

Also pinging @patschi because he won't believe this is finally happening.

andryyy avatar Apr 29 '22 10:04 andryyy

Hi, I had created the commit for acme.sh for two reasons:

  • I run acme.sh in a container, so I had to customize the _ssl_path.
  • At this occasion I also added the support for ecc certificates, because I thought that the ecdsa mailcow commit will be implemented soon.

However, since 2019 ECDSA support has not been implemented in Mailcow, so the ecc function in acme.sh is not very useful at the moment. Just call acme.sh without "--ecc", then the normal RSA certificates (cert.pem, key.pem) will be created.

christianbur avatar Jun 05 '22 14:06 christianbur

@christianbur I've always had mailcow running with my ecc certificate with no issue. We just need #2443 to be merged

SecT0uch avatar Jul 07 '22 12:07 SecT0uch

As I said otherwhere: "You" or "me" is not everyone and there will be cases it will fail. We just don't know what and when and where.

andryyy avatar Jul 07 '22 14:07 andryyy

The current mailcow standard is as follows

  • cert.pem, key.pem -> Mailcow rsa cert files
  • ecdsa-cert.pem, ecdsa-key.pem -> Mailcow ecdsa cert files

With your change to the acem.sh mailcow deploy plugin, ecdsa certs are now also written to the standard cert files (cert.pem, key.pem). You are probably currently running Mailcow exclusively with ecdsa certs, but https://github.com/mailcow/mailcow-dockerized/pull/2443 and https://github.com/acmesh-official/acme.sh/blob/master/deploy/mailcow.sh are designed to run dual ecdsa AND rsa certs. If you want to run Mailcow with ecdsa certs only, this is ok, but it is not the default. So I think it's better to switch back to the dual variant of the deploy plugin (maybe Mailcow will get ecdsa support eventually) and you can use the symlink variant (need to be made only once), then the Depoly plugin will work for you.

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 13:08 christianbur

As you can see, the PR you linked #2443 is open and not merged into the project! Do you understand what it means ?

You can check by yourself, none of this changes are in the master branch.

From @andryyy answer, they won't merge it into the project.

So no, this is not standard:

ecdsa-cert.pem, ecdsa-key.pem -> Mailcow ecdsa cert files

You are probably (one of) the only person running mailcow with these changes in production.

SecT0uch avatar Aug 24 '22 14:08 SecT0uch

Whether mailcow will eventually get dual support for ecdsa and rsa we both don't know.

However, I currently see the following problem with your modification of the acme.sh mailcow depoy plugin. If someone (unknowingly) calls the acme.sh deploy plugin with --ecc, the ecdsa cert is ALSO written to the cert file (cert.pem, key.pem), this causes DANE/TLSA to break (Because TLSA was set only for the RSA Cert). With my change the error could not happen, because you had to set a symlink once (in full knowledge). Therefore I find your change dangerous.

christianbur avatar Aug 24 '22 16:08 christianbur

I didn't do any modification I restored the state it was in before.

If he accidentally deploys with --ecc then, he re-runs the command without and the wrong cert gets erased by the good one.

Again, if you have a custom setup and need to change the plugin, do it. But please do not submit a PR that break everyone else's installation.

Of course if the PR #2443 get merged then we will re-adapt the acme script accordingly. But until then the changes you submitted are breaking standard installations.

SecT0uch avatar Aug 24 '22 19:08 SecT0uch