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

redisInitiateSSLWithContext hangs when I'm connected to the non-TLS port. #1177

Open
savincen opened this issue Apr 12, 2023 · 7 comments
Open

Comments

@savincen
Copy link

My application using hiredis is able to successfully call redisInitiateSSLWithContext when connected to the TLS enabled port. However, a call to redisInitiateSSLWithContext hangs when connected by way of the non-TLS port. Is that a bug? Is there a way I can programmatically avoid this? For example, before making the call to redisInitiateSSLWithContext, is there something I can check to make sure the port to which I connected is TLS Enabled?

@savincen
Copy link
Author

With respect to the parameters given to redisCreateSSLContext, the comments for each say optional. However, depending on the server's configuration, I assume the hiredis client application's redisCreateSSLContext call will need to at least specify one of the first two args, correct? Or is there a way to configure a client application environment such that the app could provid NULL for the first 5 args?

/* Create SSL context /
ssl_context = redisCreateSSLContext(
"cacertbundle.crt", /
File name of trusted CA/ca bundle file, optional /
"/path/to/certs", /
Path of trusted certificates, optional /
"client_cert.pem", /
File name of client certificate file, optional /
"client_key.pem", /
File name of client private key, optional /
"redis.mydomain.com", /
Server name to request (SNI), optional */
&ssl_error);

Regarding the first two, is at least one of them required?

If the server was configured with tls-auth-clients no to disable client authentication, I assume the third and fourth args are optional.  Correct?

@uglide
Copy link

uglide commented Apr 12, 2023

AFAIK, You need to check the first few bytes on the socket without consuming them (call recv() with the MSG_PEEK flag) and see if they look like a TLS ClientHello message as defined in RFC 2246.

Something like this:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <arpa/inet.h>

#define MAX_BUF_SIZE 2048

int main(int argc, char *argv[]) {
    int sockfd, num_bytes;
    struct sockaddr_in server_addr;
    char buffer[MAX_BUF_SIZE];

    // create socket
    sockfd = socket(AF_INET, SOCK_STREAM, 0);
    if (sockfd < 0) {
        perror("Error creating socket");
        exit(1);
    }

    // set server address
    memset(&server_addr, 0, sizeof(server_addr));
    server_addr.sin_family = AF_INET;
    server_addr.sin_port = htons(6380); 
    inet_pton(AF_INET, "my.redis.com", &server_addr.sin_addr);

    // connect to server
    if (connect(sockfd, (struct sockaddr*)&server_addr, sizeof(server_addr)) < 0) {
        perror("Error connecting to server");
        exit(1);
    }

    // peek at data in socket buffer
    num_bytes = recv(sockfd, buffer, MAX_BUF_SIZE, MSG_PEEK);
    if (num_bytes < 0) {
        perror("Error peeking at data");
        exit(1);
    }

    // check if ClientHello message is present
    if (memcmp(buffer, "\x16\x03\x01", 3) == 0 && buffer[5] == '\x01') {
        // TLS connection code here
    } else {
        // Plain socket connection here
    }
    // close socket
    close(sockfd);

    return 0;
}

@savincen
Copy link
Author

If it matters, my copy of hiredis.h says: #define HIREDIS_SONAME 1.0.3-dev

Btw, regarding the SSL/TLS context created, will it automatically be renegotiated when it expires?

@savincen
Copy link
Author

Igor, with respect to your sample above, are you suggesting that my client application do the following as a means of determining whether or not the port has been configured to require TLS?

  1. create a socket
  2. connect to the Redis server
  3. Send a client TLS hello message to the Redis server
  4. Peek at the beginning of the response from the server to see if it is the server's TLS hellow message.

The reason I ask, is because your sample doesn't illustrate the sending of the client hellow mesagel, and the check you appear to make seems to be looking to see if it is a client hello message.

@savincen
Copy link
Author

I downloaded version 1.1 of hiredis and built my application against it, instead of the older version mentioned above. Unfortunately, I still encounter the same hang when using the latest hiredis libraries.

It seems that redisInitiateSSLWithContext should be updated to avoid this hang, rather than client applications trying avoid it. The redisInitiateSSLWithContext routine is hanging when calling its redisSSLConnect, which calls SSL_connect:

where
1* In routine libpthread:read (+0x2D)
3 Returns to libcrypto:BIO_read (+0x6B)
4 Called from libssl:ssl23_read_bytes (+0x46)
5 Called from libssl:ssl23_connect (+0x115)
6 Called from libhiredis_ssl:\ssl\redisSSLConnect 353 (+0x3)
7 Called from libhiredis_ssl:\ssl\redisInitiateSSLWithContext 420 (+0x6)
8 Called from tkredis:\tkredis\redisInstConnect 827 (+0x1B)
9 Called from tkredis:\tkredis\redisInstReconnect 1026 (+0x1E)
10 Called from tkredis:\tkredis\redisInstDeleteKey 1212 (+0x14)
11 Called from tkvmase:\vmasredis2\vmasredis13 2335 (+0x2D)
12 Called from tkvmasd:\tkvmasd\tkvmasd 630 (+0x3E)
13 Called from tkmk:\sktMain\sktMain 108 (+0x19)
14 Called from tkmk:\TKPOSbktMain\bktMain 116 (+0xA)
15 Returns to libpthread:start_thread (+0xC5)
16 Called from libc:clone (+0x6B)

@savincen
Copy link
Author

Here is one of the ways I have been starting the server. If there is anything I should change, or add, that may help, please let me know.

docker run --mount type=bind,src=/my/redis/utls,dst=/data/tls -p 6379:6379 -p 6378:6378
redislabs/redismod --tls-port 6379 --port 6378 --loglevel debug --requirepass defusrpw
--tls-cert-file /data/tls/redis.crt --tls-key-file /data/tls/redis.key
--tls-ca-cert-file /data/tls/ca.crt

@savincen
Copy link
Author

savincen commented Oct 11, 2023

While I still think it would be a good idea for redisInitiateSSLWithContext to recognize this situation(the port is not configured for TLS), and gracefully return quickly. I assume most applications avoid this trouble by using redisSetTimeout to set a reasonable command timeout. It appeared to me as a hang because I was not calling redisSetTimeout to specify a timeout, thus it was waiting(forever) for an expected reply that will not come in that scenario.

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

No branches or pull requests

2 participants