TLS server support SNI based certificate selection #596

cspiel1 · 2022-11-15T15:02:43Z

tls: support multiple TLS server certificates

Multiple certificates combined with private key and host name for the host name
verification can be added with a new function `tls_add_certf()`.

For incoming SIP requests a certificate is selected by SNI in the TLS client
hello before `SSL_accept()` is called.

e.g. use case: incoming OPTIONS from SIP servers for client is alive polling

TODO: maybe in follow up PRs in order to keep this PR small.

cleanup/combine with

commit 21f36e31522b8d2106e6c1daf64ea0eba626f79c
Author: Christoph Huber <[email protected]>
Date:   Mon Apr 12 09:45:50 2021 +0200

    transp: add and use different client certificates

The names tls_set_verify_server() and tls_set_verify_client() are not very proper. Maybe we could add a deprecated logging/doxygen and add new functions:
- tls_conn_enable_verify_peer(struct tls_conn *tc, const char *host) with optional host parameter and
- tls_trust_all(struct tls *tls)

alfredh · 2022-11-19T12:33:31Z

src/tls/openssl/tls.c

+	if (!pl_isset(&pl))
+	    return EINVAL;
+
+	/* skip record header, handshake header, client version and random */


is it possible to do the TLS message parsing in OpenSSL instead ?

does it work with TLS 1.2 and TLS 1.3 ?

is it possible to do the TLS message parsing in OpenSSL instead ?
does it work with TLS 1.2 and TLS 1.3 ?

I searched in the openssl code for a while and couldn't find an adequate function. Today, we found the API function:
SSL_CTX_set_client_hello_cb()

I try to learn from the apache source code how they do cert selection via SNI. I set this PR to draft so far.

cspiel1 · 2022-11-28T15:55:45Z

Apache copied code from openssl/test/helpers/handshake.c for parsing the SNI from the client hello.

The last commit in this PR also uses SSL_CTX_set_client_hello_cb() and SSL_client_hello_get0_ext() to do most of the parsing.

I guess it works only for TLS 1.3 because in man page of SSL_client_hello_isv2 there is:
"The SSLv2 format has substantial differences from the normal SSLv3 format, including using three bytes per cipher suite, and not allowing extensions."

Note: I have to test this before we can proceed.

alfredh · 2022-11-29T08:10:16Z

https://stackoverflow.com/questions/5113333/how-to-implement-server-name-indication-sni

it would be nice to implement this without any manual parsing of packets.

we support TLS v1.2 and v1.3
SSLv2/3 is deprecated.

cspiel1 · 2022-12-05T08:18:01Z

Thanks for this hint Alfred!
Currently we have some release pressure. I'll try to rewrite this PR as soon as possible.

alfredh · 2022-12-07T10:30:31Z

src/tls/openssl/sni.c

+	CONTENT_TYPE_HANDSH = 22,
+	HANDSH_CLIENT_HELLO = 1,
+	RECORD_LAYER_SKIP   = 4,
+};


are these used ?

alfredh · 2022-12-07T10:31:11Z

src/tls/openssl/sni.c

+	*len = (uint8_t) (*pl->p);
+	pl_advance(pl, 1);
+	return 0;
+}


struct pl is designed for operating on strings.

for this it is probably better to use mbuf

alfredh · 2022-12-07T10:32:05Z

src/tls/openssl/sni.c

+		return NULL;
+
+	sz = (int) sni->l + 1;
+	cn = mem_zalloc(sz, NULL);


check for OOM

please add a test to retest with SNI

alfredh · 2022-12-07T10:33:07Z

src/tls/openssl/tls.c

@@ -143,7 +154,7 @@ static int keytype2int(enum tls_keytype type)

 #if OPENSSL_VERSION_NUMBER >= 0x10100000L && \
 	!defined(LIBRESSL_VERSION_NUMBER)
-static int verify_handler(int ok, X509_STORE_CTX *ctx)
+int tls_verify_handler(int ok, X509_STORE_CTX *ctx)


I would prefer handlers to be static.

Okay.

The perfect name for a function in tls.c that turns on SSL_VERIFY_PEER for a TLS client and sets a verify handler would be: void tls_set_verify_client(tls)

But there is already a function with this name that does the opposite. It's sets the handler verify_trust_all. :-(

alfredh · 2022-12-07T10:34:03Z

src/tls/openssl/tls_tcp.c

@@ -241,7 +241,7 @@ static bool recv_handler(int *err, struct mbuf *mb, bool *estab, void *arg)
 			*err = tls_accept(tc);
 		}

-		DEBUG_INFO("state=0x%04x\n", SSL_state(tc->ssl));
+		DEBUG_INFO("state: %s\n", SSL_state_string_long(tc->ssl));


perhaps move to separate PR ?

cspiel1 · 2022-12-07T12:19:38Z

Thanks! First I try to rewrite this like you already mentioned: #596 (comment)

cspiel1 · 2022-12-07T14:21:04Z

Separated: #613

cspiel1 · 2022-12-07T14:35:40Z

TODO: Test the change to the SSL_CTX_set_tlsext_servername_callback.

alfredh · 2022-12-12T12:59:17Z

I would like to suggest the following:

Create a new patch this in re
Create a matching test in retest

The test can be a back-to-back test with a TLS connection
from Client to Server, that is using SNI.

If you want you can close this PR and make a new one.

cspiel1 · 2022-12-12T15:53:42Z

New PR is not necessary. I re-based and squashed the commits.

Now I'll start to work on the retest.

alfredh · 2022-12-13T08:12:56Z

some general comments:

use const char * instead of struct pl in the public API
in for loops, move declaration of i into for loop
in all openssl Free functions, check if they can handle a NULL pointer
in all places where openssl return error, call ERR_clear_error()
global symbols in the tls module should have prefix: tls_
move sni.h into tls.h
add doxygen comment to public functions

alfredh · 2022-12-13T08:14:20Z

src/tls/openssl/sni.c

+#include <re_dbg.h>
+
+#include <re_list.h>
+#include <re_hash.h>


move these 2 lines up, so that include re_* is grouped

alfredh · 2022-12-13T08:15:53Z

src/tls/openssl/tls.c

+	}
+
+	BIO_free(bio);
+	bio = BIO_new_file(certf, "r");


BIO_new_file is called twice, is it needed ?

Can you use the same BIO for cert and private key ?

I already read about BIO_reset(). But can't remember the result. I will test it when I have the retest.

cspiel1 · 2022-12-14T15:44:45Z

in all openssl Free functions, check if they can handle a NULL pointer

Yes they have.

ASN1 free functions --> checked in the source code
X509_free and EVP_PKEY_free --> checked in man page
sk_X509_pop_free --> checked in the code

in all places where openssl return error, call ERR_clear_error()

Okay, I'll put it in the "out:" section like it is done in other function in the tls module. It doesn't hurt if it called too often.

global symbols in the tls module should have prefix: tls_

I move some functions from tls.c to sni.c and add tls_verify_handler() to the private API tls.h.

add doxygen comment to public functions

My understanding is that public functions are in re_tls.h. I think it would also be good to add doxygen for the new tls.h functions.

Multiple certificates combined with private key and host name for the host name verification can be added with a new function `tls_add_certf()`. For incoming SIP requests a certificate is selected by SNI in the TLS client hello before `SSL_accept()` is called.

- use const char * instead of struct pl in the public API - in for loops, move declaration of i into for loop - in all places where openssl return error, call ERR_clear_error() - global symbols in the tls module should have prefix: tls_ - move sni.h into tls.h - add doxygen comment to global and private API functions

src/tls/openssl/tls.c

alfredh · 2022-12-27T12:57:52Z

src/tls/openssl/tls.c

@@ -266,6 +290,7 @@ int tls_alloc(struct tls **tlsp, enum tls_method method, const char *keyfile,
 		}
 	}

+	tls_enable_sni(tls);


should SNI be enabled by default, also for clients ?

We can enable it in first call of tls_add_certf().

alfredh · 2022-12-27T13:00:03Z

src/tls/openssl/tls.c

+		}
+	}
+
+	BIO_reset(bio);


is this line needed ?

Yes. See #596 (comment)!

These steps are done in tls_add_certf():

first read certificate

read potentially following CAs

reset the BIO and read the key anywhere in the file

cspiel1 changed the title ~~tls multiple server certs~~ TLS server support SNI based certificate selection Nov 15, 2022

cspiel1 force-pushed the tls_multiple_server_certs branch from f1192f5 to fa0565c Compare November 15, 2022 16:08

sreimers added this to the v2.11.0 milestone Nov 15, 2022

cspiel1 force-pushed the tls_multiple_server_certs branch from fa0565c to 3ab50cc Compare November 16, 2022 14:55

cspiel1 marked this pull request as ready for review November 16, 2022 14:56

alfredh reviewed Nov 19, 2022

View reviewed changes

cspiel1 marked this pull request as draft November 28, 2022 09:32

cspiel1 mentioned this pull request Nov 28, 2022

TLS server support SNI based certificate selection baresip/baresip#2330

Merged

alfredh reviewed Dec 7, 2022

View reviewed changes

src/tls/openssl/sni.c Outdated

CONTENT_TYPE_HANDSH = 22,

HANDSH_CLIENT_HELLO = 1,

RECORD_LAYER_SKIP = 4,

};

Copy link

Contributor

alfredh Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these used ?

alfredh reviewed Dec 7, 2022

View reviewed changes

cspiel1 force-pushed the tls_multiple_server_certs branch 2 times, most recently from d865232 to fdc8a82 Compare December 8, 2022 07:50

cspiel1 force-pushed the tls_multiple_server_certs branch from fdc8a82 to 1dc4bdc Compare December 12, 2022 15:51

alfredh reviewed Dec 13, 2022

View reviewed changes

cspiel1 force-pushed the tls_multiple_server_certs branch from ec150d1 to 6a272d7 Compare December 20, 2022 09:54

cspiel1 added 5 commits December 21, 2022 10:12

tls: replace pl by const char * in API function parameter

28ebe1f

tls: fix common name comparision

1f899a7

tls: use BIO_reset() in tls_add_certf()

74eeb28

tls: cast size_t to int

26868cf

cspiel1 force-pushed the tls_multiple_server_certs branch from a342c18 to 26868cf Compare December 21, 2022 09:12

cspiel1 marked this pull request as ready for review December 21, 2022 09:35

alfredh reviewed Dec 22, 2022

View reviewed changes

src/tls/openssl/tls.c Show resolved Hide resolved

tls: check for out of memory

41c4e72

alfredh reviewed Dec 27, 2022

View reviewed changes

tls: enable sni when first TLS server certificate is added

6e0edb1

cspiel1 merged commit 8aa9428 into baresip:main Dec 29, 2022

cspiel1 deleted the tls_multiple_server_certs branch December 29, 2022 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TLS server support SNI based certificate selection #596

TLS server support SNI based certificate selection #596

cspiel1 commented Nov 15, 2022 •

edited

Loading

alfredh Nov 19, 2022

cspiel1 Nov 28, 2022

cspiel1 commented Nov 28, 2022 •

edited

Loading

alfredh commented Nov 29, 2022

cspiel1 commented Dec 5, 2022

alfredh Dec 7, 2022

alfredh Dec 7, 2022

alfredh Dec 7, 2022

alfredh Dec 7, 2022

cspiel1 Dec 7, 2022

alfredh Dec 7, 2022

cspiel1 commented Dec 7, 2022

cspiel1 commented Dec 7, 2022

cspiel1 commented Dec 7, 2022 •

edited

Loading

alfredh commented Dec 12, 2022

cspiel1 commented Dec 12, 2022

alfredh commented Dec 13, 2022

alfredh Dec 13, 2022

alfredh Dec 13, 2022

cspiel1 Dec 14, 2022

cspiel1 commented Dec 14, 2022 •

edited

Loading

alfredh Dec 27, 2022

cspiel1 Dec 29, 2022

alfredh Dec 27, 2022

cspiel1 Dec 29, 2022

TLS server support SNI based certificate selection #596

TLS server support SNI based certificate selection #596

Conversation

cspiel1 commented Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cspiel1 commented Nov 28, 2022 • edited Loading

alfredh commented Nov 29, 2022

cspiel1 commented Dec 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cspiel1 commented Dec 7, 2022

cspiel1 commented Dec 7, 2022

cspiel1 commented Dec 7, 2022 • edited Loading

alfredh commented Dec 12, 2022

cspiel1 commented Dec 12, 2022

alfredh commented Dec 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cspiel1 commented Dec 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cspiel1 commented Nov 15, 2022 •

edited

Loading

cspiel1 commented Nov 28, 2022 •

edited

Loading

cspiel1 commented Dec 7, 2022 •

edited

Loading

cspiel1 commented Dec 14, 2022 •

edited

Loading