baresip / re

Generic library for real-time communications with async IO support
BSD 3-Clause "New" or "Revised" License
135 stars 83 forks source link

tls: refactoring SNI ctx usage for libressl support #1136

Closed sreimers closed 4 months ago

sreimers commented 4 months ago

Replaces unsupported (by LibreSSL) SSL_certs_clear with recommended SSL_set_SSL_CTX():

https://www.openssl.org/docs/man3.3/man3/SSL_CTX_set_tlsext_servername_callback.html

The servername callback should return one of the following values:

SSL_TLSEXT_ERR_OK This is used to indicate that the servername requested by the client has been accepted. Typically a server will call SSL_set_SSL_CTX() in the callback to set up a different configuration for the selected servername in this case.

sreimers commented 4 months ago

@cspiel1 can you review and test with your SNI setup?

cspiel1 commented 4 months ago

Yes, I'll try to do so. It will take some time to arrange the setup. Maybe I have to ask also our QA team.

cspiel1 commented 4 months ago

By means of debugging I could verify that the correct certificate was selected. But then I had no luck with the cert verification on the callee (TLS server side), also in main branch. The next two days will be difficult to work on this.

cspiel1 commented 4 months ago

I am coming closer with the test setup. Will have a result soon.

cspiel1 commented 4 months ago

I have some troubles:

sreimers commented 4 months ago
  • The callee (TLS server) seems to verify the client certificate incl host name check. sip_verify_client no seems to make no difference.

Not sure why, but verify client is always enabled for SNI here: https://github.com/baresip/re/blob/9f90b79830411568edc3436e5431a6467027227f/src/tls/openssl/sni.c#L158

  • It seems that the setting sip_capath does not work.

It works for test, does it also not work with main?

cspiel1 commented 4 months ago

Beside of these issues, that also occur in main branch this PR seems to be okay. SNI works.

tls: tls_verify_handler: subject_name = /C=AU/ST=Some-State/O=Retest/CN=Mr Retest Root CA
tls: tls_verify_handler: issuer_name  = /C=AU/ST=Some-State/O=Retest/CN=Mr Retest Root CA
tls: tls verify ok = 1
tls: tls_verify_handler: subject_name = /C=AU/ST=Some-State/O=Retest/CN=Mr Retest Intermediate CA
tls: tls_verify_handler: issuer_name  = /C=AU/ST=Some-State/O=Retest/CN=Mr Retest Root CA
tls: tls verify ok = 1
tls: tls_verify_handler: subject_name = /C=AU/ST=Some-State/O=Retest/CN=server.foo.local
tls: tls_verify_handler: issuer_name  = /C=AU/ST=Some-State/O=Retest/CN=Mr Retest Intermediate CA
tls: tls verify ok = 1

With another cert chain, and a second account it works also. But as mentioned above, the sip_cafile has to be set to the second chain.

The code looks good. I suggest to merge this.

cspiel1 commented 4 months ago

Not sure why, but verify client is always enabled for SNI here:

Ah, ok. Not sure if this is wanted.

It works for test, does it also not work with main?

Yes. This should be analysed further.

maximilianfridrich commented 2 months ago

@sreimers I have noticed that this PR changed the behavior quite a bit. Was that on purpose?

E.g. if the peer does not add the server_name extension, then the handshake always fails now. Before, the handshake proceeded.

cspiel1 commented 2 months ago

There are different options to choose a certificate. E.g. Certificates configured in accounts. Or globally in config file.

Thus a missing server_name should not be the reason that TLS handshake fails. As soon as a wrong certificate or no certificate is offered the TLS handshake should fail.

sreimers commented 2 months ago

@sreimers I have noticed that this PR changed the behavior quite a bit. Was that on purpose?

I think this is a bug, can you try:


diff --git a/src/tls/openssl/sni.c b/src/tls/openssl/sni.c
index 8298e40f..8be0e7f7 100644
--- a/src/tls/openssl/sni.c
+++ b/src/tls/openssl/sni.c
@@ -166,10 +166,8 @@ static int ssl_servername_handler(SSL *ssl, int *al, void *arg)
        const char *sni;

        sni = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name);
-       if (!str_isset(sni)) {
-               *al = SSL_AD_UNRECOGNIZED_NAME;
-               return SSL_TLSEXT_ERR_ALERT_FATAL;
-       }
+       if (!str_isset(sni))
+               return SSL_TLSEXT_ERR_OK;

        /* find and apply matching certificate */
        uc = tls_cert_for_sni(tls, sni);
maximilianfridrich commented 2 months ago

Implemented in