Add support for using SSL connections to Redis. #129
Conversation
| @@ -15,6 +15,7 @@ NAPALM_TIMEOUT=10 | |||
| MAX_PAGE_SIZE=1000 | |||
| REDIS_HOST=redis | |||
| REDIS_PASSWORD=H733Kdjndks81 | |||
| REDIS_SSL=false | |||
There was a problem hiding this comment.
I think this is not necessary, as the default is false anyway, and we don't have all the variables in the list either.
There was a problem hiding this comment.
I was on the fence; initially I almost made the pull request with suggested change of making it true by default in the env file since using a password without SSL is still pretty fundamentally insecure (the AUTH command sends it as plaintext). Redis ElastiCache in AWS doesn't even permit using auth without SSL. But I wasn't sure how widely common it would be for people to actually have their Redis servers configured for SSL. Thus explicitly calling out the unsafe default combination of parameters here seemed like a compromise.
There was a problem hiding this comment.
🤔 Fair point.
Turning TLS on by default would not work, because the Redis which is started with the default docker-compose.yml configuration of netbox-docker is not configured for TLS, and it would be hard to do so.
There was a problem hiding this comment.
I dislike that it's called REDIS_SSL, as SSL is dead and was superseded by TLS almost 20 years ago. What do you think about changing the name to REDIS_TLS?
There was a problem hiding this comment.
I'm fine with either name, it's just that for better or worse the redis-py library kwarg for this parameter is named 'ssl'. I just tried to thread it back up the stack with the matching name. So django-rq (the real root dependency here) and the main netbox project have already accepted the pull requests with the REDIS_SSL naming. Thus the risk (albeit small) to using a different name here would be the potential for confusion, especially in terms of making the mapping between netbox-docker and netbox itself.
cimnine
added
the
enhancement
|
Thank you once more for this. I'm planing on merging this as soon as I have some minutes to spare. |
|
Just a quick update: I've decided to wait with merging this until the feature is actually released in Netbox. |
|
Sure, that's pretty much how I figured it would need to be sequenced. |
Requires a build or release of netbox containing pull request netbox-community/netbox#3012.