Re: TLS changes and breakage after 3.3.2 -> 3.4.0 upgrade

[Viktor Dukhovni]

On Mon, Mar 04, 2019 at 07:43:17PM -0500, Wietse Venema wrote:

> Viktor Dukhovni:
> > The reason there's no logging of a problem, is that there is no
> > problem to log! :-(  The certificate initialization was successful,
> > but due to a bug in reporting success/failure to the caller, the
> > successful outcome was treated as a failure (whose reason would
> > have already been logged if it were a real failure).
> > 
> > The patch is a one liner, below.
> 
> I have uploaded postfix-3.4.1-RC1 (stable) and postfix-3.5-20190304 
> (unstable).

Below is a more complete patch (relative to 3.4.0, including same
one-liner), with tests that exercise the problem code-path.

We could refactor functions in tls_certkey.c to be consistent with
respect to whether 0 or non-zero is a successful return code.  The
lower-level internal functions return 0 on success, but the external
API returns 0 on failure.  I failed to handle one case of impedance
mismatch.  :-(

-- 
        Viktor.

--- a/Makefile.in
+++ b/Makefile.in
@@ -58,6 +58,15 @@ tls_certkey_tests: test
            $(SHLIB_ENV) $(VALGRIND) ./tls_certkey -m $$pem > $$pem.out 2>&1 || 
exit 1; \
            diff $$pem.ref $$pem.out || exit 1; \
            echo "  $$pem: OK"; \
+           $(SHLIB_ENV) $(VALGRIND) ./tls_certkey -k $$pem $$pem > $$pem.out 
2>&1 || exit 1; \
+           diff $$pem.ref $$pem.out || exit 1; \
+           echo "  $$pem (with key in $$pem): OK"; \
+           case $$pem in good-*) \
+               ln -sf $$pem tmpkey.pem; \
+               $(SHLIB_ENV) $(VALGRIND) ./tls_certkey -k tmpkey.pem $$pem > 
$$pem.out 2>&1 || exit 1; \
+               diff $$pem.ref $$pem.out || exit 1; \
+               echo "  $$pem (with key in tmpkey.pem): OK";; \
+           esac; \
        done; \
        for pem in bad-*.pem; do \
            $(SHLIB_ENV) $(VALGRIND) ./tls_certkey $$pem > $$pem.out 2>&1 && 
exit 1 || : ok; \
--- a/tls_certkey.c
+++ b/tls_certkey.c
@@ -589,7 +589,7 @@ static int set_cert_stuff(SSL_CTX *ctx, const char 
*cert_type,
      * single pass, avoiding potential race conditions during key rollover.
      */
     if (strcmp(cert_file, key_file) == 0)
-       return (load_mixed_file(ctx, cert_file));
+       return (load_mixed_file(ctx, cert_file) == 0);
 
     /*
      * We need both the private key (in key_file) and the public key
@@ -690,6 +690,7 @@ int     main(int argc, char *argv[])
     int     ch;
     int     mixed = 0;
     int     ret;
+    char   *key_file = 0;
     SSL_CTX *ctx;
 
 #if OPENSSL_VERSION_NUMBER < 0x10100000L
@@ -707,8 +708,11 @@ int     main(int argc, char *argv[])
        tls_print_errors();
        exit(1);
     }
-    while ((ch = GETOPT(argc, argv, "m")) > 0) {
+    while ((ch = GETOPT(argc, argv, "mk:")) > 0) {
        switch (ch) {
+       case 'k':
+           key_file = optarg;
+           break;
        case 'm':
            mixed = 1;
            break;
@@ -722,7 +726,9 @@ int     main(int argc, char *argv[])
     if (argc < 1)
        usage();
 
-    if (mixed)
+    if (key_file)
+       ret = set_cert_stuff(ctx, "any", argv[0], key_file) == 0;
+    else if (mixed)
        ret = load_mixed_file(ctx, argv[0]);
     else
        ret = load_chain_files(ctx, argv[0]);