Avoid Forcing SSL in Production Environment

[DomiiBunn]

Description:
Currently, the option to force SSL is hardcoded in the production environment. This can lead to complications for deployments behind reverse proxies, especially when utilizing services like Cloudflare Zero Trust or Nginx, which handle certificate management.

Suggested Improvement:
Consider making the SSL enforcement a configurable option through an environment variable. This enhancement would enhance flexibility and simplify deployments for users relying on reverse proxies and external services for SSL termination.

maybe/config/environments/production.rb

Line 46 in 622fc07

config.force_ssl = true

Benefits:

1. Deployment Flexibility: Users deploying behind reverse proxies with SSL termination services can seamlessly integrate without conflicting SSL configurations.

2. Simplified Configuration: By allowing SSL enforcement to be set via an environment variable, deployment configurations become more straightforward, reducing potential errors.

3. Adaptability: Users can easily adapt the SSL enforcement setting based on their specific deployment requirements, making the application more versatile.

Additional Context:
Provide documentation updates reflecting the new environment variable and its impact on SSL enforcement. This ensures users are informed and can make informed decisions during the deployment process.

[robzolkos]

Is setting the assume_ssl config a better way to handle this? (I've not ever seen the force_ssl bit switched off in prod and thinking assume_ssl was introduced for the scenarios you describe?)

[DomiiBunn]

It could be however if a deployment is made via docker it might cause other issues for local enviroments that might not want to go into the configs.

Avoids future issues imo

[ivarprudnikov]

Currently, the option to force SSL is hardcoded in the production environment.

I see it as a sensible default. You should tweak the environment config file before the deployment to production anyway.

The already suggested use of assume_ssl config seems what folks could use in this scenario.

[DomiiBunn]

Why not just expost all the config options as environment flags, still set everything correctly as the fallback but not require the end user to edit multiple configs rather than just a .env file/docker env variables/system environment while deploying (like 90% of other open source software does)

…

On Wed, Feb 7, 2024, 11:28 Ivar (aɪvɑr) ***@***.***> wrote: Currently, the option to force SSL is hardcoded in the production environment. I see it as a sensible default. You should tweak the environment config file before the deployment to production anyway. The already suggested use of assume_ssl <https://api.rubyonrails.org/v7.1.0/classes/ActionDispatch/AssumeSSL.html> config seems what folks could use in this scenario. — Reply to this email directly, view it on GitHub <#308 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMK42CTNXYB7RYWFS7HCFELYSNJOHAVCNFSM6AAAAABC2INTLKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZRG42DGMRTGU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[zachgoll]

Thoughts on this solution everyone?

config.assume_ssl = ENV["DISABLE_SSL"].blank?
config.force_ssl  = ENV["DISABLE_SSL"].blank?

[DomiiBunn]

Why not .env FORCE_SSL=true Idk rubby but in nodejs functSettingSslForce(process.env.FORCE_SSL || true) So if a value exists in the environment it'll be used, otherwise fails secure

…

On Wed, Feb 7, 2024, 23:21 Zach Gollwitzer ***@***.***> wrote: Thoughts on this solution everyone? config.assume_ssl = ENV["DISABLE_SSL"].blank?config.force_ssl = ENV["DISABLE_SSL"].blank? — Reply to this email directly, view it on GitHub <#308 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMK42CURA6TFC5VPULJOSRTYSP5AFAVCNFSM6AAAAABC2INTLKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZTGAZTIMRWGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[robzolkos]

I think if going that way they should be independent env vars. If they were designed to be simultaneously set to the same value then the config option would have been combined in Rails. Otherwise I think this should work ok.

Other suggestion would be to force a value if it isn't set in the env to whatever rails defaults are and also default it to the best state (SSL on) with it being clear that you are DISABLING it.

config.force_ssl = ENV["DISABLE_SSL"] != "true"

When DISABLE_SSL is "true", config.force_ssl becomes false, SSL is disabled.
When DISABLE_SSL is "false" or set to any value other than "true", or if DISABLE_SSL is not set at all, config.force_ssl remains true, enforcing SSL.

We are not FORCING something that is DEFAULT on, we are DISABLING something that is enabled by default.

Edit: ENV["DISABLE_SSL"].blank? may do exactly that lol, I'm just tired (just want to make sure we're covering the scenario if env var is not used at all)

[DomiiBunn]

I was not aware it's independent.

Than yeah by all means the

config.force_ssl = ENV["DISABLE_SSL"] != "true"

solution is basically what I'd expect

It's nearly midnight here. Can't blame you for beeing tired ^.^

[zachgoll]

config.force_ssl = ENV["DISABLE_SSL"] != "true" makes sense to me and agree, probably a bit easier to reason about than using .blank?

[haydenk]

I think the negated check makes it confusing.

If DISABLE_SSL is not "true" then force ssl is true

You're setting the config option force_ssl but your environment variable is "DISABLE_SSL" and you're checking if it is not a value.

ENV['DISABLE_SSL'].blank? would definitely work if you want to ignore the value and just simply check if the environment variable "DISABLE_SSL" exists. It's a lot better than ENV["DISABLE_SSL"] != "true", IMO.

[robzolkos]

ENV['DISABLE_SSL'].blank? would definitely work if you want to ignore the value and just simply check if the environment variable "DISABLE_SSL" exists. It's a lot better than ENV["DISABLE_SSL"] != "true", IMO.

Yeah you're right. I've thought about this some more and @zachgoll's proposed solution will work perfectly for this. It defaults to secure SSL (as per rails defaults) but can be explicitly disabled.

[haydenk]

I like using a FORCE_SSL environment variable better but the ENV['DISABLE_SSL'].blank? is a better option than checking if it's not true

e.g. ENV.fetch('FORCE_SSL', true) == "true"

[ezekg]

Thoughts on this solution everyone?

config.assume_ssl = ENV["DISABLE_SSL"].blank?
config.force_ssl  = ENV["DISABLE_SSL"].blank?

This is actually what Campfire does in config/environments/production.rb, so maybe that's a good route to take:

# Always be SSL'ing (unless told not to)
config.assume_ssl = ENV["DISABLE_SSL"].blank?
config.force_ssl  = ENV["DISABLE_SSL"].blank?

[jinnabaalu]

Added code changes for the same along with docker-compose for running the application. #416

Here is what I have added config.force_ssl = ENV.fetch("SSL_ENABLE", "true") == "true"

[ezekg]

It's worth mentioning that any well-behaved reverse proxy should be setting the X-Forwarded-* headers. Namely, X-Forwarded-Proto, which Rails uses to determine if the request is coming from a TLS-terminating reverse proxy. If X-Forwarded-Proto: https, then Rails will NOT redirect the request, because it knows it's coming from a TLS-terminating reverse proxy. So the only case where config.force_ssl = false is actually needed is for misbehaving reverse proxies.

[zachgoll]

Going to fold this issue into #295 as it would be a good change to make alongside a Dockerfile addition.

We will go ahead with this solution as discussed above:

config.assume_ssl = ENV["DISABLE_SSL"].blank?
config.force_ssl  = ENV["DISABLE_SSL"].blank?

[bensheldon]

I came to those via an unrelated issue, but I just wanted to share that this is the best way in Rails (that I'm aware of) to convert ENV strings to booleans:

config.force_ssl = ActiveModel::Type::Boolean.new.cast(ENV.fetch('RAILS_FORCE_SSL', true))

Which allows overriding with RAILS_FORCE_SSL=0 or RAILS_FORCE_SSL=false as well as casting other values to true/false too.

[zachgoll]

@bensheldon thanks for sharing, I like this solution a lot.