phoenixframework / phoenix_pubsub_redis

The Redis PubSub adapter for the Phoenix framework
175 stars 66 forks source link

Cannot configure SSL connection #42

Closed rafal0p closed 4 years ago

rafal0p commented 4 years ago

Enabling ssl is done by appending [ssl: true] to the config. But there is no further way of customizing this connection, and as Bram Verburg said at ElixirConfEU it’s important to be able to do so. Basically erlang by default accepts any certificates, not just the trusted ones. In addition to that AWS Redis requires some more adjustments to work.

Currently Redix configuration options are whitelisted, and then passed to Redix.PubSub.start_link/1. In Redix itself there is also a whitelist, but much richer.

I see two options to solve this:

  1. Drop the @redis_opts alltogether, and pass everything from config
  2. Expand accepted options to be able to properly configure tls connection

Which one do you think is better?

mitchellhenke commented 4 years ago

Thanks for opening an issue!

My first thought would be to not whitelist and do something like Plug does with Cowboy (with :cowboy_options) where there is an explicit :redix_options option that gets passed on to Redix.

rafal0p commented 4 years ago

If I understand you correctly, you are suggesting to split config options and have pubsub itself and redis configured separately, like that:

config :my_app, MyApp.Endpoint,
  pubsub: [
    # those are used by pubsub lib itself
    adapter: Phoenix.PubSub.Redis,
    name: "MyApp.PubSub",
    node_name: System.get_env("NODE"),
    redis_pool_size: 10,
    # those are just passed to Redix
    redis_options: [
      host: "192.168.1.100",
      password: "secret"
      port: 6379,
      ssl: true,
      # those are security-related, currently impossible to pass
      socket_opts: [
        verify: :verify_peer,
        depth: 3,
        cacertfile: System.get_env("CACERTFILE")
      ]
    ]
  ]

It makes sense, everything is in place, there are no surprises, I like that.

But this is a breaking change. Suddenly host option is nested inside redis_options, it's a quick fix, but still - everyone must do that.

The alternative is to somehow try to merge old opts and new opts, setting precedence one over another. Main drawback is that it starts to be confusing for the user programmer - where to set what, what is overridden, and how.

mitchellhenke commented 4 years ago

Given the split is breaking, it's probably not worthwhile at this point

43 allows :socket_opts, which allows configuring the more important security aspects

thiamsantos commented 4 years ago

@mitchellhenke The project would be open to a PR to work on a backward compatible change to support the split in the configs?

mitchellhenke commented 4 years ago

@thiamsantos I realized that there may be an opportunity to include a breaking change along these lines, as the upgrade to phoenix_pubsub 2.0 will be breaking anyway

I'll have to chat with pubsub maintainers to see what the plan is, and go from there 🙂

josevalim commented 4 years ago

The PR is in!