search

corecode / dma

The DragonFly Mail Agent, a small Mail Transport Agent (MTA), designed for home and office use.
Other
231 stars 50 forks source link

No verification of remote TLS certificates #129

Closed rianhunter closed 1 year ago

rianhunter commented 1 year ago

It seems like the only checking of the smarthost certificate is through the "FINGERPRINT" config option. There doesn't seem to be any code that checks that the supplied certificate is signed by a valid CA, that the certificate matches the hostname we used to resolve the IP, nor does the client use SNI so that the server can supply the right certificate.

Is this intentional? If not, would a patch implementing these features be appreciated? Of course I'd be willing to modify the patch to match the maintainer's code preferences.

corecode commented 1 year ago

Not requiring a certificate chain to a set of roots is deliberate, because TLS for SMTP is opportunistic and many mail servers use a self signed certificate. Of course this is different for communication to a known smartserver. That's why the FINGERPRINT option exists, but it is quite crude of course.

I agree that we should support SNI.

CA checks should be optional. I think we should see how much complexity such a check adds to the code.

rianhunter commented 1 year ago

many mail servers use a self signed certificate.

That is reasonable. Not sure how many people are using dma with a big smarthost like gmail, probably not many.

CA checks should be optional.

My initial thought was that CA checks should be the default but could be disabled optionally. Either default you want works for me.

Should I go ahead and make a first attempt at a patch, or is this something about which you feel strongly? Thanks for the reply!

corecode commented 1 year ago

On October 11, 2023 6:09:39 PM GMT+02:00, Rian Hunter @.***> wrote:

many mail servers use a self signed certificate.

That is reasonable. Not sure how many people are using dma with a big smarthost like gmail, probably not many.

CA checks should be optional.

My initial thought was that CA checks should be the default but could be disabled optionally. Either default you want works for me.

well what would we do then? disconnect and deliver without TLS? seems a worse option.

Should I go ahead and make a first attempt at a patch, or is this something about which you feel strongly? Thanks for the reply!

go ahead with the patch. how should FINGERPRINT interact with a potential CA? I don't think we should only support the big OS root CA list, but should support pinning the root/intermediate CA (via FINGERPRINT? via cacert file?)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

rianhunter commented 1 year ago

well what would we do then? disconnect and deliver without TLS? seems a worse option.

My original idea was that if the user has specified SECURETRANSFER and not FINGERPRINT, then by default it would validate the remote certificate against the CA list and check that the hostname used is the one on the certificate. If the remote certificate fails the check, then failing the delivery and reporting in syslog (Similar to how a connection failure would work). If the user wanted the old behavior they could specify a new option like "NOCERTVALIDATION". If the user has specified "NOCERTVALIDATION" and not FINGERPRINT, then it behaves as it does today.

If we want to maintain the current defaults, then instead the option "DOCERTVALIDATION" would invoke the new behavior of validation. I think this is what you prefer. I don't mind either way but I think generally more secure defaults are preferred.

how should FINGERPRINT interact with a potential CA?

If FINGERPRINT is specified then the validation code path is not invoked. It uses the current FINGERPRINT-based validation.

I don't think we should only support the big OS root CA list, but should support pinning the root/intermediate CA (via FINGERPRINT? via cacert file?)

For simplicity's sake I think it's best to first use the default CA list per OpenSSL defaults. Later on a new patch can introduce a "CAFILE" or "CADIR" options. Those options correspond to options that exist in OpenSSL.

go ahead with the patch.

I'll try it out!