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

separate TLS private key from [imapd|pop3d].pem #13

Merged

Conversation

mumumu
Copy link
Contributor

@mumumu mumumu commented Sep 9, 2018

closes: svarshavchik/courier#10

  • Newly added TLS_PRIVATE_KEYFILE setting and existing TLS_CERTFILE only includes cert pem file only.
  • If the TLS_PRIVATE_KEYFILE is not specified, fallback to default cert file and private key combined file.
  • Does not change mkimapcert, mkesmtpcert, and mkpop3cert helper scripts.
  • OpenSSL/GnuTLS support, both
  • Support Virtual Hosting
    • SNI
    • Virtual Host on the same IP Address

@svarshavchik
Copy link
Owner

In the OpenSSL version both get_ip_concated_readable_file() and get_servername_concated_readable_file() will leak memory if they return NULL.

The added 2nd "#ifdef HAVE_OPENSSL_SNI" results in a hard to read diff, because git's diff thinks that this is the original line, and then shows a bunch of unchanged code by removing and adding it. The resulting change isn't easy to understand.

There are a number of ways to make it look better, like adding an extra spaces, "#ifdefHAVE_OPEEN_SNI" or, perhaps, putting

#ifndef HAVE_OPESSL_SNI
#define USE_SNI
#endif

At the beginning of the file and using "#ifdef USE_SNI" instead, something like that.

@mumumu mumumu force-pushed the new_feature_TLS_PRIVATE_KEYFILE branch from 26e5456 to 141976c Compare September 11, 2018 16:35
@mumumu
Copy link
Contributor Author

mumumu commented Sep 11, 2018

In the OpenSSL version both get_ip_concated_readable_file() and
get_servername_concated_readable_file() will leak memory if they return NULL.

Thank you for pointing out. Fixed.

The added 2nd "#ifdef HAVE_OPENSSL_SNI" results in a hard to read diff,

OK. I moved newly added function location to make diff more readable.

@svarshavchik
Copy link
Owner

Looks fine, after another review and a look.

A major release is being prepared, shortly. This'll be merged after the next release. Don't know yet if I'll use github to merge this branch, or if I will do it directly.

@mumumu
Copy link
Contributor Author

mumumu commented Sep 13, 2018

Don't know yet if I'll use github to merge this branch, or if I will do it directly.

If you use github to merge this branch, I'll very glad, because merge result is simple to understand for everyone.

@gashtaan
Copy link

Hi guys, should this be working feature now? I'm trying it on Ubuntu 20.04, there already is the option TLS_PRIVATE_KEYFILE documented in imapd-ssl configuration file, but seems it is ignored and not actually implemented... I've even tried to put some bogus non-existent path here, but it never produces any error anywhere.

@gashtaan
Copy link

I take it back, problem was on my side, I didn't have private key file properly readable. It would be best to produce error or at least warning in such case, not just silently ignore it like you do...
private_key_file = check_readable_file(private_key_file) ? private_key_file : NULL;
But anyway, thanks for the great feature 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

separate TLS private key from [imapd|pop3d].pem
3 participants