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

Feature request: configure SSL settings in CHANNEL_LAYERS #235

Closed
jray-afk opened this issue Jan 20, 2021 · 28 comments
Closed

Feature request: configure SSL settings in CHANNEL_LAYERS #235

jray-afk opened this issue Jan 20, 2021 · 28 comments

Comments

@jray-afk
Copy link

Hi, I posted about my adventure trying to configure SSL for Redis in the suggested Google Groups page which has all the requested information prepopulated in the body of this issue form, haven't gotten a response yet: https://groups.google.com/g/django-users/c/S094vik_gW0

After digging into the channels_redis code in this repo I realized that I probably need to submit a new feature request here in order to configure SSL settings in CHANNEL_LAYERS. (Unless, do you know of another way to configure SSL using channels_redis?)

Thank you!

@carltongibson
Copy link
Member

So... we'd need to by able to provide an SSLContext when creating connections.

See aioredis.create_connection().

Happy to accept input here.

@jray-afk
Copy link
Author

@carltongibson I appreciate the response. Yes that makes sense.

For now I will set up symmetric_encryption_keys to add some level of protection, but ultimately I would prefer setting up SSL if possible.

@carltongibson
Copy link
Member

Super! If you get started and need some input, let me know. Thanks! 👍

@skadz
Copy link

skadz commented Feb 5, 2021

I was just running in to this issue myself today, so happy to help test!

@jray-afk
Copy link
Author

jray-afk commented Feb 6, 2021

@skadz glad to hear I'm not the only one! Haven't had a chance to fork off of this project and try to implement the changes, but I may have to go rogue and go off of the version I built my project on, 2.3.2

@skadz
Copy link

skadz commented Feb 11, 2021

@skadz glad to hear I'm not the only one! Haven't had a chance to fork off of this project and try to implement the changes, but I may have to go rogue and go off of the version I built my project on, 2.3.2

@jray-afk Does this mean you actually have made the changes and got it working and just need to do the fork/submit PR or that you hacked some sort of other "fix" in? I'd like to get this implemented ASAP and if you have something that is working, I'd love to help you get it in to the main code, but also can help test anything you've done so far (or would like to know about how you hacked it in ;)

@nmacianx
Copy link

nmacianx commented Mar 9, 2021

@jray-afk @skadz any updates here? I'm facing the very same issue and help would be much appreciated!!

@carltongibson
Copy link
Member

Just awaiting someone to pick it up and submit a pull request I think

@dablak
Copy link

dablak commented Mar 10, 2021

When configuring the hosts in the Django settings you can use a dictionary and pass the SSL context option amongst other parameters :

# Mute the Heroku Redis error:
# "[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: self signed certificate in certificate chain"
# by disabling hostname check.

ssl_context = ssl.SSLContext()
ssl_context.check_hostname = False

heroku_redis_ssl_host = {
    'address': 'rediss://:password@127.0.0.1:6379/0'  # The 'rediss' schema denotes a SSL connection.
    'ssl': ssl_context
}

CHANNEL_LAYERS = {
    'default': {
        'BACKEND': 'channels_redis.core.RedisChannelLayer',
        'CONFIG': {
            'hosts': (heroku_redis_ssl_host,)
        }
    },
}

@carltongibson
Copy link
Member

@dablak Thanks for the comment — should get people going — but if we can ssl_context.check_hostname = False, surely we can configure the SSLContext correctly too? 🤔

Is this just documentation?

@dablak
Copy link

dablak commented Mar 10, 2021

I think that the SSLContext can be configured correctly in this way. If you check channels_redis.core.RedisChannelLayer.decode_hosts() you can see that if the hosts in the configuration are a dict then they are used as-is.

for entry in hosts:
    if isinstance(entry, dict):
        result.append(entry)
    else:
        result.append({"address": entry})

The dict items are then used as the parameters passed to aioredis.create_redis() in channels_redis.core.ConnectionPool.pop().

conn = await aioredis.create_redis(**self.host)

This is not in the documentation for channels_redis, but it should be.

@ezeakeal
Copy link

ezeakeal commented Nov 5, 2021

In case it helps others;

  • I have Redis setup with SSL certs (rediss://...).
  • channels==2.1.2
  • channels-redis==2.4.2

Here is a snippet from my Django config

redis_backend_url = "rediss://%s:%s/5" % (
    os.environ.get("REDIS_HOST", "redis_int"),
    os.environ.get("REDIS_PORT", 6379)
)
ssl_context = ssl.SSLContext()
ssl_context.load_cert_chain(
    certfile="/opt/redis/certs/client.crt",
    keyfile="/opt/redis/certs/client.key",
)
CHANNEL_LAYERS = {
    "default": {
        "BACKEND": "channels_redis.core.RedisChannelLayer",
        "CONFIG": {
            "hosts": ({
                'address': redis_backend_url,
                'ssl': ssl_context
            },)
        },
    }
}

@Amplitude88
Copy link

Amplitude88 commented Oct 27, 2022

Hello,

unfortunately for me it isn't working. I have a frontend on netlify which build a websocket connection to a django channels backend. Everything works on localserver as expected. If i try to connect in production to the channels backend it doesn't work.

in production i'll get the error which was mentioned here.
[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: self-signed certificate in certificate chain (_ssl.c:997)

Anyway how i try to set the context, channels doesn't accept the connection on heroku.

ssl_context = ssl.SSLContext()
ssl_context.check_hostname = False

    CHANNEL_LAYERS = {
        'default': {
            'BACKEND': 'channels_redis.core.RedisChannelLayer',
            'CONFIG': {
                'hosts': ({
                    'address': env('REDIS_TLS_URL'),
                    'ssl': ssl_context
                },)
            }
        }
    }

Please help

@nasir733
Copy link

@Amplitude88 the above solution stopped working for me as well were you able to solve this issue?

@Amplitude88
Copy link

Amplitude88 commented Oct 31, 2022

@Amplitude88 the above solution stopped working for me as well were you able to solve this issue?

Hey mate, yes i was in able to solve it. I've downgraded to 3.4.1
In Version 4 the problem is not solvable with the provided solution above.

Best regards,
Amp

@nasir733
Copy link

@Amplitude88 Yeah it also stopped working for me after I upgraded to channels v4 but it was working for a few days I think or i might have not noticed it since I don't have unit testing for my project

@nasir733
Copy link

@carltongibson @ezeakeal @andrewgodwin @adamchainz do you know why it might have stopped working? and how can we fix it ?

@ezeakeal
Copy link

ezeakeal commented Oct 31, 2022 via email

@carltongibson
Copy link
Member

@nasir733 please don't @mention whole lists of people to try and draw attention to your issue. It's just spam.

You need to look at how connection params are passed down to redis-py, and from there your particular redis provider (be it local or hosted or ...)

Once you've worked that out you may be in a position to make a documentation PR to resolve this issue.

@nasir733
Copy link

@nasir733 please don't @mention whole lists of people to try and draw attention to your issue. It's just spam.

You need to look at how connection params are passed down to redis-py, and from there your particular redis provider (be it local or hosted or ...)

Once you've worked that out you may be in a position to make a documentation PR to resolve this issue.

thanks for the immediate response I will try to fix the problem and make a PR. I might need some help since it will be my first PR to a major opensource project
sorry 😔 for tagging so many people

@aerickson1337
Copy link

aerickson1337 commented Nov 1, 2022

I dug into this a little after having similar issues to Amplitude88 and having to downgrade to 3.4.x

This seems like it might actually be a bug in the redis.asyncio.connection Connection class? in either the redis.asyncio.connection SSLConnection class or the redis.asyncio.connection RedisSSLContext class. There's a slot defined in the redis.asyncio Connection class for ssl_context but no arg for it in the init() so it's always ignored / set to None. Similar story in RedisSSLContext and SSLConnection where there are other SSL args but no arg for ssl_context so it ends up being None or ignored always as well.

Probably missing something somewhere, but initial thoughts are it might be a pretty easy fix upstream in redis.asyncio since they just need to allow for passing in ssl_context in either RedisSSLContext, SSLConnection, or Connection? Currently it seems like if you pass in ssl_context all of the redis.asyncio classes are configured to just drop it on the ground and set it to None which is whats causing people problems.

It seems strange to me that if you specifically pass in the individual SSLContext parameters e.g. ssl_keyfile, ssl_certfile, etc those are configured correctly and get handled by SSLConnection and RedisSSLContext, but the ssl_context specifically even though referenced and used in those classes is just dropped on the ground or built from scratch instead of allowing a passed in ssl_context.

If anyone can sanity check this or has ideas why redis.asyncio might have done it that way would be happy to hear anything on it. If anyone can confirm they think this is an upstream bug in redis.asyncio I can try to make some time to put together a repro for an upstream redis.asyncio issue post.

TLDR: Seems like possible redis.asyncio bug?

edit: clarify redis.asyncio not actual asyncio

@marcinowski
Copy link

I've submitted a PR for a similar issue last week which I think might solve the problem with current versions #337

@aerickson1337
Copy link

Hey marcinowski, that PR looks good and is what I did in my first attempt at getting it working. I think it should solve the problem but it would break the previous channels_redis SSL API from 3.4.x that let you pass in an SSLContext directly. From more digging around in redis-py and their implementation it seems like they don't support passing an SSLContext directly. I did find a work around for people who need to maintain that functionality of passing the SSLContext directly instead of as params, but it also changes the existing channels_redis channel layer API compared to older versions which I wasn't a fan of either.

monkeypatch the SSL classes used in redis-py to allow ssl_context

from redis.asyncio.connection import Connection, RedisSSLContext
from typing import Optional

class CustomSSLConnection(Connection):
    def __init__(
        self,
        ssl_context: Optional[str] = None,
        **kwargs,
    ):
        super().__init__(**kwargs)
        self.ssl_context = RedisSSLContext(ssl_context)

class RedisSSLContext:
    __slots__ = (
        "context",
    )

    def __init__(
        self,
        ssl_context,
    ):
        self.context = ssl_context

    def get(self):
        return self.context

using in channel declaration/connection

import ssl
from NameOfOurMonkeyPatchFile import CustomSSLConnection
context = ssl.create_default_context(purpose=ssl.Purpose.CLIENT_AUTH, cafile='some.crt')
context.load_cert_chain(certfile='some.crt', keyfile='some.key')
channel_settings = {
    'CHANNEL_LAYERS': {
        'default': {
            'BACKEND': 'channels_redis.core.RedisChannelLayer',
            'CONFIG': {
                "hosts": [
                    {
                        'host': f"{os.environ['REDIS']}",
                        'port': 6379,
                        'connection_class': CustomSSLConnection,
                        'ssl_context': context,
                    }
                ],
            },
        },
    }
}

I think the solution marcinowski provided is probably the best solution moving forward if redis-py doesn't want to support passing in an SSLContext directly and instead doing it with configuration options.

Either way seems like 4.0 ends up changing how the redis SSL connection API in the channel_layer declaration works. Not sure what the best path forward is but seems like there needs to be: 3.4.x SSL connection docs, 4.x SSL connection docs, and 3.4.x->4.x SSL changes docs.

I could probably take a stab at updating the docs with that but would probably need to wait and see what the verdict is on macrinowski's PR -> #337 before it would make sense to write those up.

@paulina7m
Copy link

paulina7m commented Dec 24, 2022

I've spent many days trying to combine all suggestions I found on resolving this issue on Heroku with Channels 4.0.0.
What worked for me, is slightly modified version of @aerickson1337 solution.

from redis.asyncio.connection import Connection, RedisSSLContext
from typing import Optional
import ssl
from urllib.parse import urlparse

class CustomSSLConnection(Connection):
    def __init__(
        self,
        ssl_context: Optional[str] = None,
        **kwargs,
    ):
        super().__init__(**kwargs)
        self.ssl_context = RedisSSLContext(ssl_context)

class RedisSSLContext:
    __slots__ = (
        "context",
    )

    def __init__(
        self,
        ssl_context,
    ):
        self.context = ssl_context

    def get(self):
        return self.context


url = urlparse(os.environ.get("REDIS_URL"))

ssl_context = ssl.SSLContext()
ssl_context.check_hostname = False

CHANNEL_LAYERS = {
    'default': {
        'BACKEND': 'channels_redis.core.RedisChannelLayer',
        'CONFIG': {
            'hosts': [
                    {
                        'host': url.hostname,
                        'port': url.port,
                        'username': url.username,
                        'password': url.password,
                        'connection_class': CustomSSLConnection,
                        'ssl_context': ssl_context,
                    }
                ],
        }
    },
}

ah, it feels so good to finally see Django Channels working properly with Heroku Data for Redis.
Thank you everyone on this thread.

@justuswilhelm
Copy link

@paulina7m
The urlparse was a crucial part of information for me and a further look at the code confirmed that channels_redis won't allow custom connection parameters such as connection_class if instead a host containing an address is specified (if "address" in host: in create_pool).

That isn't ideal behavior, because having to decode the Redis URL defeats the purpose of having a well formed URL in the first place. I will also try to solve this by parsing the URL and manually passing the components for now.

Of course on the other hand platform providers like Heroku could up their Redis security and not have us disregard invalid or self signed certs so easily. I assumed Let's Encrypt solved this issue, but maybe it hasn't for all purposes?

@sevdog
Copy link
Contributor

sevdog commented Jun 7, 2023

Just to add an other info here: redis-py has not yet documented "how to provide kwargs to perform an SSL/TLS connection" (see redis/redis-py#780).

There are already currently an SSLConnection and a RedisSSLContext class in redis-py (which are used by the from_url method). So there is no need to create/use custom classes.

@carltongibson
Copy link
Member

This should be resolved by #370.

@carltongibson
Copy link
Member

Copying @vsobotka's #373 (comment) for future searchers.

Earlier this year we upgraded redis-py for our app on Heroku from 3.9 to 4.5.4. We immediately ran into #235 and had to place in a workaround, with the ssl context. Everything went fine, until now, when we upgraded redis-py again, to 5.0.1. We immediately got "Connection reset by peer" error, preventing a stable connection. I was not able to find anything that would help. What helped us was to remove the workaround from #235 and use the newly supported option ssl_cert_reqs in layer config. So far this looks like it works perfectly well.

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

No branches or pull requests